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.

I don't think it's worth spending that much time on rewriting it.

Note that I was only talking about moving that class to another package. Now if it's using some package private stuff it's making this more complex indeed.

Honestly it does not make much sense to have CommentAddAction here and CommentDeleteAction elsewhere so +1 to keep it here.

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.

surli · 2025-01-10T10:30:38Z

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

          <a class="edit btn btn-default btn-xs" rel="nofollow" href="$doc.getURL('view', "viewer=comments&amp;number=${comment.number}&amp;xredirect=$doc.getURL('view')")" title="$services.localization.render('core.viewers.comments.edit')">$services.icon.renderHTML('pencil')</a>
        #end
      #end
      <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 ($hasCommentEditionRight)


Hmmm I'm not so sure about that change... Yeah I know I commented to have consistency in an early review :)
Have you tested if it actually works to edit a comment for a page in which the comment user doesn't have edit right?

surli · 2025-01-10T10:32:53Z

...orm-core/xwiki-platform-oldcore/src/test/java/com/xpn/xwiki/web/CommentDeleteActionTest.java

+ * @version $Id$
+ * @since 17.0.0RC1
+ */
+@ComponentList


Hmm wrong annotation I think: this one takes as values a list of component. I guess you wanted to use @ComponentTest?

Note that you should not use @ComponentTest either since you have @OldcoreTest (it's one or the other).

tmortagne · 2025-01-10T13:47:34Z

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

          <a class="edit btn btn-default btn-xs" rel="nofollow" href="$doc.getURL('view', "viewer=comments&amp;number=${comment.number}&amp;xredirect=$doc.getURL('view')")" title="$services.localization.render('core.viewers.comments.edit')">$services.icon.renderHTML('pencil')</a>
        #end
      #end
      <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 ($hasCommentEditionRight)


Is the indentation change on purpose ?

tmortagne · 2025-01-10T13:51:20Z

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

+{
+    private static final String FAIL_MESSAGE = "failed";
+
+    private static final Logger LOGGER = LoggerFactory.getLogger(CommentDeleteAction.class);


It's a component, so you can inject a Logger (you find this kind of stuff in other actions because they used to not be components, but for new code let's do things by the book).

tmortagne · 2025-01-10T13:56:37Z

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

@@ -41,6 +43,8 @@
 @Singleton
 public class ObjectRemoveAction extends XWikiAction
 {
+    private static final String FAIL_MESSAGE = "failed";
+    private static final Logger LOGGER = LoggerFactory.getLogger(ObjectRemoveAction.class);


Same comment as above about injected Loggers.

tmortagne · 2025-01-10T13:59:22Z

...orm-core/xwiki-platform-oldcore/src/test/java/com/xpn/xwiki/web/CommentDeleteActionTest.java

+ */
+@ComponentList
+@ReferenceComponentList
+@OldcoreTest(mockXWiki = false)


mockXWiki = false sounds a bit strange, this was basically introduced for tests of XWiki, but tests not really testing XWiki itself generally don't want to do that

tmortagne · 2025-01-10T14:29:55Z

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

+ * @since 17.0.0RC1
+ */
+@Component
+@Named("commentdelete")


Nothing critical but commentremove would feel more consistent with objectremove, especially since it's reusing ObjectRemoveForm.

tmortagne · 2025-01-10T14:32:04Z

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

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


Might worth moving most of this class in some AbstractObjectRemoveAction that would also be extended by ObjectRemoveAction (to me it seems the main different is that CommentRemoveAction would indicate a hardcoded class to AbstractObjectRemoveAction instead of using the one provided by ObjectRemoveForm).

tmortagne · 2025-01-10T14:35:12Z

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

+                ScriptContext.ENGINE_SCOPE);
+        } else {
+            // Comment class reference
+            DocumentReference commentClass = new DocumentReference(context.getWikiId(), XWiki.SYSTEM_SPACE,


Why do you need to create a DocumentReference ?

tmortagne · 2025-01-10T14:38:16Z

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

+        ObjectRemoveForm form = (ObjectRemoveForm) context.getForm();
+        BaseObject obj = null;
+
+        String className = form.getClassName();


Manipulating the class name does not make sense for this action, since it's always about comments (or only to return an error if someone tried to be clever and use this action to delete another type of xobject with just comment right).

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

surli reviewed Jan 10, 2025

View reviewed changes

tmortagne reviewed Jan 10, 2025

View reviewed changes

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 Jan 10, 2025

tmortagne Jan 10, 2025

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

surli Jan 10, 2025

surli Jan 10, 2025

tmortagne Jan 10, 2025 •

edited

Loading

tmortagne Jan 10, 2025

tmortagne Jan 10, 2025

tmortagne Jan 10, 2025

tmortagne Jan 10, 2025 •

edited

Loading

tmortagne Jan 10, 2025 •

edited

Loading

tmortagne Jan 10, 2025 •

edited

Loading

tmortagne Jan 10, 2025

tmortagne Jan 10, 2025

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sereza7 commented Dec 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmortagne Jan 10, 2025 • edited Loading

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

tmortagne Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

tmortagne Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

tmortagne Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmortagne Jan 10, 2025 •

edited

Loading

tmortagne Jan 10, 2025 •

edited

Loading

tmortagne Jan 10, 2025 •

edited

Loading

tmortagne Jan 10, 2025 •

edited

Loading