From: Simon Brandhof <simon.brandhof@sonarsource.com>
Date: Thu, 3 Nov 2016 14:40:04 +0000 (+0100)
Subject: SONAR-6912 do not remove a group member if no more admins
X-Git-Tag: 6.2-RC1~216
X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=806a8066d12e5589b1833d368f989bf64456b21c;p=sonarqube.git

SONAR-6912 do not remove a group member if no more admins
---

diff --git a/server/sonar-server/src/main/java/org/sonar/server/usergroups/ws/RemoveUserAction.java b/server/sonar-server/src/main/java/org/sonar/server/usergroups/ws/RemoveUserAction.java
index 5726167f209..d1dfc46ca6b 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/usergroups/ws/RemoveUserAction.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/usergroups/ws/RemoveUserAction.java
@@ -26,6 +26,7 @@ import org.sonar.api.server.ws.WebService.NewController;
 import org.sonar.db.DbClient;
 import org.sonar.db.DbSession;
 import org.sonar.db.user.UserDto;
+import org.sonar.server.exceptions.BadRequestException;
 import org.sonar.server.organization.DefaultOrganizationProvider;
 import org.sonar.server.user.UserSession;
 
@@ -76,6 +77,8 @@ public class RemoveUserAction implements UserGroupsWsAction {
       String login = request.mandatoryParam(PARAM_LOGIN);
       UserDto user = getUser(dbSession, login);
 
+      ensureLastAdminIsNotRemoved(dbSession, group, user);
+
       dbClient.userGroupDao().delete(dbSession, group.getId(), user.getId());
       dbClient.userDao().updateRootFlagFromPermissions(dbSession, user.getId(), defaultOrganizationProvider.get().getUuid());
       dbSession.commit();
@@ -84,6 +87,17 @@ public class RemoveUserAction implements UserGroupsWsAction {
     }
   }
 
