SONAR-9426 Set external identity info when reactivating local user

6 files changed, 62 insertions, 20 deletions

diff --git a/server/sonar-db-dao/src/test/java/org/sonar/db/user/UserTesting.java b/server/sonar-db-dao/src/test/java/org/sonar/db/user/UserTesting.java
index b9bba58527b..6c8f0d71592 100644
--- a/server/sonar-db-dao/src/test/java/org/sonar/db/user/UserTesting.java
+++ b/server/sonar-db-dao/src/test/java/org/sonar/db/user/UserTesting.java
@@ -77,6 +77,13 @@ public class UserTesting {
   public static UserDto newDisabledUser(String login) {
     return newUserDto()
       .setLogin(login)
-      .setActive(false);
+      .setActive(false)
+      // All these fields are reset when disabling a user
+      .setScmAccounts((String) null)
+      .setExternalIdentity(null)
+      .setExternalIdentityProvider(null)
+      .setEmail(null)
+      .setCryptedPassword(null)
+      .setSalt(null);
   }
 }
diff --git a/server/sonar-server/src/main/java/org/sonar/server/authentication/UserIdentityAuthenticator.java b/server/sonar-server/src/main/java/org/sonar/server/authentication/UserIdentityAuthenticator.java
index 396cea50b03..58f35204e38 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/authentication/UserIdentityAuthenticator.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/authentication/UserIdentityAuthenticator.java
@@ -121,8 +121,7 @@ public class UserIdentityAuthenticator {
     UpdateUser update = UpdateUser.create(userDto.getLogin())
       .setEmail(identity.getEmail())
       .setName(identity.getName())
-      .setExternalIdentity(new ExternalIdentity(provider.getKey(), identity.getProviderLogin()))
-      .setPassword(null);
+      .setExternalIdentity(new ExternalIdentity(provider.getKey(), identity.getProviderLogin()));
     userUpdater.updateAndCommit(dbSession, update, u -> syncGroups(dbSession, identity, u));
   }
 
