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,159 @@ | ||
| /* | ||
| * 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.xwiki.component.annotation.Component; | ||
| import org.xwiki.model.reference.DocumentReference; | ||
| import org.xwiki.security.authorization.ContextualAuthorizationManager; | ||
| import org.xwiki.store.TemporaryAttachmentSessionsManager; | ||
| import org.xwiki.user.CurrentUserReference; | ||
| import org.xwiki.user.UserReferenceResolver; | ||
|
|
||
| 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; | ||
|
|
||
| /** | ||
| * Action used to remove a comment from a page, requires comment right but not edit right. | ||
| * | ||
| * @version $Id$ | ||
| * @since 16.1.0RC1 | ||
| */ | ||
| @Component | ||
| @Named("commentdelete") | ||
|
There was a problem hiding this comment. Nothing critical but There was a problem hiding this comment. Updated the action name in a71eee7 👍 |
||
| @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 There was a problem hiding this comment. @tmortagne I tried factorize this code by moving it to an abstract class like you recommended. However, I hit the revapi fail: AFAIU, there was nothing final in this class, so everything is fine. Do you know how we should ignore this warning or find an alternative solution to avoid triggering it? |
||
| { | ||
| @Inject | ||
| private UserReferenceResolver<CurrentUserReference> currentUserReferenceUserReferenceResolver; | ||
|
|
||
| @Inject | ||
| private TemporaryAttachmentSessionsManager temporaryAttachmentSessionsManager; | ||
|
|
||
| @Inject | ||
| private ContextualAuthorizationManager authorization; | ||
|
|
||
| @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 { | ||
| obj = doc.getObject(className, 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.removeObject(obj); | ||
| doc.setAuthorReference(userReference); | ||
|
|
||
| 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().write("failed"); | ||
| response.setContentLength(6); | ||
|
There was a problem hiding this comment. 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"? There was a problem hiding this comment. Note that this part is pretty much the same as See 6edbec1 👍 |
||
| } catch (IOException e) { | ||
|
There was a problem hiding this comment. Note that most parts are the same as Addressed in 767daac 👍 |
||
| } | ||
| 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.
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.
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.
Done in 9f841ae 👍