From cb95ab82013c2ce242b3f34bd7a7072e181e4f62 Mon Sep 17 00:00:00 2001 From: Duarte Meneses Date: Tue, 7 Sep 2021 15:36:16 -0500 Subject: [PATCH] SONAR-15172 Fix SSF-173 --- .../authentication/UserRegistrarImpl.java | 28 +++----- .../authentication/UserRegistrarImplTest.java | 70 +++++++++++++------ 2 files changed, 56 insertions(+), 42 deletions(-) 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 dc003eede15..fc221e8a27b 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 @@ -96,22 +96,14 @@ public class UserRegistrarImpl implements UserRegistrar { } // 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); + throw failAuthenticationException(userIdentity, source); + } else if (provider.getKey().equals("github")) { + validateEmailToAvoidLoginRecycling(userIdentity, user, 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 dbEmail = user.getEmail(); @@ -122,18 +114,14 @@ public class UserRegistrarImpl implements UserRegistrar { String externalEmail = userIdentity.getEmail(); if (!dbEmail.equals(externalEmail)) { - LOGGER.warn("User with login '{}' tried to login with email '{}' which doesn't match the email on record '{}'", - userIdentity.getProviderLogin(), externalEmail, dbEmail); - throw loginAlreadyUsedException(userIdentity, source); + LOGGER.warn("User with login '{}' tried to login with email '{}' which doesn't match the email on record '{}'", userIdentity.getProviderLogin(), externalEmail, dbEmail); + throw failAuthenticationException(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 failAuthenticationException(UserIdentity userIdentity, AuthenticationEvent.Source source) { + String message = String.format("Failed to authenticate with login '%s'", userIdentity.getProviderLogin()); + return authException(userIdentity, source, message, message); } private static AuthenticationException authException(UserIdentity userIdentity, AuthenticationEvent.Source source, String message, String publicMessage) { 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 fdcb33b13c9..f0be0dc151a 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 @@ -66,7 +66,7 @@ public class UserRegistrarImplTest { .setEmail("john@email.com") .build(); - private static final TestIdentityProvider IDENTITY_PROVIDER = new TestIdentityProvider() + private static final TestIdentityProvider GH_IDENTITY_PROVIDER = new TestIdentityProvider() .setKey("github") .setName("name of github") .setEnabled(true) @@ -241,7 +241,7 @@ public class UserRegistrarImplTest { 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()); + Source source = Source.realm(AuthenticationEvent.Method.FORM, GH_IDENTITY_PROVIDER.getName()); expectedException.expect(authenticationException().from(source) .withLogin(USER_IDENTITY.getProviderLogin()) @@ -274,7 +274,7 @@ public class UserRegistrarImplTest { @Test public void authenticate_existing_user_doesnt_change_group_membership() { - UserDto user = db.users().insertUser(u -> u.setExternalIdentityProvider(IDENTITY_PROVIDER.getKey())); + UserDto user = db.users().insertUser(u -> u.setExternalIdentityProvider(GH_IDENTITY_PROVIDER.getKey())); GroupDto group1 = db.users().insertGroup("group1"); db.users().insertMember(group1, user); db.users().insertMember(defaultGroup, user); @@ -292,7 +292,7 @@ public class UserRegistrarImplTest { .setEmail("Old email") .setExternalId(USER_IDENTITY.getProviderId()) .setExternalLogin("old identity") - .setExternalIdentityProvider(IDENTITY_PROVIDER.getKey())); + .setExternalIdentityProvider(GH_IDENTITY_PROVIDER.getKey())); underTest.register(newUserRegistration()); @@ -310,7 +310,7 @@ public class UserRegistrarImplTest { .setEmail(USER_IDENTITY.getEmail()) .setExternalId("Old id") .setExternalLogin(USER_IDENTITY.getProviderLogin()) - .setExternalIdentityProvider(IDENTITY_PROVIDER.getKey())); + .setExternalIdentityProvider(GH_IDENTITY_PROVIDER.getKey())); underTest.register(newUserRegistration()); @@ -328,7 +328,7 @@ public class UserRegistrarImplTest { .setEmail(USER_IDENTITY.getEmail()) .setExternalId(USER_IDENTITY.getProviderId()) .setExternalLogin("old identity") - .setExternalIdentityProvider(IDENTITY_PROVIDER.getKey())); + .setExternalIdentityProvider(GH_IDENTITY_PROVIDER.getKey())); underTest.register(newUserRegistration()); @@ -338,7 +338,7 @@ public class UserRegistrarImplTest { .extracting(UserDto::getLogin, UserDto::getName, UserDto::getEmail, UserDto::getExternalId, UserDto::getExternalLogin, UserDto::getExternalIdentityProvider, UserDto::isActive) .containsExactly("old login", USER_IDENTITY.getName(), USER_IDENTITY.getEmail(), USER_IDENTITY.getProviderId(), USER_IDENTITY.getProviderLogin(), - IDENTITY_PROVIDER.getKey(), true); + GH_IDENTITY_PROVIDER.getKey(), true); verify(auditPersister, never()).updateUserPassword(any(), any()); } @@ -349,7 +349,7 @@ public class UserRegistrarImplTest { .setExternalId("Old id") .setEmail("john@email.com") .setExternalLogin("johndoo") - .setExternalIdentityProvider(IDENTITY_PROVIDER.getKey())); + .setExternalIdentityProvider(GH_IDENTITY_PROVIDER.getKey())); underTest.register(newUserRegistration(UserIdentity.builder() .setProviderId(null) @@ -385,27 +385,53 @@ public class UserRegistrarImplTest { assertThatThrownBy(() -> underTest.register(registration)) .isInstanceOf(AuthenticationException.class) - .hasMessage(String.format("Login '%s' is already used", USER_IDENTITY.getProviderLogin())); + .hasMessage(String.format("Failed to authenticate with login '%s'", USER_IDENTITY.getProviderLogin())); } @Test - public void do_not_authenticate_and_update_existing_user_matching_external_login_if_emails_do_not_match() { + public void do_not_authenticate_and_update_existing_github_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())); + .setExternalIdentityProvider(GH_IDENTITY_PROVIDER.getKey())); assertThatThrownBy(() -> underTest.register(newUserRegistration())) .isInstanceOf(AuthenticationException.class) - .hasMessage(String.format("Login '%s' is already used", USER_IDENTITY.getProviderLogin())); + .hasMessage(String.format("Failed to authenticate with login '%s'", 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 authenticate_and_update_existing_user_matching_external_login_and_emails_mismatch() { + UserRegistration registration = UserRegistration.builder() + .setUserIdentity(USER_IDENTITY) + .setProvider(new TestIdentityProvider() + .setKey("other") + .setName("name of other") + .setEnabled(true)) + .setSource(Source.local(BASIC)) + .build(); + + UserDto user = db.users().insertUser(u -> u + .setLogin("Old login") + .setName("Old name") + .setEmail("another-email@sonarsource.com") + .setExternalId("Old id") + .setExternalLogin(USER_IDENTITY.getProviderLogin()) + .setExternalIdentityProvider(registration.getProvider().getKey())); + + underTest.register(registration); + 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.getLogin(), "John", "john@email.com", "ABCD", "johndoo", "other", true); + } + @Test public void authenticate_and_update_existing_user_matching_external_login_if_email_is_missing() { db.users().insertUser(u -> u @@ -414,7 +440,7 @@ public class UserRegistrarImplTest { .setExternalId("Old id") .setEmail(null) .setExternalLogin(USER_IDENTITY.getProviderLogin()) - .setExternalIdentityProvider(IDENTITY_PROVIDER.getKey())); + .setExternalIdentityProvider(GH_IDENTITY_PROVIDER.getKey())); underTest.register(newUserRegistration()); @@ -443,7 +469,7 @@ public class UserRegistrarImplTest { .setLogin("Old login") .setExternalId(USER_IDENTITY.getProviderId()) .setExternalLogin("old identity") - .setExternalIdentityProvider(IDENTITY_PROVIDER.getKey())); + .setExternalIdentityProvider(GH_IDENTITY_PROVIDER.getKey())); underTest.register(newUserRegistration()); @@ -461,7 +487,7 @@ public class UserRegistrarImplTest { .setEmail(USER_IDENTITY.getEmail()) .setExternalId("Old id") .setExternalLogin(USER_IDENTITY.getProviderLogin()) - .setExternalIdentityProvider(IDENTITY_PROVIDER.getKey())); + .setExternalIdentityProvider(GH_IDENTITY_PROVIDER.getKey())); underTest.register(newUserRegistration()); @@ -471,7 +497,7 @@ public class UserRegistrarImplTest { 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.getExternalIdentityProvider()).isEqualTo(GH_IDENTITY_PROVIDER.getKey()); assertThat(userDto.isRoot()).isFalse(); } @@ -485,7 +511,7 @@ public class UserRegistrarImplTest { .setEmail("john@email.com") .build(); - Source source = Source.realm(AuthenticationEvent.Method.FORM, IDENTITY_PROVIDER.getName()); + Source source = Source.realm(AuthenticationEvent.Method.FORM, GH_IDENTITY_PROVIDER.getName()); expectedException.expect(authenticationException().from(source) .withLogin(userIdentity.getProviderLogin()) .andPublicMessage("This account is already associated with another authentication method." @@ -499,7 +525,7 @@ public class UserRegistrarImplTest { @Test public void authenticate_existing_user_succeeds_when_email_has_not_changed() { UserDto currentUser = db.users().insertUser(u -> u.setEmail("john@email.com") - .setExternalIdentityProvider(IDENTITY_PROVIDER.getKey())); + .setExternalIdentityProvider(GH_IDENTITY_PROVIDER.getKey())); UserIdentity userIdentity = UserIdentity.builder() .setProviderId(currentUser.getExternalId()) .setProviderLogin(currentUser.getExternalLogin()) @@ -516,7 +542,7 @@ public class UserRegistrarImplTest { @Test public void authenticate_existing_user_and_add_new_groups() { UserDto user = db.users().insertUser(newUserDto() - .setExternalIdentityProvider(IDENTITY_PROVIDER.getKey()) + .setExternalIdentityProvider(GH_IDENTITY_PROVIDER.getKey()) .setExternalLogin(USER_IDENTITY.getProviderLogin()) .setEmail(USER_IDENTITY.getEmail()) .setActive(true) @@ -532,7 +558,7 @@ public class UserRegistrarImplTest { @Test public void authenticate_existing_user_and_remove_groups() { UserDto user = db.users().insertUser(newUserDto() - .setExternalIdentityProvider(IDENTITY_PROVIDER.getKey()) + .setExternalIdentityProvider(GH_IDENTITY_PROVIDER.getKey()) .setExternalLogin(USER_IDENTITY.getProviderLogin()) .setEmail(USER_IDENTITY.getEmail()) .setActive(true) @@ -550,7 +576,7 @@ public class UserRegistrarImplTest { @Test public void authenticate_existing_user_and_remove_all_groups_expect_default() { UserDto user = db.users().insertUser(newUserDto() - .setExternalIdentityProvider(IDENTITY_PROVIDER.getKey()) + .setExternalIdentityProvider(GH_IDENTITY_PROVIDER.getKey()) .setExternalLogin(USER_IDENTITY.getProviderLogin())); GroupDto group1 = db.users().insertGroup("group1"); GroupDto group2 = db.users().insertGroup("group2"); @@ -570,7 +596,7 @@ public class UserRegistrarImplTest { private static UserRegistration newUserRegistration(UserIdentity userIdentity, Source source) { return UserRegistration.builder() .setUserIdentity(userIdentity) - .setProvider(IDENTITY_PROVIDER) + .setProvider(GH_IDENTITY_PROVIDER) .setSource(source) .build(); } -- 2.39.5