From 759926d3cf6da3713b326c9653701a6f40496401 Mon Sep 17 00:00:00 2001
From: Julien Lancelot <julien.lancelot@sonarsource.com>
Date: Wed, 9 Jan 2019 18:15:18 +0100
Subject: [PATCH] SONARCLOUD-332 Use EXTERNAL_LOGIN to authenticate users

---
 .../org/sonar/db/version/schema-h2.ddl        |  1 +
 .../main/java/org/sonar/db/user/UserDao.java  |  5 ++
 .../java/org/sonar/db/user/UserMapper.java    |  4 +-
 .../org/sonar/db/user/UserMapper.xml          | 11 +++-
 .../java/org/sonar/db/user/UserDaoTest.java   | 10 ++++
 ...niqueIndexOnExternalLoginInUsersTable.java | 57 +++++++++++++++++++
 .../db/migration/version/v76/DbVersion76.java |  1 +
 ...eIndexOnExternalLoginInUsersTableTest.java | 54 ++++++++++++++++++
 .../version/v76/DbVersion76Test.java          |  2 +-
 .../users.sql                                 | 27 +++++++++
 .../authentication/UserRegistrarImpl.java     | 24 +++++---
 .../authentication/UserRegistrarImplTest.java | 25 ++++++++
 12 files changed, 209 insertions(+), 12 deletions(-)
 create mode 100644 server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/version/v76/AddUniqueIndexOnExternalLoginInUsersTable.java
 create mode 100644 server/sonar-db-migration/src/test/java/org/sonar/server/platform/db/migration/version/v76/AddUniqueIndexOnExternalLoginInUsersTableTest.java
 create mode 100644 server/sonar-db-migration/src/test/resources/org/sonar/server/platform/db/migration/version/v76/AddUniqueIndexOnExternalLoginInUsersTableTest/users.sql

