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

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.")

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<GsonTeam> 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;
+  }
+
 }

server/sonar-auth-github/src/main/java/org/sonar/auth/github/UserIdentityGenerator.java

@@ -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);
-  }
-}

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);

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);
   }
 
 }

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);
   }
 }

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());

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<GsonTeam> 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);
-  }
 }

server/sonar-auth-gitlab/src/test/java/org/sonar/auth/gitlab/IntegrationTest.java

@@ -94,7 +94,6 @@ public class IntegrationTest {
     ArgumentCaptor<UserIdentity> 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");

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(

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();
   }
 

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<String> 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<UserDto> selectUserByEmail(String email) {
+    List<UserDto> users = dbClient.userDao().selectByEmail(db.getSession(), email);
+    if (users.size() > 1) {
+      return Optional.empty();
+    }
+    return Optional.of(users.get(0));
+  }
+
+  public Optional<UserDto> selectUserByExternalLoginAndIdentityProvider(String login, String identityProvider) {
+    return Optional.ofNullable(dbClient.userDao().selectByExternalLoginAndIdentityProvider(db.getSession(), login, identityProvider));
+  }
+
   // USER SETTINGS
 
   @SafeVarargs

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)
     ;
   }
 }

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();
+  }
+}

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);
   }
 
 }

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());
+  }
+}

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");

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);

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);

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<UserDto> 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();
   }

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);
     }

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");

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));
   }

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);
 

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);

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)

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<UserDto> 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);

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());

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)

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<UserDto> user = db.users().selectUserByLogin(USER_LOGIN);
+    Optional<UserDto> 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() {

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();

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)");

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)));