XWIKI-21730: Delete own comments should not require edit rights on page #2836

Sereza7 · 2024-02-01T13:06:08Z

Jira URL

https://jira.xwiki.org/browse/XWIKI-21730

Changes

Description

Changed the permission needed for the delete comment action to be displayed

Clarifications

The code responsible for allowing or not to delete the comment was introduced in version 3.0-rc1 (see the commit), and apparently it was not a use case considered by those changes. So I don't see an issue with tying the comment deletion action to the comment right instead of the edit right.
The template for generating these action buttons is a bit of a mess. I considered for a moment changing the order of the buttons to make it cleaner. However after considering stakes, I realized the UI inconsistency between versions would be worst for users than the small code quality/optimization we would have gotten.

Screenshots & Video

In order to test those changes, I registered as a regular user (named '123456'). With the admin user, I changed regular user rights at the wiki level: removed the Edit right and kept the Comment right.
We can see that the delete button only appears after the changes in this PR. This is the behavior expected by the reporter of this ticket.
Before PR:

After PR:

Executed Tests

None, this change only changes the presence of the button in specific right configurations (No edit right but comment right, different from the default Edit+comment rights). I could only see one reference to this in a pageobject. This function is used only once with regular logged in user rights (in here).
Manual tests were conducted successfully, see the screenshots above.

Expected merging strategy

Prefers squash: Yes
Backport on branches:
- 15.10.X - minor change with very low risk

* Changed the permission needed for the delete comment action to be displayed

surli · 2024-02-02T09:06:04Z

...go-skin/xwiki-platform-flamingo-skin-resources/src/main/resources/flamingo/commentsinline.vm

@@ -184,7 +184,7 @@ $xwiki.ssfx.use('uicomponents/viewers/comments.css', true)
      <a class="permalink btn btn-default btn-xs" data-toggle="modal" data-target="#permalinkModal" rel="nofollow"
        href="$doc.getURL('view', 'viewer=comments')#xwikicomment_${comment.number}"
        title="$services.localization.render('core.viewers.comments.permalink')">$services.icon.renderHTML('link')</a>
-      #if ($hasAdmin || ($hasEdit && $isUserComment))
+      #if ($hasAdmin || ($xwiki.hasAccessLevel('comment') && $isUserComment))


To be consistent I would reuse the same condition than the one used for editing a comment, which is right now #if($hasAdmin || $isUserComment) (see line 180).

Addressed in f536a96 and f943ae6 . 👍

This selector was a bit more restrictive, actually checking for the comment right. Now, any user can delete their comments even if they don't have any right on a page.

* Added backend support for deletion of comments. This new action is very similar to regular object removal (which was the action used before), but is based on the Comment right instead of the Edit right. See this demo: https://youtu.be/LwnLWfZFYZw

Sereza7 · 2024-02-07T14:14:16Z

Added backend support for deletion of comments. This new action is very similar to regular object removal (which was the action used before), but is based on the Comment right instead of the Edit right.

Changes are mostly contained in the oldcore module, because that's where all the backend logic for actions used in those buttons was already contained.

See a demo of this updated action.
This demo worked with just creating two symlinks on the updated jars for xwiki-oldcore and xwiki-security-authorization-bridge , and updating the commentsinline.vm template.

* Fixed typo

surli · 2024-02-07T14:58:17Z

...go-skin/xwiki-platform-flamingo-skin-resources/src/main/resources/flamingo/commentsinline.vm

@@ -184,7 +184,7 @@ $xwiki.ssfx.use('uicomponents/viewers/comments.css', true)
      <a class="permalink btn btn-default btn-xs" data-toggle="modal" data-target="#permalinkModal" rel="nofollow"
        href="$doc.getURL('view', 'viewer=comments')#xwikicomment_${comment.number}"
        title="$services.localization.render('core.viewers.comments.permalink')">$services.icon.renderHTML('link')</a>
-      #if ($hasAdmin || ($hasEdit && $isUserComment))
+      #if ($hasAdmin || $isUserComment)


What about introducing a new variable to use in both if for edit and delete? That way we ensure that we'll keep them consistent.

Done in 9f841ae 👍

surli · 2024-02-07T14:59:48Z

...latform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/web/CommentDeleteAction.java

+ * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA, or see the FSF site: http://www.fsf.org.
+ */
+package com.xpn.xwiki.web;


Hmmm normally we shouldn't introduce new stuff in this package, we should instead put stuff in org.xwiki... package, even in oldcore... Now it would make this class far from the other actions, so I'm not sure here...

We need a new action for the map in XWikiCachingRightService. This means that we cannot just reuse another action.

All actions are already here.

The code I used in this class is very similar to ObjectRemoveAction. It's mostly oldcore quality code. It'd probably need even more quality improvements to be anywhere else and I don't think it's worth spending that much time on rewriting it.

surli · 2024-02-07T15:00:46Z

...latform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/web/CommentDeleteAction.java