diff --git a/server/sonar-server/src/main/java/org/sonar/server/user/UserUpdater.java b/server/sonar-server/src/main/java/org/sonar/server/user/UserUpdater.java
index b29394d3845..cc14249f719 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/user/UserUpdater.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/user/UserUpdater.java
@@ -112,15 +112,11 @@ public class UserUpdater {
     UpdateUser updateUser = UpdateUser.create(login)
       .setName(newUser.name())
       .setEmail(newUser.email())
-      .setScmAccounts(newUser.scmAccounts());
+      .setScmAccounts(newUser.scmAccounts())
+      .setExternalIdentity(newUser.externalIdentity());
     if (newUser.password() != null) {
       updateUser.setPassword(newUser.password());
     }
-    if (newUser.externalIdentity() != null) {
-      updateUser.setExternalIdentity(newUser.externalIdentity());
-    }
-    // Hack to allow to change the password of the user
-    existingUser.setLocal(true);
     setOnboarded(existingUser);
     updateDto(dbSession, updateUser, existingUser);
     updateUser(dbSession, existingUser);
@@ -213,8 +209,6 @@ public class UserUpdater {
     ExternalIdentity externalIdentity = updateUser.externalIdentity();
     if (updateUser.isExternalIdentityChanged() && !isSameExternalIdentity(userDto, externalIdentity)) {
       setExternalIdentity(userDto, externalIdentity);
-      userDto.setSalt(null);
-      userDto.setCryptedPassword(null);
       return true;
     }
     return false;
@@ -222,7 +216,7 @@ public class UserUpdater {
 
   private static boolean updatePassword(UpdateUser updateUser, UserDto userDto, List<String> messages) {
     String password = updateUser.password();
-    if (!updateUser.isExternalIdentityChanged() && updateUser.isPasswordChanged() && validatePasswords(password, messages) && checkPasswordChangeAllowed(userDto, messages)) {
+    if (updateUser.isPasswordChanged() && validatePasswords(password, messages) && checkPasswordChangeAllowed(userDto, messages)) {
       setEncryptedPassword(password, userDto);
       return true;
     }
@@ -248,10 +242,9 @@ public class UserUpdater {
   }
 
   private static boolean isSameExternalIdentity(UserDto dto, @Nullable ExternalIdentity externalIdentity) {
-    return (externalIdentity == null && dto.getExternalIdentity() == null) ||
-      (externalIdentity != null
-        && Objects.equals(dto.getExternalIdentity(), externalIdentity.getId())
-        && Objects.equals(dto.getExternalIdentityProvider(), externalIdentity.getProvider()));
+    return externalIdentity != null
+      && Objects.equals(dto.getExternalIdentity(), externalIdentity.getId())
+      && Objects.equals(dto.getExternalIdentityProvider(), externalIdentity.getProvider());
   }
 
   private static void setExternalIdentity(UserDto dto, @Nullable ExternalIdentity externalIdentity) {
@@ -263,6 +256,8 @@ public class UserUpdater {
       dto.setExternalIdentity(externalIdentity.getId());
       dto.setExternalIdentityProvider(externalIdentity.getProvider());
       dto.setLocal(false);
+      dto.setSalt(null);
+      dto.setCryptedPassword(null);
     }
   }
 
diff --git a/server/sonar-server/src/test/java/org/sonar/server/user/UserUpdaterCreateTest.java b/server/sonar-server/src/test/java/org/sonar/server/user/UserUpdaterCreateTest.java
index bd5118bc61b..8bedc870c2e 100644
--- a/server/sonar-server/src/test/java/org/sonar/server/user/UserUpdaterCreateTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/user/UserUpdaterCreateTest.java
@@ -637,7 +637,7 @@ public class UserUpdaterCreateTest {
   }
 
   @Test
-  public void update_external_provider_when_reactivating_user() {
+  public void reactivate_user_with_external_provider() {
     db.users().insertUser(newDisabledUser(DEFAULT_LOGIN)
       .setLocal(true));
     createDefaultGroup();
@@ -651,9 +651,29 @@ public class UserUpdaterCreateTest {
     session.commit();
 
     UserDto dto = dbClient.userDao().selectByLogin(session, DEFAULT_LOGIN);
+    assertThat(dto.isLocal()).isFalse();
     assertThat(dto.getExternalIdentity()).isEqualTo("john");
     assertThat(dto.getExternalIdentityProvider()).isEqualTo("github");
-    assertThat(dto.isLocal()).isFalse();
+  }
+
+  @Test
+  public void reactivate_user_with_local_provider() {
+    db.users().insertUser(newDisabledUser(DEFAULT_LOGIN)
+      .setLocal(true));
+    createDefaultGroup();
+
+    underTest.createAndCommit(db.getSession(), NewUser.builder()
+      .setLogin(DEFAULT_LOGIN)
+      .setName("Marius2")
+      .setPassword("password")
+      .build(), u -> {
+    });
+    session.commit();
+
+    UserDto dto = dbClient.userDao().selectByLogin(session, DEFAULT_LOGIN);
+    assertThat(dto.isLocal()).isTrue();
+    assertThat(dto.getExternalIdentity()).isEqualTo(DEFAULT_LOGIN);
+    assertThat(dto.getExternalIdentityProvider()).isEqualTo("sonarqube");
   }
 
   @Test
diff --git a/server/sonar-server/src/test/java/org/sonar/server/user/UserUpdaterUpdateTest.java b/server/sonar-server/src/test/java/org/sonar/server/user/UserUpdaterUpdateTest.java
index a6dab47c7c0..592fb3ad914 100644
--- a/server/sonar-server/src/test/java/org/sonar/server/user/UserUpdaterUpdateTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/user/UserUpdaterUpdateTest.java
@@ -124,7 +124,6 @@ public class UserUpdaterUpdateTest {
     underTest.updateAndCommit(session, UpdateUser.create(DEFAULT_LOGIN)
       .setName("Marius2")
       .setEmail("marius2@email.com")
-      .setPassword(null)
       .setExternalIdentity(new ExternalIdentity("github", "john")), u -> {
       });
 
@@ -142,7 +141,6 @@ public class UserUpdaterUpdateTest {
     underTest.updateAndCommit(session, UpdateUser.create(DEFAULT_LOGIN)
       .setName("Marius2")
       .setEmail("marius2@email.com")
-      .setPassword(null)
       .setExternalIdentity(new ExternalIdentity("github", "john")), u -> {
       });
 
diff --git a/tests/src/test/java/org/sonarqube/tests/user/LocalAuthenticationTest.java b/tests/src/test/java/org/sonarqube/tests/user/LocalAuthenticationTest.java
index 662c0c22506..d5b8a50240c 100644
--- a/tests/src/test/java/org/sonarqube/tests/user/LocalAuthenticationTest.java
+++ b/tests/src/test/java/org/sonarqube/tests/user/LocalAuthenticationTest.java
@@ -31,6 +31,7 @@ import org.sonarqube.pageobjects.Navigation;
 import org.sonarqube.tests.Category4Suite;
 import org.sonarqube.tests.Tester;
 import org.sonarqube.ws.WsUserTokens;
+import org.sonarqube.ws.WsUsers;
 import org.sonarqube.ws.WsUsers.CreateWsResponse.User;
 import org.sonarqube.ws.client.GetRequest;
 import org.sonarqube.ws.client.HttpConnector;
@@ -38,12 +39,14 @@ import org.sonarqube.ws.client.PostRequest;
 import org.sonarqube.ws.client.WsClient;
 import org.sonarqube.ws.client.WsClientFactories;
 import org.sonarqube.ws.client.WsResponse;
+import org.sonarqube.ws.client.user.CreateRequest;
 import org.sonarqube.ws.client.usertoken.GenerateWsRequest;
 import org.sonarqube.ws.client.usertoken.RevokeWsRequest;
 import org.sonarqube.ws.client.usertoken.SearchWsRequest;
 import org.sonarqube.ws.client.usertoken.UserTokensService;
 
 import static java.lang.String.format;
+import static java.util.Arrays.asList;
 import static org.assertj.core.api.Assertions.assertThat;
 import static util.ItUtils.resetSettings;
 import static util.ItUtils.setServerProperty;
@@ -228,6 +231,26 @@ public class LocalAuthenticationTest {
     assertThat(checkAuthenticationWithAnyWS(null, null).code()).isEqualTo(401);
   }
 
+  @Test
+  public void authenticate_on_user_that_was_disabled() {
+    User user = tester.users().generate(u -> u.setLogin("test").setPassword("password"));
+    tester.users().service().deactivate(user.getLogin());
+
+    tester.users().service().create(CreateRequest.builder()
+      .setLogin("test")
+      .setName("Test")
+      .setEmail("test@email.com")
+      .setScmAccounts(asList("test1", "test2"))
+      .setPassword("password")
+      .build());
+
+    assertThat(checkAuthenticationWithAuthenticateWebService("test", "password")).isTrue();
+    assertThat(tester.users().getByLogin("test").get())
+      .extracting(WsUsers.SearchWsResponse.User::getLogin, WsUsers.SearchWsResponse.User::getName, WsUsers.SearchWsResponse.User::getEmail, u -> u.getScmAccounts().getScmAccountsList(),
+        WsUsers.SearchWsResponse.User::getExternalIdentity, WsUsers.SearchWsResponse.User::getExternalProvider)
+      .containsOnly("test", "Test", "test@email.com", asList("test1", "test2"), "test", "sonarqube");
+  }
+
   private boolean checkAuthenticationWithAuthenticateWebService(String login, String password) {
     String result = tester.as(login, password).wsClient().wsConnector().call(new PostRequest("/api/authentication/validate")).content();
     return result.contains("{\"valid\":true}");