XWIKI-21730: Delete own comments should not require edit rights on page #2836
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,163 @@ | ||
| /* | ||
| * See the NOTICE file distributed with this work for additional | ||
| * information regarding copyright ownership. | ||
| * | ||
| * This is free software; you can redistribute it and/or modify it | ||
| * under the terms of the GNU Lesser General Public License as | ||
| * published by the Free Software Foundation; either version 2.1 of | ||
| * the License, or (at your option) any later version. | ||
| * | ||
| * This software is distributed in the hope that it will be useful, | ||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU | ||
| * Lesser General Public License for more details. | ||
| * | ||
| * You should have received a copy of the GNU Lesser General Public | ||
| * License along with this software; if not, write to the Free | ||
| * 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; | ||
|
There was a problem hiding this comment. Hmmm normally we shouldn't introduce new stuff in this package, we should instead put stuff in There was a problem hiding this comment.
There was a problem hiding this comment.
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. There was a problem hiding this comment. Honestly it does not make much sense to have CommentAddAction here and CommentDeleteAction elsewhere so +1 to keep it here. |
||
|
|
||
| import java.io.IOException; | ||
|
|
||
| import javax.inject.Inject; | ||
| import javax.inject.Named; | ||
| import javax.inject.Singleton; | ||
| import javax.script.ScriptContext; | ||
| import javax.servlet.http.HttpServletResponse; | ||
|
|
||
| import org.apache.commons.lang3.StringUtils; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
| import org.xwiki.component.annotation.Component; | ||
| import org.xwiki.model.reference.DocumentReference; | ||
|
|
||
| import com.xpn.xwiki.XWiki; | ||
| import com.xpn.xwiki.XWikiContext; | ||
| import com.xpn.xwiki.XWikiException; | ||
| import com.xpn.xwiki.doc.XWikiDocument; | ||
| import com.xpn.xwiki.objects.BaseObject; | ||
|
|
||
| import org.xwiki.user.CurrentUserReference; | ||
| import org.xwiki.user.UserReference; | ||
| import org.xwiki.user.UserReferenceResolver; | ||
|
|
||
| /** | ||
| * Action used to remove a comment from a page, requires comment right but not edit right. Note that this class is | ||
| * largely inspired by ObjectRemoveAction and Comment | ||
| * | ||
| * @version $Id$ | ||
| * @since 17.0.0RC1 | ||
| */ | ||
| @Component | ||
| @Named("commentdelete") | ||
|
There was a problem hiding this comment. Nothing critical but |
||
| @Singleton | ||
| public class CommentDeleteAction extends XWikiAction | ||
|
There was a problem hiding this comment. A test class should be provided too. There was a problem hiding this comment. 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 Added the test class in 1579ded 👍 There was a problem hiding this comment. Might worth moving most of this class in some |
||
| { | ||
| private static final String FAIL_MESSAGE = "failed"; | ||
|
|
||
| private static final Logger LOGGER = LoggerFactory.getLogger(CommentDeleteAction.class); | ||
|
There was a problem hiding this comment. 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). |
||
|
|
||
| @Inject | ||
| private UserReferenceResolver<CurrentUserReference> currentUserResolver; | ||
|
|
||
| @Override | ||
| protected Class<? extends XWikiForm> getFormClass() | ||
| { | ||
| return ObjectRemoveForm.class; | ||
| } | ||
|
|
||
| protected BaseObject getObject(XWikiDocument doc, XWikiContext context) | ||
| { | ||
| ObjectRemoveForm form = (ObjectRemoveForm) context.getForm(); | ||
| BaseObject obj = null; | ||
|
|
||
| String className = form.getClassName(); | ||
|
There was a problem hiding this comment. 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). |
||
| int classId = form.getClassId(); | ||
| String attributeName = "message"; | ||
| if (StringUtils.isBlank(className)) { | ||
| getCurrentScriptContext().setAttribute(attributeName, | ||
| localizePlainOrReturnKey("platform.core.action.commentRemove.noClassnameSpecified"), | ||
| ScriptContext.ENGINE_SCOPE); | ||
| } else if (classId < 0) { | ||
| getCurrentScriptContext().setAttribute(attributeName, | ||
| localizePlainOrReturnKey("platform.core.action.commentRemove.noCommentSpecified"), | ||
| ScriptContext.ENGINE_SCOPE); | ||
| } else { | ||
| // Comment class reference | ||
| DocumentReference commentClass = new DocumentReference(context.getWikiId(), XWiki.SYSTEM_SPACE, | ||
|
There was a problem hiding this comment. Why do you need to create a DocumentReference ? |
||
| XWikiDocument.COMMENTSCLASS_REFERENCE.getName()); | ||
| obj = doc.getXObject(commentClass, classId); | ||
| if (obj == null) { | ||
| getCurrentScriptContext().setAttribute(attributeName, | ||
| localizePlainOrReturnKey("platform.core.action.commentRemove.invalidComment"), | ||
| ScriptContext.ENGINE_SCOPE); | ||
| } | ||
| } | ||
| return obj; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean action(XWikiContext context) throws XWikiException | ||
| { | ||
| // CSRF prevention | ||
| if (!csrfTokenCheck(context)) { | ||
| return false; | ||
| } | ||
|
|
||
| XWiki xwiki = context.getWiki(); | ||
| XWikiResponse response = context.getResponse(); | ||
| XWikiDocument doc = context.getDoc(); | ||
| DocumentReference userReference = context.getUserReference(); | ||
|
|
||
| // We need to clone this document first, since a cached storage would return the same object for the | ||
| // following requests, so concurrent request might get a partially modified object, or worse, if an error | ||
| // occurs during the save, the cached object will not reflect the actual document at all. | ||
| doc = doc.clone(); | ||
|
|
||
| BaseObject obj = getObject(doc, context); | ||
| if (obj == null) { | ||
| return true; | ||
| } | ||
|
|
||
| doc.removeXObject(obj); | ||
| UserReference currentUserReference = this.currentUserResolver.resolve(CurrentUserReference.INSTANCE); | ||
| doc.getAuthors().setEffectiveMetadataAuthor(currentUserReference); | ||
|
|
||
| String changeComment = localizePlainOrReturnKey("core.comment.deleteComment"); | ||
|
|
||
| // Make sure the user is allowed to make this modification | ||
| context.getWiki().checkSavingDocument(userReference, doc, changeComment, true, context); | ||
|
There was a problem hiding this comment. And this check is working if you don't have edit right on the doc? There was a problem hiding this comment. 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. There was a problem hiding this comment. Okay, I finally figured it out. The authorisation to do this action is handled at the level of There was a problem hiding this comment. 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. There was a problem hiding this comment.
Except for an oldcore exception that's unrelated to this PR, builds are okay. |
||
|
|
||
| xwiki.saveDocument(doc, changeComment, true, context); | ||
|
|
||
| if (Utils.isAjaxRequest(context)) { | ||
| response.setStatus(HttpServletResponse.SC_NO_CONTENT); | ||
| response.setContentLength(0); | ||
| } else { | ||
| // forward to edit | ||
| String redirect = Utils.getRedirect("edit", context); | ||
| sendRedirect(response, redirect); | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| @Override | ||
| public String render(XWikiContext context) throws XWikiException | ||
| { | ||
| if (Utils.isAjaxRequest(context)) { | ||
| XWikiResponse response = context.getResponse(); | ||
| response.setStatus(HttpServletResponse.SC_CONFLICT); | ||
| response.setContentType("text/plain"); | ||
| try { | ||
| response.getWriter().print(FAIL_MESSAGE); | ||
| } catch (IOException e) { | ||
|
There was a problem hiding this comment. Note that most parts are the same as Addressed in 767daac 👍 |
||
| LOGGER.error("Failed to send error response to AJAX comment delete request.", e); | ||
| } | ||
| return null; | ||
| } else { | ||
| return "error"; | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?