]> source.dussan.org Git - sonarqube.git/commitdiff
SONAR-9014 Prevent groups synchronization to remove 'sonar-users' membership
authorJulien Lancelot <julien.lancelot@sonarsource.com>
Wed, 29 Mar 2017 15:03:23 +0000 (17:03 +0200)
committerJulien Lancelot <julien.lancelot@sonarsource.com>
Thu, 13 Apr 2017 09:51:55 +0000 (11:51 +0200)
it/it-tests/src/test/java/it/user/BaseIdentityProviderTest.java
it/it-tests/src/test/java/it/user/SsoAuthenticationTest.java
it/it-tests/src/test/java/util/user/UserRule.java
server/sonar-server/src/main/java/org/sonar/server/authentication/UserIdentityAuthenticator.java
server/sonar-server/src/test/java/org/sonar/server/authentication/SsoAuthenticatorTest.java
server/sonar-server/src/test/java/org/sonar/server/authentication/UserIdentityAuthenticatorTest.java

index 9e33aebd20f29f4678c36d5a1a480dc5bf869d1b..b173f0b023cdc7c6fcaaa087f66a7e8b7b50cdff 100644 (file)
@@ -220,7 +220,7 @@ public class BaseIdentityProviderTest {
 
     authenticateWithFakeAuthProvider();
 
-    userRule.verifyUserGroupMembership(USER_LOGIN, GROUP1, GROUP2);
+    userRule.verifyUserGroupMembership(USER_LOGIN, GROUP1, GROUP2, "sonar-users");
   }
 
   @Test
@@ -237,7 +237,7 @@ public class BaseIdentityProviderTest {
 
     authenticateWithFakeAuthProvider();
 
-    userRule.verifyUserGroupMembership(USER_LOGIN, GROUP2, GROUP3);
+    userRule.verifyUserGroupMembership(USER_LOGIN, GROUP2, GROUP3, "sonar-users");
   }
 
   @Test
@@ -253,7 +253,7 @@ public class BaseIdentityProviderTest {
     authenticateWithFakeAuthProvider();
 
     // User is not member to any group
-    userRule.verifyUserGroupMembership(USER_LOGIN);
+    userRule.verifyUserGroupMembership(USER_LOGIN, "sonar-users");
   }
 
   @Test
