SONAR-4412 Added checks to prevent last system admin permission removal

8 files changed, 113 insertions, 5 deletions

diff --git a/sonar-core/src/main/java/org/sonar/core/user/Permissions.java b/sonar-core/src/main/java/org/sonar/core/user/Permissions.java
index ebed6f1ea15..636d0773e49 100644
--- a/sonar-core/src/main/java/org/sonar/core/user/Permissions.java
+++ b/sonar-core/src/main/java/org/sonar/core/user/Permissions.java
@@ -28,9 +28,9 @@ package org.sonar.core.user;
  */
 public interface Permissions {
 
-  public static final String SYSTEM_ADMIN = "admin";
-  public static final String QUALITY_PROFILE_ADMIN = "profileadmin";
-  public static final String DASHBOARD_SHARING = "sharedashboard";
-  public static final String SCAN_EXECUTION = "scan";
-  public static final String DRY_RUN_EXECUTION = "dryrun";
+  String SYSTEM_ADMIN = "admin";
+  String QUALITY_PROFILE_ADMIN = "profileadmin";
+  String DASHBOARD_SHARING = "sharedashboard";
+  String SCAN_EXECUTION = "scan";
+  String DRY_RUN_EXECUTION = "dryrun";
 }
diff --git a/sonar-core/src/main/java/org/sonar/core/user/RoleDao.java b/sonar-core/src/main/java/org/sonar/core/user/RoleDao.java
index d76d7620615..74884a41794 100644
--- a/sonar-core/src/main/java/org/sonar/core/user/RoleDao.java
+++ b/sonar-core/src/main/java/org/sonar/core/user/RoleDao.java
@@ -25,6 +25,7 @@ import org.sonar.api.ServerExtension;
 import org.sonar.api.task.TaskExtension;
 import org.sonar.core.persistence.MyBatis;
 
+import javax.annotation.Nullable;
 import java.util.List;
 
 public class RoleDao implements TaskExtension, ServerExtension {
@@ -144,4 +145,14 @@ public class RoleDao implements TaskExtension, ServerExtension {
       MyBatis.closeQuietly(session);
     }
   }
+
+  public int countSystemAdministrators(@Nullable String groupName) {
+    SqlSession session = mybatis.openSession();
+    try {
+      RoleMapper mapper = session.getMapper(RoleMapper.class);
+      return mapper.countSystemAdministrators(groupName);
+    } finally {
+      MyBatis.closeQuietly(session);
+    }
+  }
 }
diff --git a/sonar-core/src/main/java/org/sonar/core/user/RoleMapper.java b/sonar-core/src/main/java/org/sonar/core/user/RoleMapper.java
index efc51764ab5..da7b6c422ef 100644
--- a/sonar-core/src/main/java/org/sonar/core/user/RoleMapper.java
+++ b/sonar-core/src/main/java/org/sonar/core/user/RoleMapper.java
@@ -19,6 +19,9 @@
  */
 package org.sonar.core.user;
 
