From 20b4fdd2b25c1f96971cf02d99811c0cda45dcf4 Mon Sep 17 00:00:00 2001 From: Julien Lancelot Date: Thu, 6 Jun 2013 12:04:47 +0200 Subject: [PATCH] SONAR-3755 Removed useless security check --- .../core/issue/DefaultIssueQueryResult.java | 2 +- .../org/sonar/server/issue/ActionService.java | 16 ++++++--------- .../issue/InternalRubyIssueService.java | 4 ++-- .../org/sonar/server/issue/IssueService.java | 13 +++++------- .../sonar/server/issue/ActionServiceTest.java | 4 ++-- .../sonar/server/issue/IssueServiceTest.java | 20 ++----------------- 6 files changed, 18 insertions(+), 41 deletions(-) diff --git a/sonar-core/src/main/java/org/sonar/core/issue/DefaultIssueQueryResult.java b/sonar-core/src/main/java/org/sonar/core/issue/DefaultIssueQueryResult.java index ca92f7e8d29..4a7822c75c9 100644 --- a/sonar-core/src/main/java/org/sonar/core/issue/DefaultIssueQueryResult.java +++ b/sonar-core/src/main/java/org/sonar/core/issue/DefaultIssueQueryResult.java @@ -107,7 +107,7 @@ public class DefaultIssueQueryResult implements IssueQueryResult { if (issues != null && !issues.isEmpty()) { return issues.get(0); } - throw new IllegalStateException("No issue"); + throw new IllegalStateException("No issue found"); } @Override diff --git a/sonar-server/src/main/java/org/sonar/server/issue/ActionService.java b/sonar-server/src/main/java/org/sonar/server/issue/ActionService.java index 3e55485410d..48df6ff62d7 100644 --- a/sonar-server/src/main/java/org/sonar/server/issue/ActionService.java +++ b/sonar-server/src/main/java/org/sonar/server/issue/ActionService.java @@ -68,6 +68,12 @@ public class ActionService implements ServerComponent { this.actions = actions; } + public List listAvailableActions(String issueKey) { + IssueQueryResult queryResult = loadIssue(issueKey); + final DefaultIssue issue = (DefaultIssue) queryResult.first(); + return listAvailableActions(issue); + } + public List listAvailableActions(final Issue issue) { return newArrayList(Iterables.filter(actions.list(), new Predicate() { @Override @@ -77,21 +83,11 @@ public class ActionService implements ServerComponent { })); } - public List listAvailableActions(String issueKey) { - IssueQueryResult queryResult = loadIssue(issueKey); - final DefaultIssue issue = (DefaultIssue) queryResult.first(); - return listAvailableActions(issue); - } - public Issue execute(String issueKey, String actionKey, UserSession userSession) { Preconditions.checkArgument(!Strings.isNullOrEmpty(actionKey), "Missing action"); IssueQueryResult queryResult = loadIssue(issueKey); DefaultIssue issue = (DefaultIssue) queryResult.first(); - if (issue == null) { - throw new IllegalArgumentException("Issue is not found : " + issueKey); - } - Action action = getAction(actionKey); if (action == null) { throw new IllegalArgumentException("Action is not found : " + actionKey); 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 321506a0528..87533a774ab 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 @@ -81,11 +81,11 @@ public class InternalRubyIssueService implements ServerComponent { } public List listTransitions(String issueKey) { - return issueService.listTransitions(issueKey, UserSession.get()); + return issueService.listTransitions(issueKey); } public List listTransitions(Issue issue) { - return issueService.listTransitions(issue, UserSession.get()); + return issueService.listTransitions(issue); } public List listStatus() { 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 14ade9f87e4..331bc0ebdc4 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 @@ -92,25 +92,25 @@ public class IssueService implements ServerComponent { *

* Never return null, but return an empty list if the issue does not exist. */ - public List listTransitions(String issueKey, UserSession userSession) { - return listTransitions(loadIssue(issueKey).first(), userSession); + public List listTransitions(String issueKey) { + Issue issue = loadIssue(issueKey).first(); + return listTransitions(issue); } /** * Never return null, but an empty list if the issue does not exist. + * No security check is done since it should already have been done to get the issue */ - public List listTransitions(@Nullable Issue issue, UserSession userSession) { + public List listTransitions(@Nullable Issue issue) { if (issue == null) { return Collections.emptyList(); } - checkAuthorization(userSession, issue, UserRole.USER); return workflow.outTransitions(issue); } public Issue doTransition(String issueKey, String transition, UserSession userSession) { IssueQueryResult queryResult = loadIssue(issueKey); DefaultIssue issue = (DefaultIssue) queryResult.first(); - checkAuthorization(userSession, issue, UserRole.USER); IssueChangeContext context = IssueChangeContext.createUser(new Date(), userSession.login()); if (workflow.doTransition(issue, transition, context)) { issueStorage.save(issue); @@ -122,7 +122,6 @@ public class IssueService implements ServerComponent { public Issue assign(String issueKey, @Nullable String assignee, UserSession userSession) { IssueQueryResult queryResult = loadIssue(issueKey); DefaultIssue issue = (DefaultIssue) queryResult.first(); - checkAuthorization(userSession, issue, UserRole.USER); if (assignee != null && userFinder.findByLogin(assignee) == null) { throw new IllegalArgumentException("Unknown user: " + assignee); } @@ -140,7 +139,6 @@ public class IssueService implements ServerComponent { } IssueQueryResult queryResult = loadIssue(issueKey); DefaultIssue issue = (DefaultIssue) queryResult.first(); - checkAuthorization(userSession, issue, UserRole.USER); IssueChangeContext context = IssueChangeContext.createUser(new Date(), userSession.login()); if (issueUpdater.plan(issue, actionPlanKey, context)) { @@ -153,7 +151,6 @@ public class IssueService implements ServerComponent { public Issue setSeverity(String issueKey, String severity, UserSession userSession) { IssueQueryResult queryResult = loadIssue(issueKey); DefaultIssue issue = (DefaultIssue) queryResult.first(); - checkAuthorization(userSession, issue, UserRole.USER); IssueChangeContext context = IssueChangeContext.createUser(new Date(), userSession.login()); if (issueUpdater.setManualSeverity(issue, severity, context)) { diff --git a/sonar-server/src/test/java/org/sonar/server/issue/ActionServiceTest.java b/sonar-server/src/test/java/org/sonar/server/issue/ActionServiceTest.java index 1e4831301ad..9624fe12cfd 100644 --- a/sonar-server/src/test/java/org/sonar/server/issue/ActionServiceTest.java +++ b/sonar-server/src/test/java/org/sonar/server/issue/ActionServiceTest.java @@ -124,7 +124,7 @@ public class ActionServiceTest { actionService.execute("ABCD", "link-to-jira", mock(UserSession.class)); fail(); } catch (Exception e) { - assertThat(e).isInstanceOf(IllegalStateException.class).hasMessage("No issue"); + assertThat(e).isInstanceOf(IllegalStateException.class).hasMessage("No issue found"); } verifyZeroInteractions(function); } @@ -187,7 +187,7 @@ public class ActionServiceTest { actionService.listAvailableActions("ABCD"); fail(); } catch (Exception e) { - assertThat(e).isInstanceOf(IllegalStateException.class).hasMessage("No issue"); + assertThat(e).isInstanceOf(IllegalStateException.class).hasMessage("No issue found"); } } 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 bfeec5cbfc1..b529a572902 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 @@ -118,29 +118,25 @@ public class IssueServiceTest { @Test public void should_list_transitions() { - grantAccess(); List transitions = newArrayList(transition); when(workflow.outTransitions(issue)).thenReturn(transitions); - List result = issueService.listTransitions("ABCD", userSession); + List result = issueService.listTransitions("ABCD"); assertThat(result).hasSize(1); assertThat(result.get(0)).isEqualTo(transition); - verify(authorizationDao).isAuthorizedComponentId(anyLong(), anyInt(), eq(UserRole.USER)); } @Test public void should_return_no_transition() { - grantAccess(); when(issueQueryResult.first()).thenReturn(null); when(issueQueryResult.issues()).thenReturn(newArrayList((Issue) new DefaultIssue())); - assertThat(issueService.listTransitions("ABCD", userSession)).isEmpty(); + assertThat(issueService.listTransitions("ABCD")).isEmpty(); verifyZeroInteractions(workflow); } @Test public void should_do_transition() { - grantAccess(); when(workflow.doTransition(eq(issue), eq(transition.key()), any(IssueChangeContext.class))).thenReturn(true); Issue result = issueService.doTransition("ABCD", transition.key(), userSession); @@ -155,7 +151,6 @@ public class IssueServiceTest { assertThat(issueChangeContext.date()).isNotNull(); verify(issueNotifications).sendChanges(eq(issue), eq(issueChangeContext), eq(issueQueryResult)); - verify(authorizationDao).isAuthorizedComponentId(anyLong(), anyInt(), eq(UserRole.USER)); } @Test @@ -172,7 +167,6 @@ public class IssueServiceTest { @Test public void should_assign() { - grantAccess(); String assignee = "perceval"; when(userFinder.findByLogin(assignee)).thenReturn(new DefaultUser()); @@ -190,12 +184,10 @@ public class IssueServiceTest { assertThat(issueChangeContext.date()).isNotNull(); verify(issueNotifications).sendChanges(eq(issue), eq(issueChangeContext), eq(issueQueryResult)); - verify(authorizationDao).isAuthorizedComponentId(anyLong(), anyInt(), eq(UserRole.USER)); } @Test public void should_not_assign() { - grantAccess(); String assignee = "perceval"; when(userFinder.findByLogin(assignee)).thenReturn(new DefaultUser()); @@ -210,7 +202,6 @@ public class IssueServiceTest { @Test public void should_fail_assign_if_assignee_not_found() { - grantAccess(); String assignee = "perceval"; when(userFinder.findByLogin(assignee)).thenReturn(null); @@ -229,7 +220,6 @@ public class IssueServiceTest { @Test public void should_plan() { - grantAccess(); String actionPlanKey = "EFGH"; when(actionPlanService.findByKey(actionPlanKey, userSession)).thenReturn(new DefaultActionPlan()); @@ -247,12 +237,10 @@ public class IssueServiceTest { assertThat(issueChangeContext.date()).isNotNull(); verify(issueNotifications).sendChanges(eq(issue), eq(issueChangeContext), eq(issueQueryResult)); - verify(authorizationDao).isAuthorizedComponentId(anyLong(), anyInt(), eq(UserRole.USER)); } @Test public void should_not_plan() { - grantAccess(); String actionPlanKey = "EFGH"; when(actionPlanService.findByKey(actionPlanKey, userSession)).thenReturn(new DefaultActionPlan()); @@ -267,7 +255,6 @@ public class IssueServiceTest { @Test public void should_fail_plan_if_action_plan_not_found() { - grantAccess(); String actionPlanKey = "EFGH"; when(actionPlanService.findByKey(actionPlanKey, userSession)).thenReturn(null); @@ -285,7 +272,6 @@ public class IssueServiceTest { @Test public void should_set_severity() { - grantAccess(); String severity = "MINOR"; when(issueUpdater.setManualSeverity(eq(issue), eq(severity), any(IssueChangeContext.class))).thenReturn(true); @@ -301,12 +287,10 @@ public class IssueServiceTest { assertThat(issueChangeContext.date()).isNotNull(); verify(issueNotifications).sendChanges(eq(issue), eq(issueChangeContext), eq(issueQueryResult)); - verify(authorizationDao).isAuthorizedComponentId(anyLong(), anyInt(), eq(UserRole.USER)); } @Test public void should_not_set_severity() { - grantAccess(); String severity = "MINOR"; when(issueUpdater.setManualSeverity(eq(issue), eq(severity), any(IssueChangeContext.class))).thenReturn(false); -- 2.39.5