]> source.dussan.org Git - sonarqube.git/commitdiff
SONAR-7249 Identity provider must provide user login
authorJulien Lancelot <julien.lancelot@sonarsource.com>
Thu, 28 Jan 2016 09:22:14 +0000 (10:22 +0100)
committerJulien Lancelot <julien.lancelot@sonarsource.com>
Mon, 1 Feb 2016 12:46:16 +0000 (13:46 +0100)
20 files changed:
it/it-plugins/base-auth-plugin/src/main/java/FakeBaseIdProvider.java
it/it-plugins/oauth2-auth-plugin/src/main/java/FakeOAuth2IdProvider.java
server/sonar-server/src/main/java/org/sonar/server/authentication/UserIdentityAuthenticator.java
server/sonar-server/src/main/java/org/sonar/server/user/ExternalIdentity.java [new file with mode: 0644]
server/sonar-server/src/main/java/org/sonar/server/user/NewUser.java
server/sonar-server/src/main/java/org/sonar/server/user/UpdateUser.java
server/sonar-server/src/main/java/org/sonar/server/user/UserUpdater.java
server/sonar-server/src/test/java/org/sonar/server/authentication/BaseContextFactoryTest.java
server/sonar-server/src/test/java/org/sonar/server/authentication/OAuth2ContextFactoryTest.java
server/sonar-server/src/test/java/org/sonar/server/authentication/UserIdentityAuthenticatorTest.java
server/sonar-server/src/test/java/org/sonar/server/user/ExternalIdentityTest.java [new file with mode: 0644]
server/sonar-server/src/test/java/org/sonar/server/user/NewUserTest.java
server/sonar-server/src/test/java/org/sonar/server/user/UserUpdaterTest.java
sonar-db/src/main/java/org/sonar/db/user/UserDao.java
sonar-db/src/main/java/org/sonar/db/user/UserMapper.java
sonar-db/src/main/resources/org/sonar/db/user/UserMapper.xml
sonar-db/src/test/java/org/sonar/db/user/UserDaoTest.java
sonar-db/src/test/resources/org/sonar/db/user/UserDaoTest/select_users_by_ext_identity.xml [deleted file]
sonar-plugin-api/src/main/java/org/sonar/api/server/authentication/UserIdentity.java
sonar-plugin-api/src/test/java/org/sonar/api/server/authentication/UserIdentityTest.java

