From caf0309d6e964af9f7bfd0b34e7b04c394bcc867 Mon Sep 17 00:00:00 2001 From: Jacek Date: Thu, 17 Oct 2019 13:54:28 +0200 Subject: [PATCH] SONAR-12531 drop usage of UserIdentity#getLogin and UserIdentity#setLogin (#2139) * remove login generation strategy * add migration to remove gh login strategy + code review fixes * support same login for new user and update login for SQ authority --- .../org/sonar/auth/github/GitHubSettings.java | 23 --- .../auth/github/UserIdentityFactoryImpl.java | 15 +- .../auth/github/UserIdentityGenerator.java | 51 ----- .../github/GitHubIdentityProviderTest.java | 3 - .../sonar/auth/github/GitHubModuleTest.java | 2 +- .../sonar/auth/github/GitHubSettingsTest.java | 22 +-- .../sonar/auth/github/IntegrationTest.java | 30 +-- .../github/UserIdentityFactoryImplTest.java | 57 +----- .../sonar/auth/gitlab/IntegrationTest.java | 1 - .../sonar/auth/saml/SamlIdentityProvider.java | 1 - .../auth/saml/SamlIdentityProviderTest.java | 6 +- .../java/org/sonar/db/user/UserDbTester.java | 17 +- .../db/migration/version/v80/DbVersion80.java | 1 + ...GitHubLoginGenerationStrategyProperty.java | 39 ++++ .../version/v80/DbVersion80Test.java | 2 +- ...ubLoginGenerationStrategyPropertyTest.java | 65 +++++++ .../schema.sql | 11 ++ .../CredentialsExternalAuthentication.java | 1 - .../HttpHeadersAuthentication.java | 1 - .../authentication/UserRegistrarImpl.java | 29 +-- .../org/sonar/server/user/UserUpdater.java | 2 +- .../BaseContextFactoryTest.java | 2 - ...CredentialsExternalAuthenticationTest.java | 3 - .../HttpHeadersAuthenticationTest.java | 2 +- .../server/authentication/InitFilterTest.java | 1 - .../OAuth2CallbackFilterTest.java | 2 - .../OAuth2ContextFactoryTest.java | 2 - .../authentication/TestUserRegistrar.java | 1 - ...serRegistrarImplOrgMembershipSyncTest.java | 15 +- .../authentication/UserRegistrarImplTest.java | 183 +++++++++--------- .../server/user/ws/UpdateLoginActionTest.java | 27 ++- .../server/authentication/UserIdentity.java | 21 +- .../authentication/UserIdentityTest.java | 42 ---- 33 files changed, 297 insertions(+), 383 deletions(-) delete mode 100644 server/sonar-auth-github/src/main/java/org/sonar/auth/github/UserIdentityGenerator.java create mode 100644 server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/version/v80/RemoveGitHubLoginGenerationStrategyProperty.java create mode 100644 server/sonar-db-migration/src/test/java/org/sonar/server/platform/db/migration/version/v80/RemoveGitHubLoginGenerationStrategyPropertyTest.java create mode 100644 server/sonar-db-migration/src/test/resources/org/sonar/server/platform/db/migration/version/v80/RemoveGitHubLoginGenerationStrategyPropertyTest/schema.sql diff --git a/server/sonar-auth-github/src/main/java/org/sonar/auth/github/GitHubSettings.java b/server/sonar-auth-github/src/main/java/org/sonar/auth/github/GitHubSettings.java index 7c3f7a2ced9..df8a8eb9de5 100644 --- a/server/sonar-auth-github/src/main/java/org/sonar/auth/github/GitHubSettings.java +++ b/server/sonar-auth-github/src/main/java/org/sonar/auth/github/GitHubSettings.java @@ -26,10 +26,8 @@ import javax.annotation.Nullable; import org.sonar.api.config.Configuration; import org.sonar.api.config.PropertyDefinition; -import static java.lang.String.format; import static java.lang.String.valueOf; import static org.sonar.api.PropertyType.BOOLEAN; -import static org.sonar.api.PropertyType.SINGLE_SELECT_LIST; import static org.sonar.api.PropertyType.STRING; public class GitHubSettings { @@ -42,11 +40,6 @@ public class GitHubSettings { private static final String API_URL = "sonar.auth.github.apiUrl"; private static final String WEB_URL = "sonar.auth.github.webUrl"; - static final String LOGIN_STRATEGY = "sonar.auth.github.loginStrategy"; - static final String LOGIN_STRATEGY_UNIQUE = "Unique"; - static final String LOGIN_STRATEGY_PROVIDER_ID = "Same as GitHub login"; - static final String LOGIN_STRATEGY_DEFAULT_VALUE = LOGIN_STRATEGY_UNIQUE; - private static final String ORGANIZATIONS = "sonar.auth.github.organizations"; private static final String CATEGORY = "security"; @@ -74,10 +67,6 @@ public class GitHubSettings { return configuration.getBoolean(ALLOW_USERS_TO_SIGN_UP).orElse(false); } - String loginStrategy() { - return configuration.get(LOGIN_STRATEGY).orElse(""); - } - boolean syncGroups() { return configuration.getBoolean(GROUPS_SYNC).orElse(false); } @@ -138,18 +127,6 @@ public class GitHubSettings { .defaultValue(valueOf(true)) .index(4) .build(), - PropertyDefinition.builder(LOGIN_STRATEGY) - .name("Login generation strategy") - .description(format("When the login strategy is set to '%s', the user's login will be auto-generated the first time so that it is unique. " + - "When the login strategy is set to '%s', the user's login will be the GitHub login.", - LOGIN_STRATEGY_UNIQUE, LOGIN_STRATEGY_PROVIDER_ID)) - .category(CATEGORY) - .subCategory(SUBCATEGORY) - .type(SINGLE_SELECT_LIST) - .defaultValue(LOGIN_STRATEGY_DEFAULT_VALUE) - .options(LOGIN_STRATEGY_UNIQUE, LOGIN_STRATEGY_PROVIDER_ID) - .index(5) - .build(), PropertyDefinition.builder(GROUPS_SYNC) .name("Synchronize teams as groups") .description("For each team he belongs to, the user will be associated to a group named 'Organisation/Team' (if it exists) in SonarQube.") diff --git a/server/sonar-auth-github/src/main/java/org/sonar/auth/github/UserIdentityFactoryImpl.java b/server/sonar-auth-github/src/main/java/org/sonar/auth/github/UserIdentityFactoryImpl.java index a93798a17d6..ca0fdb3e623 100644 --- a/server/sonar-auth-github/src/main/java/org/sonar/auth/github/UserIdentityFactoryImpl.java +++ b/server/sonar-auth-github/src/main/java/org/sonar/auth/github/UserIdentityFactoryImpl.java @@ -24,23 +24,13 @@ import java.util.stream.Collectors; import javax.annotation.Nullable; import org.sonar.api.server.authentication.UserIdentity; -import static org.sonar.auth.github.UserIdentityGenerator.generateLogin; -import static org.sonar.auth.github.UserIdentityGenerator.generateName; - public class UserIdentityFactoryImpl implements UserIdentityFactory { - private final GitHubSettings settings; - - public UserIdentityFactoryImpl(GitHubSettings settings) { - this.settings = settings; - } - @Override public UserIdentity create(GsonUser user, @Nullable String email, @Nullable List teams) { UserIdentity.Builder builder = UserIdentity.builder() .setProviderId(user.getId()) .setProviderLogin(user.getLogin()) - .setLogin(generateLogin(user, settings.loginStrategy())) .setName(generateName(user)) .setEmail(email); if (teams != null) { @@ -51,4 +41,9 @@ public class UserIdentityFactoryImpl implements UserIdentityFactory { return builder.build(); } + private static String generateName(GsonUser gson) { + String name = gson.getName(); + return name == null || name.isEmpty() ? gson.getLogin() : name; + } + } diff --git a/server/sonar-auth-github/src/main/java/org/sonar/auth/github/UserIdentityGenerator.java b/server/sonar-auth-github/src/main/java/org/sonar/auth/github/UserIdentityGenerator.java deleted file mode 100644 index 4b3dbdf7dd0..00000000000 --- a/server/sonar-auth-github/src/main/java/org/sonar/auth/github/UserIdentityGenerator.java +++ /dev/null @@ -1,51 +0,0 @@ -/* - * SonarQube - * Copyright (C) 2009-2019 SonarSource SA - * mailto:info AT sonarsource DOT com - * - * This program 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. - * - * This program 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.auth.github; - -import static java.lang.String.format; -import static org.sonar.auth.github.GitHubSettings.LOGIN_STRATEGY_PROVIDER_ID; -import static org.sonar.auth.github.GitHubSettings.LOGIN_STRATEGY_UNIQUE; - -class UserIdentityGenerator { - - private UserIdentityGenerator() { - // Only static method - } - - static String generateLogin(GsonUser gsonUser, String loginStrategy) { - switch (loginStrategy) { - case LOGIN_STRATEGY_PROVIDER_ID: - return gsonUser.getLogin(); - case LOGIN_STRATEGY_UNIQUE: - return generateUniqueLogin(gsonUser); - default: - throw new IllegalStateException(format("Login strategy not supported : %s", loginStrategy)); - } - } - - static String generateName(GsonUser gson) { - String name = gson.getName(); - return name == null || name.isEmpty() ? gson.getLogin() : name; - } - - private static String generateUniqueLogin(GsonUser gsonUser) { - return format("%s@%s", gsonUser.getLogin(), GitHubIdentityProvider.KEY); - } -} diff --git a/server/sonar-auth-github/src/test/java/org/sonar/auth/github/GitHubIdentityProviderTest.java b/server/sonar-auth-github/src/test/java/org/sonar/auth/github/GitHubIdentityProviderTest.java index ac78fd84048..34876623481 100644 --- a/server/sonar-auth-github/src/test/java/org/sonar/auth/github/GitHubIdentityProviderTest.java +++ b/server/sonar-auth-github/src/test/java/org/sonar/auth/github/GitHubIdentityProviderTest.java @@ -29,7 +29,6 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; -import static org.sonar.auth.github.GitHubSettings.LOGIN_STRATEGY_DEFAULT_VALUE; public class GitHubIdentityProviderTest { @@ -55,7 +54,6 @@ public class GitHubIdentityProviderTest { public void is_enabled() { settings.setProperty("sonar.auth.github.clientId.secured", "id"); settings.setProperty("sonar.auth.github.clientSecret.secured", "secret"); - settings.setProperty("sonar.auth.github.loginStrategy", LOGIN_STRATEGY_DEFAULT_VALUE); settings.setProperty("sonar.auth.github.enabled", true); assertThat(underTest.isEnabled()).isTrue(); @@ -176,7 +174,6 @@ public class GitHubIdentityProviderTest { if (enabled) { settings.setProperty("sonar.auth.github.clientId.secured", "id"); settings.setProperty("sonar.auth.github.clientSecret.secured", "secret"); - settings.setProperty("sonar.auth.github.loginStrategy", LOGIN_STRATEGY_DEFAULT_VALUE); settings.setProperty("sonar.auth.github.enabled", true); } else { settings.setProperty("sonar.auth.github.enabled", false); diff --git a/server/sonar-auth-github/src/test/java/org/sonar/auth/github/GitHubModuleTest.java b/server/sonar-auth-github/src/test/java/org/sonar/auth/github/GitHubModuleTest.java index 0589b7a2e27..8903a766e15 100644 --- a/server/sonar-auth-github/src/test/java/org/sonar/auth/github/GitHubModuleTest.java +++ b/server/sonar-auth-github/src/test/java/org/sonar/auth/github/GitHubModuleTest.java @@ -31,7 +31,7 @@ public class GitHubModuleTest { public void verify_count_of_added_components() { ComponentContainer container = new ComponentContainer(); new GitHubModule().configure(container); - assertThat(container.size()).isEqualTo(COMPONENTS_IN_EMPTY_COMPONENT_CONTAINER + 14); + assertThat(container.size()).isEqualTo(COMPONENTS_IN_EMPTY_COMPONENT_CONTAINER + 13); } } diff --git a/server/sonar-auth-github/src/test/java/org/sonar/auth/github/GitHubSettingsTest.java b/server/sonar-auth-github/src/test/java/org/sonar/auth/github/GitHubSettingsTest.java index 422990b7e85..4e466aa92ac 100644 --- a/server/sonar-auth-github/src/test/java/org/sonar/auth/github/GitHubSettingsTest.java +++ b/server/sonar-auth-github/src/test/java/org/sonar/auth/github/GitHubSettingsTest.java @@ -24,8 +24,6 @@ import org.sonar.api.config.PropertyDefinitions; import org.sonar.api.config.internal.MapSettings; import static org.assertj.core.api.Assertions.assertThat; -import static org.sonar.auth.github.GitHubSettings.LOGIN_STRATEGY_DEFAULT_VALUE; -import static org.sonar.auth.github.GitHubSettings.LOGIN_STRATEGY_PROVIDER_ID; public class GitHubSettingsTest { @@ -37,7 +35,6 @@ public class GitHubSettingsTest { public void is_enabled() { settings.setProperty("sonar.auth.github.clientId.secured", "id"); settings.setProperty("sonar.auth.github.clientSecret.secured", "secret"); - settings.setProperty("sonar.auth.github.loginStrategy", LOGIN_STRATEGY_DEFAULT_VALUE); settings.setProperty("sonar.auth.github.enabled", true); assertThat(underTest.isEnabled()).isTrue(); @@ -51,7 +48,6 @@ public class GitHubSettingsTest { settings.setProperty("sonar.auth.github.enabled", true); settings.setProperty("sonar.auth.github.clientId.secured", (String) null); settings.setProperty("sonar.auth.github.clientSecret.secured", "secret"); - settings.setProperty("sonar.auth.github.loginStrategy", LOGIN_STRATEGY_DEFAULT_VALUE); assertThat(underTest.isEnabled()).isFalse(); } @@ -61,7 +57,6 @@ public class GitHubSettingsTest { settings.setProperty("sonar.auth.github.enabled", true); settings.setProperty("sonar.auth.github.clientId.secured", "id"); settings.setProperty("sonar.auth.github.clientSecret.secured", (String) null); - settings.setProperty("sonar.auth.github.loginStrategy", LOGIN_STRATEGY_DEFAULT_VALUE); assertThat(underTest.isEnabled()).isFalse(); } @@ -78,12 +73,6 @@ public class GitHubSettingsTest { assertThat(underTest.clientSecret()).isEqualTo("secret"); } - @Test - public void return_login_strategy() { - settings.setProperty("sonar.auth.github.loginStrategy", LOGIN_STRATEGY_PROVIDER_ID); - assertThat(underTest.loginStrategy()).isEqualTo(LOGIN_STRATEGY_PROVIDER_ID); - } - @Test public void allow_users_to_sign_up() { settings.setProperty("sonar.auth.github.allowUsersToSignUp", "true"); @@ -132,31 +121,28 @@ public class GitHubSettingsTest { public void return_organizations_single() { String setting = "example"; settings.setProperty("sonar.auth.github.organizations", setting); - String[] expected = new String[] {"example"}; String[] actual = underTest.organizations(); - assertThat(actual).isEqualTo(expected); + assertThat(actual).containsOnly(setting); } @Test public void return_organizations_multiple() { String setting = "example0,example1"; settings.setProperty("sonar.auth.github.organizations", setting); - String[] expected = new String[] {"example0", "example1"}; String[] actual = underTest.organizations(); - assertThat(actual).isEqualTo(expected); + assertThat(actual).containsOnly("example0", "example1"); } @Test public void return_organizations_empty_list() { String[] setting = null; settings.setProperty("sonar.auth.github.organizations", setting); - String[] expected = new String[] {}; String[] actual = underTest.organizations(); - assertThat(actual).isEqualTo(expected); + assertThat(actual).hasSize(0); } @Test public void definitions() { - assertThat(GitHubSettings.definitions()).hasSize(9); + assertThat(GitHubSettings.definitions()).hasSize(8); } } diff --git a/server/sonar-auth-github/src/test/java/org/sonar/auth/github/IntegrationTest.java b/server/sonar-auth-github/src/test/java/org/sonar/auth/github/IntegrationTest.java index e3ef3551850..90c8cfbf394 100644 --- a/server/sonar-auth-github/src/test/java/org/sonar/auth/github/IntegrationTest.java +++ b/server/sonar-auth-github/src/test/java/org/sonar/auth/github/IntegrationTest.java @@ -54,7 +54,7 @@ public class IntegrationTest { // load settings with default values private MapSettings settings = new MapSettings(new PropertyDefinitions(GitHubSettings.definitions())); private GitHubSettings gitHubSettings = new GitHubSettings(settings.asConfig()); - private UserIdentityFactoryImpl userIdentityFactory = new UserIdentityFactoryImpl(gitHubSettings); + private UserIdentityFactoryImpl userIdentityFactory = new UserIdentityFactoryImpl(); private ScribeGitHubApi scribeApi = new ScribeGitHubApi(gitHubSettings); private GitHubRestClient gitHubRestClient = new GitHubRestClient(gitHubSettings); @@ -111,7 +111,7 @@ public class IntegrationTest { assertThat(callbackContext.csrfStateVerified.get()).isTrue(); assertThat(callbackContext.userIdentity.getProviderId()).isEqualTo("ABCD"); - assertThat(callbackContext.userIdentity.getLogin()).isEqualTo("octocat@github"); + assertThat(callbackContext.userIdentity.getProviderLogin()).isEqualTo("octocat"); assertThat(callbackContext.userIdentity.getName()).isEqualTo("monalisa octocat"); assertThat(callbackContext.userIdentity.getEmail()).isEqualTo("octocat@github.com"); assertThat(callbackContext.redirectedToRequestedPage.get()).isTrue(); @@ -156,7 +156,7 @@ public class IntegrationTest { assertThat(callbackContext.csrfStateVerified.get()).isTrue(); assertThat(callbackContext.userIdentity.getProviderId()).isEqualTo("ABCD"); - assertThat(callbackContext.userIdentity.getLogin()).isEqualTo("octocat@github"); + assertThat(callbackContext.userIdentity.getProviderLogin()).isEqualTo("octocat"); assertThat(callbackContext.userIdentity.getName()).isEqualTo("monalisa octocat"); assertThat(callbackContext.userIdentity.getEmail()).isEqualTo("octocat@github.com"); assertThat(callbackContext.redirectedToRequestedPage.get()).isTrue(); @@ -176,7 +176,7 @@ public class IntegrationTest { assertThat(callbackContext.csrfStateVerified.get()).isTrue(); assertThat(callbackContext.userIdentity.getProviderId()).isEqualTo("ABCD"); - assertThat(callbackContext.userIdentity.getLogin()).isEqualTo("octocat@github"); + assertThat(callbackContext.userIdentity.getProviderLogin()).isEqualTo("octocat"); assertThat(callbackContext.userIdentity.getName()).isEqualTo("monalisa octocat"); assertThat(callbackContext.userIdentity.getEmail()).isNull(); assertThat(callbackContext.redirectedToRequestedPage.get()).isTrue(); @@ -294,7 +294,6 @@ public class IntegrationTest { @Test public void callback_on_successful_authentication_with_organizations_without_membership() { settings.setProperty("sonar.auth.github.organizations", "first_org,second_org"); - settings.setProperty("sonar.auth.github.loginStrategy", GitHubSettings.LOGIN_STRATEGY_PROVIDER_ID); github.enqueue(newSuccessfulAccessTokenResponse()); // response of api.github.com/user @@ -314,27 +313,6 @@ public class IntegrationTest { } } - @Test - public void callback_on_successful_authentication_with_organizations_without_membership_with_unique_login_strategy() { - settings.setProperty("sonar.auth.github.organizations", "example"); - settings.setProperty("sonar.auth.github.loginStrategy", GitHubSettings.LOGIN_STRATEGY_UNIQUE); - - github.enqueue(newSuccessfulAccessTokenResponse()); - // response of api.github.com/user - github.enqueue(new MockResponse().setBody("{\"id\":\"ABCD\", \"login\":\"octocat\", \"name\":\"monalisa octocat\",\"email\":\"octocat@github.com\"}")); - // response of api.github.com/orgs/example0/members/user - github.enqueue(new MockResponse().setResponseCode(404).setBody("{}")); - - HttpServletRequest request = newRequest("the-verifier-code"); - DumbCallbackContext callbackContext = new DumbCallbackContext(request); - try { - underTest.callback(callbackContext); - fail("exception expected"); - } catch (UnauthorizedException e) { - assertThat(e.getMessage()).isEqualTo("'octocat' must be a member of at least one organization: 'example'"); - } - } - @Test public void callback_throws_ISE_if_error_when_requesting_user_profile() { github.enqueue(newSuccessfulAccessTokenResponse()); diff --git a/server/sonar-auth-github/src/test/java/org/sonar/auth/github/UserIdentityFactoryImplTest.java b/server/sonar-auth-github/src/test/java/org/sonar/auth/github/UserIdentityFactoryImplTest.java index cad51844221..3dbe8bc6128 100644 --- a/server/sonar-auth-github/src/test/java/org/sonar/auth/github/UserIdentityFactoryImplTest.java +++ b/server/sonar-auth-github/src/test/java/org/sonar/auth/github/UserIdentityFactoryImplTest.java @@ -19,8 +19,6 @@ */ package org.sonar.auth.github; -import java.util.Arrays; -import java.util.List; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; @@ -36,56 +34,15 @@ public class UserIdentityFactoryImplTest { public ExpectedException expectedException = ExpectedException.none(); private MapSettings settings = new MapSettings(new PropertyDefinitions(GitHubSettings.definitions())); - private UserIdentityFactoryImpl underTest = new UserIdentityFactoryImpl(new GitHubSettings(settings.asConfig())); - - /** - * Keep the same login as at GitHub - */ - @Test - public void create_for_provider_strategy() { - GsonUser gson = new GsonUser("ABCD", "octocat", "monalisa octocat", "octocat@github.com"); - settings.setProperty(GitHubSettings.LOGIN_STRATEGY, GitHubSettings.LOGIN_STRATEGY_PROVIDER_ID); - - UserIdentity identity = underTest.create(gson, gson.getEmail(), null); - - assertThat(identity.getProviderId()).isEqualTo("ABCD"); - assertThat(identity.getLogin()).isEqualTo("octocat"); - assertThat(identity.getName()).isEqualTo("monalisa octocat"); - assertThat(identity.getEmail()).isEqualTo("octocat@github.com"); - } + private UserIdentityFactoryImpl underTest = new UserIdentityFactoryImpl(); @Test public void no_email() { GsonUser gson = new GsonUser("ABCD", "octocat", "monalisa octocat", null); - settings.setProperty(GitHubSettings.LOGIN_STRATEGY, GitHubSettings.LOGIN_STRATEGY_PROVIDER_ID); - - UserIdentity identity = underTest.create(gson, null, null); - - assertThat(identity.getLogin()).isEqualTo("octocat"); - assertThat(identity.getName()).isEqualTo("monalisa octocat"); - assertThat(identity.getEmail()).isNull(); - } - - @Test - public void create_for_provider_strategy_with_teams() { - GsonUser gson = new GsonUser("ABCD", "octocat", "monalisa octocat", "octocat@github.com"); - List teams = Arrays.asList( - new GsonTeam("developers", new GsonTeam.GsonOrganization("SonarSource"))); - settings.setProperty(GitHubSettings.LOGIN_STRATEGY, GitHubSettings.LOGIN_STRATEGY_PROVIDER_ID); - - UserIdentity identity = underTest.create(gson, null, teams); - - assertThat(identity.getGroups()).containsOnly("SonarSource/developers"); - } - - @Test - public void create_for_unique_login_strategy() { - GsonUser gson = new GsonUser("ABCD", "octocat", "monalisa octocat", "octocat@github.com"); - settings.setProperty(GitHubSettings.LOGIN_STRATEGY, GitHubSettings.LOGIN_STRATEGY_UNIQUE); UserIdentity identity = underTest.create(gson, null, null); - assertThat(identity.getLogin()).isEqualTo("octocat@github"); + assertThat(identity.getProviderLogin()).isEqualTo("octocat"); assertThat(identity.getName()).isEqualTo("monalisa octocat"); assertThat(identity.getEmail()).isNull(); } @@ -107,14 +64,4 @@ public class UserIdentityFactoryImplTest { assertThat(identity.getName()).isEqualTo("octocat"); } - - @Test - public void throw_ISE_if_strategy_is_not_supported() { - settings.setProperty(GitHubSettings.LOGIN_STRATEGY, "xxx"); - - expectedException.expect(IllegalStateException.class); - expectedException.expectMessage("Login strategy not supported : xxx"); - - underTest.create(new GsonUser("ABCD", "octocat", "octocat", "octocat@github.com"), null, null); - } } diff --git a/server/sonar-auth-gitlab/src/test/java/org/sonar/auth/gitlab/IntegrationTest.java b/server/sonar-auth-gitlab/src/test/java/org/sonar/auth/gitlab/IntegrationTest.java index 158d8457910..c552d435e07 100644 --- a/server/sonar-auth-gitlab/src/test/java/org/sonar/auth/gitlab/IntegrationTest.java +++ b/server/sonar-auth-gitlab/src/test/java/org/sonar/auth/gitlab/IntegrationTest.java @@ -94,7 +94,6 @@ public class IntegrationTest { ArgumentCaptor argument = ArgumentCaptor.forClass(UserIdentity.class); verify(callbackContext).authenticate(argument.capture()); assertThat(argument.getValue()).isNotNull(); - assertThat(argument.getValue().getLogin()).isNull(); assertThat(argument.getValue().getProviderId()).isEqualTo("123"); assertThat(argument.getValue().getProviderLogin()).isEqualTo("toto"); assertThat(argument.getValue().getName()).isEqualTo("Toto Toto"); diff --git a/server/sonar-auth-saml/src/main/java/org/sonar/auth/saml/SamlIdentityProvider.java b/server/sonar-auth-saml/src/main/java/org/sonar/auth/saml/SamlIdentityProvider.java index 1107d4f6acd..acd609a5fbb 100644 --- a/server/sonar-auth-saml/src/main/java/org/sonar/auth/saml/SamlIdentityProvider.java +++ b/server/sonar-auth-saml/src/main/java/org/sonar/auth/saml/SamlIdentityProvider.java @@ -111,7 +111,6 @@ public class SamlIdentityProvider implements OAuth2IdentityProvider { LOGGER.trace("Attributes received : {}", auth.getAttributes()); String login = getNonNullFirstAttribute(auth, samlSettings.getUserLogin()); UserIdentity.Builder userIdentityBuilder = UserIdentity.builder() - .setLogin(login) .setProviderLogin(login) .setName(getNonNullFirstAttribute(auth, samlSettings.getUserName())); samlSettings.getUserEmail().ifPresent( diff --git a/server/sonar-auth-saml/src/test/java/org/sonar/auth/saml/SamlIdentityProviderTest.java b/server/sonar-auth-saml/src/test/java/org/sonar/auth/saml/SamlIdentityProviderTest.java index 7ec3cf1a150..c72a10789e5 100644 --- a/server/sonar-auth-saml/src/test/java/org/sonar/auth/saml/SamlIdentityProviderTest.java +++ b/server/sonar-auth-saml/src/test/java/org/sonar/auth/saml/SamlIdentityProviderTest.java @@ -111,7 +111,7 @@ public class SamlIdentityProviderTest { underTest.callback(callbackContext); assertThat(callbackContext.redirectedToRequestedPage.get()).isTrue(); - assertThat(callbackContext.userIdentity.getLogin()).isEqualTo("johndoe"); + assertThat(callbackContext.userIdentity.getProviderLogin()).isEqualTo("johndoe"); assertThat(callbackContext.verifyState.get()).isTrue(); } @@ -122,7 +122,6 @@ public class SamlIdentityProviderTest { underTest.callback(callbackContext); - assertThat(callbackContext.userIdentity.getLogin()).isEqualTo("johndoe"); assertThat(callbackContext.userIdentity.getName()).isEqualTo("John Doe"); assertThat(callbackContext.userIdentity.getEmail()).isEqualTo("johndoe@email.com"); assertThat(callbackContext.userIdentity.getProviderLogin()).isEqualTo("johndoe"); @@ -136,7 +135,6 @@ public class SamlIdentityProviderTest { underTest.callback(callbackContext); - assertThat(callbackContext.userIdentity.getLogin()).isEqualTo("johndoe"); assertThat(callbackContext.userIdentity.getName()).isEqualTo("John Doe"); assertThat(callbackContext.userIdentity.getEmail()).isNull(); assertThat(callbackContext.userIdentity.getProviderLogin()).isEqualTo("johndoe"); @@ -151,7 +149,7 @@ public class SamlIdentityProviderTest { underTest.callback(callbackContext); - assertThat(callbackContext.userIdentity.getLogin()).isEqualTo("johndoe"); + assertThat(callbackContext.userIdentity.getProviderLogin()).isEqualTo("johndoe"); assertThat(callbackContext.userIdentity.getGroups()).isEmpty(); } diff --git a/server/sonar-db-dao/src/testFixtures/java/org/sonar/db/user/UserDbTester.java b/server/sonar-db-dao/src/testFixtures/java/org/sonar/db/user/UserDbTester.java index bc58c94d529..472fe0c6012 100644 --- a/server/sonar-db-dao/src/testFixtures/java/org/sonar/db/user/UserDbTester.java +++ b/server/sonar-db-dao/src/testFixtures/java/org/sonar/db/user/UserDbTester.java @@ -41,11 +41,6 @@ import static com.google.common.base.Preconditions.checkArgument; import static java.lang.String.format; import static java.util.Arrays.stream; import static org.sonar.db.permission.OrganizationPermission.ADMINISTER; -import static org.sonar.db.user.GroupTesting.newGroupDto; -import static org.sonar.db.user.UserTesting.newDisabledUser; -import static org.sonar.db.user.UserTesting.newUserDto; -import static org.sonar.db.user.UserTesting.newUserSettingDto; -import static org.sonar.db.user.UserTokenTesting.newUserToken; public class UserDbTester { private static final Set PUBLIC_PERMISSIONS = ImmutableSet.of(UserRole.USER, UserRole.CODEVIEWER); // FIXME to check with Simon @@ -117,6 +112,18 @@ public class UserDbTester { return Optional.ofNullable(dbClient.userDao().selectByLogin(db.getSession(), login)); } + public Optional selectUserByEmail(String email) { + List users = dbClient.userDao().selectByEmail(db.getSession(), email); + if (users.size() > 1) { + return Optional.empty(); + } + return Optional.of(users.get(0)); + } + + public Optional selectUserByExternalLoginAndIdentityProvider(String login, String identityProvider) { + return Optional.ofNullable(dbClient.userDao().selectByExternalLoginAndIdentityProvider(db.getSession(), login, identityProvider)); + } + // USER SETTINGS @SafeVarargs diff --git a/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/version/v80/DbVersion80.java b/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/version/v80/DbVersion80.java index 5bb2ab5e9e4..3cc49559ff0 100644 --- a/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/version/v80/DbVersion80.java +++ b/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/version/v80/DbVersion80.java @@ -35,6 +35,7 @@ public class DbVersion80 implements DbVersion { .add(3006, "Create NEW_CODE_PERIOD table", CreateNewCodePeriodTable.class) .add(3007, "Populate NEW_CODE_PERIOD table", PopulateNewCodePeriodTable.class) .add(3008, "Remove leak period properties", RemoveLeakPeriodProperties.class) + .add(3009, "Remove GitHub login generation strategy property", RemoveGitHubLoginGenerationStrategyProperty.class) ; } } diff --git a/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/version/v80/RemoveGitHubLoginGenerationStrategyProperty.java b/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/version/v80/RemoveGitHubLoginGenerationStrategyProperty.java new file mode 100644 index 00000000000..bc30f182220 --- /dev/null +++ b/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/version/v80/RemoveGitHubLoginGenerationStrategyProperty.java @@ -0,0 +1,39 @@ +/* + * SonarQube + * Copyright (C) 2009-2019 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program 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. + * + * This program 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.platform.db.migration.version.v80; + +import java.sql.SQLException; +import org.sonar.db.Database; +import org.sonar.server.platform.db.migration.step.DataChange; + +public class RemoveGitHubLoginGenerationStrategyProperty extends DataChange { + private static final String GH_LOGIN_STRATEGY_PROP_KEY = "sonar.auth.github.loginStrategy"; + + public RemoveGitHubLoginGenerationStrategyProperty(Database db) { + super(db); + } + + @Override + protected void execute(Context context) throws SQLException { + context.prepareUpsert("delete from properties where prop_key='" + GH_LOGIN_STRATEGY_PROP_KEY + "'") + .execute() + .commit(); + } +} diff --git a/server/sonar-db-migration/src/test/java/org/sonar/server/platform/db/migration/version/v80/DbVersion80Test.java b/server/sonar-db-migration/src/test/java/org/sonar/server/platform/db/migration/version/v80/DbVersion80Test.java index a4a6ecde6fa..fe63e2debcb 100644 --- a/server/sonar-db-migration/src/test/java/org/sonar/server/platform/db/migration/version/v80/DbVersion80Test.java +++ b/server/sonar-db-migration/src/test/java/org/sonar/server/platform/db/migration/version/v80/DbVersion80Test.java @@ -35,7 +35,7 @@ public class DbVersion80Test { @Test public void verify_migration_count() { - verifyMigrationCount(underTest, 9); + verifyMigrationCount(underTest, 10); } } diff --git a/server/sonar-db-migration/src/test/java/org/sonar/server/platform/db/migration/version/v80/RemoveGitHubLoginGenerationStrategyPropertyTest.java b/server/sonar-db-migration/src/test/java/org/sonar/server/platform/db/migration/version/v80/RemoveGitHubLoginGenerationStrategyPropertyTest.java new file mode 100644 index 00000000000..ec9b6eb9aec --- /dev/null +++ b/server/sonar-db-migration/src/test/java/org/sonar/server/platform/db/migration/version/v80/RemoveGitHubLoginGenerationStrategyPropertyTest.java @@ -0,0 +1,65 @@ +/* + * SonarQube + * Copyright (C) 2009-2019 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program 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. + * + * This program 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.platform.db.migration.version.v80; + +import java.sql.SQLException; +import java.time.Instant; +import org.junit.Assert; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.ExpectedException; +import org.sonar.db.CoreDbTester; + +public class RemoveGitHubLoginGenerationStrategyPropertyTest { + private static final String PROPERTIES_TABLE_NAME = "properties"; + + @Rule + public CoreDbTester dbTester = CoreDbTester.createForSchema(RemoveGitHubLoginGenerationStrategyPropertyTest.class, "schema.sql"); + + @Rule + public ExpectedException expectedException = ExpectedException.none(); + + private RemoveGitHubLoginGenerationStrategyProperty underTest = new RemoveGitHubLoginGenerationStrategyProperty(dbTester.database()); + + @Test + public void remove_github_login_strategy_property() throws SQLException { + insertGitHubLoginStrategy(); + + int propertiesCount = dbTester.countRowsOfTable(PROPERTIES_TABLE_NAME); + Assert.assertEquals(1, propertiesCount); + + underTest.execute(); + + //should delete properties + propertiesCount = dbTester.countRowsOfTable(PROPERTIES_TABLE_NAME); + Assert.assertEquals(0, propertiesCount); + + //should not fail if executed twice + underTest.execute(); + } + + private void insertGitHubLoginStrategy() { + dbTester.executeInsert(PROPERTIES_TABLE_NAME, + "prop_key", "sonar.auth.github.loginStrategy", + "is_empty", false, + "text_value", "Unique", + "created_at", Instant.now().toEpochMilli()); + } +} diff --git a/server/sonar-db-migration/src/test/resources/org/sonar/server/platform/db/migration/version/v80/RemoveGitHubLoginGenerationStrategyPropertyTest/schema.sql b/server/sonar-db-migration/src/test/resources/org/sonar/server/platform/db/migration/version/v80/RemoveGitHubLoginGenerationStrategyPropertyTest/schema.sql new file mode 100644 index 00000000000..d84c238cd48 --- /dev/null +++ b/server/sonar-db-migration/src/test/resources/org/sonar/server/platform/db/migration/version/v80/RemoveGitHubLoginGenerationStrategyPropertyTest/schema.sql @@ -0,0 +1,11 @@ +CREATE TABLE "PROPERTIES" ( + "ID" INTEGER NOT NULL GENERATED BY DEFAULT AS IDENTITY (START WITH 1, INCREMENT BY 1), + "PROP_KEY" VARCHAR(512) NOT NULL, + "RESOURCE_ID" INTEGER, + "USER_ID" INTEGER, + "IS_EMPTY" BOOLEAN NOT NULL, + "TEXT_VALUE" VARCHAR(4000), + "CLOB_VALUE" CLOB, + "CREATED_AT" BIGINT +); +CREATE INDEX "PROPERTIES_KEY" ON "PROPERTIES" ("PROP_KEY"); diff --git a/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/CredentialsExternalAuthentication.java b/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/CredentialsExternalAuthentication.java index 6a0187e35c9..1f427a56e41 100644 --- a/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/CredentialsExternalAuthentication.java +++ b/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/CredentialsExternalAuthentication.java @@ -133,7 +133,6 @@ public class CredentialsExternalAuthentication implements Startable { private UserDto synchronize(String userLogin, UserDetails details, HttpServletRequest request, AuthenticationEvent.Method method) { String name = details.getName(); UserIdentity.Builder userIdentityBuilder = UserIdentity.builder() - .setLogin(userLogin) .setName(isEmpty(name) ? userLogin : name) .setEmail(trimToNull(details.getEmail())) .setProviderLogin(userLogin); diff --git a/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/HttpHeadersAuthentication.java b/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/HttpHeadersAuthentication.java index 29ed2e65195..b0e4203678a 100644 --- a/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/HttpHeadersAuthentication.java +++ b/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/HttpHeadersAuthentication.java @@ -157,7 +157,6 @@ public class HttpHeadersAuthentication implements Startable { String name = getHeaderValue(headerValuesByNames, SONAR_WEB_SSO_NAME_HEADER.getKey()); String email = getHeaderValue(headerValuesByNames, SONAR_WEB_SSO_EMAIL_HEADER.getKey()); UserIdentity.Builder userIdentityBuilder = UserIdentity.builder() - .setLogin(login) .setName(name == null ? login : name) .setEmail(email) .setProviderLogin(login); 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 6f408e3c7fc..8b06ed5e6a5 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 @@ -62,6 +62,7 @@ import static org.sonar.core.util.stream.MoreCollectors.uniqueIndex; public class UserRegistrarImpl implements UserRegistrar { private static final Logger LOGGER = Loggers.get(UserRegistrarImpl.class); + private static final String SQ_AUTHORITY = "sonarqube"; private final DbClient dbClient; private final UserUpdater userUpdater; @@ -102,16 +103,7 @@ public class UserRegistrarImpl implements UserRegistrar { return user; } // Then, try with the external login, for instance when external ID has changed - user = dbClient.userDao().selectByExternalLoginAndIdentityProvider(dbSession, userIdentity.getProviderLogin(), provider.getKey()); - if (user != null) { - return user; - } - // Last, try with login, for instance when external ID and external login has been updated - String login = userIdentity.getLogin(); - if (login == null) { - return null; - } - return dbClient.userDao().selectByLogin(dbSession, login); + return dbClient.userDao().selectByExternalLoginAndIdentityProvider(dbSession, userIdentity.getProviderLogin(), provider.getKey()); } private UserDto registerNewUser(DbSession dbSession, @Nullable UserDto disabledUser, UserRegistration authenticatorParameters) { @@ -131,10 +123,6 @@ public class UserRegistrarImpl implements UserRegistrar { authenticatorParameters.getProvider().getKey(), authenticatorParameters.getUserIdentity().getProviderLogin(), authenticatorParameters.getUserIdentity().getProviderId())); - String login = authenticatorParameters.getUserIdentity().getLogin(); - if (login != null) { - update.setLogin(login); - } Optional otherUserToIndex = detectEmailUpdate(dbSession, authenticatorParameters); userUpdater.updateAndCommit(dbSession, userDto, update, beforeCommit(dbSession, false, authenticatorParameters), toArray(otherUserToIndex)); return userDto; @@ -180,12 +168,10 @@ public class UserRegistrarImpl implements UserRegistrar { } private static boolean isSameUser(UserDto existingUser, UserRegistration authenticatorParameters) { - // Check if same login - return Objects.equals(existingUser.getLogin(), authenticatorParameters.getUserIdentity().getLogin()) || // Check if same identity provider and same provider id or provider login - (Objects.equals(existingUser.getExternalIdentityProvider(), authenticatorParameters.getProvider().getKey()) && - (Objects.equals(existingUser.getExternalId(), getProviderIdOrProviderLogin(authenticatorParameters.getUserIdentity())) - || Objects.equals(existingUser.getExternalLogin(), authenticatorParameters.getUserIdentity().getProviderLogin()))); + 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) { @@ -257,14 +243,15 @@ public class UserRegistrarImpl implements UserRegistrar { .setPublicMessage(format("'%s' users are not allowed to sign up", identityProviderKey)) .build(); } + String providerLogin = authenticatorParameters.getUserIdentity().getProviderLogin(); return NewUser.builder() - .setLogin(authenticatorParameters.getUserIdentity().getLogin()) + .setLogin(SQ_AUTHORITY.equals(identityProviderKey) ? providerLogin : null) .setEmail(authenticatorParameters.getUserIdentity().getEmail()) .setName(authenticatorParameters.getUserIdentity().getName()) .setExternalIdentity( new ExternalIdentity( identityProviderKey, - authenticatorParameters.getUserIdentity().getProviderLogin(), + providerLogin, authenticatorParameters.getUserIdentity().getProviderId())) .build(); } diff --git a/server/sonar-webserver-auth/src/main/java/org/sonar/server/user/UserUpdater.java b/server/sonar-webserver-auth/src/main/java/org/sonar/server/user/UserUpdater.java index 4c41bbd9ff7..f4081a2482a 100644 --- a/server/sonar-webserver-auth/src/main/java/org/sonar/server/user/UserUpdater.java +++ b/server/sonar-webserver-auth/src/main/java/org/sonar/server/user/UserUpdater.java @@ -229,7 +229,7 @@ public class UserUpdater { dbClient.propertiesDao().selectByKeyAndMatchingValue(dbSession, DEFAULT_ISSUE_ASSIGNEE, userDto.getLogin()) .forEach(p -> dbClient.propertiesDao().saveProperty(p.setValue(newLogin))); userDto.setLogin(newLogin); - if (userDto.isLocal()) { + if (userDto.isLocal() || SQ_AUTHORITY.equals(userDto.getExternalIdentityProvider())) { userDto.setExternalLogin(newLogin); userDto.setExternalId(newLogin); } diff --git a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/BaseContextFactoryTest.java b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/BaseContextFactoryTest.java index de51e18f546..dd016d5cfc1 100644 --- a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/BaseContextFactoryTest.java +++ b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/BaseContextFactoryTest.java @@ -47,7 +47,6 @@ public class BaseContextFactoryTest { private static final UserIdentity USER_IDENTITY = UserIdentity.builder() .setProviderId("ABCD") .setProviderLogin("johndoo") - .setLogin("id:johndoo") .setName("John") .setEmail("john@email.com") .build(); @@ -92,7 +91,6 @@ public class BaseContextFactoryTest { assertThat(userIdentityAuthenticator.isAuthenticated()).isTrue(); verify(threadLocalUserSession).set(any(UserSession.class)); verify(jwtHttpHandler).generateToken(userArgumentCaptor.capture(), eq(request), eq(response)); - assertThat(userArgumentCaptor.getValue().getLogin()).isEqualTo(USER_IDENTITY.getLogin()); assertThat(userArgumentCaptor.getValue().getExternalId()).isEqualTo(USER_IDENTITY.getProviderId()); assertThat(userArgumentCaptor.getValue().getExternalLogin()).isEqualTo(USER_IDENTITY.getProviderLogin()); assertThat(userArgumentCaptor.getValue().getExternalIdentityProvider()).isEqualTo("github"); diff --git a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/CredentialsExternalAuthenticationTest.java b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/CredentialsExternalAuthenticationTest.java index ee90eb0d094..94eb47c57de 100644 --- a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/CredentialsExternalAuthenticationTest.java +++ b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/CredentialsExternalAuthenticationTest.java @@ -92,7 +92,6 @@ public class CredentialsExternalAuthenticationTest { assertThat(userIdentityAuthenticator.isAuthenticated()).isTrue(); assertThat(userIdentityAuthenticator.getAuthenticatorParameters().getExistingEmailStrategy()).isEqualTo(FORBID); - assertThat(userIdentityAuthenticator.getAuthenticatorParameters().getUserIdentity().getLogin()).isEqualTo(LOGIN); assertThat(userIdentityAuthenticator.getAuthenticatorParameters().getUserIdentity().getProviderLogin()).isEqualTo(LOGIN); assertThat(userIdentityAuthenticator.getAuthenticatorParameters().getUserIdentity().getProviderId()).isNull(); assertThat(userIdentityAuthenticator.getAuthenticatorParameters().getUserIdentity().getName()).isEqualTo("name"); @@ -169,7 +168,6 @@ public class CredentialsExternalAuthenticationTest { executeAuthenticate("LOGIN"); assertThat(userIdentityAuthenticator.isAuthenticated()).isTrue(); - assertThat(userIdentityAuthenticator.getAuthenticatorParameters().getUserIdentity().getLogin()).isEqualTo("login"); assertThat(userIdentityAuthenticator.getAuthenticatorParameters().getUserIdentity().getProviderLogin()).isEqualTo("login"); verify(authenticationEvent).loginSuccess(request, "login", Source.realm(BASIC, REALM_NAME)); } @@ -182,7 +180,6 @@ public class CredentialsExternalAuthenticationTest { executeAuthenticate("LoGiN"); assertThat(userIdentityAuthenticator.isAuthenticated()).isTrue(); - assertThat(userIdentityAuthenticator.getAuthenticatorParameters().getUserIdentity().getLogin()).isEqualTo("LoGiN"); assertThat(userIdentityAuthenticator.getAuthenticatorParameters().getUserIdentity().getProviderLogin()).isEqualTo("LoGiN"); verify(authenticationEvent).loginSuccess(request, "LoGiN", Source.realm(BASIC, REALM_NAME)); } 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 0a1672b9dcc..ef3d77e2f35 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 @@ -152,7 +152,7 @@ public class HttpHeadersAuthenticationTest { public void update_user_when_authenticating_exiting_user() { startWithSso(); setNotUserInToken(); - insertUser(newUserDto().setLogin(DEFAULT_LOGIN).setName("old name").setEmail("old email"), group1); + insertUser(newUserDto().setLogin(DEFAULT_LOGIN).setExternalLogin(DEFAULT_LOGIN).setExternalIdentityProvider("sonarqube").setName("old name").setEmail("old email"), group1); // Name, email and groups are different HttpServletRequest request = createRequest(DEFAULT_LOGIN, DEFAULT_NAME, DEFAULT_EMAIL, GROUP2); diff --git a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/InitFilterTest.java b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/InitFilterTest.java index c23bb1ed59a..8118282a4aa 100644 --- a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/InitFilterTest.java +++ b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/InitFilterTest.java @@ -328,7 +328,6 @@ public class InitFilterTest { public void init(Context context) { throw new EmailAlreadyExistsRedirectionException(existingUser.getEmail(), existingUser, UserIdentity.builder() .setProviderLogin("john.github") - .setLogin("john.github") .setName(existingUser.getName()) .setEmail(existingUser.getEmail()) .build(), this); diff --git a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/OAuth2CallbackFilterTest.java b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/OAuth2CallbackFilterTest.java index a42f29cf059..47c745b5d60 100644 --- a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/OAuth2CallbackFilterTest.java +++ b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/OAuth2CallbackFilterTest.java @@ -289,7 +289,6 @@ public class OAuth2CallbackFilterTest { public void callback(CallbackContext context) { throw new EmailAlreadyExistsRedirectionException(existingUser.getEmail(), existingUser, UserIdentity.builder() .setProviderLogin("john.github") - .setLogin("john.github") .setName(existingUser.getName()) .setEmail(existingUser.getEmail()) .build(), this); @@ -324,7 +323,6 @@ public class OAuth2CallbackFilterTest { public void callback(CallbackContext context) { super.callback(context); context.authenticate(UserIdentity.builder() - .setLogin(login) .setProviderLogin(login) .setEmail(login + "@toto.com") .setName("name of " + login) diff --git a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/OAuth2ContextFactoryTest.java b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/OAuth2ContextFactoryTest.java index cf2eb5f19fe..8d5d6229eb4 100644 --- a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/OAuth2ContextFactoryTest.java +++ b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/OAuth2ContextFactoryTest.java @@ -55,7 +55,6 @@ public class OAuth2ContextFactoryTest { private static final UserIdentity USER_IDENTITY = UserIdentity.builder() .setProviderId("ABCD") .setProviderLogin("johndoo") - .setLogin("id:johndoo") .setName("John") .setEmail("john@email.com") .build(); @@ -135,7 +134,6 @@ public class OAuth2ContextFactoryTest { verify(threadLocalUserSession).set(any(UserSession.class)); ArgumentCaptor userArgumentCaptor = ArgumentCaptor.forClass(UserDto.class); verify(jwtHttpHandler).generateToken(userArgumentCaptor.capture(), eq(request), eq(response)); - assertThat(userArgumentCaptor.getValue().getLogin()).isEqualTo(USER_IDENTITY.getLogin()); assertThat(userArgumentCaptor.getValue().getExternalId()).isEqualTo(USER_IDENTITY.getProviderId()); assertThat(userArgumentCaptor.getValue().getExternalLogin()).isEqualTo(USER_IDENTITY.getProviderLogin()); assertThat(userArgumentCaptor.getValue().getExternalIdentityProvider()).isEqualTo(PROVIDER_KEY); diff --git a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/TestUserRegistrar.java b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/TestUserRegistrar.java index 4244c842138..0c45e64d5d0 100644 --- a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/TestUserRegistrar.java +++ b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/TestUserRegistrar.java @@ -32,7 +32,6 @@ public class TestUserRegistrar implements UserRegistrar { String providerId = registration.getUserIdentity().getProviderId(); return UserTesting.newUserDto() .setLocal(false) - .setLogin(registration.getUserIdentity().getLogin()) .setExternalLogin(registration.getUserIdentity().getProviderLogin()) .setExternalId(providerId == null ? registration.getUserIdentity().getProviderLogin() : providerId) .setExternalIdentityProvider(registration.getProvider().getKey()); diff --git a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/UserRegistrarImplOrgMembershipSyncTest.java b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/UserRegistrarImplOrgMembershipSyncTest.java index c43fdb7f616..b9404baaab4 100644 --- a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/UserRegistrarImplOrgMembershipSyncTest.java +++ b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/UserRegistrarImplOrgMembershipSyncTest.java @@ -63,7 +63,6 @@ public class UserRegistrarImplOrgMembershipSyncTest { private static UserIdentity USER_IDENTITY = UserIdentity.builder() .setProviderId("ABCD") .setProviderLogin("johndoo") - .setLogin(USER_LOGIN) .setName("John") .setEmail("john@email.com") .build(); @@ -127,7 +126,7 @@ public class UserRegistrarImplOrgMembershipSyncTest { .setOrganizationAlmIds(ImmutableSet.of(gitHubInstall.getOrganizationAlmId())) .build()); - UserDto user = db.users().selectUserByLogin(USER_LOGIN).get(); + UserDto user = db.users().selectUserByExternalLoginAndIdentityProvider(USER_IDENTITY.getProviderLogin(), GITHUB_PROVIDER.getKey()).get(); db.organizations().assertUserIsMemberOfOrganization(organization, user); } @@ -147,7 +146,7 @@ public class UserRegistrarImplOrgMembershipSyncTest { .setOrganizationAlmIds(null) .build()); - UserDto user = db.users().selectUserByLogin(USER_LOGIN).get(); + UserDto user = db.users().selectUserByExternalLoginAndIdentityProvider(USER_IDENTITY.getProviderLogin(), GITHUB_PROVIDER.getKey()).get(); db.organizations().assertUserIsNotMemberOfOrganization(organization, user); } @@ -167,7 +166,7 @@ public class UserRegistrarImplOrgMembershipSyncTest { .setOrganizationAlmIds(ImmutableSet.of(gitHubInstall.getOrganizationAlmId())) .build()); - UserDto user = db.users().selectUserByLogin(USER_LOGIN).get(); + UserDto user = db.users().selectUserByExternalLoginAndIdentityProvider(USER_IDENTITY.getProviderLogin(), BITBUCKET_PROVIDER.getKey()).get(); db.organizations().assertUserIsNotMemberOfOrganization(organization, user); } @@ -192,7 +191,7 @@ public class UserRegistrarImplOrgMembershipSyncTest { .setOrganizationAlmIds(ImmutableSet.of(almAppInstall.getOrganizationAlmId())) .build()); - UserDto user = db.users().selectUserByLogin(USER_LOGIN).get(); + UserDto user = db.users().selectUserByExternalLoginAndIdentityProvider(USER_IDENTITY.getProviderLogin(), identityProvider.getKey()).get(); db.organizations().assertUserIsNotMemberOfOrganization(organization, user); } @@ -226,7 +225,11 @@ public class UserRegistrarImplOrgMembershipSyncTest { db.users().insertDefaultGroup(organization, "Members"); AlmAppInstallDto gitHubInstall = db.alm().insertAlmAppInstall(a -> a.setAlm(GITHUB)); db.alm().insertOrganizationAlmBinding(organization, gitHubInstall, true); - UserDto user = db.users().insertDisabledUser(u -> u.setLogin(USER_LOGIN)); + UserDto user = db.users().insertDisabledUser(u -> u + .setLogin(USER_LOGIN) + .setExternalId(USER_IDENTITY.getProviderId()) + .setExternalIdentityProvider(GITHUB_PROVIDER.getKey()) + ); underTest.register(UserRegistration.builder() .setUserIdentity(USER_IDENTITY) 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 4673a5fce22..8feeaf668b1 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 @@ -62,12 +62,11 @@ public class UserRegistrarImplTest { private System2 system2 = new AlwaysIncreasingSystem2(); - private static String USER_LOGIN = "github-johndoo"; + private static String USER_LOGIN = "johndoo"; private static UserIdentity USER_IDENTITY = UserIdentity.builder() .setProviderId("ABCD") .setProviderLogin("johndoo") - .setLogin(USER_LOGIN) .setName("John") .setEmail("john@email.com") .build(); @@ -111,14 +110,14 @@ public class UserRegistrarImplTest { public void authenticate_new_user() { organizationFlags.setEnabled(true); - underTest.register(UserRegistration.builder() + UserDto createdUser = underTest.register(UserRegistration.builder() .setUserIdentity(USER_IDENTITY) .setProvider(IDENTITY_PROVIDER) .setSource(Source.realm(BASIC, IDENTITY_PROVIDER.getName())) .setExistingEmailStrategy(ExistingEmailStrategy.FORBID) .build()); - UserDto user = db.users().selectUserByLogin(USER_LOGIN).get(); + UserDto user = db.users().selectUserByLogin(createdUser.getLogin()).get(); assertThat(user).isNotNull(); assertThat(user.isActive()).isTrue(); assertThat(user.getName()).isEqualTo("John"); @@ -130,6 +129,38 @@ public class UserRegistrarImplTest { checkGroupMembership(user); } + @Test + public void authenticate_new_user_with_sq_identity() { + organizationFlags.setEnabled(false); + GroupDto defaultGroup = insertDefaultGroup(); + + TestIdentityProvider sqIdentityProvider = new TestIdentityProvider() + .setKey("sonarqube") + .setName("sonarqube identity name") + .setEnabled(true) + .setAllowsUsersToSignUp(true); + + UserDto createdUser = underTest.register(UserRegistration.builder() + .setUserIdentity(USER_IDENTITY) + .setProvider(sqIdentityProvider) + .setSource(Source.realm(BASIC, sqIdentityProvider.getName())) + .setExistingEmailStrategy(ExistingEmailStrategy.FORBID) + .build()); + + UserDto user = db.users().selectUserByLogin(createdUser.getLogin()).get(); + assertThat(user).isNotNull(); + assertThat(user.isActive()).isTrue(); + assertThat(user.getLogin()).isEqualTo("johndoo"); + assertThat(user.getName()).isEqualTo("John"); + assertThat(user.getEmail()).isEqualTo("john@email.com"); + assertThat(user.getExternalLogin()).isEqualTo("johndoo"); + assertThat(user.getExternalIdentityProvider()).isEqualTo("sonarqube"); + assertThat(user.getExternalId()).isEqualTo("ABCD"); + assertThat(user.isLocal()).isFalse(); + assertThat(user.isRoot()).isFalse(); + checkGroupMembership(user, defaultGroup); + } + @Test public void authenticate_new_user_generate_login_when_no_login_provided() { organizationFlags.setEnabled(true); @@ -162,9 +193,9 @@ public class UserRegistrarImplTest { GroupDto group1 = db.users().insertGroup(db.getDefaultOrganization(), "group1"); GroupDto group2 = db.users().insertGroup(db.getDefaultOrganization(), "group2"); - authenticate(USER_LOGIN, "group1", "group2", "group3"); + UserDto loggedInUser = authenticate(USER_LOGIN, "group1", "group2", "group3"); - Optional user = db.users().selectUserByLogin(USER_LOGIN); + Optional user = db.users().selectUserByLogin(loggedInUser.getLogin()); checkGroupMembership(user.get(), group1, group2); } @@ -185,13 +216,16 @@ public class UserRegistrarImplTest { @Test public void does_not_force_default_group_when_authenticating_new_user_if_organizations_are_enabled() { organizationFlags.setEnabled(true); - UserDto user = db.users().insertUser(); + UserDto user = db.users().insertUser(newUserDto() + .setExternalIdentityProvider(IDENTITY_PROVIDER.getKey()) + .setExternalLogin(USER_IDENTITY.getProviderLogin()) + ); GroupDto group1 = db.users().insertGroup(db.getDefaultOrganization(), "group1"); GroupDto defaultGroup = insertDefaultGroup(); db.users().insertMember(group1, user); db.users().insertMember(defaultGroup, user); - authenticate(user.getLogin(), "group1"); + authenticate(user.getExternalLogin(), "group1"); checkGroupMembership(user, group1); } @@ -201,14 +235,14 @@ public class UserRegistrarImplTest { organizationFlags.setEnabled(true); settings.setProperty(ONBOARDING_TUTORIAL_SHOW_TO_NEW_USERS.getKey(), true); - underTest.register(UserRegistration.builder() + UserDto user = underTest.register(UserRegistration.builder() .setUserIdentity(USER_IDENTITY) .setProvider(IDENTITY_PROVIDER) .setSource(Source.local(BASIC)) .setExistingEmailStrategy(ExistingEmailStrategy.FORBID) .build()); - assertThat(db.users().selectUserByLogin(USER_LOGIN).get().isOnboarded()).isFalse(); + assertThat(db.users().selectUserByLogin(user.getLogin()).get().isOnboarded()).isFalse(); } @Test @@ -216,14 +250,14 @@ public class UserRegistrarImplTest { organizationFlags.setEnabled(true); settings.setProperty(ONBOARDING_TUTORIAL_SHOW_TO_NEW_USERS.getKey(), false); - underTest.register(UserRegistration.builder() + UserDto user = underTest.register(UserRegistration.builder() .setUserIdentity(USER_IDENTITY) .setProvider(IDENTITY_PROVIDER) .setSource(Source.local(BASIC)) .setExistingEmailStrategy(ExistingEmailStrategy.FORBID) .build()); - assertThat(db.users().selectUserByLogin(USER_LOGIN).get().isOnboarded()).isTrue(); + assertThat(db.users().selectUserByLogin(user.getLogin()).get().isOnboarded()).isTrue(); } @Test @@ -231,21 +265,20 @@ public class UserRegistrarImplTest { organizationFlags.setEnabled(true); UserIdentity newUser = UserIdentity.builder() .setProviderId(null) - .setLogin("john") .setProviderLogin("johndoo") .setName("JOhn") .build(); - underTest.register(UserRegistration.builder() + UserDto user = underTest.register(UserRegistration.builder() .setUserIdentity(newUser) .setProvider(IDENTITY_PROVIDER) .setSource(Source.local(BASIC)) .setExistingEmailStrategy(ExistingEmailStrategy.FORBID) .build()); - assertThat(db.users().selectUserByLogin(newUser.getLogin()).get()) + assertThat(db.users().selectUserByLogin(user.getLogin()).get()) .extracting(UserDto::getLogin, UserDto::getExternalId, UserDto::getExternalLogin) - .contains("john", "johndoo", "johndoo"); + .contains(user.getLogin(), "johndoo", "johndoo"); } @Test @@ -254,19 +287,18 @@ public class UserRegistrarImplTest { UserDto existingUser = db.users().insertUser(u -> u.setEmail("john@email.com")); UserIdentity newUser = UserIdentity.builder() .setProviderLogin("johndoo") - .setLogin("new_login") .setName(existingUser.getName()) .setEmail(existingUser.getEmail()) .build(); - underTest.register(UserRegistration.builder() + UserDto user = underTest.register(UserRegistration.builder() .setUserIdentity(newUser) .setProvider(IDENTITY_PROVIDER) .setSource(Source.local(BASIC)) .setExistingEmailStrategy(ExistingEmailStrategy.ALLOW) .build()); - UserDto newUserReloaded = db.users().selectUserByLogin(newUser.getLogin()).get(); + UserDto newUserReloaded = db.users().selectUserByLogin(user.getLogin()).get(); assertThat(newUserReloaded.getEmail()).isEqualTo(existingUser.getEmail()); UserDto existingUserReloaded = db.users().selectUserByLogin(existingUser.getLogin()).get(); assertThat(existingUserReloaded.getEmail()).isNull(); @@ -278,7 +310,6 @@ public class UserRegistrarImplTest { UserDto existingUser = db.users().insertUser(u -> u.setEmail("john@email.com")); UserIdentity newUser = UserIdentity.builder() .setProviderLogin("johndoo") - .setLogin("new_login") .setName(existingUser.getName()) .setEmail(existingUser.getEmail()) .build(); @@ -356,22 +387,22 @@ public class UserRegistrarImplTest { @Test public void authenticate_and_update_existing_user_matching_login() { + insertDefaultGroup(); db.users().insertUser(u -> u - .setLogin(USER_LOGIN) .setName("Old name") .setEmail("Old email") .setExternalId("old id") .setExternalLogin("old identity") .setExternalIdentityProvider("old provide")); - underTest.register(UserRegistration.builder() + UserDto user = underTest.register(UserRegistration.builder() .setUserIdentity(USER_IDENTITY) .setProvider(IDENTITY_PROVIDER) .setSource(Source.local(BASIC)) .setExistingEmailStrategy(ExistingEmailStrategy.FORBID) .build()); - assertThat(db.users().selectUserByLogin(USER_LOGIN).get()) + 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); } @@ -393,7 +424,6 @@ public class UserRegistrarImplTest { .setExistingEmailStrategy(ExistingEmailStrategy.FORBID) .build()); - assertThat(db.users().selectUserByLogin("Old login")).isNotPresent(); assertThat(db.getDbClient().userDao().selectByUuid(db.getSession(), user.getUuid())) .extracting(UserDto::getLogin, UserDto::getName, UserDto::getEmail, UserDto::getExternalId, UserDto::getExternalLogin, UserDto::getExternalIdentityProvider, UserDto::isActive) @@ -417,15 +447,14 @@ public class UserRegistrarImplTest { .setExistingEmailStrategy(ExistingEmailStrategy.FORBID) .build()); - assertThat(db.users().selectUserByLogin("Old login")).isNotPresent(); assertThat(db.getDbClient().userDao().selectByUuid(db.getSession(), user.getUuid())) - .extracting(UserDto::getLogin, UserDto::getName, UserDto::getEmail, UserDto::getExternalId, UserDto::getExternalLogin, UserDto::getExternalIdentityProvider, + .extracting(UserDto::getName, UserDto::getEmail, UserDto::getExternalId, UserDto::getExternalLogin, UserDto::getExternalIdentityProvider, UserDto::isActive) - .contains(USER_LOGIN, "John", "john@email.com", "ABCD", "johndoo", "github", true); + .contains("John", "john@email.com", "ABCD", "johndoo", "github", true); } @Test - public void authenticate_existing_user_and_update_only_login() { + public void authenticate_existing_user_and_login_should_not_be_updated() { UserDto user = db.users().insertUser(u -> u .setLogin("old login") .setName(USER_IDENTITY.getName()) @@ -441,55 +470,29 @@ public class UserRegistrarImplTest { .setExistingEmailStrategy(ExistingEmailStrategy.FORBID) .build()); - assertThat(db.users().selectUserByLogin("Old login")).isNotPresent(); - 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(USER_LOGIN, USER_IDENTITY.getName(), USER_IDENTITY.getEmail(), USER_IDENTITY.getProviderId(), USER_IDENTITY.getProviderLogin(), - IDENTITY_PROVIDER.getKey(), - true); - } - - @Test - public void authenticate_existing_user_and_update_only_identity_provider_key() { - UserDto user = db.users().insertUser(u -> u - .setLogin(USER_LOGIN) - .setName(USER_IDENTITY.getName()) - .setEmail(USER_IDENTITY.getEmail()) - .setExternalId(USER_IDENTITY.getProviderId()) - .setExternalLogin(USER_IDENTITY.getProviderLogin()) - .setExternalIdentityProvider("old identity provider")); - - underTest.register(UserRegistration.builder() - .setUserIdentity(USER_IDENTITY) - .setProvider(IDENTITY_PROVIDER) - .setSource(Source.local(BASIC)) - .setExistingEmailStrategy(ExistingEmailStrategy.FORBID) - .build()); - + //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(USER_LOGIN, USER_IDENTITY.getName(), USER_IDENTITY.getEmail(), USER_IDENTITY.getProviderId(), USER_IDENTITY.getProviderLogin(), + .containsExactlyInAnyOrder("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_login_when_external_id_is_null() { + public void authenticate_existing_user_matching_external_login_when_external_id_is_null() { UserDto user = db.users().insertUser(u -> u - .setLogin(USER_LOGIN) .setName("Old name") .setEmail("Old email") .setExternalId("Old id") - .setExternalLogin("old identity") + .setExternalLogin("johndoo") .setExternalIdentityProvider(IDENTITY_PROVIDER.getKey())); underTest.register(UserRegistration.builder() .setUserIdentity(UserIdentity.builder() .setProviderId(null) .setProviderLogin("johndoo") - .setLogin(USER_LOGIN) .setName("John") .setEmail("john@email.com") .build()) @@ -526,7 +529,7 @@ public class UserRegistrarImplTest { 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(), user.getName(), user.getEmail(), user.getExternalId(), user.getExternalLogin(), user.getExternalIdentityProvider(), true); + .contains(user.getExternalLogin(), user.getName(), user.getEmail(), user.getExternalId(), user.getExternalLogin(), user.getExternalIdentityProvider(), true); } @Test @@ -557,9 +560,9 @@ public class UserRegistrarImplTest { .setActive(false) .setName("Old name") .setEmail("Old email") - .setExternalId("old id") - .setExternalLogin("old identity") - .setExternalIdentityProvider("old provide")); + .setExternalId("Old id") + .setExternalLogin(USER_IDENTITY.getProviderLogin()) + .setExternalIdentityProvider(IDENTITY_PROVIDER.getKey())); underTest.register(UserRegistration.builder() .setUserIdentity(USER_IDENTITY) @@ -582,25 +585,27 @@ public class UserRegistrarImplTest { public void authenticate_existing_user_when_email_already_exists_and_strategy_is_ALLOW() { organizationFlags.setEnabled(true); UserDto existingUser = db.users().insertUser(u -> u.setEmail("john@email.com")); - UserDto currentUser = db.users().insertUser(u -> u.setEmail(null)); + UserDto currentUser = db.users().insertUser(u -> u.setExternalLogin("johndoo").setExternalIdentityProvider(IDENTITY_PROVIDER.getKey()).setEmail(null)); + UserIdentity userIdentity = UserIdentity.builder() - .setLogin(currentUser.getLogin()) - .setProviderLogin("johndoo") + .setProviderLogin(currentUser.getExternalLogin()) .setName("John") .setEmail("john@email.com") .build(); - underTest.register(UserRegistration.builder() + currentUser = underTest.register(UserRegistration.builder() .setUserIdentity(userIdentity) .setProvider(IDENTITY_PROVIDER) .setSource(Source.local(BASIC)) .setExistingEmailStrategy(ExistingEmailStrategy.ALLOW) .build()); - UserDto currentUserReloaded = db.users().selectUserByLogin(currentUser.getLogin()).get(); - assertThat(currentUserReloaded.getEmail()).isEqualTo("john@email.com"); UserDto existingUserReloaded = db.users().selectUserByLogin(existingUser.getLogin()).get(); assertThat(existingUserReloaded.getEmail()).isNull(); + + UserDto currentUserReloaded = db.users().selectUserByLogin(currentUser.getLogin()).get(); + assertThat(currentUserReloaded.getEmail()).isEqualTo("john@email.com"); + } @Test @@ -609,7 +614,6 @@ public class UserRegistrarImplTest { UserDto existingUser = db.users().insertUser(u -> u.setEmail("john@email.com")); UserDto currentUser = db.users().insertUser(u -> u.setEmail(null)); UserIdentity userIdentity = UserIdentity.builder() - .setLogin(currentUser.getLogin()) .setProviderLogin("johndoo") .setName("John") .setEmail("john@email.com") @@ -631,7 +635,6 @@ public class UserRegistrarImplTest { UserDto existingUser = db.users().insertUser(u -> u.setEmail("john@email.com")); UserDto currentUser = db.users().insertUser(u -> u.setEmail(null)); UserIdentity userIdentity = UserIdentity.builder() - .setLogin(currentUser.getLogin()) .setProviderLogin("johndoo") .setName("John") .setEmail("john@email.com") @@ -657,7 +660,6 @@ public class UserRegistrarImplTest { UserDto currentUser = db.users().insertUser(u -> u.setEmail("john@email.com") .setExternalIdentityProvider(IDENTITY_PROVIDER.getKey())); UserIdentity userIdentity = UserIdentity.builder() - .setLogin(currentUser.getLogin()) .setProviderId(currentUser.getExternalId()) .setProviderLogin(currentUser.getExternalLogin()) .setName("John") @@ -679,13 +681,14 @@ public class UserRegistrarImplTest { public void authenticate_existing_user_and_add_new_groups() { organizationFlags.setEnabled(true); UserDto user = db.users().insertUser(newUserDto() - .setLogin(USER_LOGIN) + .setExternalIdentityProvider(IDENTITY_PROVIDER.getKey()) + .setExternalLogin(USER_IDENTITY.getProviderLogin()) .setActive(true) .setName("John")); GroupDto group1 = db.users().insertGroup(db.getDefaultOrganization(), "group1"); GroupDto group2 = db.users().insertGroup(db.getDefaultOrganization(), "group2"); - authenticate(USER_LOGIN, "group1", "group2", "group3"); + authenticate(USER_IDENTITY.getProviderLogin(), "group1", "group2", "group3"); checkGroupMembership(user, group1, group2); } @@ -694,7 +697,8 @@ public class UserRegistrarImplTest { public void authenticate_existing_user_and_remove_groups() { organizationFlags.setEnabled(true); UserDto user = db.users().insertUser(newUserDto() - .setLogin(USER_LOGIN) + .setExternalIdentityProvider(IDENTITY_PROVIDER.getKey()) + .setExternalLogin(USER_IDENTITY.getProviderLogin()) .setActive(true) .setName("John")); GroupDto group1 = db.users().insertGroup(db.getDefaultOrganization(), "group1"); @@ -702,7 +706,7 @@ public class UserRegistrarImplTest { db.users().insertMember(group1, user); db.users().insertMember(group2, user); - authenticate(USER_LOGIN, "group1"); + authenticate(USER_IDENTITY.getProviderLogin(), "group1"); checkGroupMembership(user, group1); } @@ -710,7 +714,10 @@ public class UserRegistrarImplTest { @Test public void authenticate_existing_user_and_remove_all_groups_expect_default_when_organizations_are_disabled() { organizationFlags.setEnabled(false); - UserDto user = db.users().insertUser(); + UserDto user = db.users().insertUser(newUserDto() + .setExternalIdentityProvider(IDENTITY_PROVIDER.getKey()) + .setExternalLogin(USER_IDENTITY.getProviderLogin()) + ); GroupDto group1 = db.users().insertGroup(db.getDefaultOrganization(), "group1"); GroupDto group2 = db.users().insertGroup(db.getDefaultOrganization(), "group2"); GroupDto defaultGroup = insertDefaultGroup(); @@ -718,7 +725,7 @@ public class UserRegistrarImplTest { db.users().insertMember(group2, user); db.users().insertMember(defaultGroup, user); - authenticate(user.getLogin()); + authenticate(user.getExternalLogin()); checkGroupMembership(user, defaultGroup); } @@ -726,13 +733,16 @@ public class UserRegistrarImplTest { @Test public void does_not_force_default_group_when_authenticating_existing_user_when_organizations_are_enabled() { organizationFlags.setEnabled(true); - UserDto user = db.users().insertUser(); + UserDto user = db.users().insertUser(newUserDto() + .setExternalIdentityProvider(IDENTITY_PROVIDER.getKey()) + .setExternalLogin(USER_IDENTITY.getProviderLogin()) + ); GroupDto group1 = db.users().insertGroup(db.getDefaultOrganization(), "group1"); GroupDto defaultGroup = insertDefaultGroup(); db.users().insertMember(group1, user); db.users().insertMember(defaultGroup, user); - authenticate(user.getLogin(), "group1"); + authenticate(user.getExternalLogin(), "group1"); checkGroupMembership(user, group1); } @@ -742,7 +752,8 @@ public class UserRegistrarImplTest { organizationFlags.setEnabled(true); OrganizationDto org = db.organizations().insert(); UserDto user = db.users().insertUser(newUserDto() - .setLogin(USER_LOGIN) + .setExternalIdentityProvider(IDENTITY_PROVIDER.getKey()) + .setExternalLogin(USER_IDENTITY.getProviderLogin()) .setActive(true) .setName("John")); String groupName = "a-group"; @@ -753,7 +764,6 @@ public class UserRegistrarImplTest { underTest.register(UserRegistration.builder() .setUserIdentity(UserIdentity.builder() .setProviderLogin("johndoo") - .setLogin(user.getLogin()) .setName(user.getName()) .setGroups(newHashSet(groupName)) .build()) @@ -765,11 +775,10 @@ public class UserRegistrarImplTest { checkGroupMembership(user, groupInDefaultOrg); } - private void authenticate(String login, String... groups) { - underTest.register(UserRegistration.builder() + private UserDto authenticate(String providerLogin, String... groups) { + return underTest.register(UserRegistration.builder() .setUserIdentity(UserIdentity.builder() - .setProviderLogin("johndoo") - .setLogin(login) + .setProviderLogin(providerLogin) .setName("John") // No group .setGroups(stream(groups).collect(MoreCollectors.toSet())) @@ -781,7 +790,7 @@ public class UserRegistrarImplTest { } private void checkGroupMembership(UserDto user, GroupDto... expectedGroups) { - assertThat(db.users().selectGroupIdsOfUser(user)).containsOnly(stream(expectedGroups).map(GroupDto::getId).collect(Collectors.toList()).toArray(new Integer[] {})); + assertThat(db.users().selectGroupIdsOfUser(user)).containsOnly(stream(expectedGroups).map(GroupDto::getId).collect(Collectors.toList()).toArray(new Integer[]{})); } private GroupDto insertDefaultGroup() { diff --git a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/user/ws/UpdateLoginActionTest.java b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/user/ws/UpdateLoginActionTest.java index c1c322d9e16..301ee897045 100644 --- a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/user/ws/UpdateLoginActionTest.java +++ b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/user/ws/UpdateLoginActionTest.java @@ -68,7 +68,7 @@ public class UpdateLoginActionTest { organizationUpdater)); @Test - public void update_login_from_sonarqube_account() { + public void update_login_from_sonarqube_account_when_user_is_local() { userSession.logIn().setSystemAdministrator(); UserDto user = db.users().insertUser(u -> u .setLogin("old_login") @@ -92,6 +92,31 @@ public class UpdateLoginActionTest { assertThat(userReloaded.getSalt()).isNotNull().isEqualTo(user.getSalt()); } + @Test + public void update_login_from_sonarqube_account_when_user_is_not_local() { + userSession.logIn().setSystemAdministrator(); + UserDto user = db.users().insertUser(u -> u + .setLogin("old_login") + .setLocal(false) + .setExternalIdentityProvider("sonarqube") + .setExternalLogin("old_login") + .setExternalId("old_login")); + + ws.newRequest() + .setParam("login", user.getLogin()) + .setParam("newLogin", "new_login") + .execute(); + + assertThat(db.getDbClient().userDao().selectByLogin(db.getSession(), "old_login")).isNull(); + UserDto userReloaded = db.getDbClient().userDao().selectByUuid(db.getSession(), user.getUuid()); + assertThat(userReloaded.getLogin()).isEqualTo("new_login"); + assertThat(userReloaded.getExternalLogin()).isEqualTo("new_login"); + assertThat(userReloaded.getExternalId()).isEqualTo("new_login"); + assertThat(userReloaded.isLocal()).isFalse(); + assertThat(userReloaded.getCryptedPassword()).isNotNull().isEqualTo(user.getCryptedPassword()); + assertThat(userReloaded.getSalt()).isNotNull().isEqualTo(user.getSalt()); + } + @Test public void update_login_from_external_account() { userSession.logIn().setSystemAdministrator(); diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/server/authentication/UserIdentity.java b/sonar-plugin-api/src/main/java/org/sonar/api/server/authentication/UserIdentity.java index 5d398b9d5ad..8699cb93270 100644 --- a/sonar-plugin-api/src/main/java/org/sonar/api/server/authentication/UserIdentity.java +++ b/sonar-plugin-api/src/main/java/org/sonar/api/server/authentication/UserIdentity.java @@ -27,7 +27,6 @@ import javax.annotation.concurrent.Immutable; import org.sonar.api.user.UserGroupValidation; import static java.util.Objects.requireNonNull; -import static org.apache.commons.lang.StringUtils.isBlank; import static org.apache.commons.lang.StringUtils.isNotBlank; import static org.sonar.api.utils.Preconditions.checkArgument; @@ -42,8 +41,6 @@ public final class UserIdentity { @Nullable private final String id; private final String providerLogin; - @Nullable - private final String login; private final String name; @Nullable private final String email; @@ -53,7 +50,6 @@ public final class UserIdentity { private UserIdentity(Builder builder) { this.id = builder.id; this.providerLogin = builder.providerLogin; - this.login = builder.login; this.name = builder.name; this.email = builder.email; this.groupsProvided = builder.groupsProvided; @@ -86,10 +82,15 @@ public final class UserIdentity { * * Since 7.4, a unique login will be generated if result is null and the user referenced by {@link #getProviderId()} * or {@link #getProviderLogin()} does not already exist. + * + * @deprecated since 8.0, should not be used as it is internal managed field + * + * @return null */ @CheckForNull + @Deprecated public String getLogin() { - return login; + return null; } /** @@ -134,7 +135,6 @@ public final class UserIdentity { public static class Builder { private String id; private String providerLogin; - private String login; private String name; private String email; private boolean groupsProvided = false; @@ -162,9 +162,11 @@ public final class UserIdentity { /** * @see UserIdentity#getLogin() + * + * @deprecated since 8.0, has no effect as it is internal managed field */ + @Deprecated public Builder setLogin(@Nullable String login) { - this.login = login; return this; } @@ -210,7 +212,6 @@ public final class UserIdentity { public UserIdentity build() { validateId(id); validateProviderLogin(providerLogin); - validateLogin(login); validateName(name); validateEmail(email); return new UserIdentity(this); @@ -225,10 +226,6 @@ public final class UserIdentity { checkArgument(providerLogin.length() <= 255, "Provider login size is incorrect (maximum 255 characters)"); } - private static void validateLogin(@Nullable String login) { - checkArgument(isBlank(login) || (login.length() <= 255 && login.length() >= 2), "User login size is incorrect (Between 2 and 255 characters)"); - } - private static void validateName(String name) { checkArgument(isNotBlank(name), "User name must not be blank"); checkArgument(name.length() <= 200, "User name size is too big (200 characters max)"); diff --git a/sonar-plugin-api/src/test/java/org/sonar/api/server/authentication/UserIdentityTest.java b/sonar-plugin-api/src/test/java/org/sonar/api/server/authentication/UserIdentityTest.java index d954757300f..7b583ecaf4e 100644 --- a/sonar-plugin-api/src/test/java/org/sonar/api/server/authentication/UserIdentityTest.java +++ b/sonar-plugin-api/src/test/java/org/sonar/api/server/authentication/UserIdentityTest.java @@ -37,14 +37,12 @@ public class UserIdentityTest { UserIdentity underTest = UserIdentity.builder() .setProviderId("4321") .setProviderLogin("john") - .setLogin("1234") .setName("John") .setEmail("john@email.com") .build(); assertThat(underTest.getProviderId()).isEqualTo("4321"); assertThat(underTest.getProviderLogin()).isEqualTo("john"); - assertThat(underTest.getLogin()).isEqualTo("1234"); assertThat(underTest.getName()).isEqualTo("John"); assertThat(underTest.getEmail()).isEqualTo("john@email.com"); assertThat(underTest.shouldSyncGroups()).isFalse(); @@ -61,7 +59,6 @@ public class UserIdentityTest { assertThat(underTest.getProviderLogin()).isEqualTo("john"); assertThat(underTest.getName()).isEqualTo("John"); assertThat(underTest.getProviderId()).isNull(); - assertThat(underTest.getLogin()).isNull(); assertThat(underTest.getEmail()).isNull(); assertThat(underTest.shouldSyncGroups()).isFalse(); assertThat(underTest.getGroups()).isEmpty(); @@ -74,41 +71,15 @@ public class UserIdentityTest { UserIdentity.builder() .setProviderId(Strings.repeat("1", 256)) .setProviderLogin("john") - .setLogin("1234") .setName("John") .build(); } - @Test - public void fail_when_login_is_too_long() { - thrown.expect(IllegalArgumentException.class); - thrown.expectMessage("User login size is incorrect (Between 2 and 255 characters)"); - UserIdentity.builder() - .setProviderLogin("john") - .setLogin(Strings.repeat("1", 256)) - .setName("John") - .setEmail("john@email.com") - .build(); - } - - @Test - public void fail_when_login_is_too_small() { - thrown.expect(IllegalArgumentException.class); - thrown.expectMessage("User login size is incorrect (Between 2 and 255 characters)"); - UserIdentity.builder() - .setProviderLogin("john") - .setLogin("j") - .setName("John") - .setEmail("john@email.com") - .build(); - } - @Test public void fail_when_provider_login_is_null() { thrown.expect(IllegalArgumentException.class); thrown.expectMessage("Provider login must not be blank"); UserIdentity.builder() - .setLogin("1234") .setName("John") .setEmail("john@email.com") .build(); @@ -120,7 +91,6 @@ public class UserIdentityTest { thrown.expectMessage("Provider login must not be blank"); UserIdentity.builder() .setProviderLogin("") - .setLogin("1234") .setName("John") .setEmail("john@email.com") .build(); @@ -132,7 +102,6 @@ public class UserIdentityTest { thrown.expectMessage("Provider login size is incorrect (maximum 255 characters)"); UserIdentity.builder() .setProviderLogin(Strings.repeat("1", 256)) - .setLogin("1234") .setName("John") .setEmail("john@email.com") .build(); @@ -144,7 +113,6 @@ public class UserIdentityTest { thrown.expectMessage("User name must not be blank"); UserIdentity.builder() .setProviderLogin("john") - .setLogin("1234") .setEmail("john@email.com") .build(); } @@ -155,7 +123,6 @@ public class UserIdentityTest { thrown.expectMessage("User name must not be blank"); UserIdentity.builder() .setProviderLogin("john") - .setLogin("1234") .setName("") .setEmail("john@email.com") .build(); @@ -167,7 +134,6 @@ public class UserIdentityTest { thrown.expectMessage("User name size is too big (200 characters max)"); UserIdentity.builder() .setProviderLogin("john") - .setLogin("1234") .setName(Strings.repeat("1", 201)) .setEmail("john@email.com") .build(); @@ -179,7 +145,6 @@ public class UserIdentityTest { thrown.expectMessage("User email size is too big (100 characters max)"); UserIdentity.builder() .setProviderLogin("john") - .setLogin("1234") .setName("John") .setEmail(Strings.repeat("1", 101)) .build(); @@ -189,7 +154,6 @@ public class UserIdentityTest { public void create_user_with_groups() { UserIdentity underTest = UserIdentity.builder() .setProviderLogin("john") - .setLogin("1234") .setName("John") .setEmail("john@email.com") .setGroups(newHashSet("admin", "user")) @@ -206,7 +170,6 @@ public class UserIdentityTest { UserIdentity.builder() .setProviderLogin("john") - .setLogin("1234") .setName("John") .setEmail("john@email.com") .setGroups(null); @@ -219,7 +182,6 @@ public class UserIdentityTest { UserIdentity.builder() .setProviderLogin("john") - .setLogin("1234") .setName("John") .setEmail("john@email.com") .setGroups(newHashSet("")); @@ -232,7 +194,6 @@ public class UserIdentityTest { UserIdentity.builder() .setProviderLogin("john") - .setLogin("1234") .setName("John") .setEmail("john@email.com") .setGroups(newHashSet(" ")); @@ -245,7 +206,6 @@ public class UserIdentityTest { UserIdentity.builder() .setProviderLogin("john") - .setLogin("1234") .setName("John") .setEmail("john@email.com") .setGroups(newHashSet((String)null)); @@ -258,7 +218,6 @@ public class UserIdentityTest { UserIdentity.builder() .setProviderLogin("john") - .setLogin("1234") .setName("John") .setEmail("john@email.com") .setGroups(newHashSet("Anyone")); @@ -271,7 +230,6 @@ public class UserIdentityTest { UserIdentity.builder() .setProviderLogin("john") - .setLogin("1234") .setName("John") .setEmail("john@email.com") .setGroups(newHashSet(Strings.repeat("group", 300))); -- 2.39.5