From ea775c680b15bc3de5d28f3c67195f5ad8ffbb9b Mon Sep 17 00:00:00 2001 From: Julien Lancelot Date: Thu, 6 Jun 2013 12:34:00 +0200 Subject: [PATCH] SONAR-3755 Improve creation of manual issue --- .../org/sonar/core/resource/ResourceDto.java | 2 +- .../sonar/core/resource/ResourceQuery.java | 2 +- .../org/sonar/server/issue/IssueService.java | 38 ++++---- .../sonar/server/issue/IssueServiceTest.java | 93 +++++++------------ 4 files changed, 54 insertions(+), 81 deletions(-) diff --git a/sonar-core/src/main/java/org/sonar/core/resource/ResourceDto.java b/sonar-core/src/main/java/org/sonar/core/resource/ResourceDto.java index 33af18cbb3d..cbf8cd69121 100644 --- a/sonar-core/src/main/java/org/sonar/core/resource/ResourceDto.java +++ b/sonar-core/src/main/java/org/sonar/core/resource/ResourceDto.java @@ -21,7 +21,7 @@ package org.sonar.core.resource; import java.util.Date; -public final class ResourceDto { +public class ResourceDto { private Long id; private String key; diff --git a/sonar-core/src/main/java/org/sonar/core/resource/ResourceQuery.java b/sonar-core/src/main/java/org/sonar/core/resource/ResourceQuery.java index fff662a1690..2da1623cbb2 100644 --- a/sonar-core/src/main/java/org/sonar/core/resource/ResourceQuery.java +++ b/sonar-core/src/main/java/org/sonar/core/resource/ResourceQuery.java @@ -22,7 +22,7 @@ package org.sonar.core.resource; /** * @since 3.0 */ -public final class ResourceQuery { +public class ResourceQuery { private String[] qualifiers = null; private String key = null; private boolean excludeDisabled = false; diff --git a/sonar-server/src/main/java/org/sonar/server/issue/IssueService.java b/sonar-server/src/main/java/org/sonar/server/issue/IssueService.java index 331bc0ebdc4..13e980527af 100644 --- a/sonar-server/src/main/java/org/sonar/server/issue/IssueService.java +++ b/sonar-server/src/main/java/org/sonar/server/issue/IssueService.java @@ -19,10 +19,8 @@ */ package org.sonar.server.issue; -import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Strings; import org.sonar.api.ServerComponent; -import org.sonar.api.component.Component; import org.sonar.api.issue.Issue; import org.sonar.api.issue.IssueQuery; import org.sonar.api.issue.IssueQueryResult; @@ -39,6 +37,7 @@ import org.sonar.core.issue.workflow.IssueWorkflow; import org.sonar.core.issue.workflow.Transition; import org.sonar.core.resource.ResourceDao; import org.sonar.core.resource.ResourceDto; +import org.sonar.core.resource.ResourceQuery; import org.sonar.core.user.AuthorizationDao; import org.sonar.server.user.UserSession; @@ -109,6 +108,7 @@ public class IssueService implements ServerComponent { } public Issue doTransition(String issueKey, String transition, UserSession userSession) { + verifyLoggedIn(userSession); IssueQueryResult queryResult = loadIssue(issueKey); DefaultIssue issue = (DefaultIssue) queryResult.first(); IssueChangeContext context = IssueChangeContext.createUser(new Date(), userSession.login()); @@ -120,6 +120,7 @@ public class IssueService implements ServerComponent { } public Issue assign(String issueKey, @Nullable String assignee, UserSession userSession) { + verifyLoggedIn(userSession); IssueQueryResult queryResult = loadIssue(issueKey); DefaultIssue issue = (DefaultIssue) queryResult.first(); if (assignee != null && userFinder.findByLogin(assignee) == null) { @@ -134,6 +135,7 @@ public class IssueService implements ServerComponent { } public Issue plan(String issueKey, @Nullable String actionPlanKey, UserSession userSession) { + verifyLoggedIn(userSession); if (!Strings.isNullOrEmpty(actionPlanKey) && actionPlanService.findByKey(actionPlanKey, userSession) == null) { throw new IllegalArgumentException("Unknown action plan: " + actionPlanKey); } @@ -149,6 +151,7 @@ public class IssueService implements ServerComponent { } public Issue setSeverity(String issueKey, String severity, UserSession userSession) { + verifyLoggedIn(userSession); IssueQueryResult queryResult = loadIssue(issueKey); DefaultIssue issue = (DefaultIssue) queryResult.first(); @@ -161,18 +164,23 @@ public class IssueService implements ServerComponent { } public DefaultIssue createManualIssue(DefaultIssue issue, UserSession userSession) { - checkAuthorization(userSession, issue, UserRole.USER); + verifyLoggedIn(userSession); + ResourceDto resourceDto = resourceDao.getResource(ResourceQuery.create().setKey(issue.componentKey())); + if (resourceDto == null) { + throw new IllegalArgumentException("Unknown component: " + issue.componentKey()); + } + if (!authorizationDao.isAuthorizedComponentId(resourceDto.getId(), userSession.userId(), UserRole.USER)) { + // TODO throw unauthorized + throw new IllegalStateException("User does not have the required role"); + } if (!"manual".equals(issue.ruleKey().repository())) { throw new IllegalArgumentException("Issues can be created only on rules marked as 'manual': " + issue.ruleKey()); } + Rule rule = ruleFinder.findByKey(issue.ruleKey()); if (rule == null) { throw new IllegalArgumentException("Unknown rule: " + issue.ruleKey()); } - Component component = resourceDao.findByKey(issue.componentKey()); - if (component == null) { - throw new IllegalArgumentException("Unknown component: " + issue.componentKey()); - } Date now = new Date(); issue.setCreationDate(now); @@ -194,25 +202,11 @@ public class IssueService implements ServerComponent { return workflow.statusKeys(); } - @VisibleForTesting - void checkAuthorization(UserSession userSession, Issue issue, String requiredRole) { + private void verifyLoggedIn(UserSession userSession) { if (!userSession.isLoggedIn()) { // must be logged throw new IllegalStateException("User is not logged in"); } - if (!authorizationDao.isAuthorizedComponentId(findRootProject(issue.componentKey()).getId(), userSession.userId(), requiredRole)) { - // TODO throw unauthorized - throw new IllegalStateException("User does not have the required role"); - } } - @VisibleForTesting - ResourceDto findRootProject(String componentKey) { - ResourceDto resourceDto = resourceDao.getRootProjectByComponentKey(componentKey); - if (resourceDto == null) { - // TODO throw 404 - throw new IllegalArgumentException("Component '" + componentKey + "' does not exists."); - } - return resourceDto; - } } diff --git a/sonar-server/src/test/java/org/sonar/server/issue/IssueServiceTest.java b/sonar-server/src/test/java/org/sonar/server/issue/IssueServiceTest.java index b529a572902..d1c001141c9 100644 --- a/sonar-server/src/test/java/org/sonar/server/issue/IssueServiceTest.java +++ b/sonar-server/src/test/java/org/sonar/server/issue/IssueServiceTest.java @@ -23,7 +23,6 @@ package org.sonar.server.issue; import org.junit.Before; import org.junit.Test; import org.mockito.ArgumentCaptor; -import org.sonar.api.component.Component; import org.sonar.api.issue.Issue; import org.sonar.api.issue.IssueQuery; import org.sonar.api.issue.IssueQueryResult; @@ -42,6 +41,7 @@ import org.sonar.core.issue.workflow.IssueWorkflow; import org.sonar.core.issue.workflow.Transition; import org.sonar.core.resource.ResourceDao; import org.sonar.core.resource.ResourceDto; +import org.sonar.core.resource.ResourceQuery; import org.sonar.core.user.AuthorizationDao; import org.sonar.core.user.DefaultUser; import org.sonar.server.user.UserSession; @@ -155,7 +155,6 @@ public class IssueServiceTest { @Test public void should_not_do_transition() { - grantAccess(); when(workflow.doTransition(eq(issue), eq(transition.key()), any(IssueChangeContext.class))).thenReturn(false); Issue result = issueService.doTransition("ABCD", transition.key(), userSession); @@ -165,6 +164,19 @@ public class IssueServiceTest { verifyZeroInteractions(issueNotifications); } + + @Test + public void should_fail_do_transition_if_not_logged() { + when(userSession.isLoggedIn()).thenReturn(false); + try { + issueService.doTransition("ABCD", transition.key(), userSession); + fail(); + } catch (Exception e) { + assertThat(e).isInstanceOf(IllegalStateException.class).hasMessage("User is not logged in"); + } + verifyZeroInteractions(authorizationDao); + } + @Test public void should_assign() { String assignee = "perceval"; @@ -303,11 +315,10 @@ public class IssueServiceTest { @Test public void should_create_manual_issue() { - grantAccess(); RuleKey ruleKey = RuleKey.of("manual", "manualRuleKey"); DefaultIssue manualIssue = new DefaultIssue().setKey("GHIJ").setRuleKey(RuleKey.of("manual", "manualRuleKey")).setComponentKey("org.sonar.Sample"); when(ruleFinder.findByKey(ruleKey)).thenReturn(Rule.create("manual", "manualRuleKey")); - when(resourceDao.findByKey("org.sonar.Sample")).thenReturn(mock(Component.class)); + when(resourceDao.getResource(any(ResourceQuery.class))).thenReturn(mock(ResourceDto.class)); Issue result = issueService.createManualIssue(manualIssue, userSession); assertThat(result).isNotNull(); @@ -318,9 +329,26 @@ public class IssueServiceTest { verify(authorizationDao).isAuthorizedComponentId(anyLong(), anyInt(), eq(UserRole.USER)); } + @Test + public void should_fail_create_manual_issue_if_not_having_required_role() { + RuleKey ruleKey = RuleKey.of("manual", "manualRuleKey"); + when(resourceDao.getResource(any(ResourceQuery.class))).thenReturn(mock(ResourceDto.class)); + when(ruleFinder.findByKey(ruleKey)).thenReturn(Rule.create("manual", "manualRuleKey")); + when(authorizationDao.isAuthorizedComponentId(anyLong(), eq(10), anyString())).thenReturn(false); + + DefaultIssue manualIssue = new DefaultIssue().setKey("GHIJ").setRuleKey(ruleKey).setComponentKey("org.sonar.Sample"); + try { + issueService.createManualIssue(manualIssue, userSession); + fail(); + } catch (Exception e) { + assertThat(e).isInstanceOf(IllegalStateException.class).hasMessage("User does not have the required role"); + } + } + @Test public void should_fail_create_manual_issue_if_not_manual_rule() { - grantAccess(); + when(resourceDao.getResource(any(ResourceQuery.class))).thenReturn(mock(ResourceDto.class)); + RuleKey ruleKey = RuleKey.of("squid", "s100"); DefaultIssue manualIssue = new DefaultIssue().setKey("GHIJ").setRuleKey(ruleKey).setComponentKey("org.sonar.Sample"); try { @@ -329,13 +357,13 @@ public class IssueServiceTest { } catch (Exception e) { assertThat(e).isInstanceOf(IllegalArgumentException.class).hasMessage("Issues can be created only on rules marked as 'manual': squid:s100"); } - verifyZeroInteractions(issueStorage); } @Test public void should_fail_create_manual_issue_if_rule_not_found() { - grantAccess(); + when(resourceDao.getResource(any(ResourceQuery.class))).thenReturn(mock(ResourceDto.class)); + RuleKey ruleKey = RuleKey.of("manual", "manualRuleKey"); DefaultIssue manualIssue = new DefaultIssue().setKey("GHIJ").setRuleKey(RuleKey.of("manual", "manualRuleKey")).setComponentKey("org.sonar.Sample"); when(ruleFinder.findByKey(ruleKey)).thenReturn(null); @@ -345,72 +373,23 @@ public class IssueServiceTest { } catch (Exception e) { assertThat(e).isInstanceOf(IllegalArgumentException.class).hasMessage("Unknown rule: manual:manualRuleKey"); } - verifyZeroInteractions(issueStorage); } @Test public void should_fail_create_manual_issue_if_component_not_found() { - grantAccess(); RuleKey ruleKey = RuleKey.of("manual", "manualRuleKey"); DefaultIssue manualIssue = new DefaultIssue().setKey("GHIJ").setRuleKey(RuleKey.of("manual", "manualRuleKey")).setComponentKey("org.sonar.Sample"); when(ruleFinder.findByKey(ruleKey)).thenReturn(Rule.create("manual", "manualRuleKey")); - when(resourceDao.findByKey("org.sonar.Sample")).thenReturn(null); + when(resourceDao.getResource(any(ResourceQuery.class))).thenReturn(null); try { issueService.createManualIssue(manualIssue, userSession); fail(); } catch (Exception e) { assertThat(e).isInstanceOf(IllegalArgumentException.class).hasMessage("Unknown component: org.sonar.Sample"); } - verifyZeroInteractions(issueStorage); } - @Test - public void should_fail_if_not_logged() { - when(userSession.isLoggedIn()).thenReturn(false); - try { - issueService.checkAuthorization(userSession, issue, UserRole.USER); - fail(); - } catch (Exception e) { - assertThat(e).isInstanceOf(IllegalStateException.class).hasMessage("User is not logged in"); - } - verifyZeroInteractions(authorizationDao); - } - - @Test - public void should_fail_if_not_having_required_role() { - grantAccess(); - when(userSession.isLoggedIn()).thenReturn(true); - when(authorizationDao.isAuthorizedComponentId(anyLong(), anyInt(), anyString())).thenReturn(false); - try { - issueService.checkAuthorization(userSession, issue, UserRole.USER); - fail(); - } catch (Exception e) { - assertThat(e).isInstanceOf(IllegalStateException.class).hasMessage("User does not have the required role"); - } - } - - @Test - public void should_find_project() { - ResourceDto project = new ResourceDto().setKey("org.sonar.Sample").setId(1l); - when(resourceDao.getRootProjectByComponentKey(anyString())).thenReturn(project); - assertThat(issueService.findRootProject("org.sonar.Sample")).isEqualTo(project); - } - - @Test - public void should_fail_to_find_project() { - when(resourceDao.getRootProjectByComponentKey(anyString())).thenReturn(null); - try { - issueService.findRootProject("org.sonar.Sample"); - fail(); - } catch (Exception e) { - assertThat(e).isInstanceOf(IllegalArgumentException.class).hasMessage("Component 'org.sonar.Sample' does not exists."); - } - } - - private void grantAccess(){ - when(resourceDao.getRootProjectByComponentKey(anyString())).thenReturn(new ResourceDto().setKey("org.sonar.Sample").setId(1l)); - } } -- 2.39.5