]> source.dussan.org Git - sonarqube.git/commitdiff
SONAR-4450 Prevent users from adding empty comments on issues
authorJean-Baptiste Vilain <jean-baptiste.vilain@sonarsource.com>
Wed, 7 Aug 2013 10:36:22 +0000 (12:36 +0200)
committerJean-Baptiste Vilain <jean-baptiste.vilain@sonarsource.com>
Wed, 7 Aug 2013 10:36:22 +0000 (12:36 +0200)
sonar-server/src/main/java/org/sonar/server/issue/InternalRubyIssueService.java
sonar-server/src/main/java/org/sonar/server/issue/IssueCommentService.java
sonar-server/src/main/webapp/WEB-INF/app/controllers/issue_controller.rb
sonar-server/src/main/webapp/javascripts/issue.js
sonar-server/src/test/java/org/sonar/server/issue/IssueCommentServiceTest.java [new file with mode: 0644]

index b9846e8232b2e61b58d033c63961c70d352eb6d7..8082a506c3db32c87e34f485658e922430568c1b 100644 (file)
@@ -171,8 +171,14 @@ public class InternalRubyIssueService implements ServerComponent {
     return commentService.deleteComment(commentKey, UserSession.get());
   }
 
-  public IssueComment editComment(String commentKey, String newText) {
-    return commentService.editComment(commentKey, newText, UserSession.get());
+  public Result<IssueComment> editComment(String commentKey, String newText) {
+    Result<IssueComment> result = Result.of();
+    try {
+      result.set(commentService.editComment(commentKey, newText, UserSession.get()));
+    } catch (Exception e) {
+      result.addError(e.getMessage());
+    }
+    return result;
   }
 
   public IssueComment findComment(String commentKey) {
index 10528901b289a74f72f3ebc358dde3dbd47e4d45..385b291c7b2135fc6bd583af2d97f1ef32068743 100644 (file)
@@ -21,6 +21,7 @@ package org.sonar.server.issue;
 
 import com.google.common.base.Objects;
 import com.google.common.base.Strings;
+import org.apache.commons.lang.StringUtils;
 import org.sonar.api.ServerComponent;
 import org.sonar.api.issue.IssueComment;
 import org.sonar.api.issue.IssueQuery;
@@ -34,6 +35,10 @@ import org.sonar.core.issue.IssueUpdater;
 import org.sonar.core.issue.db.IssueChangeDao;
 import org.sonar.core.issue.db.IssueChangeDto;
 import org.sonar.core.issue.db.IssueStorage;
+import org.sonar.server.exceptions.BadRequestException;
+import org.sonar.server.exceptions.ForbiddenException;
+import org.sonar.server.exceptions.NotFoundException;
+import org.sonar.server.exceptions.UnauthorizedException;
 import org.sonar.server.user.UserSession;
 
 import java.util.Arrays;
@@ -64,8 +69,12 @@ public class IssueCommentService implements ServerComponent {
 
   public IssueComment addComment(String issueKey, String text, UserSession userSession) {
     verifyLoggedIn(userSession);
-
     IssueQueryResult queryResult = loadIssue(issueKey);
+
+    if(StringUtils.isBlank(text)) {
+      throw new BadRequestException("Cannot add empty comments to an issue");
+    }
+
     DefaultIssue issue = (DefaultIssue) queryResult.first();
 
     IssueChangeContext context = IssueChangeContext.createUser(new Date(), userSession.login());
@@ -78,12 +87,10 @@ public class IssueCommentService implements ServerComponent {
   public IssueComment deleteComment(String commentKey, UserSession userSession) {
     DefaultIssueComment comment = changeDao.selectCommentByKey(commentKey);
     if (comment == null) {
-      // TODO throw 404
-      throw new IllegalStateException();
+      throw new NotFoundException("Comment not found: " + commentKey);
     }
     if (Strings.isNullOrEmpty(comment.userLogin()) || !Objects.equal(comment.userLogin(), userSession.login())) {
-      // TODO throw unauthorized
-      throw new IllegalStateException();
+      throw new ForbiddenException("You can only delete your own comments");
     }
 
     // check authorization
@@ -95,13 +102,14 @@ public class IssueCommentService implements ServerComponent {
 
   public IssueComment editComment(String commentKey, String text, UserSession userSession) {
     DefaultIssueComment comment = changeDao.selectCommentByKey(commentKey);
-    if (comment == null) {
-      // TODO throw 404
-      throw new IllegalStateException();
+    if (StringUtils.isBlank(text)) {
+      throw new BadRequestException("Cannot add empty comments to an issue");
+    }
+    if(comment == null) {
+      throw new NotFoundException("Comment not found: " + commentKey);
     }
     if (Strings.isNullOrEmpty(comment.userLogin()) || !Objects.equal(comment.userLogin(), userSession.login())) {
-      // TODO throw unauthorized
-      throw new IllegalStateException();
+      throw new ForbiddenException("You can only edit your own comments");
     }
 
     // check authorization
@@ -117,8 +125,7 @@ public class IssueCommentService implements ServerComponent {
 
   private void verifyLoggedIn(UserSession userSession) {
     if (!userSession.isLoggedIn()) {
-      // must be logged
-      throw new IllegalStateException("User is not logged in");
+      throw new UnauthorizedException("User is not logged in");
     }
   }
 
index 0695d27692f839a092b4c04a3d2ec19b0a244c3b..ebfab7c080934041078f9630b6f66ec840ec2bad 100644 (file)
@@ -115,13 +115,18 @@ class IssueController < ApplicationController
   def edit_comment
     verify_ajax_request
     verify_post_request
-    require_parameters :key, :text
+    require_parameters :key
 
     text = Api::Utils.read_post_request_param(params[:text])
-    comment = Internal.issues.editComment(params[:key], text)
+    edit_result = Internal.issues.editComment(params[:key], text)
 
-    @issue_results = Api.issues.find(comment.issueKey)
-    render :partial => 'issue/issue', :locals => {:issue => @issue_results.issues.get(0)}
+    if edit_result.ok
+      @issue_results = Api.issues.find(edit_result.get.issueKey)
+      render :partial => 'issue/issue', :locals => {:issue => @issue_results.issues.get(0)}
+    else
+      @errors = edit_result.errors
+      render :partial => 'issue/error', :status => edit_result.httpStatus
+    end
   end
 
   # Delete an existing comment
index d3ed8d9a43f368942c13c01db68509c65699392a..02818576dac4c0b200f4c664e3f6b72a52498bcf 100644 (file)
@@ -43,20 +43,20 @@ function submitIssueForm(elt) {
   var formElt = $j(elt).closest('form');
   formElt.find('.loading').removeClass('hidden');
   formElt.find(':submit').prop('disabled', true);
+  var issueElt = formElt.closest('[data-issue-key]');
+
   $j.ajax({
       type: "POST",
       url: baseUrl + '/issue/do_action',
       data: formElt.serialize()}
   ).success(function (htmlResponse) {
-      var issueElt = formElt.closest('[data-issue-key]');
-      var issueKey = issueElt.attr('data-issue-key');
       var replaced = $j(htmlResponse);
       issueElt.replaceWith(replaced);
       notifyIssueChange(issueKey);
     }
   ).fail(function (jqXHR, textStatus) {
       closeIssueForm(elt);
-      alert(textStatus);
+      issueElt.find('.code-issue-actions').replaceWith(jqXHR.responseText);
     });
   return false;
 }
@@ -142,6 +142,11 @@ function doEditIssueComment(elt) {
       var replaced = $j(htmlResponse);
       issueElt.replaceWith(replaced);
       notifyIssueChange(issueKey);
+    },
+    error: function (jqXHR, textStatus) {
+      closeIssueForm(elt);
+      var commentElt = formElt.closest('[data-comment-key]');
+      commentElt.replaceWith(jqXHR.responseText);
     }
   });
   return false;
diff --git a/sonar-server/src/test/java/org/sonar/server/issue/IssueCommentServiceTest.java b/sonar-server/src/test/java/org/sonar/server/issue/IssueCommentServiceTest.java
new file mode 100644 (file)
index 0000000..a759f85
--- /dev/null
@@ -0,0 +1,105 @@
+/*
+ * SonarQube, open source software quality management tool.
+ * Copyright (C) 2008-2013 SonarSource
+ * mailto:contact AT sonarsource DOT com
+ *
+ * SonarQube 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 3 of the License, or (at your option) any later version.
+ *
+ * SonarQube 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 program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+
+package org.sonar.server.issue;
+
+import com.google.common.collect.Lists;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import org.sonar.api.issue.Issue;
+import org.sonar.api.issue.IssueQuery;
+import org.sonar.api.issue.IssueQueryResult;
+import org.sonar.core.issue.DefaultIssueQueryResult;
+import org.sonar.core.issue.IssueNotifications;
+import org.sonar.core.issue.IssueUpdater;
+import org.sonar.core.issue.db.IssueChangeDao;
+import org.sonar.core.issue.db.IssueStorage;
+import org.sonar.core.permission.Permission;
+import org.sonar.server.exceptions.BadRequestException;
+import org.sonar.server.user.MockUserSession;
+
+import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.stub;
+
+public class IssueCommentServiceTest {
+
+  private IssueUpdater updater;
+  private IssueChangeDao changeDao;
+  private IssueStorage storage;
+  private DefaultIssueFinder finder;
+  private IssueNotifications issueNotifications;
+  private IssueCommentService issueCommentService;
+
+  @Rule
+  public ExpectedException throwable = ExpectedException.none();
+
+  @BeforeClass
+  public static void setUpUser() {
+    MockUserSession.set().setLogin("admin").setPermissions(Permission.SYSTEM_ADMIN);
+  }
+
+  @Before
+  public void setUp() {
+    updater = mock(IssueUpdater.class);
+    changeDao = mock(IssueChangeDao.class);
+    storage = mock(IssueStorage.class);
+
+    finder = mock(DefaultIssueFinder.class);
+    Issue issue = mock(Issue.class);
+    IssueQueryResult result = new DefaultIssueQueryResult(Lists.newArrayList(issue));
+    stub(finder.find(any(IssueQuery.class))).toReturn(result);
+
+    issueNotifications = mock(IssueNotifications.class);
+
+    issueCommentService = new IssueCommentService(updater, changeDao, storage, finder, issueNotifications);
+  }
+
+  @Test
+  public void should_prevent_adding_empty_comment() throws Exception {
+    throwable.expect(BadRequestException.class);
+
+    issueCommentService.addComment("myIssue", " ", MockUserSession.get());
+  }
+
+  @Test
+  public void should_prevent_adding_null_comment() throws Exception {
+    throwable.expect(BadRequestException.class);
+
+    issueCommentService.addComment("myIssue", null, MockUserSession.get());
+  }
+
+  @Test
+  public void should_prevent_updating_empty_comment() throws Exception {
+    throwable.expect(BadRequestException.class);
+
+    issueCommentService.editComment("myComment", "", MockUserSession.get());
+  }
+
+  @Test
+  public void should_prevent_updating_null_comment() throws Exception {
+    throwable.expect(BadRequestException.class);
+
+    issueCommentService.editComment("myComment", null, MockUserSession.get());
+  }
+}