+  /**
+   * Ensure that there are still users with admin global permission if user is removed from the group.
+   */
+  private void ensureLastAdminIsNotRemoved(DbSession dbSession, GroupId group, UserDto user) {
+    int remainingAdmins = dbClient.authorizationDao().countUsersWithGlobalPermissionExcludingGroupMember(dbSession,
+      group.getOrganizationUuid(), SYSTEM_ADMIN, group.getId(), user.getId());
+    if (remainingAdmins == 0) {
+      throw new BadRequestException("The last administrator user cannot be removed");
+    }
+  }
+
   private UserDto getUser(DbSession dbSession, String userLogin) {
     return checkFound(dbClient.userDao().selectActiveUserByLogin(dbSession, userLogin),
       "User with login '%s' is not found'", userLogin);
diff --git a/server/sonar-server/src/test/java/org/sonar/server/usergroups/ws/RemoveUserActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/usergroups/ws/RemoveUserActionTest.java
index 4ff9e6cbbe6..423d1fd91d3 100644
--- a/server/sonar-server/src/test/java/org/sonar/server/usergroups/ws/RemoveUserActionTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/usergroups/ws/RemoveUserActionTest.java
@@ -33,6 +33,7 @@ import org.sonar.db.user.UserDto;
 import org.sonar.db.user.UserMembershipDto;
 import org.sonar.db.user.UserMembershipQuery;
 import org.sonar.server.exceptions.ForbiddenException;
+import org.sonar.server.exceptions.BadRequestException;
 import org.sonar.server.exceptions.NotFoundException;
 import org.sonar.server.organization.TestDefaultOrganizationProvider;
 import org.sonar.server.tester.UserSessionRule;
@@ -63,6 +64,9 @@ public class RemoveUserActionTest {
 
   @Test
   public void does_nothing_if_user_is_not_in_group() throws Exception {
+    // keep an administrator
+    insertAnAdministratorInDefaultOrganization();
+
     GroupDto group = db.users().insertGroup(db.getDefaultOrganization(), "admins");
     UserDto user = db.users().insertUser("my-admin");
     loginAsAdminOnDefaultOrganization();
@@ -78,6 +82,8 @@ public class RemoveUserActionTest {
 
   @Test
   public void remove_user_by_group_id() throws Exception {
+    // keep an administrator
+    insertAnAdministratorInDefaultOrganization();
     GroupDto users = db.users().insertGroup(db.getDefaultOrganization(), "users");
     UserDto user = db.users().insertUser("my-admin");
     db.users().insertMember(users, user);
@@ -94,8 +100,9 @@ public class RemoveUserActionTest {
 
   @Test
   public void remove_user_by_group_name_in_default_organization() throws Exception {
-    GroupDto group = db.users().insertGroup(db.getDefaultOrganization(), "group_name");
-    UserDto user = db.users().insertUser("user_login");
+    insertAnAdministratorInDefaultOrganization();
+    GroupDto group = db.users().insertGroup(db.getDefaultOrganization(), "a_group");
+    UserDto user = db.users().insertUser("a_user");
     db.users().insertMember(group, user);
     loginAsAdminOnDefaultOrganization();
 
@@ -112,8 +119,10 @@ public class RemoveUserActionTest {
   public void remove_user_by_group_name_in_specific_organization() throws Exception {
     OrganizationDto org = db.organizations().insert();
     GroupDto group = db.users().insertGroup(org, "a_group");
-    UserDto user = db.users().insertUser("user_login");
+    UserDto user = db.users().insertUser("a_user");
     db.users().insertMember(group, user);
+    // keep an administrator
+    db.users().insertAdminByUserPermission(org);
 
     loginAsAdmin(org);
 
@@ -129,6 +138,9 @@ public class RemoveUserActionTest {
 
   @Test
   public void remove_user_only_from_one_group() throws Exception {
+    // keep an administrator
+    insertAnAdministratorInDefaultOrganization();
+
     OrganizationDto defaultOrg = db.getDefaultOrganization();
     GroupDto users = db.users().insertGroup(defaultOrg, "user");
     GroupDto admins = db.users().insertGroup(defaultOrg, "admins");
@@ -174,6 +186,9 @@ public class RemoveUserActionTest {
 
   @Test
   public void sets_root_flag_to_false_when_removing_user_from_group_of_default_organization_with_admin_permission() throws Exception {
+    // keep an administrator
+    insertAnAdministratorInDefaultOrganization();
+
     GroupDto adminGroup = db.users().insertAdminGroup();
     UserDto user1 = db.users().insertRootByGroupPermission("user1", adminGroup);
     UserDto user2 = db.users().insertRootByGroupPermission("user2", adminGroup);
@@ -244,7 +259,24 @@ public class RemoveUserActionTest {
 
     newRequest()
       .setParam("id", group.getId().toString())
-      .setParam("login", user.getLogin())
+      .setParam("login", user.getLogin());
+  }
+
+  @Test
+  public void fail_to_remove_the_last_administrator() throws Exception {
+    OrganizationDto org = db.organizations().insert();
+    GroupDto adminGroup = db.users().insertGroup(org, "sonar-admins");
+    db.users().insertPermissionOnGroup(adminGroup, GlobalPermissions.SYSTEM_ADMIN);
+    UserDto adminUser = db.users().insertUser("the-single-admin");
+    db.users().insertMember(adminGroup, adminUser);
+    loginAsAdmin(org);
+
+    expectedException.expect(BadRequestException.class);
+    expectedException.expectMessage("The last administrator user cannot be removed");
+
+    newRequest()
+      .setParam("id", adminGroup.getId().toString())
+      .setParam("login", adminUser.getLogin())
       .execute();
   }
 
@@ -299,4 +331,7 @@ public class RemoveUserActionTest {
       .anyMatch(dto -> dto.getLogin().equals(userDto.getLogin()));
   }
 
+  private UserDto insertAnAdministratorInDefaultOrganization() {
+    return db.users().insertAdminByUserPermission(db.getDefaultOrganization());
+  }
 }
diff --git a/sonar-db/src/main/java/org/sonar/db/permission/AuthorizationDao.java b/sonar-db/src/main/java/org/sonar/db/permission/AuthorizationDao.java
index 9b60f610117..50845b65cea 100644
--- a/sonar-db/src/main/java/org/sonar/db/permission/AuthorizationDao.java
+++ b/sonar-db/src/main/java/org/sonar/db/permission/AuthorizationDao.java
@@ -79,7 +79,7 @@ public class AuthorizationDao implements Dao {
   }
 
   /**
-   * The number of users who will still have the permission when the group {@code excludedGroupId}
+   * The number of users who will still have the permission if the group {@code excludedGroupId}
    * is deleted. The anyone virtual group is not taken into account.
    */
   public int countUsersWithGlobalPermissionExcludingGroup(DbSession dbSession, String organizationUuid,
@@ -88,7 +88,7 @@ public class AuthorizationDao implements Dao {
   }
 
   /**
-   * The number of users who will still have the permission when the user {@code excludedUserId}
+   * The number of users who will still have the permission if the user {@code excludedUserId}
    * is deleted. The anyone virtual group is not taken into account.
    */
   public int countUsersWithGlobalPermissionExcludingUser(DbSession dbSession, String organizationUuid,
@@ -96,6 +96,17 @@ public class AuthorizationDao implements Dao {
     return mapper(dbSession).countUsersWithGlobalPermissionExcludingUser(organizationUuid, permission, excludedUSerId);
   }
 
+  /**
+   * The number of users who will still have the permission if the user {@code userId}
+   * is removed from group {@code groupId}. The anyone virtual group is not taken into account.
+   * Contrary to {@link #countUsersWithGlobalPermissionExcludingUser(DbSession, String, String, long)}, user
+   * still exists and may have the permission directly or through other groups.
+   */
+  public int countUsersWithGlobalPermissionExcludingGroupMember(DbSession dbSession, String organizationUuid,
+    String permission, long groupId, long userId) {
+    return mapper(dbSession).countUsersWithGlobalPermissionExcludingGroupMember(organizationUuid, permission, groupId, userId);
+  }
+
   public Set<Long> keepAuthorizedProjectIds(DbSession dbSession, Collection<Long> componentIds, @Nullable Integer userId, String role) {
     return executeLargeInputsIntoSet(
       componentIds,
diff --git a/sonar-db/src/main/java/org/sonar/db/permission/AuthorizationMapper.java b/sonar-db/src/main/java/org/sonar/db/permission/AuthorizationMapper.java
index df686f1cac6..5c7e2ae853e 100644
--- a/sonar-db/src/main/java/org/sonar/db/permission/AuthorizationMapper.java
+++ b/sonar-db/src/main/java/org/sonar/db/permission/AuthorizationMapper.java
@@ -43,6 +43,9 @@ public interface AuthorizationMapper {
   int countUsersWithGlobalPermissionExcludingUser(@Param("organizationUuid") String organizationUuid, @Param("permission") String permission,
     @Param("excludedUserId") long excludedUserId);
 
+  int countUsersWithGlobalPermissionExcludingGroupMember(@Param("organizationUuid") String organizationUuid,
+                                                         @Param("permission") String permission, @Param("groupId") long groupId, @Param("userId") long userId);
+
   Set<Long> keepAuthorizedProjectIdsForAnonymous(@Param("role") String role, @Param("componentIds") Collection<Long> componentIds);
 
   Set<Long> keepAuthorizedProjectIdsForUser(@Param("userId") long userId, @Param("role") String role, @Param("componentIds") Collection<Long> componentIds);
diff --git a/sonar-db/src/main/resources/org/sonar/db/permission/AuthorizationMapper.xml b/sonar-db/src/main/resources/org/sonar/db/permission/AuthorizationMapper.xml
index 37738662baf..2300f9d8c9e 100644
--- a/sonar-db/src/main/resources/org/sonar/db/permission/AuthorizationMapper.xml
+++ b/sonar-db/src/main/resources/org/sonar/db/permission/AuthorizationMapper.xml
@@ -120,6 +120,30 @@
     ) remaining
   </select>
 
+  <select id="countUsersWithGlobalPermissionExcludingGroupMember" parameterType="map" resultType="int">
+    select count(1) from
+    (
+    select gu.user_id
+    from groups_users gu
+    inner join group_roles gr on gr.group_id = gu.group_id
+    where
+    gr.organization_uuid = #{organizationUuid,jdbcType=VARCHAR} and
+    gr.role = #{permission,jdbcType=VARCHAR} and
+    gr.resource_id is null and
+    gr.group_id is not null and
+    (gu.group_id != #{groupId,jdbcType=BIGINT} or gu.user_id != #{userId,jdbcType=BIGINT})
+
+    union
+
+    select ur.user_id
+    from user_roles ur
+    where
+    ur.organization_uuid = #{organizationUuid,jdbcType=VARCHAR} and
+    ur.resource_id is null and
+    ur.role = #{permission,jdbcType=VARCHAR}
+    ) remaining
+  </select>
+
   <select id="keepAuthorizedProjectIdsForUser" parameterType="map" resultType="long">
     SELECT gr.resource_id
     FROM group_roles gr
diff --git a/sonar-db/src/test/java/org/sonar/db/permission/AuthorizationDaoTest.java b/sonar-db/src/test/java/org/sonar/db/permission/AuthorizationDaoTest.java
index e1592e31365..53696810791 100644
--- a/sonar-db/src/test/java/org/sonar/db/permission/AuthorizationDaoTest.java
+++ b/sonar-db/src/test/java/org/sonar/db/permission/AuthorizationDaoTest.java
@@ -43,6 +43,9 @@ public class AuthorizationDaoTest {
   private static final Long PROJECT_ID_WITHOUT_SNAPSHOT = 400L;
   private static final String PROJECT = "pj-w-snapshot";
   private static final String PROJECT_WIHOUT_SNAPSHOT = "pj-wo-snapshot";
+  private static final long MISSING_ID = -1L;
+  private static final String A_PERMISSION = "a-permission";
+  private static final String DOES_NOT_EXIST = "does-not-exist";
 
   @Rule
   public DbTester db = DbTester.create(System2.INSTANCE);
@@ -527,4 +530,35 @@ public class AuthorizationDaoTest {
       // Only 100 and 101 has 'user' role on project
       newHashSet(100L, 101L, 102L), "user", PROJECT_ID)).isEmpty();
   }
+
+  @Test
+  public void countUsersWithGlobalPermissionExcludingGroupMember() {
+    // u1 has the direct permission, u2 and u3 have the permission through their group
+    UserDto u1 = db.users().insertUser();
+    db.users().insertPermissionOnUser(org, u1, A_PERMISSION);
+    db.users().insertPermissionOnGroup(group1, A_PERMISSION);
+    db.users().insertPermissionOnGroup(group1, "another-permission");
+    UserDto u2 = db.users().insertUser();
+    db.users().insertMember(group1, u2);
+    UserDto u3 = db.users().insertUser();
+    db.users().insertMember(group1, u3);
+
+    // excluding u2 membership --> remain u1 and u3
+    int count = underTest.countUsersWithGlobalPermissionExcludingGroupMember(dbSession, org.getUuid(), A_PERMISSION, group1.getId(), u2.getId());
+    assertThat(count).isEqualTo(2);
+
+    // excluding unknown memberships
+    count = underTest.countUsersWithGlobalPermissionExcludingGroupMember(dbSession, org.getUuid(), A_PERMISSION, group1.getId(), MISSING_ID);
+    assertThat(count).isEqualTo(3);
+    count = underTest.countUsersWithGlobalPermissionExcludingGroupMember(dbSession, org.getUuid(), A_PERMISSION, MISSING_ID, u2.getId());
+    assertThat(count).isEqualTo(3);
+
+    // another organization
+    count = underTest.countUsersWithGlobalPermissionExcludingGroupMember(dbSession, DOES_NOT_EXIST, A_PERMISSION,  group1.getId(), u2.getId());
+    assertThat(count).isEqualTo(0);
+
+    // another permission
+    count = underTest.countUsersWithGlobalPermissionExcludingGroupMember(dbSession, org.getUuid(), DOES_NOT_EXIST,  group1.getId(), u2.getId());
+    assertThat(count).isEqualTo(0);
+  }
 }
diff --git a/sonar-db/src/test/java/org/sonar/db/user/UserDbTester.java b/sonar-db/src/test/java/org/sonar/db/user/UserDbTester.java
index d81044aa564..ed11b06605e 100644
--- a/sonar-db/src/test/java/org/sonar/db/user/UserDbTester.java
+++ b/sonar-db/src/test/java/org/sonar/db/user/UserDbTester.java
@@ -120,6 +120,12 @@ public class UserDbTester {
     return rootByGroupPermissionUser;
   }
 
+  public UserDto insertAdminByUserPermission(OrganizationDto org) {
+    UserDto user = db.users().insertUser();
+    db.users().insertPermissionOnUser(org, user, GlobalPermissions.SYSTEM_ADMIN);
+    return user;
+  }
+
   private GroupDto createOrCheckAdminGroup(@Nullable GroupDto groupDto) {
     if (groupDto == null) {
       GroupDto adminGroup = insertGroup(db.getDefaultOrganization());