From: Julien Lancelot Date: Wed, 29 Mar 2017 15:03:23 +0000 (+0200) Subject: SONAR-9014 Prevent groups synchronization to remove 'sonar-users' membership X-Git-Tag: 6.4-RC1~431 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=7473ac7bf8e69bd1324b98e2a4f7e3c6c6967e22;p=sonarqube.git SONAR-9014 Prevent groups synchronization to remove 'sonar-users' membership --- diff --git a/it/it-tests/src/test/java/it/user/BaseIdentityProviderTest.java b/it/it-tests/src/test/java/it/user/BaseIdentityProviderTest.java index 9e33aebd20f..b173f0b023c 100644 --- a/it/it-tests/src/test/java/it/user/BaseIdentityProviderTest.java +++ b/it/it-tests/src/test/java/it/user/BaseIdentityProviderTest.java @@ -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 diff --git a/it/it-tests/src/test/java/it/user/SsoAuthenticationTest.java b/it/it-tests/src/test/java/it/user/SsoAuthenticationTest.java index 50d25bcb3b6..fdd127d56df 100644 --- a/it/it-tests/src/test/java/it/user/SsoAuthenticationTest.java +++ b/it/it-tests/src/test/java/it/user/SsoAuthenticationTest.java @@ -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 diff --git a/it/it-tests/src/test/java/util/user/UserRule.java b/it/it-tests/src/test/java/util/user/UserRule.java index 038e0f150eb..9dcc39defeb 100644 --- a/it/it-tests/src/test/java/util/user/UserRule.java +++ b/it/it-tests/src/test/java/util/user/UserRule.java @@ -19,12 +19,12 @@ */ 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 userGroupName = FluentIterable.from(userGroup.getGroups()).transform(ToGroupName.INSTANCE).toList(); - assertThat(userGroupName).containsOnly(groups); + List 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 { - INSTANCE; - - @Override - public String apply(@Nonnull Groups.Group group) { - return group.getName(); - } - } } diff --git a/server/sonar-server/src/main/java/org/sonar/server/authentication/UserIdentityAuthenticator.java b/server/sonar-server/src/main/java/org/sonar/server/authentication/UserIdentityAuthenticator.java index 04c89c71003..bd300a41ea6 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/authentication/UserIdentityAuthenticator.java +++ b/server/sonar-server/src/main/java/org/sonar/server/authentication/UserIdentityAuthenticator.java @@ -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 userGroups = new HashSet<>(dbClient.groupMembershipDao().selectGroupsByLogins(dbSession, singletonList(userLogin)).get(userLogin)); - Set identityGroups = userIdentity.getGroups(); - LOGGER.debug("List of groups returned by the identity provider '{}'", identityGroups); - - Collection groupsToAdd = Sets.difference(identityGroups, userGroups); - Collection groupsToRemove = Sets.difference(userGroups, identityGroups); - Collection allGroups = new ArrayList<>(groupsToAdd); - allGroups.addAll(groupsToRemove); - DefaultOrganization defaultOrganization = defaultOrganizationProvider.get(); - Map 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 userGroups = new HashSet<>(dbClient.groupMembershipDao().selectGroupsByLogins(dbSession, singletonList(userLogin)).get(userLogin)); + Set identityGroups = userIdentity.getGroups(); + LOGGER.debug("List of groups returned by the identity provider '{}'", identityGroups); + + Collection groupsToAdd = Sets.difference(identityGroups, userGroups); + Collection groupsToRemove = Sets.difference(userGroups, identityGroups); + Collection allGroups = new ArrayList<>(groupsToAdd); + allGroups.addAll(groupsToRemove); + DefaultOrganization defaultOrganization = defaultOrganizationProvider.get(); + Map 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 groupsToAdd, Map groupsByName) { @@ -153,8 +155,11 @@ public class UserIdentityAuthenticator { } private void removeGroups(DbSession dbSession, UserDto userDto, Collection groupsToRemove, Map 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()); }); diff --git a/server/sonar-server/src/test/java/org/sonar/server/authentication/SsoAuthenticatorTest.java b/server/sonar-server/src/test/java/org/sonar/server/authentication/SsoAuthenticatorTest.java index e79ebdfc0f1..c99f91d13fa 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/authentication/SsoAuthenticatorTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/authentication/SsoAuthenticatorTest.java @@ -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()); } diff --git a/server/sonar-server/src/test/java/org/sonar/server/authentication/UserIdentityAuthenticatorTest.java b/server/sonar-server/src/test/java/org/sonar/server/authentication/UserIdentityAuthenticatorTest.java index 9f3fc517940..dc1bfe4dc28 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/authentication/UserIdentityAuthenticatorTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/authentication/UserIdentityAuthenticatorTest.java @@ -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 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[] {})); + } }