SONAR-6483 WS permissions/add_group add by group id or group name & rely on PermissionUpdater

12 files changed, 342 insertions, 41 deletions

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 28ad37a2955..867a553dc9a 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
@@ -47,11 +47,10 @@ import org.sonar.server.user.UserSession;
 @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";
@@ -62,6 +61,7 @@ public class PermissionService {
   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) {
     this.dbClient = dbClient;
@@ -92,9 +92,19 @@ public class PermissionService {
    * To be used only by jruby webapp
    */
   public void addPermission(Map<String, Object> params) {
-    addPermission(PermissionChange.buildFromParams(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 {
diff --git a/server/sonar-server/src/main/java/org/sonar/server/permission/PermissionUpdater.java b/server/sonar-server/src/main/java/org/sonar/server/permission/PermissionUpdater.java
new file mode 100644
index 00000000000..159269f1da9
--- /dev/null
+++ b/server/sonar-server/src/main/java/org/sonar/server/permission/PermissionUpdater.java
@@ -0,0 +1,220 @@
+/*
+ * 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 java.util.List;
+import javax.annotation.CheckForNull;
+import javax.annotation.Nullable;
+import org.sonar.api.security.DefaultGroups;
+import org.sonar.api.web.UserRole;
+import org.sonar.core.permission.GlobalPermissions;
+import org.sonar.db.DbClient;
+import org.sonar.db.DbSession;
+import org.sonar.db.component.ComponentDto;
+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;
+
+public class PermissionUpdater {
+
+  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 IssueAuthorizationIndexer issueAuthorizationIndexer;
+  private final UserSession userSession;
+  private final ComponentFinder componentFinder;
+
+  public PermissionUpdater(DbClient dbClient, PermissionRepository permissionRepository,
+    IssueAuthorizationIndexer issueAuthorizationIndexer, UserSession userSession, ComponentFinder componentFinder) {
+    this.dbClient = dbClient;
+    this.permissionRepository = permissionRepository;
+    this.issueAuthorizationIndexer = issueAuthorizationIndexer;
+    this.userSession = userSession;
+    this.componentFinder = componentFinder;
+  }
+
+  public static List<String> globalPermissions() {
+    return GlobalPermissions.ALL;
+  }
+
+  public void addPermission(PermissionChange change) {
+    DbSession session = dbClient.openSession(false);
+    try {
+      applyChange(Operation.ADD, change, session);
+    } finally {
+      dbClient.closeSession(session);
+    }
+  }
+
+  public void removePermission(PermissionChange change) {
+    DbSession session = dbClient.openSession(false);
+    try {
+      applyChange(Operation.REMOVE, change, session);
+    } finally {
+      session.close();
+    }
+  }
+
+  private void applyChange(Operation operation, PermissionChange change, DbSession session) {
+    userSession.checkLoggedIn();
+    change.validate();
+    boolean changed;
+    if (change.user() != null) {
+      changed = applyChangeOnUser(session, operation, change);
+    } else {
+      changed = applyChangeOnGroup(session, operation, change);
+    }
+    if (changed) {
+      session.commit();
+      if (change.component() != null) {
+        indexProjectPermissions();
+      }
+    }
+  }
+
+  private boolean applyChangeOnGroup(DbSession session, Operation operation, PermissionChange permissionChange) {
+    Long componentId = getComponentId(session, permissionChange.component());
+    checkProjectAdminPermission(permissionChange.component());
+
+    List<String> existingPermissions = dbClient.roleDao().selectGroupPermissions(session, permissionChange.group(), componentId);
+    if (shouldSkipPermissionChange(operation, existingPermissions, permissionChange)) {
+      return false;
+    }
+
+    Long targetedGroup = getTargetedGroup(session, permissionChange.group());
+    String permission = permissionChange.permission();
+    if (Operation.ADD == operation) {
+      checkNotAnyoneAndAdmin(permission, permissionChange.group());
+      permissionRepository.insertGroupPermission(componentId, targetedGroup, permission, session);
+    } else {
+      checkAdminUsersExistOutsideTheRemovedGroup(session, permissionChange, targetedGroup);
+      permissionRepository.deleteGroupPermission(componentId, targetedGroup, permission, session);
+    }
+    return true;
+  }
+
+  private 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.component());
+    checkProjectAdminPermission(permissionChange.component());
+
+    List<String> existingPermissions = dbClient.roleDao().selectUserPermissions(session, permissionChange.user(), componentId);
+    if (shouldSkipPermissionChange(operation, existingPermissions, permissionChange)) {
+      return false;
+    }
+
+    Long targetedUser = getTargetedUser(session, permissionChange.user());
+    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);
+    badRequestIfNullResult(user, OBJECT_TYPE_USER, userLogin);
+    return user.getId();
+  }
+
+  @Nullable
+  private Long getTargetedGroup(DbSession session, String group) {
+    if (DefaultGroups.isAnyone(group)) {
+      return null;
+    } else {
+      GroupDto groupDto = dbClient.userDao().selectGroupByName(group, session);
+      badRequestIfNullResult(groupDto, OBJECT_TYPE_GROUP, group);
+      return groupDto.getId();
+    }
+  }
+
+  private boolean shouldSkipPermissionChange(Operation operation, List<String> 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 Object badRequestIfNullResult(@Nullable Object 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);
+    } else {
+      if (!userSession.hasGlobalPermission(GlobalPermissions.SYSTEM_ADMIN) && !userSession.hasProjectPermission(UserRole.ADMIN, projectKey)) {
+        throw new ForbiddenException("Insufficient privileges");
+      }
+    }
+  }
+
+  private void indexProjectPermissions() {
+    issueAuthorizationIndexer.index();
+  }
+}
diff --git a/server/sonar-server/src/main/java/org/sonar/server/permission/ws/AddGroupAction.java b/server/sonar-server/src/main/java/org/sonar/server/permission/ws/AddGroupAction.java
index 30f7bfc2a44..9d4749bccf5 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/permission/ws/AddGroupAction.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/permission/ws/AddGroupAction.java
@@ -20,29 +20,40 @@
 
 package org.sonar.server.permission.ws;
 
+import javax.annotation.Nullable;
 import org.sonar.api.server.ws.Request;
 import org.sonar.api.server.ws.Response;
 import org.sonar.api.server.ws.WebService;
 import org.sonar.core.permission.GlobalPermissions;
-import org.sonar.server.permission.PermissionService;
+import org.sonar.db.DbClient;
+import org.sonar.db.DbSession;
+import org.sonar.db.user.GroupDto;
+import org.sonar.server.exceptions.BadRequestException;
+import org.sonar.server.exceptions.NotFoundException;
 import org.sonar.server.permission.PermissionChange;
+import org.sonar.server.permission.PermissionUpdater;
 
 public class AddGroupAction implements PermissionsWsAction {
 
   public static final String ACTION = "add_group";
   public static final String PARAM_PERMISSION = "permission";
   public static final String PARAM_GROUP_NAME = "groupName";
+  public static final String PARAM_GROUP_ID = "groupId";
 
-  private final PermissionService permissionService;
+  private final PermissionUpdater permissionUpdater;
+  private final DbClient dbClient;
 
-  public AddGroupAction(PermissionService permissionService) {
-    this.permissionService = permissionService;
+  public AddGroupAction(PermissionUpdater permissionUpdater, DbClient dbClient) {
+    this.permissionUpdater = permissionUpdater;
+    this.dbClient = dbClient;
   }
 
   @Override
   public void define(WebService.NewController context) {
     WebService.NewAction action = context.createAction(ACTION)
-      .setDescription("Add permission to a group.<br /> Requires 'Administer System' permission.")
+      .setDescription("Add permission to a group.<br /> " +
+        "The group name or group id must be provided. <br />" +
+        "Requires 'Administer System' permission.")
       .setSince("5.2")
       .setPost(true)
       .setHandler(this);
@@ -53,21 +64,55 @@ public class AddGroupAction implements PermissionsWsAction {
       .setPossibleValues(GlobalPermissions.ALL);
 
     action.createParam(PARAM_GROUP_NAME)
-      .setRequired(true)
       .setDescription("Group name or 'anyone' (whatever the case)")
       .setExampleValue("sonar-administrators");
+
+    action.createParam(PARAM_GROUP_ID)
+      .setDescription("Group ID")
+      .setExampleValue("42");
   }
 
   @Override
   public void handle(Request request, Response response) throws Exception {
     String permission = request.mandatoryParam(PARAM_PERMISSION);
-    String groupName = request.mandatoryParam(PARAM_GROUP_NAME);
-    permissionService.addPermission(
+    String groupNameParam = request.param(PARAM_GROUP_NAME);
+    Long groupId = request.paramAsLong(PARAM_GROUP_ID);
+
+    String groupName = searchName(groupNameParam, groupId);
+
+    permissionUpdater.addPermission(
       new PermissionChange()
         .setPermission(permission)
         .setGroup(groupName)
-      );
+    );
 
     response.noContent();
   }
+
+  private String searchName(@Nullable String groupNameParam, @Nullable Long groupId) {
+    checkParameters(groupNameParam, groupId);
+    if (groupNameParam != null) {
+      return groupNameParam;
+    }
+
+    DbSession dbSession = dbClient.openSession(false);
+    try {
+      GroupDto group = dbClient.groupDao().selectById(dbSession, groupId);
+      if (group == null) {
+        throw new NotFoundException(String.format("Group with id '%d' not found", groupId));
+      }
+
+      return group.getName();
+    } finally {
+      dbClient.closeSession(dbSession);
+    }
+  }
+
+  private void checkParameters(@Nullable String groupName, @Nullable Long groupId) {
+    if (groupName != null ^ groupId != null) {
+      return;
+    }
+
+    throw new BadRequestException("Group name or group id must be provided, not both");
+  }
 }
diff --git a/server/sonar-server/src/main/java/org/sonar/server/permission/ws/AddUserAction.java b/server/sonar-server/src/main/java/org/sonar/server/permission/ws/AddUserAction.java
index db164279b8d..39cd5f0a370 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/permission/ws/AddUserAction.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/permission/ws/AddUserAction.java
@@ -24,8 +24,8 @@ import org.sonar.api.server.ws.Request;
 import org.sonar.api.server.ws.Response;
 import org.sonar.api.server.ws.WebService;
 import org.sonar.core.permission.GlobalPermissions;
-import org.sonar.server.permission.PermissionService;
 import org.sonar.server.permission.PermissionChange;
+import org.sonar.server.permission.PermissionUpdater;
 
 public class AddUserAction implements PermissionsWsAction {
 
@@ -33,10 +33,10 @@ public class AddUserAction implements PermissionsWsAction {
   public static final String PARAM_PERMISSION = "permission";
   public static final String PARAM_USER_LOGIN = "login";
 
-  private final PermissionService permissionService;
+  private final PermissionUpdater permissionUpdater;
 
-  public AddUserAction(PermissionService permissionService) {
-    this.permissionService = permissionService;
+  public AddUserAction(PermissionUpdater permissionUpdater) {
+    this.permissionUpdater = permissionUpdater;
   }
 
   @Override
@@ -62,11 +62,11 @@ public class AddUserAction implements PermissionsWsAction {
   public void handle(Request request, Response response) throws Exception {
     String permission = request.mandatoryParam(PARAM_PERMISSION);
     String userLogin = request.mandatoryParam(PARAM_USER_LOGIN);
-    permissionService.addPermission(
+    permissionUpdater.addPermission(
       new PermissionChange()
         .setPermission(permission)
         .setUser(userLogin)
-      );
+    );
 
     response.noContent();
   }
diff --git a/server/sonar-server/src/main/java/org/sonar/server/platform/platformlevel/PlatformLevel4.java b/server/sonar-server/src/main/java/org/sonar/server/platform/platformlevel/PlatformLevel4.java
index e86eb3208d5..efed066e65e 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/platform/platformlevel/PlatformLevel4.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/platform/platformlevel/PlatformLevel4.java
@@ -172,6 +172,7 @@ import org.sonar.server.notification.email.EmailNotificationChannel;
 import org.sonar.server.permission.PermissionFinder;
 import org.sonar.server.permission.PermissionService;
 import org.sonar.server.permission.PermissionTemplateService;
+import org.sonar.server.permission.PermissionUpdater;
 import org.sonar.server.permission.ws.PermissionsWsModule;
 import org.sonar.server.platform.BackendCleanup;
 import org.sonar.server.platform.SettingsChangeNotifier;
@@ -553,6 +554,7 @@ public class PlatformLevel4 extends PlatformLevel {
     // permissions
       PermissionRepository.class,
       PermissionService.class,
+      PermissionUpdater.class,
       PermissionTemplateService.class,
       PermissionFinder.class,
       PermissionsWsModule.class,
diff --git a/server/sonar-server/src/test/java/org/sonar/server/issue/IssueBulkChangeServiceMediumTest.java b/server/sonar-server/src/test/java/org/sonar/server/issue/IssueBulkChangeServiceMediumTest.java
index 1c05f9e3cc1..7cc491d8d36 100644
--- a/server/sonar-server/src/test/java/org/sonar/server/issue/IssueBulkChangeServiceMediumTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/issue/IssueBulkChangeServiceMediumTest.java
@@ -47,8 +47,8 @@ import org.sonar.db.user.UserDto;
 import org.sonar.server.component.ComponentTesting;
 import org.sonar.server.component.SnapshotTesting;
 import org.sonar.server.issue.index.IssueIndexer;
-import org.sonar.server.permission.PermissionService;
 import org.sonar.server.permission.PermissionChange;
+import org.sonar.server.permission.PermissionUpdater;
 import org.sonar.server.rule.db.RuleDao;
 import org.sonar.server.tester.ServerTester;
 import org.sonar.server.tester.UserSessionRule;
@@ -98,7 +98,7 @@ public class IssueBulkChangeServiceMediumTest {
     // project can be seen by anyone
     session.commit();
     userSessionRule.login("admin").setGlobalPermissions(GlobalPermissions.SYSTEM_ADMIN);
-    tester.get(PermissionService.class).addPermission(new PermissionChange().setComponentKey(project.getKey()).setGroup(DefaultGroups.ANYONE).setPermission(UserRole.USER));
+    tester.get(PermissionUpdater.class).addPermission(new PermissionChange().setComponentKey(project.getKey()).setGroup(DefaultGroups.ANYONE).setPermission(UserRole.USER));
 
     userSession = userSessionRule.login("john")
       .addProjectPermissions(UserRole.USER, project.key());
diff --git a/server/sonar-server/src/test/java/org/sonar/server/issue/IssueCommentServiceMediumTest.java b/server/sonar-server/src/test/java/org/sonar/server/issue/IssueCommentServiceMediumTest.java
index 2834e980860..892b35285fb 100644
--- a/server/sonar-server/src/test/java/org/sonar/server/issue/IssueCommentServiceMediumTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/issue/IssueCommentServiceMediumTest.java
@@ -46,8 +46,8 @@ import org.sonar.db.rule.RuleTesting;
 import org.sonar.server.component.ComponentTesting;
 import org.sonar.server.component.SnapshotTesting;
 import org.sonar.server.issue.index.IssueIndexer;
-import org.sonar.server.permission.PermissionService;
 import org.sonar.server.permission.PermissionChange;
+import org.sonar.server.permission.PermissionUpdater;
 import org.sonar.server.rule.db.RuleDao;
 import org.sonar.server.search.IndexClient;
 import org.sonar.server.tester.ServerTester;
@@ -95,7 +95,7 @@ public class IssueCommentServiceMediumTest {
     // project can be seen by anyone
     session.commit();
     userSessionRule.login("admin").setGlobalPermissions(GlobalPermissions.SYSTEM_ADMIN);
-    tester.get(PermissionService.class).addPermission(new PermissionChange().setComponentKey(project.getKey()).setGroup(DefaultGroups.ANYONE).setPermission(UserRole.USER));
+    tester.get(PermissionUpdater.class).addPermission(new PermissionChange().setComponentKey(project.getKey()).setGroup(DefaultGroups.ANYONE).setPermission(UserRole.USER));
 
     userSessionRule.login("gandalf");
 
diff --git a/server/sonar-server/src/test/java/org/sonar/server/issue/IssueServiceMediumTest.java b/server/sonar-server/src/test/java/org/sonar/server/issue/IssueServiceMediumTest.java
index 52bd7cdd062..eeb9de0cda8 100644
--- a/server/sonar-server/src/test/java/org/sonar/server/issue/IssueServiceMediumTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/issue/IssueServiceMediumTest.java
@@ -63,7 +63,7 @@ import org.sonar.server.issue.index.IssueIndex;
 import org.sonar.server.issue.index.IssueIndexDefinition;
 import org.sonar.server.issue.index.IssueIndexer;
 import org.sonar.server.permission.PermissionChange;
-import org.sonar.server.permission.PermissionService;
+import org.sonar.server.permission.PermissionUpdater;
 import org.sonar.server.rule.db.RuleDao;
 import org.sonar.server.source.index.FileSourcesUpdaterHelper;
 import org.sonar.server.source.index.SourceLineIndexer;
@@ -587,7 +587,7 @@ public class IssueServiceMediumTest {
     session.commit();
 
     // project can be seen by group "anyone"
-    tester.get(PermissionService.class).addPermission(new PermissionChange().setComponentKey(project.getKey()).setGroup(DefaultGroups.ANYONE).setPermission(UserRole.USER));
+    tester.get(PermissionUpdater.class).addPermission(new PermissionChange().setComponentKey(project.getKey()).setGroup(DefaultGroups.ANYONE).setPermission(UserRole.USER));
     userSessionRule.login();
 
     return project;
diff --git a/server/sonar-server/src/test/java/org/sonar/server/issue/ws/SearchActionComponentsMediumTest.java b/server/sonar-server/src/test/java/org/sonar/server/issue/ws/SearchActionComponentsMediumTest.java
index 90274dd1f73..15c32756fde 100644
--- a/server/sonar-server/src/test/java/org/sonar/server/issue/ws/SearchActionComponentsMediumTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/issue/ws/SearchActionComponentsMediumTest.java
@@ -44,7 +44,7 @@ import org.sonar.server.issue.IssueTesting;
 import org.sonar.server.issue.filter.IssueFilterParameters;
 import org.sonar.server.issue.index.IssueIndexer;
 import org.sonar.server.permission.PermissionChange;
-import org.sonar.server.permission.PermissionService;
+import org.sonar.server.permission.PermissionUpdater;
 import org.sonar.server.rule.db.RuleDao;
 import org.sonar.server.tester.ServerTester;
 import org.sonar.server.tester.UserSessionRule;
@@ -552,7 +552,7 @@ public class SearchActionComponentsMediumTest {
 
   private void setAnyoneProjectPermission(ComponentDto project, String permission) {
     userSessionRule.login("admin").setGlobalPermissions(GlobalPermissions.SYSTEM_ADMIN);
-    tester.get(PermissionService.class).addPermission(new PermissionChange().setComponentKey(project.getKey()).setGroup(DefaultGroups.ANYONE).setPermission(permission));
+    tester.get(PermissionUpdater.class).addPermission(new PermissionChange().setComponentKey(project.getKey()).setGroup(DefaultGroups.ANYONE).setPermission(permission));
   }
 
   private IssueDto insertIssue(IssueDto issue) {
diff --git a/server/sonar-server/src/test/java/org/sonar/server/issue/ws/SearchActionMediumTest.java b/server/sonar-server/src/test/java/org/sonar/server/issue/ws/SearchActionMediumTest.java
index 683b94a983c..8526c45dd9a 100644
--- a/server/sonar-server/src/test/java/org/sonar/server/issue/ws/SearchActionMediumTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/issue/ws/SearchActionMediumTest.java
@@ -53,7 +53,7 @@ import org.sonar.server.issue.IssueTesting;
 import org.sonar.server.issue.filter.IssueFilterParameters;
 import org.sonar.server.issue.index.IssueIndexer;
 import org.sonar.server.permission.PermissionChange;
-import org.sonar.server.permission.PermissionService;
+import org.sonar.server.permission.PermissionUpdater;
 import org.sonar.server.rule.db.RuleDao;
 import org.sonar.server.search.QueryContext;
 import org.sonar.server.tester.ServerTester;
@@ -663,7 +663,7 @@ public class SearchActionMediumTest {
   private void setDefaultProjectPermission(ComponentDto project) {
     // project can be seen by anyone and by code viewer
     userSessionRule.login("admin").setGlobalPermissions(GlobalPermissions.SYSTEM_ADMIN);
-    tester.get(PermissionService.class).addPermission(new PermissionChange().setComponentKey(project.getKey()).setGroup(DefaultGroups.ANYONE).setPermission(UserRole.USER));
+    tester.get(PermissionUpdater.class).addPermission(new PermissionChange().setComponentKey(project.getKey()).setGroup(DefaultGroups.ANYONE).setPermission(UserRole.USER));
     userSessionRule.login();
   }
 
diff --git a/server/sonar-server/src/test/java/org/sonar/server/permission/ws/AddGroupActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/permission/ws/AddGroupActionTest.java
index c24989ba181..26745d5185b 100644
--- a/server/sonar-server/src/test/java/org/sonar/server/permission/ws/AddGroupActionTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/permission/ws/AddGroupActionTest.java
@@ -23,15 +23,19 @@ package org.sonar.server.permission.ws;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
+import org.junit.experimental.categories.Category;
 import org.junit.rules.ExpectedException;
 import org.mockito.ArgumentCaptor;
 import org.sonar.api.utils.System2;
 import org.sonar.db.DbTester;
+import org.sonar.db.user.GroupDto;
+import org.sonar.server.exceptions.BadRequestException;
 import org.sonar.server.exceptions.ServerException;
-import org.sonar.server.permission.PermissionService;
 import org.sonar.server.permission.PermissionChange;
+import org.sonar.server.permission.PermissionUpdater;
 import org.sonar.server.tester.UserSessionRule;
 import org.sonar.server.ws.WsTester;
+import org.sonar.test.DbTests;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.Mockito.mock;
@@ -39,6 +43,7 @@ import static org.mockito.Mockito.verify;
 import static org.sonar.core.permission.GlobalPermissions.SYSTEM_ADMIN;
 import static org.sonar.server.permission.ws.AddGroupAction.ACTION;
 
+@Category(DbTests.class)
 public class AddGroupActionTest {
   UserSessionRule userSession = UserSessionRule.standalone();
   WsTester ws;
@@ -46,14 +51,15 @@ public class AddGroupActionTest {
   public DbTester db = DbTester.create(System2.INSTANCE);
   @Rule
   public ExpectedException expectedException = ExpectedException.none();
-  private PermissionService permissionService;
+  private PermissionUpdater permissionUpdater;
 
   @Before
   public void setUp() {
-    permissionService = mock(PermissionService.class);
+    permissionUpdater = mock(PermissionUpdater.class);
     ws = new WsTester(new PermissionsWs(
-      new AddGroupAction(permissionService)));
+      new AddGroupAction(permissionUpdater, db.getDbClient())));
     userSession.login("admin").setGlobalPermissions(SYSTEM_ADMIN);
+
   }
 
   @Test
@@ -64,14 +70,31 @@ public class AddGroupActionTest {
       .execute();
 
     ArgumentCaptor<PermissionChange> permissionChangeCaptor = ArgumentCaptor.forClass(PermissionChange.class);
-    verify(permissionService).addPermission(permissionChangeCaptor.capture());
+    verify(permissionUpdater).addPermission(permissionChangeCaptor.capture());
     PermissionChange permissionChange = permissionChangeCaptor.getValue();
     assertThat(permissionChange.group()).isEqualTo("sonar-administrators");
     assertThat(permissionChange.permission()).isEqualTo(SYSTEM_ADMIN);
   }
 
   @Test
-  public void get_request_are_not_authorized() throws Exception {
+  public void search_with_group_id() throws Exception {
+    GroupDto group = db.getDbClient().groupDao().insert(db.getSession(), new GroupDto()
+      .setName("sonar-administrators"));
+    db.getSession().commit();
+
+    ws.newPostRequest(PermissionsWs.ENDPOINT, ACTION)
+      .setParam(AddGroupAction.PARAM_GROUP_ID, group.getId().toString())
+      .setParam(AddGroupAction.PARAM_PERMISSION, SYSTEM_ADMIN)
+      .execute();
+
+    ArgumentCaptor<PermissionChange> permissionChangeCaptor = ArgumentCaptor.forClass(PermissionChange.class);
+    verify(permissionUpdater).addPermission(permissionChangeCaptor.capture());
+    PermissionChange permissionChange = permissionChangeCaptor.getValue();
+    assertThat(permissionChange.group()).isEqualTo("sonar-administrators");
+  }
+
+  @Test
+  public void fail_when_get_request() throws Exception {
     expectedException.expect(ServerException.class);
 
     ws.newGetRequest(PermissionsWs.ENDPOINT, ACTION)
@@ -81,8 +104,9 @@ public class AddGroupActionTest {
   }
 
   @Test
-  public void fail_when_group_name_is_missing() throws Exception {
-    expectedException.expect(IllegalArgumentException.class);
+  public void fail_when_group_name_and_group_id_are_missing() throws Exception {
+    expectedException.expect(BadRequestException.class);
+    expectedException.expectMessage("Group name or group id must be provided, not both");
 
     ws.newPostRequest(PermissionsWs.ENDPOINT, ACTION)
       .setParam(AddGroupAction.PARAM_PERMISSION, SYSTEM_ADMIN)
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 22f82007879..ee7aac66774 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
@@ -28,8 +28,8 @@ import org.mockito.ArgumentCaptor;
 import org.sonar.api.utils.System2;
 import org.sonar.db.DbTester;
 import org.sonar.server.exceptions.ServerException;
-import org.sonar.server.permission.PermissionService;
 import org.sonar.server.permission.PermissionChange;
+import org.sonar.server.permission.PermissionUpdater;
 import org.sonar.server.tester.UserSessionRule;
 import org.sonar.server.ws.WsTester;
 
@@ -46,13 +46,13 @@ public class AddUserActionTest {
   public DbTester db = DbTester.create(System2.INSTANCE);
   @Rule
   public ExpectedException expectedException = ExpectedException.none();
-  private PermissionService permissionService;
+  private PermissionUpdater permissionUpdater;
 
   @Before
   public void setUp() {
-    permissionService = mock(PermissionService.class);
+    permissionUpdater = mock(PermissionUpdater.class);
     ws = new WsTester(new PermissionsWs(
-      new AddUserAction(permissionService)));
+      new AddUserAction(permissionUpdater)));
     userSession.login("admin").setGlobalPermissions(SYSTEM_ADMIN);
   }
 
@@ -64,7 +64,7 @@ public class AddUserActionTest {
       .execute();
 
     ArgumentCaptor<PermissionChange> permissionChangeCaptor = ArgumentCaptor.forClass(PermissionChange.class);
-    verify(permissionService).addPermission(permissionChangeCaptor.capture());
+    verify(permissionUpdater).addPermission(permissionChangeCaptor.capture());
     PermissionChange permissionChange = permissionChangeCaptor.getValue();
     assertThat(permissionChange.user()).isEqualTo("ray.bradbury");
     assertThat(permissionChange.permission()).isEqualTo(SYSTEM_ADMIN);