index e3d1051f3e24630a3f8fb424ee7f65854b11e673..9095523d58fb5b836e99fa499e2a9d81a41aa27e 100644 (file)
@@ -25,6 +25,7 @@ import org.sonar.api.server.authentication.UserIdentity;
 public class FakeBaseIdProvider implements BaseIdentityProvider {
 
   private static final String ENABLED = "sonar.auth.fake-base-id-provider.enabled";
+  private static final String ALLOWS_USERS_TO_SIGN_UP = "sonar.auth.fake-base-id-provider.allowsUsersToSignUp";
   private static final String USER_INFO = "sonar.auth.fake-base-id-provider.user";
 
   private final Settings settings;
@@ -41,9 +42,10 @@ public class FakeBaseIdProvider implements BaseIdentityProvider {
     }
     String[] userInfos = userInfoProperty.split(",");
     context.authenticate(UserIdentity.builder()
-      .setId(userInfos[0])
-      .setName(userInfos[1])
-      .setEmail(userInfos[2])
+      .setLogin(userInfos[0])
+      .setProviderLogin(userInfos[1])
+      .setName(userInfos[2])
+      .setEmail(userInfos[3])
       .build());
 
     try {
@@ -75,6 +77,10 @@ public class FakeBaseIdProvider implements BaseIdentityProvider {
 
   @Override
   public boolean allowsUsersToSignUp() {
+    if (settings.hasKey(ALLOWS_USERS_TO_SIGN_UP)) {
+      return settings.getBoolean(ALLOWS_USERS_TO_SIGN_UP);
+    }
+    // If property is not defined, default behaviour is not always allow users to sign up
     return true;
   }
 }
index 1044796442ce99c55446a5574bf9eb2517ec609c..907d375bf8cdc620421d5a8ea95ed27ccc3f1660 100644 (file)
@@ -52,9 +52,10 @@ public class FakeOAuth2IdProvider implements OAuth2IdentityProvider {
     }
     String[] userInfos = userInfoProperty.split(",");
     context.authenticate(UserIdentity.builder()
-      .setId(userInfos[0])
-      .setName(userInfos[1])
-      .setEmail(userInfos[2])
+      .setLogin(userInfos[0])
+      .setProviderLogin(userInfos[1])
+      .setName(userInfos[2])
+      .setEmail(userInfos[3])
       .build());
     context.redirectToRequestedPage();
   }
@@ -71,7 +72,7 @@ public class FakeOAuth2IdProvider implements OAuth2IdentityProvider {
 
   @Override
   public String getIconPath() {
-    return "/static/baseauthplugin/base.png";
+    return "/static/oauth2authplugin/oauth2.png";
   }
 
   @Override
index 7ef534e27b0401fdac0ae2fbf6585fb1e0a92cd0..a876b088ad34e030523583958cfdbe2f248f0bc6 100644 (file)
  */
 package org.sonar.server.authentication;
 
-import com.google.common.base.Optional;
 import javax.servlet.http.HttpSession;
 import org.sonar.api.server.authentication.IdentityProvider;
 import org.sonar.api.server.authentication.UserIdentity;
-import org.sonar.core.util.UuidFactory;
 import org.sonar.db.DbClient;
 import org.sonar.db.DbSession;
 import org.sonar.db.user.UserDto;
+import org.sonar.server.user.ExternalIdentity;
 import org.sonar.server.user.NewUser;
 import org.sonar.server.user.UpdateUser;
 import org.sonar.server.user.UserUpdater;
@@ -35,12 +34,10 @@ public class UserIdentityAuthenticator {
 
   private final DbClient dbClient;
   private final UserUpdater userUpdater;
-  private final UuidFactory uuidFactory;
 
-  public UserIdentityAuthenticator(DbClient dbClient, UserUpdater userUpdater, UuidFactory uuidFactory) {
+  public UserIdentityAuthenticator(DbClient dbClient, UserUpdater userUpdater) {
     this.dbClient = dbClient;
     this.userUpdater = userUpdater;
-    this.uuidFactory = uuidFactory;
   }
 
   public void authenticate(UserIdentity user, IdentityProvider provider, HttpSession session) {
@@ -53,14 +50,14 @@ public class UserIdentityAuthenticator {
   private long register(UserIdentity user, IdentityProvider provider) {
     DbSession dbSession = dbClient.openSession(false);
     try {
-      String userId = user.getId();
-      Optional<UserDto> userDto = dbClient.userDao().selectByExternalIdentity(dbSession, userId, provider.getKey());
-      if (userDto.isPresent() && userDto.get().isActive()) {
-        userUpdater.update(dbSession, UpdateUser.create(userDto.get().getLogin())
+      String userLogin = user.getLogin();
+      UserDto userDto = dbClient.userDao().selectByLogin(dbSession, userLogin);
+      if (userDto != null && userDto.isActive()) {
+        userUpdater.update(dbSession, UpdateUser.create(userDto.getLogin())
           .setEmail(user.getEmail())
           .setName(user.getName())
-          );
-        return userDto.get().getId();
+          .setExternalIdentity(new ExternalIdentity(provider.getKey(), user.getProviderLogin())));
+        return userDto.getId();
       }
 
       if (!provider.allowsUsersToSignUp()) {
@@ -73,12 +70,12 @@ public class UserIdentityAuthenticator {
       }
 
       userUpdater.create(dbSession, NewUser.create()
-        .setLogin(uuidFactory.create())
+        .setLogin(userLogin)
         .setEmail(user.getEmail())
         .setName(user.getName())
-        .setExternalIdentity(new NewUser.ExternalIdentity(provider.getKey(), userId))
+        .setExternalIdentity(new ExternalIdentity(provider.getKey(), user.getProviderLogin()))
         );
-      return dbClient.userDao().selectOrFailByExternalIdentity(dbSession, userId, provider.getKey()).getId();
+      return dbClient.userDao().selectOrFailByLogin(dbSession, userLogin).getId();
 
     } finally {
       dbClient.closeSession(dbSession);
diff --git a/server/sonar-server/src/main/java/org/sonar/server/user/ExternalIdentity.java b/server/sonar-server/src/main/java/org/sonar/server/user/ExternalIdentity.java
new file mode 100644 (file)
index 0000000..bc597b9
--- /dev/null
@@ -0,0 +1,40 @@
+/*
+ * SonarQube
+ * Copyright (C) 2009-2016 SonarSource SA
+ * mailto:contact 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.user;
+
+import static java.util.Objects.requireNonNull;
+
+public class ExternalIdentity {
+  private String provider;
+  private String id;
+
+  public ExternalIdentity(String provider, String id) {
+    this.provider = requireNonNull(provider, "Identity provider cannot be null");
+    this.id = requireNonNull(id, "Identity id cannot be null");
+  }
+
+  public String getProvider() {
+    return provider;
+  }
+
+  public String getId() {
+    return id;
+  }
+}
index 01d49b93f9deff1eca399f3816f51b86b67882fc..5036657213df269839027a3f80ccbde236cbd4ec 100644 (file)
@@ -23,8 +23,6 @@ import java.util.List;
 import javax.annotation.CheckForNull;
 import javax.annotation.Nullable;
 
-import static java.util.Objects.requireNonNull;
-
 public class NewUser {
 
   private String login;
@@ -102,21 +100,4 @@ public class NewUser {
     return new NewUser();
   }
 
-  public static class ExternalIdentity {
-    private String provider;
-    private String id;
-
-    public ExternalIdentity(String provider, String id) {
-      this.provider = requireNonNull(provider, "Identity provider cannot be null");
-      this.id = requireNonNull(id, "Identity id cannot be null");
-    }
-
-    public String getProvider() {
-      return provider;
-    }
-
-    public String getId() {
-      return id;
-    }
-  }
 }
index a1925ce9fbaa8b77b0974467cda5d14cf7a75f19..d0c2e2c3841dfe9f761b1eed43986247670771fd 100644 (file)
  */
 package org.sonar.server.user;
 
-import com.google.common.base.Preconditions;
-
+import java.util.List;
 import javax.annotation.CheckForNull;
 import javax.annotation.Nullable;
 
-import java.util.List;
+import static com.google.common.base.Preconditions.checkNotNull;
 
 public class UpdateUser {
 
@@ -36,10 +35,13 @@ public class UpdateUser {
   private String password;
   private String passwordConfirmation;
 
-  boolean isNameChanged;
-  boolean isEmailChanged;
-  boolean isScmAccountsChanged;
-  boolean isPasswordChanged;
+  private ExternalIdentity externalIdentity;
+
+  boolean nameChanged;
+  boolean emailChanged;
+  boolean scmAccountsChanged;
+  boolean passwordChanged;
+  boolean externalIdentityChanged;
 
   private UpdateUser(String login) {
     // No direct call to this constructor
@@ -57,7 +59,7 @@ public class UpdateUser {
 
   public UpdateUser setName(@Nullable String name) {
     this.name = name;
-    isNameChanged = true;
+    nameChanged = true;
     return this;
   }
 
@@ -68,7 +70,7 @@ public class UpdateUser {
 
   public UpdateUser setEmail(@Nullable String email) {
     this.email = email;
-    isEmailChanged = true;
+    emailChanged = true;
     return this;
   }
 
@@ -79,7 +81,7 @@ public class UpdateUser {
 
   public UpdateUser setScmAccounts(@Nullable List<String> scmAccounts) {
     this.scmAccounts = scmAccounts;
-    isScmAccountsChanged = true;
+    scmAccountsChanged = true;
     return this;
   }
 
@@ -90,7 +92,7 @@ public class UpdateUser {
 
   public UpdateUser setPassword(@Nullable String password) {
     this.password = password;
-    isPasswordChanged = true;
+    passwordChanged = true;
     return this;
   }
 
@@ -101,28 +103,43 @@ public class UpdateUser {
 
   public UpdateUser setPasswordConfirmation(@Nullable String passwordConfirmation) {
     this.passwordConfirmation = passwordConfirmation;
-    isPasswordChanged = true;
+    passwordChanged = true;
+    return this;
+  }
+
+  @CheckForNull
+  public ExternalIdentity externalIdentity() {
+    return externalIdentity;
+  }
+
+  public UpdateUser setExternalIdentity(@Nullable ExternalIdentity externalIdentity) {
+    this.externalIdentity = externalIdentity;
+    externalIdentityChanged = true;
     return this;
   }
 
   public boolean isNameChanged() {
-    return isNameChanged;
+    return nameChanged;
   }
 
   public boolean isEmailChanged() {
-    return isEmailChanged;
+    return emailChanged;
   }
 
   public boolean isScmAccountsChanged() {
-    return isScmAccountsChanged;
+    return scmAccountsChanged;
   }
 
   public boolean isPasswordChanged() {
-    return isPasswordChanged;
+    return passwordChanged;
+  }
+
+  public boolean isExternalIdentityChanged() {
+    return externalIdentityChanged;
   }
 
   public static UpdateUser create(String login) {
-    Preconditions.checkNotNull(login);
+    checkNotNull(login);
     return new UpdateUser(login);
   }
 }
index 0c8a052a849c45e2b98a14ac29a091eafe1236a8..915a6619b267f9eae32957c2021db69be9bc1be6 100644 (file)
@@ -20,7 +20,6 @@
 package org.sonar.server.user;
 
 import com.google.common.base.Joiner;
-import com.google.common.base.Optional;
 import com.google.common.base.Predicate;
 import com.google.common.base.Strings;
 import com.google.common.collect.Iterables;
@@ -102,12 +101,12 @@ public class UserUpdater {
     boolean isUserReactivated = false;
     UserDto userDto = createNewUserDto(dbSession, newUser);
     String login = userDto.getLogin();
-    Optional<UserDto> existingUser = dbClient.userDao().selectByExternalIdentity(dbSession, userDto.getExternalIdentity(), userDto.getExternalIdentityProvider());
-    if (!existingUser.isPresent()) {
+    UserDto existingUser = dbClient.userDao().selectByLogin(dbSession, userDto.getLogin());
+    if (existingUser == null) {
       saveUser(dbSession, userDto);
       addDefaultGroup(dbSession, userDto);
     } else {
-      isUserReactivated = reactivateUser(dbSession, existingUser.get(), login, newUser);
+      isUserReactivated = reactivateUser(dbSession, existingUser, login, newUser);
     }
     dbSession.commit();
     notifyNewUser(userDto.getLogin(), userDto.getName(), newUser.email());
@@ -122,7 +121,8 @@ 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());
     }
@@ -207,14 +207,7 @@ public class UserUpdater {
       userDto.setScmAccounts(scmAccounts);
     }
 
-    NewUser.ExternalIdentity externalIdentity = newUser.externalIdentity();
-    if (externalIdentity == null) {
-      userDto.setExternalIdentity(login);
-      userDto.setExternalIdentityProvider(SQ_AUTHORITY);
-    } else {
-      userDto.setExternalIdentity(externalIdentity.getId());
-      userDto.setExternalIdentityProvider(externalIdentity.getProvider());
-    }
+    setExternalIdentity(userDto, newUser.externalIdentity());
 
     if (!messages.isEmpty()) {
       throw new BadRequestException(messages);
@@ -254,11 +247,23 @@ public class UserUpdater {
       }
     }
 
+    setExternalIdentity(userDto, updateUser.externalIdentity());
+
     if (!messages.isEmpty()) {
       throw new BadRequestException(messages);
     }
   }
 
+  private static void setExternalIdentity(UserDto dto, @Nullable ExternalIdentity externalIdentity) {
+    if (externalIdentity == null) {
+      dto.setExternalIdentity(dto.getLogin());
+      dto.setExternalIdentityProvider(SQ_AUTHORITY);
+    } else {
+      dto.setExternalIdentity(externalIdentity.getId());
+      dto.setExternalIdentityProvider(externalIdentity.getProvider());
+    }
+  }
+
   private static void checkNotEmptyParam(@Nullable String value, String param, List<Message> messages) {
     if (Strings.isNullOrEmpty(value)) {
       messages.add(Message.of(Validation.CANT_BE_EMPTY_MESSAGE, param));
index 08ca98fa2247ff720780880a4bc40833ad6e70cb..b90972d1d5830ae8a8f6ef81f21b493998f656a9 100644 (file)
@@ -38,7 +38,8 @@ public class BaseContextFactoryTest {
   static String PUBLIC_ROOT_URL = "https://mydomain.com";
 
   static UserIdentity USER_IDENTITY = UserIdentity.builder()
-    .setId("johndoo")
+    .setProviderLogin("johndoo")
+    .setLogin("id:johndoo")
     .setName("John")
     .setEmail("john@email.com")
     .build();
index bb291b0667a31ccd58cbe83b32efd81f886d7af0..4dfb61728fca4e3ef65ff2706439fa808ea23e50 100644 (file)
@@ -46,7 +46,8 @@ public class OAuth2ContextFactoryTest {
   static String NOT_SECURED_PUBLIC_URL = "http://mydomain.com";
 
   static UserIdentity USER_IDENTITY = UserIdentity.builder()
-    .setId("johndoo")
+    .setProviderLogin("johndoo")
+    .setLogin("id:johndoo")
     .setName("John")
     .setEmail("john@email.com")
     .build();
index ab3d706ca69fa0bc3cdefc6d0de866fdf08bd379..7a826681dd77084a4ca351536e1eabd842c0285e 100644 (file)
@@ -19,7 +19,6 @@
  */
 package org.sonar.server.authentication;
 
-import com.google.common.base.Optional;
 import javax.servlet.http.HttpSession;
 import org.junit.Before;
 import org.junit.Rule;
@@ -27,7 +26,6 @@ import org.junit.Test;
 import org.junit.rules.ExpectedException;
 import org.mockito.ArgumentCaptor;
 import org.sonar.api.server.authentication.UserIdentity;
-import org.sonar.core.util.UuidFactory;
 import org.sonar.db.DbClient;
 import org.sonar.db.DbSession;
 import org.sonar.db.user.UserDao;
@@ -44,12 +42,13 @@ import static org.mockito.Mockito.when;
 
 public class UserIdentityAuthenticatorTest {
 
-  static String USER_LOGIN = "ABCD";
+  static String USER_LOGIN = "github-johndoo";
   static UserDto ACTIVE_USER = new UserDto().setId(10L).setLogin(USER_LOGIN).setActive(true);
   static UserDto UNACTIVE_USER = new UserDto().setId(11L).setLogin("UNACTIVE").setActive(false);
 
   static UserIdentity USER_IDENTITY = UserIdentity.builder()
-    .setId("johndoo")
+    .setProviderLogin("johndoo")
+    .setLogin(USER_LOGIN)
     .setName("John")
     .setEmail("john@email.com")
     .build();
@@ -68,21 +67,19 @@ public class UserIdentityAuthenticatorTest {
 
   HttpSession httpSession = mock(HttpSession.class);
   UserUpdater userUpdater = mock(UserUpdater.class);
-  UuidFactory uuidFactory = mock(UuidFactory.class);
 
-  UserIdentityAuthenticator underTest = new UserIdentityAuthenticator(dbClient, userUpdater, uuidFactory);
+  UserIdentityAuthenticator underTest = new UserIdentityAuthenticator(dbClient, userUpdater);
 
   @Before
   public void setUp() throws Exception {
     when(dbClient.openSession(false)).thenReturn(dbSession);
     when(dbClient.userDao()).thenReturn(userDao);
-    when(uuidFactory.create()).thenReturn(USER_LOGIN);
   }
 
   @Test
   public void authenticate_new_user() throws Exception {
-    when(userDao.selectByExternalIdentity(dbSession, USER_IDENTITY.getId(), IDENTITY_PROVIDER.getKey())).thenReturn(Optional.<UserDto>absent());
-    when(userDao.selectOrFailByExternalIdentity(dbSession, USER_IDENTITY.getId(), IDENTITY_PROVIDER.getKey())).thenReturn(ACTIVE_USER);
+    when(userDao.selectByLogin(dbSession, USER_IDENTITY.getLogin())).thenReturn(null);
+    when(userDao.selectOrFailByLogin(dbSession, USER_IDENTITY.getLogin())).thenReturn(ACTIVE_USER);
 
     underTest.authenticate(USER_IDENTITY, IDENTITY_PROVIDER, httpSession);
 
@@ -99,23 +96,25 @@ public class UserIdentityAuthenticatorTest {
 
   @Test
   public void authenticate_existing_user() throws Exception {
-    when(userDao.selectByExternalIdentity(dbSession, USER_IDENTITY.getId(), IDENTITY_PROVIDER.getKey())).thenReturn(Optional.of(ACTIVE_USER));
+    when(userDao.selectByLogin(dbSession, USER_IDENTITY.getLogin())).thenReturn(ACTIVE_USER);
 
     underTest.authenticate(USER_IDENTITY, IDENTITY_PROVIDER, httpSession);
 
     ArgumentCaptor<UpdateUser> updateUserArgumentCaptor = ArgumentCaptor.forClass(UpdateUser.class);
     verify(userUpdater).update(eq(dbSession), updateUserArgumentCaptor.capture());
-    UpdateUser newUser = updateUserArgumentCaptor.getValue();
+    UpdateUser updateUser = updateUserArgumentCaptor.getValue();
 
-    assertThat(newUser.login()).isEqualTo(USER_LOGIN);
-    assertThat(newUser.name()).isEqualTo("John");
-    assertThat(newUser.email()).isEqualTo("john@email.com");
+    assertThat(updateUser.login()).isEqualTo(USER_LOGIN);
+    assertThat(updateUser.name()).isEqualTo("John");
+    assertThat(updateUser.email()).isEqualTo("john@email.com");
+    assertThat(updateUser.externalIdentity().getProvider()).isEqualTo("github");
+    assertThat(updateUser.externalIdentity().getId()).isEqualTo("johndoo");
   }
 
   @Test
   public void authenticate_existing_disabled_user() throws Exception {
-    when(userDao.selectByExternalIdentity(dbSession, USER_IDENTITY.getId(), IDENTITY_PROVIDER.getKey())).thenReturn(Optional.of(UNACTIVE_USER));
-    when(userDao.selectOrFailByExternalIdentity(dbSession, USER_IDENTITY.getId(), IDENTITY_PROVIDER.getKey())).thenReturn(UNACTIVE_USER);
+    when(userDao.selectByLogin(dbSession, USER_IDENTITY.getLogin())).thenReturn(UNACTIVE_USER);
+    when(userDao.selectOrFailByLogin(dbSession, USER_IDENTITY.getLogin())).thenReturn(UNACTIVE_USER);
 
     underTest.authenticate(USER_IDENTITY, IDENTITY_PROVIDER, httpSession);
 
@@ -125,7 +124,7 @@ public class UserIdentityAuthenticatorTest {
 
   @Test
   public void update_session_for_rails() throws Exception {
-    when(userDao.selectByExternalIdentity(dbSession, USER_IDENTITY.getId(), IDENTITY_PROVIDER.getKey())).thenReturn(Optional.of(ACTIVE_USER));
+    when(userDao.selectByLogin(dbSession, USER_IDENTITY.getLogin())).thenReturn(ACTIVE_USER);
 
     underTest.authenticate(USER_IDENTITY, IDENTITY_PROVIDER, httpSession);
 
@@ -134,8 +133,8 @@ public class UserIdentityAuthenticatorTest {
 
   @Test
   public void fail_to_authenticate_new_user_when_allow_users_to_signup_is_false() throws Exception {
-    when(userDao.selectByExternalIdentity(dbSession, USER_IDENTITY.getId(), IDENTITY_PROVIDER.getKey())).thenReturn(Optional.<UserDto>absent());
-    when(userDao.selectOrFailByExternalIdentity(dbSession, USER_IDENTITY.getId(), IDENTITY_PROVIDER.getKey())).thenReturn(ACTIVE_USER);
+    when(userDao.selectByLogin(dbSession, USER_IDENTITY.getLogin())).thenReturn(null);
+    when(userDao.selectOrFailByLogin(dbSession, USER_IDENTITY.getLogin())).thenReturn(ACTIVE_USER);
 
     TestIdentityProvider identityProvider = new TestIdentityProvider()
       .setKey("github")
@@ -149,8 +148,8 @@ public class UserIdentityAuthenticatorTest {
 
   @Test
   public void fail_to_authenticate_new_user_when_email_already_exists() throws Exception {
-    when(userDao.selectByExternalIdentity(dbSession, USER_IDENTITY.getId(), IDENTITY_PROVIDER.getKey())).thenReturn(Optional.<UserDto>absent());
-    when(userDao.selectOrFailByExternalIdentity(dbSession, USER_IDENTITY.getId(), IDENTITY_PROVIDER.getKey())).thenReturn(ACTIVE_USER);
+    when(userDao.selectByLogin(dbSession, USER_IDENTITY.getLogin())).thenReturn(null);
+    when(userDao.selectOrFailByLogin(dbSession, USER_IDENTITY.getLogin())).thenReturn(ACTIVE_USER);
     when(userDao.doesEmailExist(dbSession, USER_IDENTITY.getEmail())).thenReturn(true);
 
     thrown.expect(EmailAlreadyExistsException.class);
diff --git a/server/sonar-server/src/test/java/org/sonar/server/user/ExternalIdentityTest.java b/server/sonar-server/src/test/java/org/sonar/server/user/ExternalIdentityTest.java
new file mode 100644 (file)
index 0000000..32a7667
--- /dev/null
@@ -0,0 +1,56 @@
+/*
+ * SonarQube
+ * Copyright (C) 2009-2016 SonarSource SA
+ * mailto:contact 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.user;
+
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class ExternalIdentityTest {
+
+  @Rule
+  public ExpectedException thrown = ExpectedException.none();
+
+  @Test
+  public void create_external_identity() throws Exception {
+    ExternalIdentity externalIdentity = new ExternalIdentity("github", "login");
+    assertThat(externalIdentity.getId()).isEqualTo("login");
+    assertThat(externalIdentity.getProvider()).isEqualTo("github");
+  }
+
+  @Test
+  public void fail_with_NPE_when_identity_provider_is_null() throws Exception {
+    thrown.expect(NullPointerException.class);
+    thrown.expectMessage("Identity provider cannot be null");
+
+    new ExternalIdentity(null, "login");
+  }
+
+  @Test
+  public void fail_with_NPE_when_identity_id_is_null() throws Exception {
+    thrown.expect(NullPointerException.class);
+    thrown.expectMessage("Identity id cannot be null");
+
+    new ExternalIdentity("github", null);
+  }
+
+}
index 4fa85b502b314fcaec8fd14ad10f31a75f4ae936..2c63c61567cd83e0f9db0d55714fb42e26e7c7c7 100644 (file)
@@ -66,25 +66,9 @@ public class NewUserTest {
       .setName("name")
       .setEmail("email")
       .setPassword("password")
-      .setExternalIdentity(new NewUser.ExternalIdentity("github", "github_login"));
+      .setExternalIdentity(new ExternalIdentity("github", "github_login"));
 
     assertThat(newUser.externalIdentity().getProvider()).isEqualTo("github");
     assertThat(newUser.externalIdentity().getId()).isEqualTo("github_login");
   }
-
-  @Test
-  public void fail_with_NPE_when_identity_provider_is_null() throws Exception {
-    thrown.expect(NullPointerException.class);
-    thrown.expectMessage("Identity provider cannot be null");
-
-    new NewUser.ExternalIdentity(null, "github_login");
-  }
-
-  @Test
-  public void fail_with_NPE_when_identity_id_is_null() throws Exception {
-    thrown.expect(NullPointerException.class);
-    thrown.expectMessage("Identity id cannot be null");
-
-    new NewUser.ExternalIdentity("github", null);
-  }
 }
index 9b1e6b7a5bda0ca69a003a3bac90286da114d887..6730b0de7c397a1bc3200e109612ddbfd186a46c 100644 (file)
@@ -162,7 +162,7 @@ public class UserUpdaterTest {
       .setLogin("ABCD")
       .setName("User")
       .setPassword("password")
-      .setExternalIdentity(new NewUser.ExternalIdentity("github", "user")));
+      .setExternalIdentity(new ExternalIdentity("github", "user")));
 
     UserDto dto = userDao.selectByLogin(session, "ABCD");
     assertThat(dto.getExternalIdentity()).isEqualTo("user");
@@ -473,7 +473,7 @@ public class UserUpdaterTest {
   }
 
   @Test
-  public void reactivate_user_when_creating_user_with_existing_login_and_with_authority() {
+  public void reactivate_user_when_creating_user_with_existing_login() {
     db.prepareDbUnit(getClass(), "reactivate_user.xml");
     when(system2.now()).thenReturn(1418215735486L);
     createDefaultGroup();
@@ -525,6 +525,24 @@ public class UserUpdaterTest {
     assertThat(result).isTrue();
   }
 
+  @Test
+  public void update_external_provider_when_reactivating_user() {
+    db.prepareDbUnit(getClass(), "reactivate_user.xml");
+    when(system2.now()).thenReturn(1418215735486L);
+    createDefaultGroup();
+
+    userUpdater.create(NewUser.create()
+      .setLogin(DEFAULT_LOGIN)
+      .setName("Marius2")
+      .setPassword("password2")
+      .setExternalIdentity(new ExternalIdentity("github", "john")));
+    session.commit();
+
+    UserDto dto = userDao.selectByLogin(session, DEFAULT_LOGIN);
+    assertThat(dto.getExternalIdentity()).isEqualTo("john");
+    assertThat(dto.getExternalIdentityProvider()).isEqualTo("github");
+  }
+
   @Test
   public void fail_to_reactivate_user_if_not_disabled() {
     db.prepareDbUnit(getClass(), "fail_to_reactivate_user_if_not_disabled.xml");
@@ -594,6 +612,24 @@ public class UserUpdaterTest {
         entry("email", "marius2@mail.com"));
   }
 
+  @Test
+  public void update_user_external_identity() {
+    db.prepareDbUnit(getClass(), "update_user.xml");
+    when(system2.now()).thenReturn(1418215735486L);
+    createDefaultGroup();
+
+    userUpdater.update(UpdateUser.create(DEFAULT_LOGIN)
+      .setName("Marius2")
+      .setPassword("password2")
+      .setExternalIdentity(new ExternalIdentity("github", "john")));
+    session.commit();
+    session.clearCache();
+
+    UserDto dto = userDao.selectByLogin(session, DEFAULT_LOGIN);
+    assertThat(dto.getExternalIdentity()).isEqualTo("john");
+    assertThat(dto.getExternalIdentityProvider()).isEqualTo("github");
+  }
+
   @Test
   public void reactivate_user_on_update() {
     db.prepareDbUnit(getClass(), "reactivate_user.xml");
index 8c01178d425eb66c851f03808803fa85bd85e320..f65e2b941a195d531bfcb8d18813529ebcf6df79 100644 (file)
@@ -20,7 +20,6 @@
 package org.sonar.db.user;
 
 import com.google.common.base.Function;
-import com.google.common.base.Optional;
 import java.util.Collection;
 import java.util.List;
 import javax.annotation.CheckForNull;
@@ -34,8 +33,6 @@ import org.sonar.db.DbSession;
 import org.sonar.db.MyBatis;
 import org.sonar.db.RowNotFoundException;
 
-import static com.google.common.base.Optional.fromNullable;
-
 public class UserDao implements Dao {
 
   private final MyBatis mybatis;
@@ -175,18 +172,6 @@ public class UserDao implements Dao {
     return mapper(session).selectNullableByScmAccountOrLoginOrEmail(scmAccountOrLoginOrEmail, like);
   }
 
-  public Optional<UserDto> selectByExternalIdentity(DbSession session, String extIdentity, String extIdentityProvider){
-    return fromNullable(mapper(session).selectByIdentity(extIdentity, extIdentityProvider));
-  }
-
-  public UserDto selectOrFailByExternalIdentity(DbSession session, String extIdentity, String extIdentityProvider) {
-    Optional<UserDto> user = selectByExternalIdentity(session, extIdentity, extIdentityProvider);
-    if (user.isPresent()) {
-      return user.get();
-    }
-    throw new RowNotFoundException(String.format("User with identity provider '%s' and id '%s' has not been found", extIdentityProvider, extIdentity));
-  }
-
   /**
    * Please note that email is case insensitive, result for searching 'mail@email.com' or 'Mail@Email.com' will be the same
    */
index 8f461e4f673b8e44911109c553862337232155db..e3c32da3ad664f31388b0cae3e5d6ea794856e36 100644 (file)
@@ -51,9 +51,6 @@ public interface UserMapper {
 
   List<UserDto> selectByLogins(List<String> logins);
 
-  @CheckForNull
-  UserDto selectByIdentity(@Param("extIdentity") String authorityId, @Param("extIdentityProvider") String authorityProvider);
-
   @CheckForNull
   GroupDto selectGroupByName(String name);
 
index 066036e250c8c07435c9dea4757228698f9357de..e60becc00e2a62adfe4ede2bf69b19f6d7cd3c75 100644 (file)
     ORDER BY u.name
   </select>
 
-  <select id="selectByIdentity" parameterType="map" resultType="User">
-    SELECT <include refid="userColumns"/>
-    FROM users u
-    <where>
-      u.external_identity=#{extIdentity}
-      AND u.external_identity_provider=#{extIdentityProvider}
-    </where>
-  </select>
-
   <select id="countByEmail" parameterType="String" resultType="long">
     SELECT count(u.id)
     FROM users u
 
   <insert id="update" parameterType="User" useGeneratedKeys="false">
     UPDATE users set name=#{name,jdbcType=VARCHAR}, email=#{email,jdbcType=VARCHAR}, active=#{active,jdbcType=BOOLEAN},
-    scm_accounts=#{scmAccounts,jdbcType=VARCHAR},
+    scm_accounts=#{scmAccounts,jdbcType=VARCHAR}, external_identity=#{externalIdentity,jdbcType=VARCHAR}, external_identity_provider=#{externalIdentityProvider,jdbcType=VARCHAR},
     salt=#{salt,jdbcType=VARCHAR}, crypted_password=#{cryptedPassword,jdbcType=BIGINT},
     updated_at=#{updatedAt,jdbcType=BIGINT}
     WHERE login = #{login}
index 57294908f9bc7938a934f58000a2b65a266ccf76..a61b3b960e5027a980737238b580df88beeca05e 100644 (file)
@@ -36,7 +36,6 @@ import org.sonar.test.DbTests;
 
 import static java.util.Arrays.asList;
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.assertj.guava.api.Assertions.assertThat;
 import static org.junit.Assert.fail;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
@@ -203,6 +202,8 @@ public class UserDaoTest {
       .setActive(false)
       .setSalt("12345")
       .setCryptedPassword("abcde")
+      .setExternalIdentity("johngithub")
+      .setExternalIdentityProvider("github")
       .setUpdatedAt(date);
     underTest.update(db.getSession(), userDto);
     db.getSession().commit();
@@ -217,6 +218,8 @@ public class UserDaoTest {
     assertThat(user.getScmAccounts()).isEqualTo(",jo.hn,john2,johndoo,");
     assertThat(user.getSalt()).isEqualTo("12345");
     assertThat(user.getCryptedPassword()).isEqualTo("abcde");
+    assertThat(user.getExternalIdentity()).isEqualTo("johngithub");
+    assertThat(user.getExternalIdentityProvider()).isEqualTo("github");
     assertThat(user.getCreatedAt()).isEqualTo(1418215735482L);
     assertThat(user.getUpdatedAt()).isEqualTo(date);
   }
@@ -324,25 +327,6 @@ public class UserDaoTest {
     assertThat(underTest.selectByLogin(session, "unknown")).isNull();
   }
 
-  @Test
-  public void select_user_by_external_identity() {
-    db.prepareDbUnit(getClass(), "select_users_by_ext_identity.xml");
-
-    assertThat(underTest.selectByExternalIdentity(session, "mariusgithub", "github")).isPresent();
-    assertThat(underTest.selectByExternalIdentity(session, "mariusgithub", "google")).isAbsent();
-    assertThat(underTest.selectByExternalIdentity(session, "unknown", "unknown")).isAbsent();
-  }
-
-  @Test
-  public void select_or_fail_by_external_identity() throws Exception {
-    db.prepareDbUnit(getClass(), "select_users_by_ext_identity.xml");
-    assertThat(underTest.selectOrFailByExternalIdentity(session, "mariusgithub", "github")).isNotNull();
-
-    thrown.expect(RowNotFoundException.class);
-    thrown.expectMessage("User with identity provider 'unknown' and id 'unknown' has not been found");
-    underTest.selectOrFailByExternalIdentity(session, "unknown", "unknown");
-  }
-
   @Test
   public void exists_by_email() throws Exception {
     db.prepareDbUnit(getClass(), "exists_by_email.xml");
@@ -351,4 +335,5 @@ public class UserDaoTest {
     assertThat(underTest.doesEmailExist(session, "Marius@LesBronzes.fr")).isTrue();
     assertThat(underTest.doesEmailExist(session, "unknown")).isFalse();
   }
+
 }
diff --git a/sonar-db/src/test/resources/org/sonar/db/user/UserDaoTest/select_users_by_ext_identity.xml b/sonar-db/src/test/resources/org/sonar/db/user/UserDaoTest/select_users_by_ext_identity.xml
deleted file mode 100644 (file)
index 247f132..0000000
+++ /dev/null
@@ -1,12 +0,0 @@
-<dataset>
-
-  <users id="101" login="marius" name="Marius" email="marius@lesbronzes.fr" active="[true]" scm_accounts="&#10;ma&#10;marius33&#10;"
-         external_identity_provider="github" external_identity="mariusgithub"
-         created_at="1418215735482" updated_at="1418215735485"
-         salt="79bd6a8e79fb8c76ac8b121cc7e8e11ad1af8365" crypted_password="650d2261c98361e2f67f90ce5c65a95e7d8ea2fg"/>
-  <users id="102" login="sbrandhof" name="Simon Brandhof" email="marius@lesbronzes.fr" active="[true]" scm_accounts="[null]"
-         external_identity_provider="google" external_identity="mariusgoogle"
-         created_at="1418215735482" updated_at="1418215735485"
-         salt="79bd6a8e79fb8c76ac8b121cc7e8e11ad1af8366" crypted_password="650d2261c98361e2f67f90ce5c65a95e7d8ea2fh"/>
-
-</dataset>
index ae1564ac26f2a32c788a2bb40c4d842756eb1a35..80bba8904f7bdcd97ebe98a33d68473b68653ae8 100644 (file)
@@ -34,22 +34,31 @@ import static org.apache.commons.lang.StringUtils.isNotBlank;
 @Immutable
 public final class UserIdentity {
 
-  private final String id;
+  private final String providerLogin;
+  private final String login;
   private final String name;
   private final String email;
 
   private UserIdentity(Builder builder) {
-    this.id = builder.id;
+    this.providerLogin = builder.providerLogin;
+    this.login = builder.login;
     this.name = builder.name;
     this.email = builder.email;
   }
 
   /**
-   * Non-blank user ID, unique for the related {@link IdentityProvider}. If two {@link IdentityProvider}
-   * define two users with the same ID, then users are considered as different.
+   * Non-blank user login for the related {@link IdentityProvider}.
    */
-  public String getId() {
-    return id;
+  public String getProviderLogin() {
+    return providerLogin;
+  }
+
+  /**
+   * Non-blank user login, unique for the SonarQube platform.
+   * If two {@link IdentityProvider} define two users with the same login, then users are considered as identical.
+   */
+  public String getLogin() {
+    return login;
   }
 
   /**
@@ -74,7 +83,8 @@ public final class UserIdentity {
   }
 
   public static class Builder {
-    private String id;
+    private String providerLogin;
+    private String login;
     private String name;
     private String email;
 
@@ -82,10 +92,18 @@ public final class UserIdentity {
     }
 
     /**
-     * @see UserIdentity#getId()
+     * @see UserIdentity#getProviderLogin()
      */
-    public Builder setId(String id) {
-      this.id = id;
+    public Builder setProviderLogin(String providerLogin) {
+      this.providerLogin = providerLogin;
+      return this;
+    }
+
+    /**
+     * @see UserIdentity#getLogin() ()
+     */
+    public Builder setLogin(String login) {
+      this.login = login;
       return this;
     }
 
@@ -106,15 +124,21 @@ public final class UserIdentity {
     }
 
     public UserIdentity build() {
-      validateId(id);
+      validateProviderLogin(providerLogin);
+      validateLogin(login);
       validateName(name);
       validateEmail(email);
       return new UserIdentity(this);
     }
 
-    private static void validateId(String id){
-      checkArgument(isNotBlank(id), "User id must not be blank");
-      checkArgument(id.length() <= 255 && id.length() >= 3, "User id size is incorrect (Between 3 and 255 characters)");
+    private static void validateProviderLogin(String providerLogin){
+      checkArgument(isNotBlank(providerLogin), "Provider login must not be blank");
+      checkArgument(providerLogin.length() <= 255, "Provider login size is incorrect (maximum 255 characters)");
+    }
+
+    private static void validateLogin(String login){
+      checkArgument(isNotBlank(login), "User login must not be blank");
+      checkArgument(login.length() <= 255 && login.length() >= 3, "User login size is incorrect (Between 3 and 255 characters)");
     }
 
     private static void validateName(String name){
index c74e1da0c0b2232b00cf62c883e5b66f0553c151..25096e6cebe626c27497a51992f79c591f91dd31 100644 (file)
@@ -29,17 +29,19 @@ import static org.assertj.core.api.Assertions.assertThat;
 public class UserIdentityTest {
 
   @Rule
-  public ExpectedException thrown= ExpectedException.none();
+  public ExpectedException thrown = ExpectedException.none();
 
   @Test
   public void create_user() throws Exception {
     UserIdentity underTest = UserIdentity.builder()
-      .setId("john")
+      .setProviderLogin("john")
+      .setLogin("1234")
       .setName("John")
       .setEmail("john@email.com")
       .build();
 
-    assertThat(underTest.getId()).isEqualTo("john");
+    assertThat(underTest.getProviderLogin()).isEqualTo("john");
+    assertThat(underTest.getLogin()).isEqualTo("1234");
     assertThat(underTest.getName()).isEqualTo("John");
     assertThat(underTest.getEmail()).isEqualTo("john@email.com");
   }
@@ -47,7 +49,8 @@ public class UserIdentityTest {
   @Test
   public void create_user_without_email() throws Exception {
     UserIdentity underTest = UserIdentity.builder()
-      .setId("john")
+      .setProviderLogin("john")
+      .setLogin("1234")
       .setName("John")
       .build();
 
@@ -55,43 +58,71 @@ public class UserIdentityTest {
   }
 
   @Test
-  public void fail_when_id_is_null() throws Exception {
+  public void fail_when_login_is_empty() throws Exception {
     thrown.expect(IllegalArgumentException.class);
-    thrown.expectMessage("User id must not be blank");
+    thrown.expectMessage("User login must not be blank");
     UserIdentity.builder()
+      .setProviderLogin("john")
+      .setLogin("")
       .setName("John")
       .setEmail("john@email.com")
       .build();
   }
 
   @Test
-  public void fail_when_id_is_empty() throws Exception {
+  public void fail_when_login_is_too_long() throws Exception {
     thrown.expect(IllegalArgumentException.class);
-    thrown.expectMessage("User id must not be blank");
+    thrown.expectMessage("User login size is incorrect (Between 3 and 255 characters)");
     UserIdentity.builder()
-      .setId("")
+      .setProviderLogin("john")
+      .setLogin(Strings.repeat("1", 256))
       .setName("John")
       .setEmail("john@email.com")
       .build();
   }
 
   @Test
-  public void fail_when_id_is_loo_long() throws Exception {
+  public void fail_when_login_is_too_small() throws Exception {
     thrown.expect(IllegalArgumentException.class);
-    thrown.expectMessage("User id size is incorrect (Between 3 and 255 characters)");
+    thrown.expectMessage("User login size is incorrect (Between 3 and 255 characters)");
     UserIdentity.builder()
-      .setId(Strings.repeat("1", 256))
+      .setProviderLogin("john")
+      .setLogin("12")
       .setName("John")
       .setEmail("john@email.com")
       .build();
   }
 
   @Test
-  public void fail_when_id_is_loo_small() throws Exception {
+  public void fail_when_provider_login_is_null() throws Exception {
     thrown.expect(IllegalArgumentException.class);
-    thrown.expectMessage("User id size is incorrect (Between 3 and 255 characters)");
+    thrown.expectMessage("Provider login must not be blank");
     UserIdentity.builder()
-      .setId("ab")
+      .setLogin("1234")
+      .setName("John")
+      .setEmail("john@email.com")
+      .build();
+  }
+
+  @Test
+  public void fail_when_provider_login_is_empty() throws Exception {
+    thrown.expect(IllegalArgumentException.class);
+    thrown.expectMessage("Provider login must not be blank");
+    UserIdentity.builder()
+      .setProviderLogin("")
+      .setLogin("1234")
+      .setName("John")
+      .setEmail("john@email.com")
+      .build();
+  }
+
+  @Test
+  public void fail_when_provider_login_is_too_long() throws Exception {
+    thrown.expect(IllegalArgumentException.class);
+    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();
@@ -102,7 +133,8 @@ public class UserIdentityTest {
     thrown.expect(IllegalArgumentException.class);
     thrown.expectMessage("User name must not be blank");
     UserIdentity.builder()
-      .setId("john")
+      .setProviderLogin("john")
+      .setLogin("1234")
       .setEmail("john@email.com")
       .build();
   }
@@ -112,7 +144,8 @@ public class UserIdentityTest {
     thrown.expect(IllegalArgumentException.class);
     thrown.expectMessage("User name must not be blank");
     UserIdentity.builder()
-      .setId("john")
+      .setProviderLogin("john")
+      .setLogin("1234")
       .setName("")
       .setEmail("john@email.com")
       .build();
@@ -123,7 +156,8 @@ public class UserIdentityTest {
     thrown.expect(IllegalArgumentException.class);
     thrown.expectMessage("User name size is too big (200 characters max)");
     UserIdentity.builder()
-      .setId("john")
+      .setProviderLogin("john")
+      .setLogin("1234")
       .setName(Strings.repeat("1", 201))
       .setEmail("john@email.com")
       .build();
@@ -134,7 +168,8 @@ public class UserIdentityTest {
     thrown.expect(IllegalArgumentException.class);
     thrown.expectMessage("User email size is too big (100 characters max)");
     UserIdentity.builder()
-      .setId("john")
+      .setProviderLogin("john")
+      .setLogin("1234")
       .setName("John")
       .setEmail(Strings.repeat("1", 101))
       .build();