diff --git a/server/sonar-db-core/src/main/resources/org/sonar/db/version/schema-h2.ddl b/server/sonar-db-core/src/main/resources/org/sonar/db/version/schema-h2.ddl
index 6790a438c54..f5907822075 100644
--- a/server/sonar-db-core/src/main/resources/org/sonar/db/version/schema-h2.ddl
+++ b/server/sonar-db-core/src/main/resources/org/sonar/db/version/schema-h2.ddl
@@ -529,6 +529,7 @@ CREATE TABLE "USERS" (
 CREATE UNIQUE INDEX "USERS_UUID" ON "USERS" ("UUID");
 CREATE UNIQUE INDEX "USERS_LOGIN" ON "USERS" ("LOGIN");
 CREATE UNIQUE INDEX "UNIQ_EXTERNAL_ID" ON "USERS" ("EXTERNAL_IDENTITY_PROVIDER", "EXTERNAL_ID");
+CREATE UNIQUE INDEX "UNIQ_EXTERNAL_LOGIN" ON "USERS" ("EXTERNAL_IDENTITY_PROVIDER", "EXTERNAL_LOGIN");
 CREATE INDEX "USERS_UPDATED_AT" ON "USERS" ("UPDATED_AT");
 
 
diff --git a/server/sonar-db-dao/src/main/java/org/sonar/db/user/UserDao.java b/server/sonar-db-dao/src/main/java/org/sonar/db/user/UserDao.java
index ea1d8fe7305..62a050f2610 100644
--- a/server/sonar-db-dao/src/main/java/org/sonar/db/user/UserDao.java
+++ b/server/sonar-db-dao/src/main/java/org/sonar/db/user/UserDao.java
@@ -173,6 +173,11 @@ public class UserDao implements Dao {
     return mapper(dbSession).selectByExternalIdAndIdentityProvider(externalId, externalIdentityProvider);
   }
 
+  @CheckForNull
+  public UserDto selectByExternalLoginAndIdentityProvider(DbSession dbSession, String externalLogin, String externalIdentityProvider) {
+    return mapper(dbSession).selectByExternalLoginAndIdentityProvider(externalLogin, externalIdentityProvider);
+  }
+
   public List<UserDto> selectByExternalIdentityProvider(DbSession dbSession, String externalIdentityProvider) {
     return mapper(dbSession).selectByExternalIdentityProvider(externalIdentityProvider);
   }
diff --git a/server/sonar-db-dao/src/main/java/org/sonar/db/user/UserMapper.java b/server/sonar-db-dao/src/main/java/org/sonar/db/user/UserMapper.java
index 23e68c690d8..4801a1bb2c3 100644
--- a/server/sonar-db-dao/src/main/java/org/sonar/db/user/UserMapper.java
+++ b/server/sonar-db-dao/src/main/java/org/sonar/db/user/UserMapper.java
@@ -62,6 +62,8 @@ public interface UserMapper {
   @CheckForNull
   UserDto selectByExternalIdAndIdentityProvider(@Param("externalId") String externalId, @Param("externalIdentityProvider") String externalExternalIdentityProvider);
 
+  UserDto selectByExternalLoginAndIdentityProvider(@Param("externalLogin") String externalLogin, @Param("externalIdentityProvider") String externalExternalIdentityProvider);
+
   List<UserDto> selectByExternalIdentityProvider(@Param("externalIdentityProvider") String externalExternalIdentityProvider);
 
   void scrollAll(ResultHandler<UserDto> handler);
@@ -71,8 +73,6 @@ public interface UserMapper {
    */
   long countRootUsersButLogin(@Param("login") String login);
 
-
-
   void insert(@Param("user") UserDto userDto);
 
   void update(@Param("user") UserDto userDto);
diff --git a/server/sonar-db-dao/src/main/resources/org/sonar/db/user/UserMapper.xml b/server/sonar-db-dao/src/main/resources/org/sonar/db/user/UserMapper.xml
index c4c4ff04812..a9f6ca1dc78 100644
--- a/server/sonar-db-dao/src/main/resources/org/sonar/db/user/UserMapper.xml
+++ b/server/sonar-db-dao/src/main/resources/org/sonar/db/user/UserMapper.xml
@@ -140,14 +140,21 @@
         SELECT
         <include refid="userColumns"/>
         FROM users u
-        WHERE u.external_id=#{externalId} AND u.external_identity_provider=#{externalIdentityProvider}
+        WHERE u.external_id=#{externalId} AND u.external_identity_provider=#{externalIdentityProvider, jdbcType=VARCHAR}
+    </select>
+
+    <select id="selectByExternalLoginAndIdentityProvider" parameterType="map" resultType="User">
+        SELECT
+        <include refid="userColumns"/>
+        FROM users u
+        WHERE u.external_login=#{externalLogin} AND u.external_identity_provider=#{externalIdentityProvider, jdbcType=VARCHAR}
     </select>
 
     <select id="selectByExternalIdentityProvider" parameterType="map" resultType="User">
         SELECT
         <include refid="userColumns"/>
         FROM users u
-        WHERE u.external_identity_provider=#{externalIdentityProvider}
+        WHERE u.external_identity_provider=#{externalIdentityProvider, jdbcType=VARCHAR}
     </select>
 
     <select id="countRootUsersButLogin" parameterType="String" resultType="long">
diff --git a/server/sonar-db-dao/src/test/java/org/sonar/db/user/UserDaoTest.java b/server/sonar-db-dao/src/test/java/org/sonar/db/user/UserDaoTest.java
index 962f8f399e3..ab53f511841 100644
--- a/server/sonar-db-dao/src/test/java/org/sonar/db/user/UserDaoTest.java
+++ b/server/sonar-db-dao/src/test/java/org/sonar/db/user/UserDaoTest.java
@@ -596,6 +596,16 @@ public class UserDaoTest {
     assertThat(underTest.selectByExternalIdAndIdentityProvider(session, "unknown", "unknown")).isNull();
   }
 
+  @Test
+  public void select_by_external_login_and_identity_provider() {
+    UserDto activeUser = db.users().insertUser();
+    UserDto disableUser = db.users().insertUser(u -> u.setActive(false));
+
+    assertThat(underTest.selectByExternalLoginAndIdentityProvider(session, activeUser.getExternalLogin(), activeUser.getExternalIdentityProvider())).isNotNull();
+    assertThat(underTest.selectByExternalLoginAndIdentityProvider(session, disableUser.getExternalLogin(), disableUser.getExternalIdentityProvider())).isNotNull();
+    assertThat(underTest.selectByExternalLoginAndIdentityProvider(session, "unknown", "unknown")).isNull();
+  }
+
   @Test
   public void setRoot_does_not_fail_on_non_existing_login() {
     underTest.setRoot(session, "unkown", true);
diff --git a/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/version/v76/AddUniqueIndexOnExternalLoginInUsersTable.java b/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/version/v76/AddUniqueIndexOnExternalLoginInUsersTable.java
new file mode 100644
index 00000000000..ee456d71392
--- /dev/null
+++ b/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/version/v76/AddUniqueIndexOnExternalLoginInUsersTable.java
@@ -0,0 +1,57 @@
+/*
+ * 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.v76;
+
+import java.sql.SQLException;
+import org.sonar.db.Database;
+import org.sonar.server.platform.db.migration.SupportsBlueGreen;
+import org.sonar.server.platform.db.migration.def.VarcharColumnDef;
+import org.sonar.server.platform.db.migration.sql.CreateIndexBuilder;
+import org.sonar.server.platform.db.migration.step.DdlChange;
+
+import static org.sonar.server.platform.db.migration.def.VarcharColumnDef.newVarcharColumnDefBuilder;
+
+@SupportsBlueGreen
+public class AddUniqueIndexOnExternalLoginInUsersTable extends DdlChange {
+
+  public AddUniqueIndexOnExternalLoginInUsersTable(Database db) {
+    super(db);
+  }
+
+  @Override
+  public void execute(Context context) throws SQLException {
+    context.execute(new CreateIndexBuilder(getDialect())
+      .setTable("users")
+      .setName("uniq_external_login")
+      .setUnique(true)
+      .addColumn(notNullableColumn("external_identity_provider", 100))
+      .addColumn(notNullableColumn("external_login", 255))
+      .build());
+  }
+
+  private static VarcharColumnDef notNullableColumn(String columnName, int limit) {
+    return newVarcharColumnDefBuilder()
+      .setColumnName(columnName)
+      .setLimit(limit)
+      .setIsNullable(false)
+      .build();
+  }
+
+}
diff --git a/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/version/v76/DbVersion76.java b/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/version/v76/DbVersion76.java
index 0e4d4b57991..ee64126ef95 100644
--- a/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/version/v76/DbVersion76.java
+++ b/server/sonar-db-migration/src/main/java/org/sonar/server/platform/db/migration/version/v76/DbVersion76.java
@@ -35,6 +35,7 @@ public class DbVersion76 implements DbVersion {
       .add(2505, "Fix the direction values of certain metrics (prepare for migration of conditions)", FixDirectionOfMetrics.class)
       .add(2506, "Migrate quality gate conditions using warning, period and no more supported operations", MigrateNoMoreUsedQualityGateConditions.class)
       .add(2507, "Delete sonar.onboardingTutorial.showToNewUsers from settings", DeleteUselessOnboardingSetting.class)
+      .add(2508, "Add unique index on EXTERNAL_LOGIN and EXTERNAL_IDENTITY_PROVIDER columns in table USERS", AddUniqueIndexOnExternalLoginInUsersTable.class)
     ;
   }
 }
diff --git a/server/sonar-db-migration/src/test/java/org/sonar/server/platform/db/migration/version/v76/AddUniqueIndexOnExternalLoginInUsersTableTest.java b/server/sonar-db-migration/src/test/java/org/sonar/server/platform/db/migration/version/v76/AddUniqueIndexOnExternalLoginInUsersTableTest.java
new file mode 100644
index 00000000000..c1051f1073f
--- /dev/null
+++ b/server/sonar-db-migration/src/test/java/org/sonar/server/platform/db/migration/version/v76/AddUniqueIndexOnExternalLoginInUsersTableTest.java
@@ -0,0 +1,54 @@
+/*
+ * 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.v76;
+
+import java.sql.SQLException;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import org.sonar.db.CoreDbTester;
+
+public class AddUniqueIndexOnExternalLoginInUsersTableTest {
+
+  @Rule
+  public final CoreDbTester db = CoreDbTester.createForSchema(AddUniqueIndexOnExternalLoginInUsersTableTest.class, "users.sql");
+
+  @Rule
+  public ExpectedException expectedException = ExpectedException.none();
+
+  private AddUniqueIndexOnExternalLoginInUsersTable underTest = new AddUniqueIndexOnExternalLoginInUsersTable(db.database());
+
+  @Test
+  public void add_unique_indexes() throws SQLException {
+    underTest.execute();
+
+    db.assertUniqueIndex("users", "uniq_external_login", "external_identity_provider", "external_login");
+  }
+
+  @Test
+  public void migration_is_not_reentrant() throws SQLException {
+    underTest.execute();
+
+    expectedException.expect(IllegalStateException.class);
+
+    underTest.execute();
+  }
+
+}
diff --git a/server/sonar-db-migration/src/test/java/org/sonar/server/platform/db/migration/version/v76/DbVersion76Test.java b/server/sonar-db-migration/src/test/java/org/sonar/server/platform/db/migration/version/v76/DbVersion76Test.java
index ab1c60e4152..220173c84dd 100644
--- a/server/sonar-db-migration/src/test/java/org/sonar/server/platform/db/migration/version/v76/DbVersion76Test.java
+++ b/server/sonar-db-migration/src/test/java/org/sonar/server/platform/db/migration/version/v76/DbVersion76Test.java
@@ -35,7 +35,7 @@ public class DbVersion76Test {
 
   @Test
   public void verify_migration_count() {
-    verifyMigrationCount(underTest, 8);
+    verifyMigrationCount(underTest, 9);
   }
 
 }
diff --git a/server/sonar-db-migration/src/test/resources/org/sonar/server/platform/db/migration/version/v76/AddUniqueIndexOnExternalLoginInUsersTableTest/users.sql b/server/sonar-db-migration/src/test/resources/org/sonar/server/platform/db/migration/version/v76/AddUniqueIndexOnExternalLoginInUsersTableTest/users.sql
new file mode 100644
index 00000000000..31365666f5a
--- /dev/null
+++ b/server/sonar-db-migration/src/test/resources/org/sonar/server/platform/db/migration/version/v76/AddUniqueIndexOnExternalLoginInUsersTableTest/users.sql
@@ -0,0 +1,27 @@
+CREATE TABLE "USERS" (
+  "ID" INTEGER NOT NULL GENERATED BY DEFAULT AS IDENTITY (START WITH 1, INCREMENT BY 1),
+  "UUID" VARCHAR(255) NOT NULL,
+  "LOGIN" VARCHAR(255) NOT NULL,
+  "NAME" VARCHAR(200),
+  "EMAIL" VARCHAR(100),
+  "CRYPTED_PASSWORD" VARCHAR(100),
+  "SALT" VARCHAR(40),
+  "HASH_METHOD" VARCHAR(10),
+  "ACTIVE" BOOLEAN DEFAULT TRUE,
+  "SCM_ACCOUNTS" VARCHAR(4000),
+  "EXTERNAL_ID" VARCHAR(255) NOT NULL,
+  "EXTERNAL_LOGIN" VARCHAR(255) NOT NULL,
+  "EXTERNAL_IDENTITY_PROVIDER" VARCHAR(100) NOT NULL,
+  "IS_ROOT" BOOLEAN NOT NULL,
+  "USER_LOCAL" BOOLEAN,
+  "ONBOARDED" BOOLEAN NOT NULL,
+  "HOMEPAGE_TYPE" VARCHAR(40),
+  "HOMEPAGE_PARAMETER" VARCHAR(40),
+  "ORGANIZATION_UUID" VARCHAR(40),
+  "CREATED_AT" BIGINT,
+  "UPDATED_AT" BIGINT
+);
+CREATE UNIQUE INDEX "USERS_UUID" ON "USERS" ("UUID");
+CREATE UNIQUE INDEX "USERS_LOGIN" ON "USERS" ("LOGIN");
+CREATE UNIQUE INDEX "UNIQ_EXTERNAL_ID" ON "USERS" ("EXTERNAL_IDENTITY_PROVIDER", "EXTERNAL_ID");
+CREATE INDEX "USERS_UPDATED_AT" ON "USERS" ("UPDATED_AT");
\ No newline at end of file
diff --git a/server/sonar-server/src/main/java/org/sonar/server/authentication/UserRegistrarImpl.java b/server/sonar-server/src/main/java/org/sonar/server/authentication/UserRegistrarImpl.java
index 1b7152790d4..c3708e6e0d6 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/authentication/UserRegistrarImpl.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/authentication/UserRegistrarImpl.java
@@ -98,13 +98,17 @@ public class UserRegistrarImpl implements UserRegistrar {
 
   @CheckForNull
   private UserDto getUser(DbSession dbSession, UserIdentity userIdentity, IdentityProvider provider) {
+    // First, try to authenticate using the external ID
     UserDto user = dbClient.userDao().selectByExternalIdAndIdentityProvider(dbSession, getProviderIdOrProviderLogin(userIdentity), provider.getKey());
     if (user != null) {
       return user;
     }
-    // We need to search by login because :
-    // 1. user may have been provisioned,
-    // 2. user may have been disabled.
+    // Then, try with the external login, for instance when for instance 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;
@@ -153,10 +157,7 @@ public class UserRegistrarImpl implements UserRegistrar {
     }
 
     UserDto existingUser = existingUsers.get(0);
-    if (existingUser == null
-      || Objects.equals(existingUser.getLogin(), authenticatorParameters.getUserIdentity().getLogin())
-      || (Objects.equals(existingUser.getExternalId(), getProviderIdOrProviderLogin(authenticatorParameters.getUserIdentity()))
-        && Objects.equals(existingUser.getExternalIdentityProvider(), authenticatorParameters.getProvider().getKey()))) {
+    if (existingUser == null || isSameUser(existingUser, authenticatorParameters)) {
       return Optional.empty();
     }
     ExistingEmailStrategy existingEmailStrategy = authenticatorParameters.getExistingEmailStrategy();
@@ -174,6 +175,15 @@ 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())));
+  }
+
   private void detectLoginUpdate(DbSession dbSession, UserDto user, UpdateUser update, UserRegistration authenticatorParameters) {
     String newLogin = update.login();
     if (!update.isLoginChanged() || user.getLogin().equals(newLogin)) {
diff --git a/server/sonar-server/src/test/java/org/sonar/server/authentication/UserRegistrarImplTest.java b/server/sonar-server/src/test/java/org/sonar/server/authentication/UserRegistrarImplTest.java
index 2d0996ab31d..ade40994d77 100644
--- a/server/sonar-server/src/test/java/org/sonar/server/authentication/UserRegistrarImplTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/authentication/UserRegistrarImplTest.java
@@ -425,6 +425,31 @@ public class UserRegistrarImplTest {
       .contains(USER_LOGIN, "John", "john@email.com", "ABCD", "johndoo", "github", true);
   }
 
+  @Test
+  public void authenticate_and_update_existing_user_matching_external_login() {
+    UserDto user = db.users().insertUser(u -> u
+      .setLogin("Old login")
+      .setName("Old name")
+      .setEmail(USER_IDENTITY.getEmail())
+      .setExternalId("Old id")
+      .setExternalLogin(USER_IDENTITY.getProviderLogin())
+      .setExternalIdentityProvider(IDENTITY_PROVIDER.getKey()));
+
+    underTest.register(UserRegistration.builder()
+      .setUserIdentity(USER_IDENTITY)
+      .setProvider(IDENTITY_PROVIDER)
+      .setSource(Source.local(BASIC))
+      .setExistingEmailStrategy(ExistingEmailStrategy.FORBID)
+      .setUpdateLoginStrategy(UpdateLoginStrategy.ALLOW)
+      .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)
+      .contains(USER_LOGIN, "John", "john@email.com", "ABCD", "johndoo", "github", true);
+  }
+
   @Test
   public void authenticate_existing_user_and_update_only_login() {
     UserDto user = db.users().insertUser(u -> u
-- 
2.39.5