+import org.apache.ibatis.annotations.Param;
+
+import javax.annotation.Nullable;
 import java.util.List;
 
 /**
@@ -45,4 +48,6 @@ public interface RoleMapper {
   int countGroupRoles(Long resourceId);
 
   int countUserRoles(Long resourceId);
+
+  int countSystemAdministrators(@Nullable @Param("groupName") String groupName);
 }
diff --git a/sonar-core/src/main/resources/org/sonar/core/user/RoleMapper.xml b/sonar-core/src/main/resources/org/sonar/core/user/RoleMapper.xml
index 95d12186b2c..db97a0986b3 100644
--- a/sonar-core/src/main/resources/org/sonar/core/user/RoleMapper.xml
+++ b/sonar-core/src/main/resources/org/sonar/core/user/RoleMapper.xml
@@ -84,4 +84,18 @@
     SELECT count(id)
     FROM group_roles WHERE resource_id=#{id}
   </select>
+
+  <select id="countSystemAdministrators" parameterType="String" resultType="int">
+    SELECT COUNT(DISTINCT u.id)
+    FROM users AS u
+    LEFT JOIN user_roles AS ur ON ur.user_id = u.id
+    INNER JOIN groups_users AS gu ON gu.user_id = u.id
+    INNER JOIN group_roles AS gr ON gr.group_id = gu.group_id
+    INNER JOIN groups AS g ON g.id = gu.group_id
+    WHERE (ur.role = 'admin' AND ur.resource_id IS NULL) OR (gr.role = 'admin' AND gr.resource_id IS NULL)
+    AND u.active = TRUE
+    <if test="groupName != null">
+      AND g.name != #{groupName}
+    </if>
+  </select>
 </mapper>
diff --git a/sonar-core/src/test/java/org/sonar/core/user/RoleDaoTest.java b/sonar-core/src/test/java/org/sonar/core/user/RoleDaoTest.java
index b98510603dc..1c9b3ee08c7 100644
--- a/sonar-core/src/test/java/org/sonar/core/user/RoleDaoTest.java
+++ b/sonar-core/src/test/java/org/sonar/core/user/RoleDaoTest.java
@@ -71,4 +71,18 @@ public class RoleDaoTest extends AbstractDaoTestCase {
 
     checkTable("groupPermissions", "group_roles", "group_id", "role");
   }
+
+  @Test
+  public void should_retrieve_system_admins_count() throws Exception {
+    setupData("systemAdminsCount");
+
+    RoleDao dao = new RoleDao(getMyBatis());
+    int overallAdminsCount = dao.countSystemAdministrators(null);
+    int adminsCountAfterWholeGroupRemoval = dao.countSystemAdministrators("sonar-administrators");
+    int adminsCountAfterNonAdminGroupRemoval = dao.countSystemAdministrators("sonar-users");
+
+    assertThat(overallAdminsCount).isEqualTo(3);
+    assertThat(adminsCountAfterWholeGroupRemoval).isEqualTo(1);
+    assertThat(adminsCountAfterNonAdminGroupRemoval).isEqualTo(3);
+  }
 }
diff --git a/sonar-core/src/test/resources/org/sonar/core/user/RoleDaoTest/systemAdminsCount.xml b/sonar-core/src/test/resources/org/sonar/core/user/RoleDaoTest/systemAdminsCount.xml
new file mode 100644
index 00000000000..f39cc938f24
--- /dev/null
+++ b/sonar-core/src/test/resources/org/sonar/core/user/RoleDaoTest/systemAdminsCount.xml
@@ -0,0 +1,23 @@
+<dataset>
+
+  <users id="200" login="admin" name="admin" active="[true]"/>
+  <users id="201" login="user_admin" name="user_admin" active="[true]"/>
+  <users id="202" login="user_in_admin_group" name="user_in_admin_group" active="[true]"/>
+  <users id="203" login="non_admin" name="non_admin" active="[true]"/>
+
+  <user_roles id="1" user_id="201" role="admin"/>
+
+  <groups_users group_id="100" user_id="200"/>
+  <groups_users group_id="100" user_id="202"/>
+  <groups_users group_id="101" user_id="201"/>
+  <groups_users group_id="101" user_id="203"/>
+
+  <groups id="100" name="sonar-administrators"/>
+  <groups id="101" name="sonar-users"/>
+
+  <group_roles id="1" group_id="100" role="admin"/>
+  <group_roles id="2" group_id="100" role="profileadmin"/>
+  <group_roles id="3" group_id="100" role="sharedashboard"/>
+  <group_roles id="4" group_id="101" role="sharedashboard"/>
+
+</dataset>
\ No newline at end of file
diff --git a/sonar-server/src/main/java/org/sonar/server/permission/InternalPermissionService.java b/sonar-server/src/main/java/org/sonar/server/permission/InternalPermissionService.java
index ca455808fd3..5693bdb7d00 100644
--- a/sonar-server/src/main/java/org/sonar/server/permission/InternalPermissionService.java
+++ b/sonar-server/src/main/java/org/sonar/server/permission/InternalPermissionService.java
@@ -24,6 +24,7 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.sonar.api.ServerComponent;
 import org.sonar.core.user.*;
+import org.sonar.server.exceptions.BadRequestException;
 import org.sonar.server.user.UserSession;
 
 import java.util.List;
@@ -59,6 +60,9 @@ public class InternalPermissionService implements ServerComponent {
     UserSession.get().checkPermission(Permissions.SYSTEM_ADMIN);
     PermissionChangeQuery permissionChangeQuery = PermissionChangeQuery.buildFromParams(params);
     if(permissionChangeQuery.isValid()) {
+      if(Permissions.SYSTEM_ADMIN.equals(permissionChangeQuery.getRole()) && REMOVE.equals(permissionChange)) {
+        checkThatAtLeastOneAdminRemains(permissionChangeQuery);
+      }
       applyPermissionChange(permissionChange, permissionChangeQuery);
     } else {
       String errorMsg = String.format("Request '%s permission %s' is invalid", permissionChange, permissionChangeQuery.getRole());
@@ -119,4 +123,14 @@ public class InternalPermissionService implements ServerComponent {
     return (ADD.equals(operation) && existingPermissions.contains(role)) ||
       (REMOVE.equals(operation) && !existingPermissions.contains(role));
   }
+
+  private void checkThatAtLeastOneAdminRemains(PermissionChangeQuery permissionChangeQuery) {
+    int remainingSystemAdmins = roleDao.countSystemAdministrators(permissionChangeQuery.getGroup());
+    if(remainingSystemAdmins == 0) {
+      String errorMsg = String.format("Cannot remove permission %s to %s - At least one system administrator should remain active",
+        permissionChangeQuery.getRole(), permissionChangeQuery.getUser() == null ? permissionChangeQuery.getGroup() : permissionChangeQuery.getUser());
+      LOG.error(errorMsg);
+      throw new BadRequestException(errorMsg);
+    }
+  }
 }
diff --git a/sonar-server/src/test/java/org/sonar/server/permission/InternalPermissionServiceTest.java b/sonar-server/src/test/java/org/sonar/server/permission/InternalPermissionServiceTest.java
index ab41597f9f7..39603f983e6 100644
--- a/sonar-server/src/test/java/org/sonar/server/permission/InternalPermissionServiceTest.java
+++ b/sonar-server/src/test/java/org/sonar/server/permission/InternalPermissionServiceTest.java
@@ -30,6 +30,7 @@ import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
 import org.sonar.core.user.*;
+import org.sonar.server.exceptions.BadRequestException;
 import org.sonar.server.exceptions.ForbiddenException;
 import org.sonar.server.user.MockUserSession;
 
@@ -136,6 +137,32 @@ public class InternalPermissionServiceTest {
   }
 
   @Test
+  public void should_prevent_last_admin_removal() throws Exception {
+    throwable.expect(BadRequestException.class);
+    params = buildParams("admin", null, Permissions.SYSTEM_ADMIN);
+    when(roleDao.countSystemAdministrators(null)).thenReturn(0);
+
+    service.removePermission(params);
+  }
+
+  @Test
+  public void should_prevent_last_admin_group_removal() throws Exception {
+    throwable.expect(BadRequestException.class);
+    params = buildParams(null, "sonar-administrators", Permissions.SYSTEM_ADMIN);
+    GroupDto adminGroups = new GroupDto().setId(2L).setName("sonar-administrators");
+
+    roleDao = mock(RoleDao.class);
+    when(roleDao.selectGroupPermissions("sonar-administrators")).thenReturn(Lists.newArrayList(Permissions.SYSTEM_ADMIN));
+    when(roleDao.countSystemAdministrators("sonar-administrators")).thenReturn(0);
+
+    userDao = mock(UserDao.class);
+    when(userDao.selectGroupByName("sonar-administrators")).thenReturn(adminGroups);
+
+    service = new InternalPermissionService(roleDao, userDao);
+    service.removePermission(params);
+  }
+
+  @Test
   public void should_fail_on_anonymous_access() throws Exception {
     throwable.expect(ForbiddenException.class);
     params = buildParams("user", null, Permissions.QUALITY_PROFILE_ADMIN);