From: Lukasz Jarocki Date: Thu, 24 Jun 2021 12:32:01 +0000 (+0200) Subject: SSF-173 SONAR-15074 X-Git-Tag: 9.0.0.45539~49 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=a39e52a53386f08d7a6d68e6dd77d597a110761c;p=sonarqube.git SSF-173 SONAR-15074 --- diff --git a/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/OAuth2ContextFactory.java b/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/OAuth2ContextFactory.java index 915257bb2d4..83f668f0e8f 100644 --- a/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/OAuth2ContextFactory.java +++ b/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/OAuth2ContextFactory.java @@ -137,7 +137,7 @@ public class OAuth2ContextFactory { @Override public void authenticate(UserIdentity userIdentity, @Nullable Set organizationAlmIds) { - Boolean allowEmailShift = oAuthParameters.getAllowEmailShift(request).orElse(false); + boolean allowEmailShift = oAuthParameters.getAllowEmailShift(request).orElse(false); UserDto userDto = userRegistrar.register( UserRegistration.builder() .setUserIdentity(userIdentity) diff --git a/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/UserRegistrarImpl.java b/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/UserRegistrarImpl.java index 5fd6d5b5919..ef61f3ee8e9 100644 --- a/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/UserRegistrarImpl.java +++ b/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/UserRegistrarImpl.java @@ -20,6 +20,7 @@ package org.sonar.server.authentication; import com.google.common.collect.Sets; + import java.util.ArrayList; import java.util.Collection; import java.util.HashSet; @@ -31,6 +32,7 @@ import java.util.Set; import java.util.function.Consumer; import javax.annotation.CheckForNull; import javax.annotation.Nullable; + import org.sonar.api.server.authentication.IdentityProvider; import org.sonar.api.server.authentication.UserIdentity; import org.sonar.api.utils.log.Logger; @@ -41,6 +43,7 @@ import org.sonar.db.user.GroupDto; import org.sonar.db.user.UserDto; import org.sonar.db.user.UserGroupDto; import org.sonar.server.authentication.UserRegistration.ExistingEmailStrategy; +import org.sonar.server.authentication.event.AuthenticationEvent; import org.sonar.server.authentication.event.AuthenticationException; import org.sonar.server.authentication.exception.EmailAlreadyExistsRedirectionException; import org.sonar.server.user.ExternalIdentity; @@ -71,7 +74,7 @@ public class UserRegistrarImpl implements UserRegistrar { @Override public UserDto register(UserRegistration registration) { try (DbSession dbSession = dbClient.openSession(false)) { - UserDto userDto = getUser(dbSession, registration.getUserIdentity(), registration.getProvider()); + UserDto userDto = getUser(dbSession, registration.getUserIdentity(), registration.getProvider(), registration.getSource()); if (userDto == null) { return registerNewUser(dbSession, null, registration); } @@ -83,18 +86,72 @@ public class UserRegistrarImpl implements UserRegistrar { } @CheckForNull - private UserDto getUser(DbSession dbSession, UserIdentity userIdentity, IdentityProvider provider) { + private UserDto getUser(DbSession dbSession, UserIdentity userIdentity, IdentityProvider provider, AuthenticationEvent.Source source) { // First, try to authenticate using the external ID UserDto user = dbClient.userDao().selectByExternalIdAndIdentityProvider(dbSession, getProviderIdOrProviderLogin(userIdentity), provider.getKey()); if (user != null) { return user; } - // Then, try with the external login, for instance when external ID has changed - return dbClient.userDao().selectByExternalLoginAndIdentityProvider(dbSession, userIdentity.getProviderLogin(), provider.getKey()); + + // Then, try with the external login, for instance when external ID has changed or is not used by the provider + user = dbClient.userDao().selectByExternalLoginAndIdentityProvider(dbSession, userIdentity.getProviderLogin(), provider.getKey()); + if (user == null) { + return null; + } + // all gitlab users have an external ID, so the other two authentication methods should never be used + if (provider.getKey().equals("gitlab")) { + throw loginAlreadyUsedException(userIdentity, source); + } + + validateEmailToAvoidLoginRecycling(userIdentity, user, source); + updateUserExternalId(dbSession, user, userIdentity); + return user; + } + + private void updateUserExternalId(DbSession dbSession, UserDto user, UserIdentity userIdentity) { + String externalId = userIdentity.getProviderId(); + if (externalId != null) { + user.setExternalId(externalId); + dbClient.userDao().update(dbSession, user); + } + } + + private static void validateEmailToAvoidLoginRecycling(UserIdentity userIdentity, UserDto user, AuthenticationEvent.Source source) { + String userEmail = user.getEmail(); + + if (userEmail == null) { + LOGGER.warn("User with login '{}' tried to login with email '{}' but we don't have a email on record", + userIdentity.getProviderLogin(), userIdentity.getEmail()); + throw loginAlreadyUsedException(userIdentity, source); + } + + if (!userEmail.equals(userIdentity.getEmail())) { + LOGGER.warn("User with login '{}' tried to login with email '{}' which doesn't match the email on record '{}'", + userIdentity.getProviderLogin(), userIdentity.getEmail(), userEmail); + throw loginAlreadyUsedException(userIdentity, source); + } + } + + private static AuthenticationException loginAlreadyUsedException(UserIdentity userIdentity, AuthenticationEvent.Source source) { + return authException( + userIdentity, + source, + String.format("Login '%s' is already used", userIdentity.getProviderLogin()), + String.format("You can't sign up because login '%s' is already used by an existing user.", userIdentity.getProviderLogin()) + ); + } + + private static AuthenticationException authException(UserIdentity userIdentity, AuthenticationEvent.Source source, String message, String publicMessage) { + return AuthenticationException.newBuilder() + .setSource(source) + .setLogin(userIdentity.getProviderLogin()) + .setMessage(message) + .setPublicMessage(publicMessage) + .build(); } private UserDto registerNewUser(DbSession dbSession, @Nullable UserDto disabledUser, UserRegistration authenticatorParameters) { - Optional otherUserToIndex = detectEmailUpdate(dbSession, authenticatorParameters); + Optional otherUserToIndex = detectEmailUpdate(dbSession, authenticatorParameters, disabledUser != null ? disabledUser.getUuid() : null); NewUser newUser = createNewUser(authenticatorParameters); if (disabledUser == null) { return userUpdater.createAndCommit(dbSession, newUser, beforeCommit(dbSession, authenticatorParameters), toArray(otherUserToIndex)); @@ -110,7 +167,7 @@ public class UserRegistrarImpl implements UserRegistrar { authenticatorParameters.getProvider().getKey(), authenticatorParameters.getUserIdentity().getProviderLogin(), authenticatorParameters.getUserIdentity().getProviderId())); - Optional otherUserToIndex = detectEmailUpdate(dbSession, authenticatorParameters); + Optional otherUserToIndex = detectEmailUpdate(dbSession, authenticatorParameters, userDto.getUuid()); userUpdater.updateAndCommit(dbSession, userDto, update, beforeCommit(dbSession, authenticatorParameters), toArray(otherUserToIndex)); return userDto; } @@ -119,7 +176,7 @@ public class UserRegistrarImpl implements UserRegistrar { return user -> syncGroups(dbSession, authenticatorParameters.getUserIdentity(), user); } - private Optional detectEmailUpdate(DbSession dbSession, UserRegistration authenticatorParameters) { + private Optional detectEmailUpdate(DbSession dbSession, UserRegistration authenticatorParameters, @Nullable String authenticatingUserUuid) { String email = authenticatorParameters.getUserIdentity().getEmail(); if (email == null) { return Optional.empty(); @@ -133,7 +190,7 @@ public class UserRegistrarImpl implements UserRegistrar { } UserDto existingUser = existingUsers.get(0); - if (existingUser == null || isSameUser(existingUser, authenticatorParameters)) { + if (existingUser == null || existingUser.getUuid().equals(authenticatingUserUuid)) { return Optional.empty(); } ExistingEmailStrategy existingEmailStrategy = authenticatorParameters.getExistingEmailStrategy(); @@ -151,13 +208,6 @@ public class UserRegistrarImpl implements UserRegistrar { } } - private static boolean isSameUser(UserDto existingUser, UserRegistration authenticatorParameters) { - // Check if same identity provider and same provider id or provider login - return (Objects.equals(existingUser.getExternalIdentityProvider(), authenticatorParameters.getProvider().getKey()) && - (Objects.equals(existingUser.getExternalId(), getProviderIdOrProviderLogin(authenticatorParameters.getUserIdentity())) - || Objects.equals(existingUser.getExternalLogin(), authenticatorParameters.getUserIdentity().getProviderLogin()))); - } - private void syncGroups(DbSession dbSession, UserIdentity userIdentity, UserDto userDto) { if (!userIdentity.shouldSyncGroups()) { return; diff --git a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/HttpHeadersAuthenticationTest.java b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/HttpHeadersAuthenticationTest.java index 089038661c7..62500919c5e 100644 --- a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/HttpHeadersAuthenticationTest.java +++ b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/HttpHeadersAuthenticationTest.java @@ -143,7 +143,7 @@ public class HttpHeadersAuthenticationTest { public void update_user_when_authenticating_exiting_user() { startWithSso(); setNotUserInToken(); - insertUser(newUserDto().setLogin(DEFAULT_LOGIN).setExternalLogin(DEFAULT_LOGIN).setExternalIdentityProvider("sonarqube").setName("old name").setEmail("old email"), group1); + insertUser(newUserDto().setLogin(DEFAULT_LOGIN).setExternalLogin(DEFAULT_LOGIN).setExternalIdentityProvider("sonarqube").setName("old name").setEmail(DEFAULT_USER.getEmail()), group1); // Name, email and groups are different HttpServletRequest request = createRequest(DEFAULT_LOGIN, DEFAULT_NAME, DEFAULT_EMAIL, GROUP2); @@ -174,6 +174,7 @@ public class HttpHeadersAuthenticationTest { insertUser(DEFAULT_USER, group1); Map headerValuesByName = new HashMap<>(); headerValuesByName.put("X-Forwarded-Login", DEFAULT_LOGIN); + headerValuesByName.put("X-Forwarded-Email", DEFAULT_USER.getEmail()); headerValuesByName.put("X-Forwarded-Groups", null); HttpServletRequest request = createRequest(headerValuesByName); @@ -217,12 +218,12 @@ public class HttpHeadersAuthenticationTest { UserDto user = insertUser(DEFAULT_USER, group1); // Refresh time was updated 6 minutes ago => more than 5 minutes setUserInToken(user, NOW - 6 * 60 * 1000L); - HttpServletRequest request = createRequest(DEFAULT_LOGIN, "new name", "new email", GROUP2); + HttpServletRequest request = createRequest(DEFAULT_LOGIN, "new name", DEFAULT_USER.getEmail(), GROUP2); underTest.authenticate(request, response); // User is updated - verifyUserInDb(DEFAULT_LOGIN, "new name", "new email", group2); + verifyUserInDb(DEFAULT_LOGIN, "new name", DEFAULT_USER.getEmail(), group2); verifyTokenIsUpdated(NOW); verify(authenticationEvent).loginSuccess(request, DEFAULT_LOGIN, Source.sso()); } @@ -232,12 +233,12 @@ public class HttpHeadersAuthenticationTest { startWithSso(); UserDto user = insertUser(DEFAULT_USER, group1); setUserInToken(user, null); - HttpServletRequest request = createRequest(DEFAULT_LOGIN, "new name", "new email", GROUP2); + HttpServletRequest request = createRequest(DEFAULT_LOGIN, "new name", DEFAULT_USER.getEmail(), GROUP2); underTest.authenticate(request, response); // User is updated - verifyUserInDb(DEFAULT_LOGIN, "new name", "new email", group2); + verifyUserInDb(DEFAULT_LOGIN, "new name", DEFAULT_USER.getEmail(), group2); verifyTokenIsUpdated(NOW); verify(authenticationEvent).loginSuccess(request, DEFAULT_LOGIN, Source.sso()); } diff --git a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/UserRegistrarImplTest.java b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/UserRegistrarImplTest.java index 280f8cb5c6c..6276d1ba27f 100644 --- a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/UserRegistrarImplTest.java +++ b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/UserRegistrarImplTest.java @@ -20,7 +20,9 @@ package org.sonar.server.authentication; import java.util.Optional; +import java.util.Set; import java.util.stream.Collectors; +import javax.annotation.Nullable; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -28,13 +30,14 @@ import org.junit.rules.ExpectedException; import org.sonar.api.config.internal.MapSettings; import org.sonar.api.impl.utils.AlwaysIncreasingSystem2; import org.sonar.api.server.authentication.UserIdentity; -import org.sonar.core.util.stream.MoreCollectors; +import org.sonar.api.utils.log.LogTester; import org.sonar.db.DbTester; import org.sonar.db.user.GroupDto; import org.sonar.db.user.UserDto; import org.sonar.server.authentication.UserRegistration.ExistingEmailStrategy; import org.sonar.server.authentication.event.AuthenticationEvent; import org.sonar.server.authentication.event.AuthenticationEvent.Source; +import org.sonar.server.authentication.event.AuthenticationException; import org.sonar.server.authentication.exception.EmailAlreadyExistsRedirectionException; import org.sonar.server.es.EsTester; import org.sonar.server.user.NewUserNotifier; @@ -44,6 +47,7 @@ import org.sonar.server.usergroups.DefaultGroupFinder; import static java.util.Arrays.stream; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.Mockito.mock; import static org.sonar.db.user.UserTesting.newUserDto; import static org.sonar.process.ProcessProperties.Property.ONBOARDING_TUTORIAL_SHOW_TO_NEW_USERS; @@ -56,7 +60,7 @@ public class UserRegistrarImplTest { private static final UserIdentity USER_IDENTITY = UserIdentity.builder() .setProviderId("ABCD") - .setProviderLogin("johndoo") + .setProviderLogin(USER_LOGIN) .setName("John") .setEmail("john@email.com") .build(); @@ -75,16 +79,13 @@ public class UserRegistrarImplTest { public DbTester db = DbTester.create(new AlwaysIncreasingSystem2()); @Rule public EsTester es = EsTester.create(); + @Rule + public LogTester logTester = new LogTester(); + private final UserIndexer userIndexer = new UserIndexer(db.getDbClient(), es.client()); private final CredentialsLocalAuthentication localAuthentication = new CredentialsLocalAuthentication(db.getDbClient(), settings.asConfig()); private final DefaultGroupFinder groupFinder = new DefaultGroupFinder(db.getDbClient()); - private final UserUpdater userUpdater = new UserUpdater( - mock(NewUserNotifier.class), - db.getDbClient(), - userIndexer, - groupFinder, - settings.asConfig(), - localAuthentication); + private final UserUpdater userUpdater = new UserUpdater(mock(NewUserNotifier.class), db.getDbClient(), userIndexer, groupFinder, settings.asConfig(), localAuthentication); private final UserRegistrarImpl underTest = new UserRegistrarImpl(db.getDbClient(), userUpdater, groupFinder); private GroupDto defaultGroup; @@ -96,19 +97,14 @@ public class UserRegistrarImplTest { @Test public void authenticate_new_user() { - UserDto createdUser = underTest.register(UserRegistration.builder() - .setUserIdentity(USER_IDENTITY) - .setProvider(IDENTITY_PROVIDER) - .setSource(Source.realm(BASIC, IDENTITY_PROVIDER.getName())) - .setExistingEmailStrategy(ExistingEmailStrategy.FORBID) - .build()); + UserDto createdUser = underTest.register(newUserRegistration()); UserDto user = db.users().selectUserByLogin(createdUser.getLogin()).get(); assertThat(user).isNotNull(); assertThat(user.isActive()).isTrue(); assertThat(user.getName()).isEqualTo("John"); assertThat(user.getEmail()).isEqualTo("john@email.com"); - assertThat(user.getExternalLogin()).isEqualTo("johndoo"); + assertThat(user.getExternalLogin()).isEqualTo(USER_LOGIN); assertThat(user.getExternalIdentityProvider()).isEqualTo("github"); assertThat(user.getExternalId()).isEqualTo("ABCD"); assertThat(user.isRoot()).isFalse(); @@ -133,10 +129,10 @@ public class UserRegistrarImplTest { UserDto user = db.users().selectUserByLogin(createdUser.getLogin()).get(); assertThat(user).isNotNull(); assertThat(user.isActive()).isTrue(); - assertThat(user.getLogin()).isEqualTo("johndoo"); + assertThat(user.getLogin()).isEqualTo(USER_LOGIN); assertThat(user.getName()).isEqualTo("John"); assertThat(user.getEmail()).isEqualTo("john@email.com"); - assertThat(user.getExternalLogin()).isEqualTo("johndoo"); + assertThat(user.getExternalLogin()).isEqualTo(USER_LOGIN); assertThat(user.getExternalIdentityProvider()).isEqualTo("sonarqube"); assertThat(user.getExternalId()).isEqualTo("ABCD"); assertThat(user.isLocal()).isFalse(); @@ -145,62 +141,40 @@ public class UserRegistrarImplTest { } @Test - public void authenticate_new_user_generate_login_when_no_login_provided() { - underTest.register(UserRegistration.builder() - .setUserIdentity(UserIdentity.builder() - .setProviderId("ABCD") - .setProviderLogin("johndoo") - .setName("John Doe") - .setEmail("john@email.com") - .build()) - .setProvider(IDENTITY_PROVIDER) - .setSource(Source.realm(BASIC, IDENTITY_PROVIDER.getName())) - .setExistingEmailStrategy(ExistingEmailStrategy.FORBID) - .build()); + public void authenticate_new_user_generates_login() { + underTest.register(newUserRegistration(UserIdentity.builder() + .setProviderId("ABCD") + .setProviderLogin(USER_LOGIN) + .setName("John Doe") + .setEmail("john@email.com") + .build())); UserDto user = db.getDbClient().userDao().selectByEmail(db.getSession(), "john@email.com").get(0); assertThat(user).isNotNull(); assertThat(user.isActive()).isTrue(); assertThat(user.getLogin()).isNotEqualTo("John Doe").startsWith("john-doe"); assertThat(user.getEmail()).isEqualTo("john@email.com"); - assertThat(user.getExternalLogin()).isEqualTo("johndoo"); + assertThat(user.getExternalLogin()).isEqualTo(USER_LOGIN); assertThat(user.getExternalIdentityProvider()).isEqualTo("github"); assertThat(user.getExternalId()).isEqualTo("ABCD"); } @Test - public void authenticate_new_user_with_groups() { + public void authenticate_new_user_assigns_user_to_groups() { GroupDto group1 = db.users().insertGroup("group1"); GroupDto group2 = db.users().insertGroup("group2"); - UserDto loggedInUser = authenticate(USER_LOGIN, "group1", "group2", "group3"); + UserDto loggedInUser = authenticate(USER_LOGIN, USER_IDENTITY.getEmail(), "group1", "group2", "group3"); Optional user = db.users().selectUserByLogin(loggedInUser.getLogin()); checkGroupMembership(user.get(), group1, group2, defaultGroup); } - @Test - public void authenticate_new_user_and_force_default_group() { - UserDto user = db.users().insertUser(); - GroupDto group1 = db.users().insertGroup("group1"); - db.users().insertMember(group1, user); - db.users().insertMember(defaultGroup, user); - - authenticate(user.getLogin(), "group1"); - - checkGroupMembership(user, group1, defaultGroup); - } - @Test public void authenticate_new_user_sets_onboarded_flag_to_false_when_onboarding_setting_is_set_to_true() { settings.setProperty(ONBOARDING_TUTORIAL_SHOW_TO_NEW_USERS.getKey(), true); - UserDto user = underTest.register(UserRegistration.builder() - .setUserIdentity(USER_IDENTITY) - .setProvider(IDENTITY_PROVIDER) - .setSource(Source.local(BASIC)) - .setExistingEmailStrategy(ExistingEmailStrategy.FORBID) - .build()); + UserDto user = underTest.register(newUserRegistration()); assertThat(db.users().selectUserByLogin(user.getLogin()).get().isOnboarded()).isFalse(); } @@ -209,36 +183,45 @@ public class UserRegistrarImplTest { public void authenticate_new_user_sets_onboarded_flag_to_true_when_onboarding_setting_is_set_to_false() { settings.setProperty(ONBOARDING_TUTORIAL_SHOW_TO_NEW_USERS.getKey(), false); - UserDto user = underTest.register(UserRegistration.builder() - .setUserIdentity(USER_IDENTITY) - .setProvider(IDENTITY_PROVIDER) - .setSource(Source.local(BASIC)) - .setExistingEmailStrategy(ExistingEmailStrategy.FORBID) - .build()); + UserDto user = underTest.register(newUserRegistration()); assertThat(db.users().selectUserByLogin(user.getLogin()).get().isOnboarded()).isTrue(); } @Test - public void external_id_is_set_to_provider_login_when_null() { + public void authenticate_new_user_sets_external_id_to_provider_login_when_id_is_null() { UserIdentity newUser = UserIdentity.builder() .setProviderId(null) .setProviderLogin("johndoo") .setName("JOhn") .build(); - UserDto user = underTest.register(UserRegistration.builder() - .setUserIdentity(newUser) - .setProvider(IDENTITY_PROVIDER) - .setSource(Source.local(BASIC)) - .setExistingEmailStrategy(ExistingEmailStrategy.FORBID) - .build()); + UserDto user = underTest.register(newUserRegistration(newUser)); assertThat(db.users().selectUserByLogin(user.getLogin()).get()) .extracting(UserDto::getLogin, UserDto::getExternalId, UserDto::getExternalLogin) .contains(user.getLogin(), "johndoo", "johndoo"); } + @Test + public void authenticate_new_user_with_gitlab_provider() { + UserRegistration registration = UserRegistration.builder() + .setUserIdentity(USER_IDENTITY) + .setProvider(new TestIdentityProvider() + .setKey("gitlab") + .setName("name of gitlab") + .setEnabled(true) + .setAllowsUsersToSignUp(true)) + .setSource(Source.local(BASIC)) + .setExistingEmailStrategy(FORBID) + .build(); + + UserDto newUser = underTest.register(registration); + assertThat(newUser) + .extracting(UserDto::getExternalIdentityProvider, UserDto::getExternalLogin) + .containsExactly("gitlab", USER_IDENTITY.getProviderLogin()); + } + @Test public void authenticate_new_user_update_existing_user_email_when_strategy_is_ALLOW() { UserDto existingUser = db.users().insertUser(u -> u.setEmail("john@email.com")); @@ -262,7 +245,7 @@ public class UserRegistrarImplTest { } @Test - public void throw_EmailAlreadyExistException_when_authenticating_new_user_when_email_already_exists_and_strategy_is_WARN() { + public void authenticate_new_user_throws_EmailAlreadyExistException_when_email_already_exists_and_strategy_is_WARN() { UserDto existingUser = db.users().insertUser(u -> u.setEmail("john@email.com")); UserIdentity newUser = UserIdentity.builder() .setProviderLogin("johndoo") @@ -281,9 +264,9 @@ public class UserRegistrarImplTest { } @Test - public void throw_AuthenticationException_when_authenticating_new_user_when_email_already_exists_and_strategy_is_FORBID() { + public void authenticate_new_user_throws_AuthenticationException_when_when_email_already_exists_and_strategy_is_FORBID() { db.users().insertUser(u -> u.setEmail("john@email.com")); - Source source = Source.realm(AuthenticationEvent.Method.FORM, IDENTITY_PROVIDER.getName()); + Source source = Source.local(BASIC); expectedException.expect(authenticationException().from(source) .withLogin(USER_IDENTITY.getProviderLogin()) @@ -291,17 +274,11 @@ public class UserRegistrarImplTest { "This means that you probably already registered with another account.")); expectedException.expectMessage("Email 'john@email.com' is already used"); - underTest.register(UserRegistration.builder() - .setUserIdentity(USER_IDENTITY) - .setProvider(IDENTITY_PROVIDER) - .setSource(source) - .setExistingEmailStrategy(FORBID) - .setExistingEmailStrategy(ExistingEmailStrategy.FORBID) - .build()); + underTest.register(newUserRegistration()); } @Test - public void throw_AuthenticationException_when_authenticating_new_user_and_email_already_exists_multiple_times() { + public void authenticate_new_user_throws_AuthenticationException_when_email_already_exists_multiple_times() { db.users().insertUser(u -> u.setEmail("john@email.com")); db.users().insertUser(u -> u.setEmail("john@email.com")); Source source = Source.realm(AuthenticationEvent.Method.FORM, IDENTITY_PROVIDER.getName()); @@ -312,17 +289,11 @@ public class UserRegistrarImplTest { "This means that you probably already registered with another account.")); expectedException.expectMessage("Email 'john@email.com' is already used"); - underTest.register(UserRegistration.builder() - .setUserIdentity(USER_IDENTITY) - .setProvider(IDENTITY_PROVIDER) - .setSource(source) - .setExistingEmailStrategy(FORBID) - .setExistingEmailStrategy(ExistingEmailStrategy.FORBID) - .build()); + underTest.register(newUserRegistration(source)); } @Test - public void fail_to_authenticate_new_user_when_allow_users_to_signup_is_false() { + public void authenticate_new_user_fails_when_allow_users_to_signup_is_false() { TestIdentityProvider identityProvider = new TestIdentityProvider() .setKey("github") .setName("Github") @@ -342,24 +313,15 @@ public class UserRegistrarImplTest { } @Test - public void authenticate_and_update_existing_user_matching_login() { - db.users().insertUser(u -> u - .setName("Old name") - .setEmail("Old email") - .setExternalId("old id") - .setExternalLogin("old identity") - .setExternalIdentityProvider("old provide")); + public void authenticate_existing_user_doesnt_change_group_membership() { + UserDto user = db.users().insertUser(u -> u.setExternalIdentityProvider(IDENTITY_PROVIDER.getKey())); + GroupDto group1 = db.users().insertGroup("group1"); + db.users().insertMember(group1, user); + db.users().insertMember(defaultGroup, user); - UserDto user = underTest.register(UserRegistration.builder() - .setUserIdentity(USER_IDENTITY) - .setProvider(IDENTITY_PROVIDER) - .setSource(Source.local(BASIC)) - .setExistingEmailStrategy(ExistingEmailStrategy.FORBID) - .build()); + authenticate(user.getExternalLogin(), user.getEmail(), "group1"); - assertThat(db.users().selectUserByLogin(user.getLogin()).get()) - .extracting(UserDto::getName, UserDto::getEmail, UserDto::getExternalId, UserDto::getExternalLogin, UserDto::getExternalIdentityProvider, UserDto::isActive) - .contains("John", "john@email.com", "ABCD", "johndoo", "github", true); + checkGroupMembership(user, group1, defaultGroup); } @Test @@ -372,12 +334,7 @@ public class UserRegistrarImplTest { .setExternalLogin("old identity") .setExternalIdentityProvider(IDENTITY_PROVIDER.getKey())); - underTest.register(UserRegistration.builder() - .setUserIdentity(USER_IDENTITY) - .setProvider(IDENTITY_PROVIDER) - .setSource(Source.local(BASIC)) - .setExistingEmailStrategy(ExistingEmailStrategy.FORBID) - .build()); + underTest.register(newUserRegistration()); assertThat(db.getDbClient().userDao().selectByUuid(db.getSession(), user.getUuid())) .extracting(UserDto::getLogin, UserDto::getName, UserDto::getEmail, UserDto::getExternalId, UserDto::getExternalLogin, UserDto::getExternalIdentityProvider, @@ -386,7 +343,7 @@ public class UserRegistrarImplTest { } @Test - public void authenticate_and_update_existing_user_matching_external_login() { + public void authenticate_and_update_existing_user_matching_external_login_and_email() { UserDto user = db.users().insertUser(u -> u .setLogin("Old login") .setName("Old name") @@ -395,12 +352,7 @@ public class UserRegistrarImplTest { .setExternalLogin(USER_IDENTITY.getProviderLogin()) .setExternalIdentityProvider(IDENTITY_PROVIDER.getKey())); - underTest.register(UserRegistration.builder() - .setUserIdentity(USER_IDENTITY) - .setProvider(IDENTITY_PROVIDER) - .setSource(Source.local(BASIC)) - .setExistingEmailStrategy(ExistingEmailStrategy.FORBID) - .build()); + underTest.register(newUserRegistration()); assertThat(db.getDbClient().userDao().selectByUuid(db.getSession(), user.getUuid())) .extracting(UserDto::getName, UserDto::getEmail, UserDto::getExternalId, UserDto::getExternalLogin, UserDto::getExternalIdentityProvider, @@ -409,7 +361,7 @@ public class UserRegistrarImplTest { } @Test - public void authenticate_existing_user_and_login_should_not_be_updated() { + public void authenticate_existing_user_should_not_update_login() { UserDto user = db.users().insertUser(u -> u .setLogin("old login") .setName(USER_IDENTITY.getName()) @@ -418,43 +370,32 @@ public class UserRegistrarImplTest { .setExternalLogin("old identity") .setExternalIdentityProvider(IDENTITY_PROVIDER.getKey())); - underTest.register(UserRegistration.builder() - .setUserIdentity(USER_IDENTITY) - .setProvider(IDENTITY_PROVIDER) - .setSource(Source.local(BASIC)) - .setExistingEmailStrategy(ExistingEmailStrategy.FORBID) - .build()); + underTest.register(newUserRegistration()); // no new user should be created assertThat(db.countRowsOfTable(db.getSession(), "users")).isEqualTo(1); assertThat(db.getDbClient().userDao().selectByUuid(db.getSession(), user.getUuid())) .extracting(UserDto::getLogin, UserDto::getName, UserDto::getEmail, UserDto::getExternalId, UserDto::getExternalLogin, UserDto::getExternalIdentityProvider, UserDto::isActive) - .containsExactlyInAnyOrder("old login", USER_IDENTITY.getName(), USER_IDENTITY.getEmail(), USER_IDENTITY.getProviderId(), USER_IDENTITY.getProviderLogin(), - IDENTITY_PROVIDER.getKey(), - true); + .containsExactly("old login", USER_IDENTITY.getName(), USER_IDENTITY.getEmail(), USER_IDENTITY.getProviderId(), USER_IDENTITY.getProviderLogin(), + IDENTITY_PROVIDER.getKey(), true); } @Test - public void authenticate_existing_user_matching_external_login_when_external_id_is_null() { + public void authenticate_existing_user_matching_external_login_and_email_when_external_id_is_null() { UserDto user = db.users().insertUser(u -> u .setName("Old name") - .setEmail("Old email") .setExternalId("Old id") + .setEmail("john@email.com") .setExternalLogin("johndoo") .setExternalIdentityProvider(IDENTITY_PROVIDER.getKey())); - underTest.register(UserRegistration.builder() - .setUserIdentity(UserIdentity.builder() - .setProviderId(null) - .setProviderLogin("johndoo") - .setName("John") - .setEmail("john@email.com") - .build()) - .setProvider(IDENTITY_PROVIDER) - .setSource(Source.local(BASIC)) - .setExistingEmailStrategy(ExistingEmailStrategy.FORBID) - .build()); + underTest.register(newUserRegistration(UserIdentity.builder() + .setProviderId(null) + .setProviderLogin("johndoo") + .setName("John") + .setEmail("john@email.com") + .build())); assertThat(db.getDbClient().userDao().selectByUuid(db.getSession(), user.getUuid())) .extracting(UserDto::getLogin, UserDto::getName, UserDto::getEmail, UserDto::getExternalId, UserDto::getExternalLogin, UserDto::getExternalIdentityProvider, @@ -463,44 +404,89 @@ public class UserRegistrarImplTest { } @Test - public void authenticate_existing_user_when_login_is_not_provided() { - UserDto user = db.users().insertUser(u -> u.setExternalIdentityProvider(IDENTITY_PROVIDER.getKey())); + public void do_not_authenticate_gitlab_user_matching_external_login() { + db.users().insertUser(u -> u + .setLogin("Old login") + .setName("Old name") + .setEmail(USER_IDENTITY.getEmail()) + .setExternalId("Old id") + .setExternalLogin(USER_IDENTITY.getProviderLogin()) + .setExternalIdentityProvider("gitlab")); - underTest.register(UserRegistration.builder() - .setUserIdentity(UserIdentity.builder() - .setProviderId(user.getExternalId()) - .setProviderLogin(user.getExternalLogin()) - // No login provided - .setName(user.getName()) - .setEmail(user.getEmail()) - .build()) - .setProvider(IDENTITY_PROVIDER) + UserRegistration registration = UserRegistration.builder() + .setUserIdentity(USER_IDENTITY) + .setProvider(new TestIdentityProvider() + .setKey("gitlab") + .setName("name of gitlab") + .setEnabled(true)) .setSource(Source.local(BASIC)) - .setExistingEmailStrategy(ExistingEmailStrategy.FORBID) - .build()); + .setExistingEmailStrategy(FORBID) + .build(); - // No new user is created - assertThat(db.countRowsOfTable(db.getSession(), "users")).isEqualTo(1); - assertThat(db.getDbClient().userDao().selectByUuid(db.getSession(), user.getUuid())) - .extracting(UserDto::getLogin, UserDto::getName, UserDto::getEmail, UserDto::getExternalId, UserDto::getExternalLogin, UserDto::getExternalIdentityProvider, - UserDto::isActive) - .contains(user.getExternalLogin(), user.getName(), user.getEmail(), user.getExternalId(), user.getExternalLogin(), user.getExternalIdentityProvider(), true); + assertThatThrownBy(() -> underTest.register(registration)) + .isInstanceOf(AuthenticationException.class) + .hasMessage(String.format("Login '%s' is already used", USER_IDENTITY.getProviderLogin())); + } + + @Test + public void do_not_authenticate_and_update_existing_user_matching_external_login_if_emails_do_not_match() { + db.users().insertUser(u -> u + .setLogin("Old login") + .setName("Old name") + .setEmail("another-email@sonarsource.com") + .setExternalId("Old id") + .setExternalLogin(USER_IDENTITY.getProviderLogin()) + .setExternalIdentityProvider(IDENTITY_PROVIDER.getKey())); + + assertThatThrownBy(() -> underTest.register(newUserRegistration())) + .isInstanceOf(AuthenticationException.class) + .hasMessage(String.format("Login '%s' is already used", USER_IDENTITY.getProviderLogin())); + + assertThat(logTester.logs()).contains(String.format("User with login '%s' tried to login with email '%s' which doesn't match the email on record '%s'", + USER_IDENTITY.getProviderLogin(), USER_IDENTITY.getEmail(), "another-email@sonarsource.com")); + } + + @Test + public void do_not_authenticate_and_update_existing_user_matching_external_login_if_email_is_missing() { + db.users().insertUser(u -> u + .setLogin("Old login") + .setName("Old name") + .setExternalId("Old id") + .setEmail(null) + .setExternalLogin(USER_IDENTITY.getProviderLogin()) + .setExternalIdentityProvider(IDENTITY_PROVIDER.getKey())); + + assertThatThrownBy(() -> underTest.register(newUserRegistration())) + .isInstanceOf(AuthenticationException.class) + .hasMessage(String.format("Login '%s' is already used", USER_IDENTITY.getProviderLogin())); + + assertThat(logTester.logs()).contains(String.format("User with login '%s' tried to login with email '%s' but we don't have a email on record", + USER_IDENTITY.getProviderLogin(), USER_IDENTITY.getEmail())); + } + + @Test + public void do_not_authenticate_and_update_existing_user_matching_external_id_if_external_provider_does_not_match() { + db.users().insertUser(u -> u + .setLogin("Old login") + .setName("Old name") + .setExternalId(USER_IDENTITY.getProviderId()) + .setEmail(null) + .setExternalLogin(USER_IDENTITY.getProviderLogin()) + .setExternalIdentityProvider("Old provider")); + + underTest.register(newUserRegistration()); + assertThat(db.countRowsOfTable("users")).isEqualTo(2); } @Test - public void authenticate_existing_user_with_login_update_and_strategy_is_ALLOW() { + public void authenticate_existing_user_should_update_login() { UserDto user = db.users().insertUser(u -> u .setLogin("Old login") .setExternalId(USER_IDENTITY.getProviderId()) .setExternalLogin("old identity") .setExternalIdentityProvider(IDENTITY_PROVIDER.getKey())); - underTest.register(UserRegistration.builder() - .setUserIdentity(USER_IDENTITY) - .setProvider(IDENTITY_PROVIDER) - .setSource(Source.local(BASIC)) - .setExistingEmailStrategy(ExistingEmailStrategy.FORBID) - .build()); + underTest.register(newUserRegistration()); assertThat(db.getDbClient().userDao().selectByUuid(db.getSession(), user.getUuid())) .extracting(UserDto::getLogin, UserDto::getExternalLogin) @@ -508,40 +494,36 @@ public class UserRegistrarImplTest { } @Test - public void authenticate_existing_disabled_user() { + public void authenticate_existing_disabled_user_should_reactivate_it() { db.users().insertUser(u -> u .setLogin(USER_LOGIN) .setActive(false) .setName("Old name") - .setEmail("Old email") + .setEmail(USER_IDENTITY.getEmail()) .setExternalId("Old id") .setExternalLogin(USER_IDENTITY.getProviderLogin()) .setExternalIdentityProvider(IDENTITY_PROVIDER.getKey())); - underTest.register(UserRegistration.builder() - .setUserIdentity(USER_IDENTITY) - .setProvider(IDENTITY_PROVIDER) - .setSource(Source.local(BASIC)) - .setExistingEmailStrategy(ExistingEmailStrategy.FORBID) - .build()); + underTest.register(newUserRegistration()); UserDto userDto = db.users().selectUserByLogin(USER_LOGIN).get(); assertThat(userDto.isActive()).isTrue(); - assertThat(userDto.getName()).isEqualTo("John"); - assertThat(userDto.getEmail()).isEqualTo("john@email.com"); - assertThat(userDto.getExternalId()).isEqualTo("ABCD"); - assertThat(userDto.getExternalLogin()).isEqualTo("johndoo"); - assertThat(userDto.getExternalIdentityProvider()).isEqualTo("github"); + assertThat(userDto.getName()).isEqualTo(USER_IDENTITY.getName()); + assertThat(userDto.getEmail()).isEqualTo(USER_IDENTITY.getEmail()); + assertThat(userDto.getExternalId()).isEqualTo(USER_IDENTITY.getProviderId()); + assertThat(userDto.getExternalLogin()).isEqualTo(USER_IDENTITY.getProviderLogin()); + assertThat(userDto.getExternalIdentityProvider()).isEqualTo(IDENTITY_PROVIDER.getKey()); assertThat(userDto.isRoot()).isFalse(); } @Test public void authenticate_existing_user_when_email_already_exists_and_strategy_is_ALLOW() { UserDto existingUser = db.users().insertUser(u -> u.setEmail("john@email.com")); - UserDto currentUser = db.users().insertUser(u -> u.setExternalLogin("johndoo").setExternalIdentityProvider(IDENTITY_PROVIDER.getKey()).setEmail(null)); + UserDto currentUser = db.users().insertUser(u -> u.setExternalId("id").setExternalLogin("login").setExternalIdentityProvider(IDENTITY_PROVIDER.getKey()).setEmail(null)); UserIdentity userIdentity = UserIdentity.builder() .setProviderLogin(currentUser.getExternalLogin()) + .setProviderId(currentUser.getExternalId()) .setName("John") .setEmail("john@email.com") .build(); @@ -562,7 +544,7 @@ public class UserRegistrarImplTest { } @Test - public void throw_EmailAlreadyExistException_when_authenticating_existing_user_when_email_already_exists_and_strategy_is_WARN() { + public void authenticating_existing_user_throws_EmailAlreadyExistException_when_email_already_exists_and_strategy_is_WARN() { UserDto existingUser = db.users().insertUser(u -> u.setEmail("john@email.com")); UserDto currentUser = db.users().insertUser(u -> u.setEmail(null)); UserIdentity userIdentity = UserIdentity.builder() @@ -582,7 +564,7 @@ public class UserRegistrarImplTest { } @Test - public void throw_AuthenticationException_when_authenticating_existing_user_when_email_already_exists_and_strategy_is_FORBID() { + public void authenticating_existing_user_throws_AuthenticationException_when_email_already_exists_and_strategy_is_FORBID() { UserDto existingUser = db.users().insertUser(u -> u.setEmail("john@email.com")); UserDto currentUser = db.users().insertUser(u -> u.setEmail(null)); UserIdentity userIdentity = UserIdentity.builder() @@ -591,22 +573,18 @@ public class UserRegistrarImplTest { .setEmail("john@email.com") .build(); - expectedException.expect(authenticationException().from(Source.realm(AuthenticationEvent.Method.FORM, IDENTITY_PROVIDER.getName())) + Source source = Source.realm(AuthenticationEvent.Method.FORM, IDENTITY_PROVIDER.getName()); + expectedException.expect(authenticationException().from(source) .withLogin(userIdentity.getProviderLogin()) .andPublicMessage("You can't sign up because email 'john@email.com' is already used by an existing user. " + "This means that you probably already registered with another account.")); expectedException.expectMessage("Email 'john@email.com' is already used"); - underTest.register(UserRegistration.builder() - .setUserIdentity(userIdentity) - .setProvider(IDENTITY_PROVIDER) - .setSource(Source.realm(AuthenticationEvent.Method.FORM, IDENTITY_PROVIDER.getName())) - .setExistingEmailStrategy(ExistingEmailStrategy.FORBID) - .build()); + underTest.register(newUserRegistration(userIdentity, source)); } @Test - public void does_not_fail_to_authenticate_user_when_email_has_not_changed_and_strategy_is_FORBID() { + public void authenticate_existing_user_succeeds_when_email_has_not_changed_and_strategy_is_FORBID() { UserDto currentUser = db.users().insertUser(u -> u.setEmail("john@email.com") .setExternalIdentityProvider(IDENTITY_PROVIDER.getKey())); UserIdentity userIdentity = UserIdentity.builder() @@ -616,12 +594,7 @@ public class UserRegistrarImplTest { .setEmail("john@email.com") .build(); - underTest.register(UserRegistration.builder() - .setUserIdentity(userIdentity) - .setProvider(IDENTITY_PROVIDER) - .setSource(Source.local(BASIC)) - .setExistingEmailStrategy(ExistingEmailStrategy.FORBID) - .build()); + underTest.register(newUserRegistration(userIdentity)); UserDto currentUserReloaded = db.users().selectUserByLogin(currentUser.getLogin()).get(); assertThat(currentUserReloaded.getEmail()).isEqualTo("john@email.com"); @@ -632,12 +605,13 @@ public class UserRegistrarImplTest { UserDto user = db.users().insertUser(newUserDto() .setExternalIdentityProvider(IDENTITY_PROVIDER.getKey()) .setExternalLogin(USER_IDENTITY.getProviderLogin()) + .setEmail(USER_IDENTITY.getEmail()) .setActive(true) .setName("John")); GroupDto group1 = db.users().insertGroup("group1"); GroupDto group2 = db.users().insertGroup("group2"); - authenticate(USER_IDENTITY.getProviderLogin(), "group1", "group2", "group3"); + authenticate(USER_IDENTITY.getProviderLogin(), USER_IDENTITY.getEmail(), "group1", "group2", "group3"); checkGroupMembership(user, group1, group2); } @@ -647,6 +621,7 @@ public class UserRegistrarImplTest { UserDto user = db.users().insertUser(newUserDto() .setExternalIdentityProvider(IDENTITY_PROVIDER.getKey()) .setExternalLogin(USER_IDENTITY.getProviderLogin()) + .setEmail(USER_IDENTITY.getEmail()) .setActive(true) .setName("John")); GroupDto group1 = db.users().insertGroup("group1"); @@ -654,7 +629,7 @@ public class UserRegistrarImplTest { db.users().insertMember(group1, user); db.users().insertMember(group2, user); - authenticate(USER_IDENTITY.getProviderLogin(), "group1"); + authenticate(USER_IDENTITY.getProviderLogin(), USER_IDENTITY.getEmail(), "group1"); checkGroupMembership(user, group1); } @@ -670,23 +645,39 @@ public class UserRegistrarImplTest { db.users().insertMember(group2, user); db.users().insertMember(defaultGroup, user); - authenticate(user.getExternalLogin()); + authenticate(user.getExternalLogin(), user.getEmail()); checkGroupMembership(user, defaultGroup); } - private UserDto authenticate(String providerLogin, String... groups) { - return underTest.register(UserRegistration.builder() - .setUserIdentity(UserIdentity.builder() - .setProviderLogin(providerLogin) - .setName("John") - // No group - .setGroups(stream(groups).collect(MoreCollectors.toSet())) - .build()) + private static UserRegistration newUserRegistration(UserIdentity userIdentity) { + return newUserRegistration(userIdentity, Source.local(BASIC)); + } + + private static UserRegistration newUserRegistration(UserIdentity userIdentity, Source source) { + return UserRegistration.builder() + .setUserIdentity(userIdentity) .setProvider(IDENTITY_PROVIDER) - .setSource(Source.local(BASIC)) + .setSource(source) .setExistingEmailStrategy(ExistingEmailStrategy.FORBID) - .build()); + .build(); + } + + private static UserRegistration newUserRegistration(Source source) { + return newUserRegistration(USER_IDENTITY, source); + } + + private static UserRegistration newUserRegistration() { + return newUserRegistration(USER_IDENTITY, Source.local(BASIC)); + } + + private UserDto authenticate(String providerLogin, @Nullable String email, String... groups) { + return underTest.register(newUserRegistration(UserIdentity.builder() + .setProviderLogin(providerLogin) + .setName("John") + .setEmail(email) + .setGroups(Set.of(groups)) + .build())); } private void checkGroupMembership(UserDto user, GroupDto... expectedGroups) {