From 0f6c5993fe8ab58f956d1dffb13219356331a048 Mon Sep 17 00:00:00 2001
From: Simon Brandhof <simon.brandhof@sonarsource.com>
Date: Wed, 28 Sep 2016 10:46:56 +0200
Subject: [PATCH] Fix UserPermissionDao#delete()

---
 .../db/permission/UserPermissionDao.java      |  8 ++++
 .../db/permission/UserPermissionMapper.xml    | 10 ++---
 .../db/permission/UserPermissionDaoTest.java  | 42 +++++++++++++++++++
 3 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/sonar-db/src/main/java/org/sonar/db/permission/UserPermissionDao.java b/sonar-db/src/main/java/org/sonar/db/permission/UserPermissionDao.java
index 8e81cd1c193..9a873d89959 100644
--- a/sonar-db/src/main/java/org/sonar/db/permission/UserPermissionDao.java
+++ b/sonar-db/src/main/java/org/sonar/db/permission/UserPermissionDao.java
@@ -101,6 +101,14 @@ public class UserPermissionDao implements Dao {
   /**
    * Delete permissions for a user, permissions for a project, or a mix of them. In all cases
    * scope can be restricted to a specified permission.
+   *
+   * Examples:
+   * <ul>
+   *   <li>{@code delete(dbSession, "marius", null, null)} deletes all permissions of Marius, including global and project permissions</li>
+   *   <li>{@code delete(dbSession, null, "ABC", null)} deletes all permissions of project ABC</li>
+   *   <li>{@code delete(dbSession, "marius", "ABC", null)} deletes the permissions of Marius on project "ABC"</li>
+   * </ul>
+   * 
    * @see UserPermissionMapper#delete(String, String, String)
    */
   public void delete(DbSession dbSession, @Nullable String login, @Nullable String projectUuid, @Nullable String permission) {
diff --git a/sonar-db/src/main/resources/org/sonar/db/permission/UserPermissionMapper.xml b/sonar-db/src/main/resources/org/sonar/db/permission/UserPermissionMapper.xml
index b865fd64777..72443999fb1 100644
--- a/sonar-db/src/main/resources/org/sonar/db/permission/UserPermissionMapper.xml
+++ b/sonar-db/src/main/resources/org/sonar/db/permission/UserPermissionMapper.xml
@@ -68,16 +68,16 @@
     delete from user_roles
     <where>
       <choose>
-        <when test="projectUuid != null">
+        <when test="login != null">
+          and user_id = (select id from users where login = #{login})
+          <if test="projectUuid != null">
           and resource_id = (select id from projects where uuid = #{projectUuid})
+          </if>
         </when>
         <otherwise>
-          and resource_id is null
+          and resource_id = (select id from projects where uuid = #{projectUuid})
         </otherwise>
       </choose>
-      <if test="login != null">
-        and user_id = (select id from users where login = #{login})
-      </if>
       <if test="permission != null">
         and role = #{permission}
       </if>
diff --git a/sonar-db/src/test/java/org/sonar/db/permission/UserPermissionDaoTest.java b/sonar-db/src/test/java/org/sonar/db/permission/UserPermissionDaoTest.java
index 9e8f8817c20..e469d866ce5 100644
--- a/sonar-db/src/test/java/org/sonar/db/permission/UserPermissionDaoTest.java
+++ b/sonar-db/src/test/java/org/sonar/db/permission/UserPermissionDaoTest.java
@@ -202,6 +202,48 @@ public class UserPermissionDaoTest {
       new UserCountPerProjectPermission(project2.getId(), ISSUE_ADMIN, 1));
   }
 
+  @Test
+  public void delete_by_project() {
+    insertGlobalPermission(SYSTEM_ADMIN, user1.getId());
+    insertProjectPermission(USER, user1.getId(), project1.getId());
+    insertProjectPermission(ISSUE_ADMIN, user2.getId(), project1.getId());
+    insertProjectPermission(ISSUE_ADMIN, user2.getId(), project2.getId());
+
+    underTest.delete(dbTester.getSession(), null, project1.uuid(), null);
+
+    assertThat(dbTester.countSql(dbTester.getSession(), "select count(id) from user_roles where resource_id=" + project1.getId())).isEqualTo(0);
+    // remains global permission and project2 permission
+    assertThat(dbTester.countRowsOfTable(dbTester.getSession(), "user_roles")).isEqualTo(2);
+  }
+
+  @Test
+  public void delete_by_user() {
+    insertGlobalPermission(SYSTEM_ADMIN, user1.getId());
+    insertProjectPermission(USER, user1.getId(), project1.getId());
+    insertProjectPermission(ISSUE_ADMIN, user2.getId(), project1.getId());
+    insertProjectPermission(ISSUE_ADMIN, user2.getId(), project2.getId());
+
+    underTest.delete(dbTester.getSession(), user1.getLogin(), null, null);
+
+    assertThat(dbTester.countSql(dbTester.getSession(), "select count(id) from user_roles where user_id=" + user1.getId())).isEqualTo(0);
+    // remains user2 permissions
+    assertThat(dbTester.countRowsOfTable(dbTester.getSession(), "user_roles")).isEqualTo(2);
+  }
+
+  @Test
+  public void delete_specific_permission() {
+    insertGlobalPermission(SYSTEM_ADMIN, user1.getId());
+    insertProjectPermission(USER, user1.getId(), project1.getId());
+    insertProjectPermission(ISSUE_ADMIN, user2.getId(), project1.getId());
+    insertProjectPermission(ISSUE_ADMIN, user2.getId(), project2.getId());
+
+    underTest.delete(dbTester.getSession(), user1.getLogin(), project1.uuid(), USER);
+
+    assertThat(dbTester.countRowsOfTable(dbTester.getSession(), "user_roles")).isEqualTo(3);
+    assertThat(dbTester.countSql(dbTester.getSession(), "select count(id) from user_roles where user_id=" + user1.getId())).isEqualTo(1);
+    assertThat(dbTester.countSql(dbTester.getSession(), "select count(id) from user_roles where role='" + SYSTEM_ADMIN + "' and user_id=" + user1.getId())).isEqualTo(1);
+  }
+
   private void expectCount(List<Long> projectIds, UserCountPerProjectPermission... expected) {
     List<UserCountPerProjectPermission> got = underTest.countUsersByProjectPermission(dbTester.getSession(), projectIds);
     assertThat(got).hasSize(expected.length);
-- 
2.39.5