Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX] partial fix for issue 386, prevent copy items in manage view of course #388

Open
wants to merge 1 commit into
base: release_8
Choose a base branch
from

Conversation

chfsx
Copy link
Contributor

@chfsx chfsx commented Jan 13, 2025

This PR prevents copying objects in manage view of courses. The plugin itself already "defines" that copy should not be possible. ILIAS seems to ignore that, I tried to track this down in ILIAS Core but just found a lot of old code and was not able to find the right place where this should be checked in ILIAS. This PR provides a workaround: while checking permissions, the plugin now always returns false for the copy-permission. this at least prevents from generating (non-functions) copies of Opencast objects in ILIAS.

@dagraf
Copy link

dagraf commented Jan 14, 2025

I did test this fix. After selecting the the target course where the object should be copied to and hitting on "Paste" I get redirected back to the page I came from (in my case the course where the originals object is situated) and an error message is shown ("Permission Denied"). This is prevents from getting non-functional copies of Opencast objects but is far from optimal. I guess, that's why you write "partial fix" in the title.

@chfsx: What kind of options to improve this behavior do we have? The easiest I could imagine would be to specify the "Permission Denied" message why copying is not possible.

@dagraf dagraf requested a review from ferishili January 14, 2025 14:45
@dagraf
Copy link

dagraf commented Jan 14, 2025

In meeting: Can not be optimized in the plugin. To fix we would need to create a bug report for the core but there is little chance that it will get fixed soon or even at any time.

Next step:
@chfsx will create such a bugreport.
@ferishili : Please review this PR.

Copy link
Contributor

@ferishili ferishili left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @chfsx, for your work!
Your solution works as expected and should be considered the definitive approach for now (until a fix from ILIAS Core is available) rather than a partial solution, in my opinion.
I have suggested two minor changes; please take a look.

Best regards.

* @param string $a_user_id
*/
public function _checkAccess($a_cmd, $a_permission, $a_ref_id, $a_obj_id = null, $a_user_id = ''): bool

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extra line should be removed or even better, replace it with inheritDoc:

Suggested change
/**
* @inheritDoc
*/

/** @var ObjectSettings $objectSettings */
$objectSettings = ObjectSettings::find($a_obj_id);
if ($all_refs = $objectSettings->getDuplicatesOnSystem()) {
if ($objectSettings && $all_refs = $objectSettings->getDuplicatesOnSystem()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not recommend using a variable and fetching a variable in a same if statement, I would recommend doing it like:

Suggested change
if ($objectSettings && $all_refs = $objectSettings->getDuplicatesOnSystem()) {
$all_refs = $objectSettings ? $objectSettings->getDuplicatesOnSystem() : null;
if (!empty($all_refs)) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

3 participants