diff options
3 files changed, 237 insertions, 19 deletions
diff --git a/server/sonar-server/src/main/java/org/sonar/server/permission/UserPermissionChanger.java b/server/sonar-server/src/main/java/org/sonar/server/permission/UserPermissionChanger.java index 3d0d4ac9787..29f065fa876 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/permission/UserPermissionChanger.java +++ b/server/sonar-server/src/main/java/org/sonar/server/permission/UserPermissionChanger.java @@ -21,11 +21,14 @@ package org.sonar.server.permission; import java.util.List; import java.util.Optional; +import org.sonar.core.permission.ProjectPermissions; import org.sonar.db.DbClient; import org.sonar.db.DbSession; import org.sonar.db.permission.UserPermissionDto; import static org.sonar.core.permission.GlobalPermissions.SYSTEM_ADMIN; +import static org.sonar.server.permission.PermissionChange.Operation.ADD; +import static org.sonar.server.permission.PermissionChange.Operation.REMOVE; import static org.sonar.server.ws.WsUtils.checkRequest; /** @@ -40,6 +43,10 @@ public class UserPermissionChanger { } public boolean apply(DbSession dbSession, UserPermissionChange change) { + ensureConsistencyWithVisibility(change); + if (isImplicitlyAlreadyDone(change)) { + return false; + } switch (change.getOperation()) { case ADD: return addPermission(dbSession, change); @@ -50,6 +57,35 @@ public class UserPermissionChanger { } } + private static boolean isImplicitlyAlreadyDone(UserPermissionChange change) { + return change.getProjectId() + .map(projectId -> isImplicitlyAlreadyDone(projectId, change)) + .orElse(false); + } + + private static boolean isImplicitlyAlreadyDone(ProjectId projectId, UserPermissionChange change) { + return isAttemptToAddPublicPermissionToPublicComponent(change, projectId); + } + + private static boolean isAttemptToAddPublicPermissionToPublicComponent(UserPermissionChange change, ProjectId projectId) { + return !projectId.isPrivate() + && change.getOperation() == ADD + && ProjectPermissions.PUBLIC_PERMISSIONS.contains(change.getPermission()); + } + + private static void ensureConsistencyWithVisibility(UserPermissionChange change) { + change.getProjectId() + .ifPresent(projectId -> checkRequest( + !isAttemptToRemovePublicPermissionFromPublicComponent(change, projectId), + "Permission %s can't be removed from a public component", change.getPermission())); + } + + private static boolean isAttemptToRemovePublicPermissionFromPublicComponent(UserPermissionChange change, ProjectId projectId) { + return !projectId.isPrivate() + && change.getOperation() == REMOVE + && ProjectPermissions.PUBLIC_PERMISSIONS.contains(change.getPermission()); + } + private boolean addPermission(DbSession dbSession, UserPermissionChange change) { if (loadExistingPermissions(dbSession, change).contains(change.getPermission())) { return false; diff --git a/server/sonar-server/src/test/java/org/sonar/server/permission/UserPermissionChangerTest.java b/server/sonar-server/src/test/java/org/sonar/server/permission/UserPermissionChangerTest.java index 0bafab157d1..1ec987c8abf 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/permission/UserPermissionChangerTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/permission/UserPermissionChangerTest.java @@ -24,14 +24,18 @@ import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; import org.sonar.api.utils.System2; +import org.sonar.core.permission.ProjectPermissions; import org.sonar.db.DbTester; import org.sonar.db.component.ComponentDto; import org.sonar.db.organization.OrganizationDto; +import org.sonar.db.permission.OrganizationPermission; import org.sonar.db.user.GroupDto; import org.sonar.db.user.UserDto; import org.sonar.server.exceptions.BadRequestException; import static org.assertj.core.api.Assertions.assertThat; +import static org.sonar.api.web.UserRole.ADMIN; +import static org.sonar.api.web.UserRole.CODEVIEWER; import static org.sonar.api.web.UserRole.ISSUE_ADMIN; import static org.sonar.api.web.UserRole.USER; import static org.sonar.core.permission.GlobalPermissions.QUALITY_GATE_ADMIN; @@ -43,7 +47,6 @@ import static org.sonar.db.permission.OrganizationPermission.SCAN; import static org.sonar.server.permission.PermissionChange.Operation.ADD; import static org.sonar.server.permission.PermissionChange.Operation.REMOVE; - public class UserPermissionChangerTest { @Rule public DbTester db = DbTester.create(System2.INSTANCE); @@ -56,7 +59,8 @@ public class UserPermissionChangerTest { private OrganizationDto org2; private UserDto user1; private UserDto user2; - private ComponentDto project; + private ComponentDto privateProject; + private ComponentDto publicProject; @Before public void setUp() throws Exception { @@ -64,30 +68,174 @@ public class UserPermissionChangerTest { org2 = db.organizations().insert(); user1 = db.users().insertUser(); user2 = db.users().insertUser(); - project = db.components().insertPrivateProject(org1); + privateProject = db.components().insertPrivateProject(org1); + publicProject = db.components().insertPublicProject(org1); + } + + @Test + public void apply_adds_any_organization_permission_to_user() { + OrganizationPermission.all() + .forEach(perm -> { + UserPermissionChange change = new UserPermissionChange(ADD, org1.getUuid(), perm.getKey(), null, UserId.from(user1)); + + apply(change); + + assertThat(db.users().selectPermissionsOfUser(user1, org1)).contains(perm); + }); + } + + @Test + public void apply_removes_any_organization_permission_to_user() { + // give ADMIN perm to user2 so that user1 is not the only one with this permission and it can be removed from user1 + db.users().insertPermissionOnUser(org1, user2, OrganizationPermission.ADMINISTER); + OrganizationPermission.all() + .forEach(perm -> db.users().insertPermissionOnUser(org1, user1, perm)); + assertThat(db.users().selectPermissionsOfUser(user1, org1)).containsOnly(OrganizationPermission.values()); + + OrganizationPermission.all() + .forEach(perm -> { + UserPermissionChange change = new UserPermissionChange(REMOVE, org1.getUuid(), perm.getKey(), null, UserId.from(user1)); + + apply(change); + + assertThat(db.users().selectPermissionsOfUser(user1, org1)).doesNotContain(perm); + }); + } + + @Test + public void apply_has_no_effect_when_adding_permission_USER_on_a_public_project() { + UserPermissionChange change = new UserPermissionChange(ADD, org1.getUuid(), USER, new ProjectId(publicProject), UserId.from(user1)); + + apply(change); + + assertThat(db.users().selectProjectPermissionsOfUser(user1, publicProject)).doesNotContain(USER); + } + + @Test + public void apply_has_no_effect_when_adding_permission_CODEVIEWER_on_a_public_project() { + UserPermissionChange change = new UserPermissionChange(ADD, org1.getUuid(), CODEVIEWER, new ProjectId(publicProject), UserId.from(user1)); + + apply(change); + + assertThat(db.users().selectProjectPermissionsOfUser(user1, publicProject)).doesNotContain(CODEVIEWER); + } + + @Test + public void apply_adds_permission_ADMIN_on_a_public_project() { + applyAddsPermissionOnAPublicProject(ADMIN); + } + + @Test + public void apply_adds_permission_ISSUE_ADMIN_on_a_public_project() { + applyAddsPermissionOnAPublicProject(ISSUE_ADMIN); + } + + @Test + public void apply_adds_permission_SCAN_EXECUTION_on_a_public_project() { + applyAddsPermissionOnAPublicProject(SCAN_EXECUTION); + } + + private void applyAddsPermissionOnAPublicProject(String permission) { + UserPermissionChange change = new UserPermissionChange(ADD, org1.getUuid(), permission, new ProjectId(publicProject), UserId.from(user1)); + + apply(change); + + assertThat(db.users().selectProjectPermissionsOfUser(user1, publicProject)).containsOnly(permission); + } + + @Test + public void apply_fails_with_BadRequestException_when_removing_permission_USER_from_a_public_project() { + UserPermissionChange change = new UserPermissionChange(REMOVE, org1.getUuid(), USER, new ProjectId(publicProject), UserId.from(user1)); + + expectedException.expect(BadRequestException.class); + expectedException.expectMessage("Permission user can't be removed from a public component"); + + apply(change); + } + + @Test + public void apply_fails_with_BadRequestException_when_removing_permission_CODEVIEWER_from_a_public_project() { + UserPermissionChange change = new UserPermissionChange(REMOVE, org1.getUuid(), CODEVIEWER, new ProjectId(publicProject), UserId.from(user1)); + + expectedException.expect(BadRequestException.class); + expectedException.expectMessage("Permission codeviewer can't be removed from a public component"); + + apply(change); + } + + @Test + public void apply_removes_permission_ADMIN_from_a_public_project() { + applyRemovesPermissionFromPublicProject(ADMIN); + } + + @Test + public void apply_removes_permission_ISSUE_ADMIN_from_a_public_project() { + applyRemovesPermissionFromPublicProject(ISSUE_ADMIN); + } + + @Test + public void apply_removes_permission_SCAN_EXECUTION_from_a_public_project() { + applyRemovesPermissionFromPublicProject(SCAN_EXECUTION); + } + + private void applyRemovesPermissionFromPublicProject(String permission) { + db.users().insertProjectPermissionOnUser(user1, permission, publicProject); + UserPermissionChange change = new UserPermissionChange(REMOVE, org1.getUuid(), permission, new ProjectId(publicProject), UserId.from(user1)); + + apply(change); + + assertThat(db.users().selectProjectPermissionsOfUser(user1, publicProject)).isEmpty(); + } + + @Test + public void apply_adds_any_permission_to_a_private_project() { + ProjectPermissions.ALL + .forEach(permission -> { + UserPermissionChange change = new UserPermissionChange(ADD, org1.getUuid(), permission, new ProjectId(privateProject), UserId.from(user1)); + + apply(change); + + assertThat(db.users().selectProjectPermissionsOfUser(user1, privateProject)).contains(permission); + }); + } + + @Test + public void apply_removes_any_permission_from_a_private_project() { + ProjectPermissions.ALL + .forEach(permission -> db.users().insertProjectPermissionOnUser(user1, permission, privateProject)); + + ProjectPermissions.ALL + .forEach(permission -> { + UserPermissionChange change = new UserPermissionChange(REMOVE, org1.getUuid(), permission, new ProjectId(privateProject), UserId.from(user1)); + + apply(change); + + assertThat(db.users().selectProjectPermissionsOfUser(user1, privateProject)).doesNotContain(permission); + }); } @Test public void add_global_permission_to_user() { UserPermissionChange change = new UserPermissionChange(ADD, org1.getUuid(), SCAN_EXECUTION, null, UserId.from(user1)); + apply(change); assertThat(db.users().selectPermissionsOfUser(user1, org1)).containsOnly(SCAN); assertThat(db.users().selectPermissionsOfUser(user1, org2)).isEmpty(); - assertThat(db.users().selectProjectPermissionsOfUser(user1, project)).isEmpty(); + assertThat(db.users().selectProjectPermissionsOfUser(user1, privateProject)).isEmpty(); assertThat(db.users().selectPermissionsOfUser(user2, org1)).isEmpty(); - assertThat(db.users().selectProjectPermissionsOfUser(user2, project)).isEmpty(); + assertThat(db.users().selectProjectPermissionsOfUser(user2, privateProject)).isEmpty(); } @Test public void add_project_permission_to_user() { - UserPermissionChange change = new UserPermissionChange(ADD, org1.getUuid(), ISSUE_ADMIN, new ProjectId(project), UserId.from(user1)); + UserPermissionChange change = new UserPermissionChange(ADD, org1.getUuid(), ISSUE_ADMIN, new ProjectId(privateProject), UserId.from(user1)); apply(change); assertThat(db.users().selectPermissionsOfUser(user1, org1)).isEmpty(); - assertThat(db.users().selectProjectPermissionsOfUser(user1, project)).contains(ISSUE_ADMIN); + assertThat(db.users().selectProjectPermissionsOfUser(user1, privateProject)).contains(ISSUE_ADMIN); assertThat(db.users().selectPermissionsOfUser(user2, org1)).isEmpty(); - assertThat(db.users().selectProjectPermissionsOfUser(user2, project)).isEmpty(); + assertThat(db.users().selectProjectPermissionsOfUser(user2, privateProject)).isEmpty(); } @Test @@ -105,7 +253,7 @@ public class UserPermissionChangerTest { expectedException.expect(BadRequestException.class); expectedException.expectMessage("Invalid project permission 'gateadmin'. Valid values are [admin, codeviewer, issueadmin, scan, user]"); - UserPermissionChange change = new UserPermissionChange(ADD, org1.getUuid(), QUALITY_GATE_ADMIN, new ProjectId(project), UserId.from(user1)); + UserPermissionChange change = new UserPermissionChange(ADD, org1.getUuid(), QUALITY_GATE_ADMIN, new ProjectId(privateProject), UserId.from(user1)); apply(change); } @@ -124,7 +272,7 @@ public class UserPermissionChangerTest { db.users().insertPermissionOnUser(org1, user1, SCAN_EXECUTION); db.users().insertPermissionOnUser(org2, user1, QUALITY_GATE_ADMIN); db.users().insertPermissionOnUser(org1, user2, QUALITY_GATE_ADMIN); - db.users().insertProjectPermissionOnUser(user1, ISSUE_ADMIN, project); + db.users().insertProjectPermissionOnUser(user1, ISSUE_ADMIN, privateProject); UserPermissionChange change = new UserPermissionChange(REMOVE, org1.getUuid(), QUALITY_GATE_ADMIN, null, UserId.from(user1)); apply(change); @@ -132,23 +280,23 @@ public class UserPermissionChangerTest { assertThat(db.users().selectPermissionsOfUser(user1, org1)).containsOnly(SCAN); assertThat(db.users().selectPermissionsOfUser(user1, org2)).containsOnly(ADMINISTER_QUALITY_GATES); assertThat(db.users().selectPermissionsOfUser(user2, org1)).containsOnly(ADMINISTER_QUALITY_GATES); - assertThat(db.users().selectProjectPermissionsOfUser(user1, project)).containsOnly(ISSUE_ADMIN); + assertThat(db.users().selectProjectPermissionsOfUser(user1, privateProject)).containsOnly(ISSUE_ADMIN); } @Test public void remove_project_permission_from_user() { ComponentDto project2 = db.components().insertPrivateProject(org1); db.users().insertPermissionOnUser(user1, ADMINISTER_QUALITY_GATES); - db.users().insertProjectPermissionOnUser(user1, ISSUE_ADMIN, project); - db.users().insertProjectPermissionOnUser(user1, USER, project); - db.users().insertProjectPermissionOnUser(user2, ISSUE_ADMIN, project); + db.users().insertProjectPermissionOnUser(user1, ISSUE_ADMIN, privateProject); + db.users().insertProjectPermissionOnUser(user1, USER, privateProject); + db.users().insertProjectPermissionOnUser(user2, ISSUE_ADMIN, privateProject); db.users().insertProjectPermissionOnUser(user1, ISSUE_ADMIN, project2); - UserPermissionChange change = new UserPermissionChange(REMOVE, org1.getUuid(), ISSUE_ADMIN, new ProjectId(project), UserId.from(user1)); + UserPermissionChange change = new UserPermissionChange(REMOVE, org1.getUuid(), ISSUE_ADMIN, new ProjectId(privateProject), UserId.from(user1)); apply(change); - assertThat(db.users().selectProjectPermissionsOfUser(user1, project)).containsOnly(USER); - assertThat(db.users().selectProjectPermissionsOfUser(user2, project)).containsOnly(ISSUE_ADMIN); + assertThat(db.users().selectProjectPermissionsOfUser(user1, privateProject)).containsOnly(USER); + assertThat(db.users().selectProjectPermissionsOfUser(user2, privateProject)).containsOnly(ISSUE_ADMIN); assertThat(db.users().selectProjectPermissionsOfUser(user1, project2)).containsOnly(ISSUE_ADMIN); } @@ -162,10 +310,10 @@ public class UserPermissionChangerTest { @Test public void do_not_fail_if_removing_a_project_permission_that_does_not_exist() { - UserPermissionChange change = new UserPermissionChange(REMOVE, org1.getUuid(), ISSUE_ADMIN, new ProjectId(project), UserId.from(user1)); + UserPermissionChange change = new UserPermissionChange(REMOVE, org1.getUuid(), ISSUE_ADMIN, new ProjectId(privateProject), UserId.from(user1)); apply(change); - assertThat(db.users().selectProjectPermissionsOfUser(user1, project)).isEmpty(); + assertThat(db.users().selectProjectPermissionsOfUser(user1, privateProject)).isEmpty(); } @Test diff --git a/server/sonar-server/src/test/java/org/sonar/server/permission/ws/AddUserActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/permission/ws/AddUserActionTest.java index ef71f25a9d7..9ad22e09e59 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/permission/ws/AddUserActionTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/permission/ws/AddUserActionTest.java @@ -31,7 +31,9 @@ import org.sonar.server.exceptions.NotFoundException; import org.sonar.server.exceptions.ServerException; import static org.assertj.core.api.Assertions.assertThat; +import static org.sonar.api.web.UserRole.CODEVIEWER; import static org.sonar.api.web.UserRole.ISSUE_ADMIN; +import static org.sonar.api.web.UserRole.USER; import static org.sonar.core.permission.GlobalPermissions.SYSTEM_ADMIN; import static org.sonar.db.component.ComponentTesting.newFileDto; import static org.sonar.db.component.ComponentTesting.newPrivateProjectDto; @@ -300,6 +302,38 @@ public class AddUserActionTest extends BasePermissionWsTest<AddUserAction> { .execute(); } + @Test + public void no_effect_when_adding_USER_permission_on_a_public_project() { + OrganizationDto organization = db.organizations().insert(); + ComponentDto project = db.components().insertPublicProject(organization); + addUserAsMemberOfOrganization(organization); + userSession.logIn().addProjectPermission(UserRole.ADMIN, project); + + newRequest() + .setParam(PARAM_USER_LOGIN, user.getLogin()) + .setParam(PARAM_PROJECT_ID, project.uuid()) + .setParam(PARAM_PERMISSION, USER) + .execute(); + + assertThat(db.users().selectAnyonePermissions(organization, project)).isEmpty(); + } + + @Test + public void no_effect_when_adding_CODEVIEWER_permission_on_a_public_project() { + OrganizationDto organization = db.organizations().insert(); + ComponentDto project = db.components().insertPublicProject(organization); + addUserAsMemberOfOrganization(organization); + userSession.logIn().addProjectPermission(UserRole.ADMIN, project); + + newRequest() + .setParam(PARAM_USER_LOGIN, user.getLogin()) + .setParam(PARAM_PROJECT_ID, project.uuid()) + .setParam(PARAM_PERMISSION, CODEVIEWER) + .execute(); + + assertThat(db.users().selectAnyonePermissions(organization, project)).isEmpty(); + } + private void addUserAsMemberOfOrganization(OrganizationDto organization) { db.organizations().addMember(organization, user); } |