From: Teryk Bellahsene Date: Mon, 7 Sep 2015 08:46:59 +0000 (+0200) Subject: Clean PermissionService of all unused code X-Git-Tag: 5.2-RC1~235 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=01d571eefe7004ff251f4472bd904fb1f504ca0d;p=sonarqube.git Clean PermissionService of all unused code --- diff --git a/server/sonar-server/src/main/java/org/sonar/server/permission/ApplyPermissionTemplateQuery.java b/server/sonar-server/src/main/java/org/sonar/server/permission/ApplyPermissionTemplateQuery.java index 2ff9b702368..e530161c1a8 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/permission/ApplyPermissionTemplateQuery.java +++ b/server/sonar-server/src/main/java/org/sonar/server/permission/ApplyPermissionTemplateQuery.java @@ -21,17 +21,12 @@ package org.sonar.server.permission; import java.util.List; -import java.util.Map; import org.sonar.server.exceptions.BadRequestException; -import org.sonar.server.util.RubyUtils; import static com.google.common.base.CharMatcher.WHITESPACE; public class ApplyPermissionTemplateQuery { - private static final String TEMPLATE_KEY = "template_key"; - private static final String COMPONENTS_KEY = "components"; - private final String templateUuid; private List componentKeys; @@ -41,12 +36,6 @@ public class ApplyPermissionTemplateQuery { validate(); } - public static ApplyPermissionTemplateQuery createFromMap(Map params) { - String templateUuid = (String) params.get(TEMPLATE_KEY); - List componentKeys = RubyUtils.toStrings(params.get(COMPONENTS_KEY)); - return new ApplyPermissionTemplateQuery(templateUuid, componentKeys); - } - public static ApplyPermissionTemplateQuery create(String templateUuid, List componentKeys) { return new ApplyPermissionTemplateQuery(templateUuid, componentKeys); } diff --git a/server/sonar-server/src/main/java/org/sonar/server/permission/PermissionService.java b/server/sonar-server/src/main/java/org/sonar/server/permission/PermissionService.java index ba3fca56e4e..c48422cc5ee 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/permission/PermissionService.java +++ b/server/sonar-server/src/main/java/org/sonar/server/permission/PermissionService.java @@ -21,10 +21,7 @@ package org.sonar.server.permission; import java.util.List; -import java.util.Map; -import javax.annotation.CheckForNull; import javax.annotation.Nullable; -import org.sonar.api.security.DefaultGroups; import org.sonar.api.server.ServerSide; import org.sonar.api.web.UserRole; import org.sonar.core.permission.GlobalPermissions; @@ -33,10 +30,7 @@ import org.sonar.db.DbSession; import org.sonar.db.component.ComponentDto; import org.sonar.db.component.ResourceDto; import org.sonar.db.permission.PermissionRepository; -import org.sonar.db.user.GroupDto; -import org.sonar.db.user.UserDto; import org.sonar.server.component.ComponentFinder; -import org.sonar.server.exceptions.BadRequestException; import org.sonar.server.exceptions.ForbiddenException; import org.sonar.server.issue.index.IssueAuthorizationIndexer; import org.sonar.server.user.UserSession; @@ -44,32 +38,19 @@ import org.sonar.server.user.UserSession; import static org.sonar.server.permission.PermissionPrivilegeChecker.checkGlobalAdminUser; import static org.sonar.server.permission.PermissionPrivilegeChecker.checkProjectAdminUserByComponentKey; -/** - * Used by ruby code 
Internal.permissions
- */ @ServerSide public class PermissionService { - private enum Operation { - ADD, REMOVE; - } - - private static final String OBJECT_TYPE_USER = "User"; - private static final String OBJECT_TYPE_GROUP = "Group"; - private static final String NOT_FOUND_FORMAT = "%s %s does not exist"; - private final DbClient dbClient; private final PermissionRepository permissionRepository; - private final PermissionFinder finder; private final IssueAuthorizationIndexer issueAuthorizationIndexer; private final UserSession userSession; private final ComponentFinder componentFinder; - public PermissionService(DbClient dbClient, PermissionRepository permissionRepository, PermissionFinder finder, - IssueAuthorizationIndexer issueAuthorizationIndexer, UserSession userSession, ComponentFinder componentFinder) { + public PermissionService(DbClient dbClient, PermissionRepository permissionRepository, IssueAuthorizationIndexer issueAuthorizationIndexer, UserSession userSession, + ComponentFinder componentFinder) { this.dbClient = dbClient; this.permissionRepository = permissionRepository; - this.finder = finder; this.issueAuthorizationIndexer = issueAuthorizationIndexer; this.userSession = userSession; this.componentFinder = componentFinder; @@ -79,73 +60,6 @@ public class PermissionService { return GlobalPermissions.ALL; } - public UserWithPermissionQueryResult findUsersWithPermission(Map params) { - return finder.findUsersWithPermission(PermissionQueryParser.toQuery(params)); - } - - public UserWithPermissionQueryResult findUsersWithPermissionTemplate(Map params) { - return finder.findUsersWithPermissionTemplate(PermissionQueryParser.toQuery(params)); - } - - public GroupWithPermissionQueryResult findGroupsWithPermission(Map params) { - return finder.findGroupsWithPermission(PermissionQueryParser.toQuery(params)); - } - - /** - * To be used only by jruby webapp - */ - public void addPermission(Map params) { - PermissionChange change = PermissionChange.buildFromParams(params); - DbSession session = dbClient.openSession(false); - try { - applyChange(Operation.ADD, change, session); - } finally { - dbClient.closeSession(session); - } - } - - /** - * @deprecated since 5.2 use PermissionUpdate.addPermission instead - */ - @Deprecated - public void addPermission(PermissionChange change) { - DbSession session = dbClient.openSession(false); - try { - applyChange(Operation.ADD, change, session); - } finally { - dbClient.closeSession(session); - } - } - - /** - * To be used only by jruby webapp - */ - public void removePermission(Map params) { - PermissionChange change = PermissionChange.buildFromParams(params); - DbSession session = dbClient.openSession(false); - try { - applyChange(Operation.REMOVE, change, session); - } finally { - session.close(); - } - } - - /** - * @deprecated since 5.2. Use PermissionUpdater.removePermission - */ - @Deprecated - public void removePermission(PermissionChange change) { - DbSession session = dbClient.openSession(false); - try { - applyChange(Operation.REMOVE, change, session); - } finally { - session.close(); - } - } - - /** - * Important - this method checks caller permissions - */ public void applyDefaultPermissionTemplate(final String componentKey) { DbSession session = dbClient.openSession(false); try { @@ -164,15 +78,6 @@ public class PermissionService { indexProjectPermissions(); } - /** - * @deprecated since 5.2 – to be deleted once Permission Template page does not rely on Ruby - */ - @Deprecated - public void applyPermissionTemplate(Map params) { - ApplyPermissionTemplateQuery query = ApplyPermissionTemplateQuery.createFromMap(params); - applyPermissionTemplate(query); - } - /** * Important - this method checks caller permissions */ @@ -197,123 +102,6 @@ public class PermissionService { indexProjectPermissions(); } - private void applyChange(Operation operation, PermissionChange change, DbSession session) { - userSession.checkLoggedIn(); - change.validate(); - boolean changed; - if (change.userLogin() != null) { - changed = applyChangeOnUser(session, operation, change); - } else { - changed = applyChangeOnGroup(session, operation, change); - } - if (changed) { - session.commit(); - if (change.componentKey() != null) { - indexProjectPermissions(); - } - } - } - - private boolean applyChangeOnGroup(DbSession session, Operation operation, PermissionChange permissionChange) { - Long componentId = getComponentId(session, permissionChange.componentKey()); - checkProjectAdminPermission(permissionChange.componentKey()); - - List existingPermissions = dbClient.roleDao().selectGroupPermissions(session, permissionChange.groupName(), componentId); - if (shouldSkipPermissionChange(operation, existingPermissions, permissionChange)) { - return false; - } - - Long targetedGroup = getTargetedGroup(session, permissionChange.groupName()); - String permission = permissionChange.permission(); - if (Operation.ADD == operation) { - checkNotAnyoneAndAdmin(permission, permissionChange.groupName()); - permissionRepository.insertGroupPermission(componentId, targetedGroup, permission, session); - } else { - checkAdminUsersExistOutsideTheRemovedGroup(session, permissionChange, targetedGroup); - permissionRepository.deleteGroupPermission(componentId, targetedGroup, permission, session); - } - return true; - } - - private static void checkNotAnyoneAndAdmin(String permission, String group) { - if (GlobalPermissions.SYSTEM_ADMIN.equals(permission) - && DefaultGroups.isAnyone(group)) { - throw new BadRequestException(String.format("It is not possible to add the '%s' permission to the '%s' group.", permission, group)); - } - } - - private boolean applyChangeOnUser(DbSession session, Operation operation, PermissionChange permissionChange) { - Long componentId = getComponentId(session, permissionChange.componentKey()); - checkProjectAdminPermission(permissionChange.componentKey()); - - List existingPermissions = dbClient.roleDao().selectUserPermissions(session, permissionChange.userLogin(), componentId); - if (shouldSkipPermissionChange(operation, existingPermissions, permissionChange)) { - return false; - } - - Long targetedUser = getTargetedUser(session, permissionChange.userLogin()); - if (Operation.ADD == operation) { - permissionRepository.insertUserPermission(componentId, targetedUser, permissionChange.permission(), session); - } else { - checkOtherAdminUsersExist(session, permissionChange); - permissionRepository.deleteUserPermission(componentId, targetedUser, permissionChange.permission(), session); - } - return true; - - } - - private void checkOtherAdminUsersExist(DbSession session, PermissionChange permissionChange) { - if (GlobalPermissions.SYSTEM_ADMIN.equals(permissionChange.permission()) - && dbClient.roleDao().countUserPermissions(session, permissionChange.permission(), null) <= 1) { - throw new BadRequestException(String.format("Last user with '%s' permission. Permission cannot be removed.", GlobalPermissions.SYSTEM_ADMIN)); - } - } - - private void checkAdminUsersExistOutsideTheRemovedGroup(DbSession session, PermissionChange permissionChange, @Nullable Long groupIdToExclude) { - if (GlobalPermissions.SYSTEM_ADMIN.equals(permissionChange.permission()) - && groupIdToExclude != null - && dbClient.roleDao().countUserPermissions(session, permissionChange.permission(), groupIdToExclude) <= 0) { - throw new BadRequestException(String.format("Last group with '%s' permission. Permission cannot be removed.", GlobalPermissions.SYSTEM_ADMIN)); - } - } - - private Long getTargetedUser(DbSession session, String userLogin) { - UserDto user = dbClient.userDao().selectActiveUserByLogin(session, userLogin); - return badRequestIfNullResult(user, OBJECT_TYPE_USER, userLogin).getId(); - } - - @Nullable - private Long getTargetedGroup(DbSession session, String group) { - if (DefaultGroups.isAnyone(group)) { - return null; - } else { - GroupDto groupDto = dbClient.groupDao().selectByName(session, group); - return badRequestIfNullResult(groupDto, OBJECT_TYPE_GROUP, group).getId(); - } - } - - private boolean shouldSkipPermissionChange(Operation operation, List existingPermissions, PermissionChange permissionChange) { - return (Operation.ADD == operation && existingPermissions.contains(permissionChange.permission())) || - (Operation.REMOVE == operation && !existingPermissions.contains(permissionChange.permission())); - } - - @CheckForNull - private Long getComponentId(DbSession session, @Nullable String componentKey) { - if (componentKey == null) { - return null; - } else { - ComponentDto component = componentFinder.getByKey(session, componentKey); - return component.getId(); - } - } - - private static T badRequestIfNullResult(@Nullable T component, String objectType, String objectKey) { - if (component == null) { - throw new BadRequestException(String.format(NOT_FOUND_FORMAT, objectType, objectKey)); - } - return component; - } - private void checkProjectAdminPermission(@Nullable String projectKey) { if (projectKey == null) { userSession.checkGlobalPermission(GlobalPermissions.SYSTEM_ADMIN); diff --git a/server/sonar-server/src/test/java/org/sonar/server/permission/ApplyPermissionTemplateQueryTest.java b/server/sonar-server/src/test/java/org/sonar/server/permission/ApplyPermissionTemplateQueryTest.java index 840fd55d86f..33897ce357f 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/permission/ApplyPermissionTemplateQueryTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/permission/ApplyPermissionTemplateQueryTest.java @@ -20,30 +20,24 @@ package org.sonar.server.permission; -import com.google.common.collect.Lists; -import com.google.common.collect.Maps; import java.util.Collections; -import java.util.Map; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; import org.sonar.server.exceptions.BadRequestException; +import static com.google.common.collect.Lists.newArrayList; import static org.assertj.core.api.Assertions.assertThat; +import static org.sonar.server.permission.ApplyPermissionTemplateQuery.create; public class ApplyPermissionTemplateQueryTest { @Rule - public ExpectedException throwable = ExpectedException.none(); + public ExpectedException expectedException = ExpectedException.none(); @Test public void should_populate_with_params() { - - Map params = Maps.newHashMap(); - params.put("template_key", "my_template_key"); - params.put("components", Lists.newArrayList("1", "2", "3")); - - ApplyPermissionTemplateQuery query = ApplyPermissionTemplateQuery.createFromMap(params); + ApplyPermissionTemplateQuery query = create("my_template_key", newArrayList("1", "2", "3")); assertThat(query.getTemplateUuid()).isEqualTo("my_template_key"); assertThat(query.getComponentKeys()).containsOnly("1", "2", "3"); @@ -51,27 +45,17 @@ public class ApplyPermissionTemplateQueryTest { @Test public void should_invalidate_query_with_empty_name() { + expectedException.expect(BadRequestException.class); + expectedException.expectMessage("Permission template is mandatory"); - throwable.expect(BadRequestException.class); - throwable.expectMessage("Permission template is mandatory"); - - Map params = Maps.newHashMap(); - params.put("template_key", ""); - params.put("components", Lists.newArrayList("1", "2", "3")); - - ApplyPermissionTemplateQuery.createFromMap(params); + ApplyPermissionTemplateQuery.create("", newArrayList("1", "2", "3")); } @Test public void should_invalidate_query_with_no_components() { + expectedException.expect(BadRequestException.class); + expectedException.expectMessage("No project provided. Please provide at least one project."); - throwable.expect(BadRequestException.class); - throwable.expectMessage("No project provided. Please provide at least one project."); - - Map params = Maps.newHashMap(); - params.put("template_key", "my_template_key"); - params.put("components", Collections.EMPTY_LIST); - - ApplyPermissionTemplateQuery.createFromMap(params); + ApplyPermissionTemplateQuery.create("my_template_key", Collections.emptyList()); } } diff --git a/server/sonar-server/src/test/java/org/sonar/server/permission/PermissionServiceMediumTest.java b/server/sonar-server/src/test/java/org/sonar/server/permission/PermissionServiceMediumTest.java deleted file mode 100644 index 8edafeacec5..00000000000 --- a/server/sonar-server/src/test/java/org/sonar/server/permission/PermissionServiceMediumTest.java +++ /dev/null @@ -1,198 +0,0 @@ -/* - * SonarQube, open source software quality management tool. - * Copyright (C) 2008-2014 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.permission; - -import com.google.common.collect.Maps; -import java.util.Collection; -import java.util.Map; -import javax.annotation.Nullable; -import org.elasticsearch.action.search.SearchResponse; -import org.elasticsearch.search.SearchHit; -import org.junit.After; -import org.junit.Before; -import org.junit.ClassRule; -import org.junit.Rule; -import org.junit.Test; -import org.sonar.api.web.UserRole; -import org.sonar.db.DbSession; -import org.sonar.db.component.ComponentDto; -import org.sonar.db.user.GroupDto; -import org.sonar.db.user.RoleDao; -import org.sonar.db.user.UserDto; -import org.sonar.db.component.ComponentTesting; -import org.sonar.server.db.DbClient; -import org.sonar.server.es.EsClient; -import org.sonar.server.issue.index.IssueIndexDefinition; -import org.sonar.server.tester.ServerTester; -import org.sonar.server.tester.UserSessionRule; - -import static org.assertj.core.api.Assertions.assertThat; - -/** - * New tests should be added in order to be able to remove PermissionServiceTest - */ -public class PermissionServiceMediumTest { - - @ClassRule - public static ServerTester tester = new ServerTester().withStartupTasks().withEsIndexes(); - @Rule - public UserSessionRule userSessionRule = UserSessionRule.forServerTester(tester); - - DbClient db; - DbSession session; - PermissionService underTest; - - ComponentDto project; - - @Before - public void setUp() { - tester.clearDbAndIndexes(); - db = tester.get(DbClient.class); - session = db.openSession(false); - underTest = tester.get(PermissionService.class); - - project = ComponentTesting.newProjectDto(); - db.componentDao().insert(session, project); - session.commit(); - } - - @After - public void after() { - session.close(); - } - - @Test - public void add_project_permission_to_user() { - // init - userSessionRule.login("admin").addProjectPermissions(UserRole.ADMIN, project.key()); - UserDto user = new UserDto().setLogin("john").setName("John"); - db.userDao().insert(session, user); - session.commit(); - assertThat(tester.get(RoleDao.class).selectUserPermissions(session, user.getLogin(), project.getId())).isEmpty(); - assertThat(countIssueAuthorizationDocs()).isZero(); - - // add permission - underTest.addPermission(params(user.getLogin(), null, project.key(), UserRole.USER)); - session.commit(); - - // Check db - assertThat(tester.get(RoleDao.class).selectUserPermissions(session, user.getLogin(), project.getId())).hasSize(1); - - // Check index of issue authorizations - assertThat(countIssueAuthorizationDocs()).isEqualTo(1); - } - - @Test - public void remove_project_permission_to_user() { - userSessionRule.login("admin").addProjectPermissions(UserRole.ADMIN, project.key()); - - UserDto user1 = new UserDto().setLogin("user1").setName("User1"); - db.userDao().insert(session, user1); - - UserDto user2 = new UserDto().setLogin("user2").setName("User2"); - db.userDao().insert(session, user2); - session.commit(); - - underTest.addPermission(params(user1.getLogin(), null, project.key(), UserRole.USER)); - underTest.addPermission(params(user2.getLogin(), null, project.key(), UserRole.USER)); - underTest.removePermission(params(user1.getLogin(), null, project.key(), UserRole.USER)); - session.commit(); - - // Check in db - assertThat(tester.get(RoleDao.class).selectUserPermissions(session, user1.getLogin(), project.getId())).isEmpty(); - assertThat(tester.get(RoleDao.class).selectUserPermissions(session, user2.getLogin(), project.getId())).hasSize(1); - - // Check index of issue authorizations - assertThat(countIssueAuthorizationDocs()).isEqualTo(1); - } - - @Test - public void remove_all_component_user_permissions() { - userSessionRule.login("admin").addProjectPermissions(UserRole.ADMIN, project.key()); - - UserDto user = new UserDto().setLogin("user1").setName("User1"); - db.userDao().insert(session, user); - session.commit(); - - underTest.addPermission(params(user.getLogin(), null, project.key(), UserRole.USER)); - underTest.removePermission(params(user.getLogin(), null, project.key(), UserRole.USER)); - session.commit(); - - // Check in db - assertThat(tester.get(RoleDao.class).selectUserPermissions(session, user.getLogin(), project.getId())).isEmpty(); - - // Check index of issue authorizations - SearchResponse docs = getAllIndexDocs(); - assertThat(docs.getHits().getTotalHits()).isEqualTo(1L); - SearchHit doc = docs.getHits().getAt(0); - assertThat((Collection) doc.sourceAsMap().get(IssueIndexDefinition.FIELD_AUTHORIZATION_USERS)).hasSize(0); - assertThat((Collection) doc.sourceAsMap().get(IssueIndexDefinition.FIELD_AUTHORIZATION_GROUPS)).hasSize(0); - } - - private SearchResponse getAllIndexDocs() { - return tester.get(EsClient.class).prepareSearch(IssueIndexDefinition.INDEX).setTypes(IssueIndexDefinition.TYPE_AUTHORIZATION).get(); - } - - @Test - public void add_and_remove_permission_to_group() { - // init - userSessionRule.login("admin").addProjectPermissions(UserRole.ADMIN, project.key()); - GroupDto group = new GroupDto().setName("group1"); - db.groupDao().insert(session, group); - session.commit(); - assertThat(tester.get(RoleDao.class).selectGroupPermissions(session, group.getName(), project.getId())).isEmpty(); - - // add permission - PermissionChange change = new PermissionChange().setPermission(UserRole.USER).setGroupName(group.getName()).setComponentKey(project.key()); - underTest.addPermission(change); - session.commit(); - - // Check db - assertThat(tester.get(RoleDao.class).selectGroupPermissions(session, group.getName(), project.getId())).hasSize(1); - - // Check index of issue authorizations - assertThat(countIssueAuthorizationDocs()).isEqualTo(1); - - // remove permission - underTest.removePermission(change); - session.commit(); - assertThat(tester.get(RoleDao.class).selectGroupPermissions(session, group.getName(), project.getId())).hasSize(0); - - SearchResponse docs = getAllIndexDocs(); - assertThat(docs.getHits().getTotalHits()).isEqualTo(1L); - SearchHit doc = docs.getHits().getAt(0); - assertThat((Collection) doc.sourceAsMap().get(IssueIndexDefinition.FIELD_AUTHORIZATION_USERS)).hasSize(0); - assertThat((Collection) doc.sourceAsMap().get(IssueIndexDefinition.FIELD_AUTHORIZATION_GROUPS)).hasSize(0); - } - - private Map params(@Nullable String login, @Nullable String group, @Nullable String component, String permission) { - Map params = Maps.newHashMap(); - params.put("user", login); - params.put("group", group); - params.put("component", component); - params.put("permission", permission); - return params; - } - - private long countIssueAuthorizationDocs() { - return tester.get(EsClient.class).prepareCount(IssueIndexDefinition.INDEX).setTypes(IssueIndexDefinition.TYPE_AUTHORIZATION).get().getCount(); - } -} diff --git a/server/sonar-server/src/test/java/org/sonar/server/permission/ws/ApplyTemplateActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/permission/ws/ApplyTemplateActionTest.java index f9f63265fd5..a56ae1e7c63 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/permission/ws/ApplyTemplateActionTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/permission/ws/ApplyTemplateActionTest.java @@ -105,7 +105,7 @@ public class ApplyTemplateActionTest { PermissionRepository repository = new PermissionRepository(dbClient, new Settings()); PermissionFinder permissionFinder = new PermissionFinder(dbClient); ComponentFinder componentFinder = new ComponentFinder(dbClient); - PermissionService permissionService = new PermissionService(dbClient, repository, permissionFinder, issueAuthorizationIndexer, userSession, componentFinder); + PermissionService permissionService = new PermissionService(dbClient, repository, issueAuthorizationIndexer, userSession, componentFinder); PermissionDependenciesFinder permissionDependenciesFinder = new PermissionDependenciesFinder(dbClient, componentFinder); ApplyTemplateAction underTest = new ApplyTemplateAction(dbClient, permissionService, permissionDependenciesFinder);