Fix quality flaws

6 files changed, 56 insertions, 35 deletions

diff --git a/sonar-server/src/main/java/org/sonar/server/issue/InternalRubyIssueService.java b/sonar-server/src/main/java/org/sonar/server/issue/InternalRubyIssueService.java
index 52c557a2e04..b9846e8232b 100644
--- a/sonar-server/src/main/java/org/sonar/server/issue/InternalRubyIssueService.java
+++ b/sonar-server/src/main/java/org/sonar/server/issue/InternalRubyIssueService.java
@@ -412,7 +412,7 @@ public class InternalRubyIssueService implements ServerComponent {
   }
 
   public boolean canUserShareIssueFilter(){
-    return issueFilterService.canShareFilter(UserSession.get().login());
+    return issueFilterService.canShareFilter(UserSession.get());
   }
 
   public String serializeFilterQuery(Map<String, Object> filterQuery) {
diff --git a/sonar-server/src/main/java/org/sonar/server/issue/IssueBulkChangeService.java b/sonar-server/src/main/java/org/sonar/server/issue/IssueBulkChangeService.java
index b6a0cff6ba2..55cf6efb5e1 100644
--- a/sonar-server/src/main/java/org/sonar/server/issue/IssueBulkChangeService.java
+++ b/sonar-server/src/main/java/org/sonar/server/issue/IssueBulkChangeService.java
@@ -32,11 +32,8 @@ import org.sonar.api.issue.internal.IssueChangeContext;
 import org.sonar.api.web.UserRole;
 import org.sonar.core.issue.IssueNotifications;
 import org.sonar.core.issue.db.IssueStorage;
-import org.sonar.server.exceptions.BadRequestException;
 import org.sonar.server.user.UserSession;
 
-import javax.annotation.CheckForNull;
-
 import java.util.Date;
 import java.util.List;
 
@@ -66,36 +63,18 @@ public class IssueBulkChangeService {
     IssueBulkChangeResult result = new IssueBulkChangeResult();
     IssueQueryResult issueQueryResult = issueFinder.find(IssueQuery.builder().issueKeys(issueBulkChangeQuery.issues()).pageSize(-1).requiredRole(UserRole.USER).build());
     List<Issue> issues = issueQueryResult.issues();
-    List<Action> bulkActions = newArrayList();
-    for (String actionName : issueBulkChangeQuery.actions()) {
-      Action action = getAction(actionName);
-      if (action == null) {
-        throw new BadRequestException("The action : '"+ actionName + "' is unknown");
-      }
-      if (action.verify(issueBulkChangeQuery.properties(actionName), issues, userSession)) {
-        bulkActions.add(action);
-      }
-    }
+    List<Action> bulkActions = getActionsToApply(issueBulkChangeQuery, issues, userSession);
 
     IssueChangeContext issueChangeContext = IssueChangeContext.createUser(new Date(), userSession.login());
     for (Issue issue : issues) {
       ActionContext actionContext = new ActionContext(issue, issueChangeContext);
       for (Action action : bulkActions) {
-        try {
-          if (applyAction(action, actionContext, issueBulkChangeQuery)) {
-            result.addIssueChanged(issue);
-          } else {
-            result.addIssueNotChanged(issue);
-          }
-        } catch (Exception e) {
-          result.addIssueNotChanged(issue);
-          LOG.info("An error occur when trying to apply the action : "+ action.key() + " on issue : "+ issue.key() + ". This issue has been ignored.", e);
-        }
+        applyAction(action, actionContext, issueBulkChangeQuery, result);
       }
       if (result.issuesChanged().contains(issue)) {
         // Apply comment action only on changed issues
         if (issueBulkChangeQuery.hasComment()) {
-          applyAction(getAction(CommentAction.KEY), actionContext, issueBulkChangeQuery);
+          applyAction(getAction(CommentAction.KEY), actionContext, issueBulkChangeQuery, result);
         }
         issueStorage.save((DefaultIssue) issue);
         issueNotifications.sendChanges((DefaultIssue) issue, issueChangeContext, issueQueryResult);
@@ -105,18 +84,38 @@ public class IssueBulkChangeService {
     return result;
   }
 
-  private boolean applyAction(Action action, ActionContext actionContext, IssueBulkChangeQuery issueBulkChangeQuery){
-    return action.supports(actionContext.issue()) && action.execute(issueBulkChangeQuery.properties(action.key()), actionContext);
+  private List<Action> getActionsToApply(IssueBulkChangeQuery issueBulkChangeQuery, List<Issue> issues, UserSession userSession) {
+    List<Action> bulkActions = newArrayList();
+    for (String actionName : issueBulkChangeQuery.actions()) {
+      Action action = getAction(actionName);
+      if (action.verify(issueBulkChangeQuery.properties(actionName), issues, userSession)) {
+        bulkActions.add(action);
+      }
+    }
+    return bulkActions;
+  }
+
+  private void applyAction(Action action, ActionContext actionContext, IssueBulkChangeQuery issueBulkChangeQuery, IssueBulkChangeResult result) {
+    Issue issue = actionContext.issue();
+    try {
+      if (action.supports(issue) && action.execute(issueBulkChangeQuery.properties(action.key()), actionContext)) {
+        result.addIssueChanged(issue);
+      } else {
+        result.addIssueNotChanged(issue);
+      }
+    } catch (Exception e) {
+      result.addIssueNotChanged(issue);
+      LOG.info("An error occur when trying to apply the action : " + action.key() + " on issue : " + issue.key() + ". This issue has been ignored.", e);
+    }
   }
 
-  @CheckForNull
   private Action getAction(final String actionKey) {
     return Iterables.find(actions, new Predicate<Action>() {
       @Override
       public boolean apply(Action action) {
         return action.key().equals(actionKey);
       }
-    }, null);
+    });
   }
 
   static class ActionContext implements Action.Context {
diff --git a/sonar-server/src/main/java/org/sonar/server/issue/IssueFilterService.java b/sonar-server/src/main/java/org/sonar/server/issue/IssueFilterService.java
index 0b54f6edf76..ccf2e6a4187 100644
--- a/sonar-server/src/main/java/org/sonar/server/issue/IssueFilterService.java
+++ b/sonar-server/src/main/java/org/sonar/server/issue/IssueFilterService.java
@@ -205,8 +205,12 @@ public class IssueFilterService implements ServerComponent {
     return issueFilterDto;
   }
 
-  public boolean canShareFilter(String user){
-    return authorizationDao.selectGlobalPermissions(user).contains(Permission.DASHBOARD_SHARING.key());
+  public boolean canShareFilter(UserSession userSession){
+    if (userSession.isLoggedIn()) {
+      String user = userSession.login();
+      return hasUserSharingPermission(user);
+    }
+    return false;
   }
 
   String getLoggedLogin(UserSession userSession) {
@@ -242,7 +246,7 @@ public class IssueFilterService implements ServerComponent {
   }
 
   private void verifyCurrentUserCanShareFilter(DefaultIssueFilter issueFilter, String user) {
-    if (issueFilter.shared() && !canShareFilter(user)) {
+    if (issueFilter.shared() && !hasUserSharingPermission(user)) {
       throw new ForbiddenException("User cannot own this filter because of insufficient rights");
     }
   }
@@ -334,4 +338,8 @@ public class IssueFilterService implements ServerComponent {
     return new IssueFilterResult(issueQueryResult, issueQuery);
   }
 
+  private boolean hasUserSharingPermission(String user){
+    return authorizationDao.selectGlobalPermissions(user).contains(Permission.DASHBOARD_SHARING.key());
+  }
+
 }
diff --git a/sonar-server/src/test/java/org/sonar/server/issue/InternalRubyIssueServiceTest.java b/sonar-server/src/test/java/org/sonar/server/issue/InternalRubyIssueServiceTest.java
index 7edbdb503cc..eb3fc3c561d 100644
--- a/sonar-server/src/test/java/org/sonar/server/issue/InternalRubyIssueServiceTest.java
+++ b/sonar-server/src/test/java/org/sonar/server/issue/InternalRubyIssueServiceTest.java
@@ -540,7 +540,7 @@ public class InternalRubyIssueServiceTest {
   @Test
   public void should_check_if_user_can_share_issue_filter(){
     service.canUserShareIssueFilter();
-    verify(issueFilterService).canShareFilter(anyString());
+    verify(issueFilterService).canShareFilter(any(UserSession.class));
   }
 
   @Test
diff --git a/sonar-server/src/test/java/org/sonar/server/issue/IssueBulkChangeServiceTest.java b/sonar-server/src/test/java/org/sonar/server/issue/IssueBulkChangeServiceTest.java
index 70281632224..6a9ac61b342 100644
--- a/sonar-server/src/test/java/org/sonar/server/issue/IssueBulkChangeServiceTest.java
+++ b/sonar-server/src/test/java/org/sonar/server/issue/IssueBulkChangeServiceTest.java
@@ -32,13 +32,13 @@ import org.sonar.api.issue.internal.IssueChangeContext;
 import org.sonar.api.web.UserRole;
 import org.sonar.core.issue.IssueNotifications;
 import org.sonar.core.issue.db.IssueStorage;
-import org.sonar.server.exceptions.BadRequestException;
 import org.sonar.server.exceptions.UnauthorizedException;
 import org.sonar.server.user.MockUserSession;
 import org.sonar.server.user.UserSession;
 
 import java.util.List;
 import java.util.Map;
+import java.util.NoSuchElementException;
 
 import static com.google.common.collect.Lists.newArrayList;
 import static com.google.common.collect.Maps.newHashMap;
@@ -277,7 +277,7 @@ public class IssueBulkChangeServiceTest {
       service.execute(issueBulkChangeQuery, userSession);
       fail();
     } catch (Exception e) {
-      assertThat(e).isInstanceOf(BadRequestException.class).hasMessage("The action : 'unknown' is unknown");
+      assertThat(e).isInstanceOf(NoSuchElementException.class);
     }
     verifyZeroInteractions(issueStorage);
     verifyZeroInteractions(issueNotifications);
diff --git a/sonar-server/src/test/java/org/sonar/server/issue/IssueFilterServiceTest.java b/sonar-server/src/test/java/org/sonar/server/issue/IssueFilterServiceTest.java
index 0cd775e1f32..101715be0db 100644
--- a/sonar-server/src/test/java/org/sonar/server/issue/IssueFilterServiceTest.java
+++ b/sonar-server/src/test/java/org/sonar/server/issue/IssueFilterServiceTest.java
@@ -652,6 +652,20 @@ public class IssueFilterServiceTest {
     verify(issueFilterSerializer).deserialize("componentRoots=struts");
   }
 
+  @Test
+  public void user_can_share_filter_if_logged_and_own_sharing_permission(){
+    when(authorizationDao.selectGlobalPermissions("john")).thenReturn(newArrayList(Permission.DASHBOARD_SHARING.key()));
+    UserSession userSession = MockUserSession.create().setLogin("john");
+    assertThat(service.canShareFilter(userSession)).isTrue();
+
+    userSession = MockUserSession.create().setLogin(null);
+    assertThat(service.canShareFilter(userSession)).isFalse();
+
+    when(authorizationDao.selectGlobalPermissions("john")).thenReturn(Collections.<String>emptyList());
+    userSession = MockUserSession.create().setLogin("john");
+    assertThat(service.canShareFilter(userSession)).isFalse();
+  }
+
   private static class Matches extends BaseMatcher<IssueFilterDto> {
 
     private final IssueFilterDto referenceFilter;