index 50d25bcb3b617ed20d4bd2b12ece70896b8716d0..fdd127d56df436cfee860e2d14307517f9f71f28 100644 (file)
@@ -99,7 +99,7 @@ public class SsoAuthenticationTest {
   public void authenticate_with_groups() {
     doCall(USER_LOGIN, null, null, GROUP_1);
 
-    USER_RULE.verifyUserGroupMembership(USER_LOGIN, GROUP_1);
+    USER_RULE.verifyUserGroupMembership(USER_LOGIN, GROUP_1, "sonar-users");
   }
 
   @Test
@@ -112,7 +112,7 @@ public class SsoAuthenticationTest {
 
     doCall(USER_LOGIN, null, null, GROUP_2 + "," + GROUP_3);
 
-    USER_RULE.verifyUserGroupMembership(USER_LOGIN, GROUP_2, GROUP_3);
+    USER_RULE.verifyUserGroupMembership(USER_LOGIN, GROUP_2, GROUP_3, "sonar-users");
   }
 
   @Test
index 038e0f150eb280d7aef45ee16025684c35d5fa70..9dcc39defebf47de0573d7c1f50067b3113e21eb 100644 (file)
  */
 package util.user;
 
-import com.google.common.base.Function;
 import com.google.common.base.Optional;
 import com.google.common.base.Predicate;
 import com.google.common.collect.FluentIterable;
 import com.sonar.orchestrator.Orchestrator;
 import java.util.List;
+import java.util.stream.Collectors;
 import javax.annotation.CheckForNull;
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
@@ -156,8 +156,8 @@ public class UserRule extends ExternalResource implements GroupManagement {
     @Override
     public void createGroup(String name, @Nullable String description) {
       PostRequest request = new PostRequest("api/user_groups/create")
-          .setParam("name", name)
-          .setParam("description", description);
+        .setParam("name", name)
+        .setParam("description", description);
       addOrganizationParam(request);
       adminWsClient().wsConnector().call(request);
     }
@@ -179,7 +179,7 @@ public class UserRule extends ExternalResource implements GroupManagement {
       for (String groupName : groupNames) {
         if (getGroupByName(groupName).isPresent()) {
           PostRequest request = new PostRequest("api/user_groups/delete")
-              .setParam("name", groupName);
+            .setParam("name", groupName);
           addOrganizationParam(request);
           adminWsClient().wsConnector().call(request);
         }
@@ -206,17 +206,17 @@ public class UserRule extends ExternalResource implements GroupManagement {
     }
 
     @Override
-    public void verifyUserGroupMembership(String userLogin, String... groups) {
+    public void verifyUserGroupMembership(String userLogin, String... expectedGroups) {
       Groups userGroup = getUserGroups(userLogin);
-      List<String> userGroupName = FluentIterable.from(userGroup.getGroups()).transform(ToGroupName.INSTANCE).toList();
-      assertThat(userGroupName).containsOnly(groups);
+      List<String> userGroupName = userGroup.getGroups().stream().map(Groups.Group::getName).collect(Collectors.toList());
+      assertThat(userGroupName).containsOnly(expectedGroups);
     }
 
     @Override
     public Groups getUserGroups(String userLogin) {
       GetRequest request = new GetRequest("api/users/groups")
-          .setParam("login", userLogin)
-          .setParam("selected", "selected");
+        .setParam("login", userLogin)
+        .setParam("selected", "selected");
       addOrganizationParam(request);
       WsResponse response = adminWsClient().wsConnector().call(request).failIfNotSuccessful();
       return Groups.parse(response.content());
@@ -226,8 +226,8 @@ public class UserRule extends ExternalResource implements GroupManagement {
     public void associateGroupsToUser(String userLogin, String... groups) {
       for (String group : groups) {
         PostRequest request = new PostRequest("api/user_groups/add_user")
-            .setParam("login", userLogin)
-            .setParam("name", group);
+          .setParam("login", userLogin)
+          .setParam("name", group);
         addOrganizationParam(request);
         adminWsClient().wsConnector().call(request).failIfNotSuccessful();
       }
@@ -314,12 +314,4 @@ public class UserRule extends ExternalResource implements GroupManagement {
     }
   }
 
-  private enum ToGroupName implements Function<Groups.Group, String> {
-    INSTANCE;
-
-    @Override
-    public String apply(@Nonnull Groups.Group group) {
-      return group.getName();
-    }
-  }
 }
index 04c89c7100350bcf37b49fb1764031aaaddbcbe6..bd300a41ea665fae0c207a16fa1be4724ac8f59c 100644 (file)
@@ -47,6 +47,7 @@ import org.sonar.server.user.UserUpdater;
 import static java.lang.String.format;
 import static java.util.Collections.singletonList;
 import static org.sonar.core.util.stream.MoreCollectors.uniqueIndex;
+import static org.sonar.server.user.UserUpdater.SONAR_USERS_GROUP_NAME;
 
 public class UserIdentityAuthenticator {
 
@@ -122,26 +123,27 @@ public class UserIdentityAuthenticator {
   }
 
   private void syncGroups(DbSession dbSession, UserIdentity userIdentity, UserDto userDto) {
-    if (userIdentity.shouldSyncGroups()) {
-      String userLogin = userIdentity.getLogin();
-      Set<String> userGroups = new HashSet<>(dbClient.groupMembershipDao().selectGroupsByLogins(dbSession, singletonList(userLogin)).get(userLogin));
-      Set<String> identityGroups = userIdentity.getGroups();
-      LOGGER.debug("List of groups returned by the identity provider '{}'", identityGroups);
-
-      Collection<String> groupsToAdd = Sets.difference(identityGroups, userGroups);
-      Collection<String> groupsToRemove = Sets.difference(userGroups, identityGroups);
-      Collection<String> allGroups = new ArrayList<>(groupsToAdd);
-      allGroups.addAll(groupsToRemove);
-      DefaultOrganization defaultOrganization = defaultOrganizationProvider.get();
-      Map<String, GroupDto> groupsByName = dbClient.groupDao().selectByNames(dbSession, defaultOrganization.getUuid(), allGroups)
-        .stream()
-        .collect(uniqueIndex(GroupDto::getName));
-
-      addGroups(dbSession, userDto, groupsToAdd, groupsByName);
-      removeGroups(dbSession, userDto, groupsToRemove, groupsByName);
-
-      dbSession.commit();
+    if (!userIdentity.shouldSyncGroups()) {
+      return;
     }
+    String userLogin = userIdentity.getLogin();
+    Set<String> userGroups = new HashSet<>(dbClient.groupMembershipDao().selectGroupsByLogins(dbSession, singletonList(userLogin)).get(userLogin));
+    Set<String> identityGroups = userIdentity.getGroups();
+    LOGGER.debug("List of groups returned by the identity provider '{}'", identityGroups);
+
+    Collection<String> groupsToAdd = Sets.difference(identityGroups, userGroups);
+    Collection<String> groupsToRemove = Sets.difference(userGroups, identityGroups);
+    Collection<String> allGroups = new ArrayList<>(groupsToAdd);
+    allGroups.addAll(groupsToRemove);
+    DefaultOrganization defaultOrganization = defaultOrganizationProvider.get();
+    Map<String, GroupDto> groupsByName = dbClient.groupDao().selectByNames(dbSession, defaultOrganization.getUuid(), allGroups)
+      .stream()
+      .collect(uniqueIndex(GroupDto::getName));
+
+    addGroups(dbSession, userDto, groupsToAdd, groupsByName);
+    removeGroups(dbSession, userDto, groupsToRemove, groupsByName);
+
+    dbSession.commit();
   }
 
   private void addGroups(DbSession dbSession, UserDto userDto, Collection<String> groupsToAdd, Map<String, GroupDto> groupsByName) {
@@ -153,8 +155,11 @@ public class UserIdentityAuthenticator {
   }
 
   private void removeGroups(DbSession dbSession, UserDto userDto, Collection<String> groupsToRemove, Map<String, GroupDto> groupsByName) {
-    groupsToRemove.stream().map(groupsByName::get).filter(Objects::nonNull).forEach(
-      groupDto -> {
+    groupsToRemove.stream().map(groupsByName::get)
+      .filter(Objects::nonNull)
+      // user should always be member of sonar-users group
+      .filter(group -> !group.getName().equals(SONAR_USERS_GROUP_NAME))
+      .forEach(groupDto -> {
         LOGGER.debug("Removing group '{}' from user '{}'", groupDto.getName(), userDto.getLogin());
         dbClient.userGroupDao().delete(dbSession, groupDto.getId(), userDto.getId());
       });
index e79ebdfc0f1ee26e782c798d8422350e41cb042c..c99f91d13fa90d439722321486ad2c2104b8e930 100644 (file)
@@ -123,7 +123,7 @@ public class SsoAuthenticatorTest {
 
     underTest.authenticate(request, response);
 
-    verifyUserInDb(DEFAULT_LOGIN, DEFAULT_NAME, DEFAULT_EMAIL, group1, group2);
+    verifyUserInDb(DEFAULT_LOGIN, DEFAULT_NAME, DEFAULT_EMAIL, group1, group2, sonarUsers);
     verifyTokenIsUpdated(NOW);
     verify(authenticationEvent).loginSuccess(request, DEFAULT_LOGIN, Source.sso());
   }
@@ -188,12 +188,12 @@ public class SsoAuthenticatorTest {
   public void does_not_update_groups_when_no_group_headers() throws Exception {
     startWithSso();
     setNotUserInToken();
-    insertUser(DEFAULT_USER, group1);
+    insertUser(DEFAULT_USER, group1, sonarUsers);
     HttpServletRequest request = createRequest(DEFAULT_LOGIN, DEFAULT_NAME, DEFAULT_EMAIL, null);
 
     underTest.authenticate(request, response);
 
-    verityUserGroups(DEFAULT_LOGIN, group1);
+    verityUserGroups(DEFAULT_LOGIN, group1, sonarUsers);
     verify(authenticationEvent).loginSuccess(request, DEFAULT_LOGIN, Source.sso());
   }
 
@@ -269,7 +269,7 @@ public class SsoAuthenticatorTest {
 
     underTest.authenticate(request, response);
 
-    verifyUserInDb("AnotherLogin", "Another name", "Another email", group2);
+    verifyUserInDb("AnotherLogin", "Another name", "Another email", group2, sonarUsers);
     verifyTokenIsUpdated(NOW);
     verify(authenticationEvent).loginSuccess(request, "AnotherLogin", Source.sso());
   }
@@ -286,7 +286,7 @@ public class SsoAuthenticatorTest {
 
     underTest.authenticate(request, response);
 
-    verifyUserInDb(DEFAULT_LOGIN, DEFAULT_NAME, DEFAULT_EMAIL, group1, group2);
+    verifyUserInDb(DEFAULT_LOGIN, DEFAULT_NAME, DEFAULT_EMAIL, group1, group2, sonarUsers);
     verify(authenticationEvent).loginSuccess(request, DEFAULT_LOGIN, Source.sso());
   }
 
@@ -302,7 +302,7 @@ public class SsoAuthenticatorTest {
 
     underTest.authenticate(request, response);
 
-    verifyUserInDb(DEFAULT_LOGIN, DEFAULT_NAME, DEFAULT_EMAIL, group1, group2);
+    verifyUserInDb(DEFAULT_LOGIN, DEFAULT_NAME, DEFAULT_EMAIL, group1, group2, sonarUsers);
     verify(authenticationEvent).loginSuccess(request, DEFAULT_LOGIN, Source.sso());
   }
 
@@ -314,7 +314,7 @@ public class SsoAuthenticatorTest {
 
     underTest.authenticate(request, response);
 
-    verifyUserInDb(DEFAULT_LOGIN, DEFAULT_LOGIN, null, group1, group2);
+    verifyUserInDb(DEFAULT_LOGIN, DEFAULT_LOGIN, null, group1, group2, sonarUsers);
     verify(authenticationEvent).loginSuccess(request, DEFAULT_LOGIN, Source.sso());
   }
 
index 9f3fc517940d0453f6ff836d123da6823d61bf43..dc1bfe4dc2826257a3fb178710479553bb8c3657 100644 (file)
@@ -19,8 +19,8 @@
  */
 package org.sonar.server.authentication;
 
-import java.util.Arrays;
 import java.util.Optional;
+import java.util.stream.Collectors;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
@@ -41,6 +41,7 @@ import org.sonar.server.user.UserUpdater;
 import org.sonar.server.user.index.UserIndexer;
 
 import static com.google.common.collect.Sets.newHashSet;
+import static java.util.Arrays.stream;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.Mockito.mock;
 import static org.sonar.db.user.UserTesting.newUserDto;
@@ -51,7 +52,6 @@ import static org.sonar.server.authentication.event.AuthenticationExceptionMatch
 public class UserIdentityAuthenticatorTest {
 
   private static String USER_LOGIN = "github-johndoo";
-  private static String DEFAULT_GROUP = "default";
 
   private static UserIdentity USER_IDENTITY = UserIdentity.builder()
     .setProviderLogin("johndoo")
@@ -102,7 +102,7 @@ public class UserIdentityAuthenticatorTest {
     assertThat(user.getExternalIdentityProvider()).isEqualTo("github");
     assertThat(user.isRoot()).isFalse();
 
-    assertThat(db.users().selectGroupIdsOfUser(user)).containsOnly(defaultGroup.getId());
+    checkGroupMembership(user, defaultGroup);
   }
 
   @Test
@@ -113,10 +113,7 @@ public class UserIdentityAuthenticatorTest {
     authenticate(USER_LOGIN, "group1", "group2", "group3");
 
     Optional<UserDto> user = db.users().selectUserByLogin(USER_LOGIN);
-    assertThat(user).isPresent();
-    assertThat(user.get().isRoot()).isFalse();
-
-    assertThat(db.users().selectGroupIdsOfUser(user.get())).containsOnly(group1.getId(), group2.getId());
+    checkGroupMembership(user.get(), group1, group2, defaultGroup);
   }
 
   @Test
@@ -169,10 +166,11 @@ public class UserIdentityAuthenticatorTest {
       .setName("John"));
     GroupDto group1 = db.users().insertGroup(db.getDefaultOrganization(), "group1");
     GroupDto group2 = db.users().insertGroup(db.getDefaultOrganization(), "group2");
+    db.users().insertMember(defaultGroup, user);
 
     authenticate(USER_LOGIN, "group1", "group2", "group3");
 
-    assertThat(db.users().selectGroupIdsOfUser(user)).containsOnly(group1.getId(), group2.getId());
+    checkGroupMembership(user, group1, group2, defaultGroup);
   }
 
   @Test
@@ -185,23 +183,25 @@ public class UserIdentityAuthenticatorTest {
     GroupDto group2 = db.users().insertGroup(db.getDefaultOrganization(), "group2");
     db.users().insertMember(group1, user);
     db.users().insertMember(group2, user);
+    db.users().insertMember(defaultGroup, user);
 
     authenticate(USER_LOGIN, "group1");
 
-    assertThat(db.users().selectGroupIdsOfUser(user)).containsOnly(group1.getId());
+    checkGroupMembership(user, group1, defaultGroup);
   }
 
   @Test
-  public void authenticate_existing_user_and_remove_all_groups() throws Exception {
+  public void authenticate_existing_user_and_remove_all_groups_expect_default() throws Exception {
     UserDto user = db.users().insertUser();
     GroupDto group1 = db.users().insertGroup(db.getDefaultOrganization(), "group1");
     GroupDto group2 = db.users().insertGroup(db.getDefaultOrganization(), "group2");
     db.users().insertMember(group1, user);
     db.users().insertMember(group2, user);
+    db.users().insertMember(defaultGroup, user);
 
     authenticate(user.getLogin());
 
-    assertThat(db.users().selectGroupIdsOfUser(user)).isEmpty();
+    checkGroupMembership(user, defaultGroup);
   }
 
   @Test
@@ -211,6 +211,7 @@ public class UserIdentityAuthenticatorTest {
       .setLogin(USER_LOGIN)
       .setActive(true)
       .setName("John"));
+    db.users().insertMember(defaultGroup, user);
     String groupName = "a-group";
     GroupDto groupInDefaultOrg = db.users().insertGroup(db.getDefaultOrganization(), groupName);
     GroupDto groupInOrg = db.users().insertGroup(org, groupName);
@@ -223,7 +224,7 @@ public class UserIdentityAuthenticatorTest {
       .setGroups(newHashSet(groupName))
       .build(), IDENTITY_PROVIDER, Source.sso());
 
-    assertThat(db.users().selectGroupIdsOfUser(user)).containsOnly(groupInDefaultOrg.getId());
+    checkGroupMembership(user, groupInDefaultOrg, defaultGroup);
   }
 
   @Test
@@ -262,8 +263,11 @@ public class UserIdentityAuthenticatorTest {
       .setLogin(login)
       .setName("John")
       // No group
-      .setGroups(Arrays.stream(groups).collect(MoreCollectors.toSet()))
+      .setGroups(stream(groups).collect(MoreCollectors.toSet()))
       .build(), IDENTITY_PROVIDER, Source.sso());
   }
 
+  private void checkGroupMembership(UserDto user, GroupDto... expectedGroups) {
+    assertThat(db.users().selectGroupIdsOfUser(user)).containsOnly(stream(expectedGroups).map(GroupDto::getId).collect(Collectors.toList()).toArray(new Integer[] {}));
+  }
 }