Integrate into media manager #246

annda · 2023-06-07T11:59:10Z

Implements #209

A rename button is added to the file detail view in media manager. The user can enter the new destination in a popup dialog.

If the move operation does not return any errors, the media manager reloads the target namespace.

Implements michitux#209

michitux

Thank you very much for this nice contribution! Overall this feature is a great addition to the plugin but I think error handling and permission checks need to be improved.

Also, this needs a protection against CSRF attacks. So a valid form token must be sent on the JavaScript side (could be copied from the delete button) and a form token check needs to be added in the ajax call handler.

michitux · 2023-06-07T19:28:31Z

action/rename.php

+            if (file_exists(mediaFN($dst))) {
+                $response['error'][] = $this->getLang('errorOverwrite');
+            }
+            if (auth_quickaclcheck($dst) < AUTH_CREATE) {
+                $response['error'][] = $this->getLang('errorPermissions');
+            }


moveMedia(), or rather checkMedia(), which is called by moveMedia, currently performs the following checks:

Source file exists.

Delete right on source file.

Source same as target.

Target file doesn't exist.

Upload right on target.

I don't see why these checks need to duplicated here. In particular, for all of these checks, there is already an error message including translations so this is basically adding duplicate code with duplicate messages. However, this is also missing some checks. Further, checking AUTH_CREATE is actually wrong I think, this is for creating pages, for uploading media files you need AUTH_UPLOAD.

The same is also true for errors that occur in moveMedia itself. In fact, moveMedia can even generate messages when the move itself succeeds but the old revisions cannot be moved. Will these messages be displayed to the user? I fear this is currently not the case but I think it would be good.

I think the easiest would be to just recover the generated messages from $MSG, see inc/Ajax.php#L293-L297 for an example.

Thank you, I have removed the duplication and use the existing checks.

action/rename.php

michitux · 2023-06-07T19:37:56Z

action/rename.php

+        $dst = cleanID($INPUT->str('dst'));
+
+        /** @var helper_plugin_move_op $MoveOperator */
+        $MoveOperator = plugin_load('helper', 'move_op');


It seems strange to me to start a local variable with an uppercase character, I've just searched for DokuWiki's code style but I couldn't find anything about this. Did I miss something?

michitux · 2023-06-07T19:44:28Z

script/MoveMediaManager.js

+            if (actionList.querySelector('button.move-btn')) continue; // already added
+
+
+            const moveButton = document.createElement('button');


Wouldn't it be better to first check if the user can at least delete the media file at the source location? I think we shouldn't display a move button to user who can't at least delete the media file as otherwise the move operation will always fail. I think it should be possible to do this by checking if there is a delete button in the actions.

I followed you suggestion and the code now checks for the existence of the delete button, which is only displayed when the user has sufficient permissions.

michitux · 2023-06-07T19:51:01Z

script/MoveMediaManager.js

+            },
+            // redirect or display error
+            function (result) {
+                if (result.error) {


Shouldn't this rather check result.success to detect if the move operation has actually been successful?

michitux · 2023-06-07T19:52:02Z

script/MoveMediaManager.js

+                if (result.error) {
+                    const errorText = document.createElement('strong');
+                    errorText.innerText = result.error.join();
+                    errorText.style = 'color: red;';


I think it would be nicer to re-use the message box style that is usually used in DokuWiki for displaying error messages (if possible, I haven't checked if there is anything in it that wouldn't work in this dialog).

Custom style replaced with DokuWiki's typical <div class="error">

Media move is prevented if source and destination media files do not have identical extensions (strict comparison). UTF-8 names should be handled correctly if UTF-8 locale is set.

annda · 2023-06-08T13:46:49Z

@michitux Thank you for your thorough review. I have pushed an additional commit that addresses the issues with media manager integration, and an extra commit with file extension check in helper_plugin_move_op.

michitux

Thank you very much for addressing all my comments. I have one small remark regarding nicer error handling for the security token check, but that's also not really a blocker for me.

action/rename.php

michitux · 2023-06-09T21:33:21Z

One more question: for the page rename button, there is the allowrename option to set a list of users/groups for which renaming should be allowed. I've already wondered in #242 if this group (or another one) should also be used to check if media renaming is allowed. Any thoughts? Media renaming is already restricted to users who are allowed to delete files, so maybe an additional restriction isn't required?

annda · 2023-06-21T15:02:29Z

It makes sense to check the allowrename setting consistently during all operations. I have applied the same check that is performed in renameOkay() when pages are being moved.

annda · 2023-06-26T08:58:31Z

@michitux Is there anything I could improve in this PR or is it ready to be merged?

michitux · 2023-06-26T19:35:47Z

I think overall this looks good, the only thing that could be improved is to hide the move button if the user cannot move files due to the allowrename setting. As this is the same for all media files, I think it could be a simple boolean property in JSINFO that indicates if the current user can rename media files according to the allowrename setting.

Checks if user matches the 'allowrename' setting

annda · 2023-06-27T08:35:12Z

0af31bb introduces an early check for allowrename as you suggested.

dregad · 2023-09-25T08:21:00Z

0af31bb introduces a regression, see #256

Warning: Trying to access array offset on value of type null in ./dokuwiki/move/action/rename.php on line 42 $USERINFO is null when user is not logged in. Regression introduced by 0af31bb (michitux#246). Fixes michitux#256, michitux#255

Integrate into media manager

673900a

Implements michitux#209

michitux requested changes Jun 7, 2023

View reviewed changes

annda added 2 commits June 8, 2023 15:29

Improve error handling and messages

779fa0e

Additional check on media file extensions

302c75c

Media move is prevented if source and destination media files do not have identical extensions (strict comparison). UTF-8 names should be handled correctly if UTF-8 locale is set.

annda requested a review from michitux June 8, 2023 13:44

michitux approved these changes Jun 9, 2023

View reviewed changes

action/rename.php Outdated Show resolved Hide resolved

Check renaming restrictions when moving media

27a1b6d

Media manager: early check if user is allowed to rename files

0af31bb

Checks if user matches the 'allowrename' setting

michitux merged commit b2e378d into michitux:master Jun 27, 2023

dregad mentioned this pull request Sep 25, 2023

access array offset on value of type null #256

Closed

dregad mentioned this pull request Apr 26, 2024

Fix PHP warning when user not logged in #275

Merged

michitux mentioned this pull request May 7, 2024

Better handling for moving media. #209

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate into media manager #246

Integrate into media manager #246

annda commented Jun 7, 2023

michitux left a comment

michitux Jun 7, 2023

annda Jun 8, 2023

michitux Jun 7, 2023

annda Jun 8, 2023

michitux Jun 7, 2023

annda Jun 8, 2023

michitux Jun 7, 2023

annda Jun 8, 2023

michitux Jun 7, 2023

annda Jun 8, 2023

annda commented Jun 8, 2023

michitux left a comment

michitux commented Jun 9, 2023

annda commented Jun 21, 2023

annda commented Jun 26, 2023

michitux commented Jun 26, 2023

annda commented Jun 27, 2023

dregad commented Sep 25, 2023

		if (actionList.querySelector('button.move-btn')) continue; // already added


		const moveButton = document.createElement('button');

Integrate into media manager #246

Integrate into media manager #246

Conversation

annda commented Jun 7, 2023

michitux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annda commented Jun 8, 2023

michitux left a comment

Choose a reason for hiding this comment

michitux commented Jun 9, 2023

annda commented Jun 21, 2023

annda commented Jun 26, 2023

michitux commented Jun 26, 2023

annda commented Jun 27, 2023

dregad commented Sep 25, 2023