+            try {
+                response.getWriter().write("failed");
+                response.setContentLength(6);
+            } catch (IOException e) {


Nothing to handle the exception?

Note that most parts are the same as ObjectRemoveAction. This empty exception handle has been in this other file since 2008 so it seems like it's not a huge problem.
This is bad quality I agree.
I added a log similar to what was done in XWikiAction.java

xwiki-platform/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/web/XWikiAction.java

Line 385 in 4e2efc8

LOGGER.error("Failed to send error response to AJAX save and continue request.", e);

Addressed in 767daac 👍

surli · 2024-02-07T15:01:45Z

...latform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/web/CommentDeleteAction.java

+            response.setContentType("text/plain");
+            try {
+                response.getWriter().write("failed");
+                response.setContentLength(6);


Hmm really not a fan of this piece of code: at the very least "failed" should be in a String variable and you should eruse it also for the length. Now I'm not sure why you just answer "failed"?

Note that this part is pretty much the same as ObjectRemoveAction.
I updated it and also changed ObjectRemoveAction to match these changes.

See 6edbec1 👍

surli · 2024-02-07T15:03:02Z

...latform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/web/CommentDeleteAction.java

+        String changeComment = localizePlainOrReturnKey("core.comment.deleteComment");
+
+        // Make sure the user is allowed to make this modification
+        context.getWiki().checkSavingDocument(userReference, doc, changeComment, true, context);


And this check is working if you don't have edit right on the doc?

From what I can understand of this code I reused from ObjectRemoveAction, this does not check the rights of the current user but the state of the document (whether it's in a saveable state or not). I'm retesting things manually to make sure everything is alright.

Okay, I finally figured it out. The authorisation to do this action is handled at the level of XWikiCachingRightService. This is also the reason why we need an independant action even thought it's pretty much the same as object deletion. From what I understand, one action can only be mapped to one right. Setting the other object editions to depend on the Comment right is wrong, we need to separate this into two actions.

I can confirm this. I forgot to put the security .jar on my local instance and the action wouldn't go through (with edit rights disabled but comment rights enabled). Updating it as expected fixed the backend permission issue.

I'm retesting things manually to make sure everything is alright.

Except for an oldcore exception that's unrelated to this PR, builds are okay.

surli · 2024-02-07T15:04:21Z

...latform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/web/CommentDeleteAction.java

+@Component
+@Named("commentdelete")
+@Singleton
+public class CommentDeleteAction extends XWikiAction


A test class should be provided too.

Added a test for this class.

For both the test and the new Java class, I made sure to reformat the code with XWiki standard on Intellij Idea
I successfully passed mvn clean install -f xwiki-platform-core/xwiki-platform-oldcore -Pquality. All that was reported by SonarLint on both of the files are the @MockComponent that are not used in the class , which is a false positive. They are needed for our test class to work properly.

Added the test class in 1579ded 👍

* Factorized code

* Added a log on error of the AJAX write request

* Updated the render function on both ObjectRemoveAction and CommentDeleteAction

* Fixed codestyle * Added a test for the new action

Sereza7 · 2024-12-23T15:38:17Z

This PR is ready for further reviewing.

XWIKI-21730: Delete own comments should not require edit rights on page

fad359f

* Changed the permission needed for the delete comment action to be displayed

Sereza7 added the backport stable-15.10.x Used for automatic backport to 15.10.x branch. label Feb 1, 2024

surli reviewed Feb 2, 2024

View reviewed changes

Sereza7 marked this pull request as draft February 2, 2024 09:10

Sereza7 and others added 2 commits February 7, 2024 14:41

Merge branch 'xwiki:master' into XWIKI-21730

c809269

Sereza7 marked this pull request as ready for review February 7, 2024 14:14

XWIKI-21730: Delete own comments should not require edit rights on page

f943ae6

* Fixed typo

surli reviewed Feb 7, 2024

View reviewed changes

Sereza7 marked this pull request as draft February 12, 2024 10:56

Sereza7 added 6 commits December 18, 2024 13:24

Merge branch 'xwiki:master' into XWIKI-21730

8bbd0e0

XWIKI-21730: Delete own comments should not require edit rights on page

9f841ae

* Factorized code

XWIKI-21730: Delete own comments should not require edit rights on page

767daac

* Added a log on error of the AJAX write request

XWIKI-21730: Delete own comments should not require edit rights on page

6edbec1

* Updated the render function on both ObjectRemoveAction and CommentDeleteAction

Merge branch 'xwiki:master' into XWIKI-21730

3201e8b

XWIKI-21730: Delete own comments should not require edit rights on page

1579ded

* Fixed codestyle * Added a test for the new action

Sereza7 marked this pull request as ready for review December 23, 2024 15:36

Sereza7 requested a review from surli December 23, 2024 15:38

Sereza7 added backport stable-16.10.x and removed backport stable-15.10.x Used for automatic backport to 15.10.x branch. labels Jan 3, 2025

Sereza7 assigned surli Jan 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

XWIKI-21730: Delete own comments should not require edit rights on page #2836

XWIKI-21730: Delete own comments should not require edit rights on page #2836

Sereza7 commented Feb 1, 2024

surli Feb 2, 2024

Sereza7 Feb 7, 2024

Sereza7 commented Feb 7, 2024

surli Feb 7, 2024

Sereza7 Dec 18, 2024

surli Feb 7, 2024

Sereza7 Dec 18, 2024

surli Feb 7, 2024

Sereza7 Dec 18, 2024

surli Feb 7, 2024

Sereza7 Dec 18, 2024

surli Feb 7, 2024

Sereza7 Dec 18, 2024

Sereza7 Dec 18, 2024

Sereza7 Dec 18, 2024

Sereza7 Dec 19, 2024

surli Feb 7, 2024

Sereza7 Dec 23, 2024

Sereza7 commented Dec 23, 2024

XWIKI-21730: Delete own comments should not require edit rights on page #2836

Are you sure you want to change the base?

XWIKI-21730: Delete own comments should not require edit rights on page #2836

Conversation

Sereza7 commented Feb 1, 2024

Jira URL

Changes

Description

Clarifications

Screenshots & Video

Executed Tests

Expected merging strategy

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sereza7 commented Feb 7, 2024

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

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

Sereza7 commented Dec 23, 2024