SONARCLOUD-220 refactor the class Credentials

6 files changed, 41 insertions, 19 deletions

diff --git a/server/sonar-server/src/main/java/org/sonar/server/authentication/BasicAuthentication.java b/server/sonar-server/src/main/java/org/sonar/server/authentication/BasicAuthentication.java
index 0ecd1f6b72f..4f4707f8c51 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/authentication/BasicAuthentication.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/authentication/BasicAuthentication.java
@@ -94,13 +94,12 @@ public class BasicAuthentication {
   }
 
   private UserDto authenticate(Credentials credentials, HttpServletRequest request) {
-    if (credentials.getPassword().isEmpty()) {
+    if (!credentials.getPassword().isPresent()) {
       UserDto userDto = authenticateFromUserToken(credentials.getLogin());
       authenticationEvent.loginSuccess(request, userDto.getLogin(), Source.local(Method.BASIC_TOKEN));
       return userDto;
-    } else {
-      return credentialsAuthentication.authenticate(credentials, request, Method.BASIC);
     }
+    return credentialsAuthentication.authenticate(credentials, request, Method.BASIC);
   }
 
   private UserDto authenticateFromUserToken(String token) {
diff --git a/server/sonar-server/src/main/java/org/sonar/server/authentication/Credentials.java b/server/sonar-server/src/main/java/org/sonar/server/authentication/Credentials.java
index 7beddbfcd63..6e84ffbaec9 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/authentication/Credentials.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/authentication/Credentials.java
@@ -20,9 +20,12 @@
 package org.sonar.server.authentication;
 
 import java.util.Objects;
+import java.util.Optional;
 import javax.annotation.Nullable;
 import javax.annotation.concurrent.Immutable;
-import org.apache.commons.lang.StringUtils;
+
+import static com.google.common.base.Preconditions.checkArgument;
+import static org.apache.commons.lang.StringUtils.defaultIfEmpty;
 
 @Immutable
 public class Credentials {
@@ -31,8 +34,9 @@ public class Credentials {
   private final String password;
 
   public Credentials(String login, @Nullable String password) {
-    this.login = Objects.requireNonNull(login, "login must not be null");
-    this.password = StringUtils.defaultString(password, "");
+    checkArgument(login != null && !login.isEmpty(), "login must not be null nor empty");
+    this.login = login;
+    this.password = defaultIfEmpty(password, null);
   }
 
   /**
@@ -43,10 +47,11 @@ public class Credentials {
   }
 
   /**
-   * Non-null password. Can be empty.
+   * Non-empty password. {@code Optional.empty()} is returned if the password is not set
+   * or initially empty. {@code Optional.of("")} is never returned.
    */
-  public String getPassword() {
-    return password;
+  public Optional<String> getPassword() {
+    return Optional.ofNullable(password);
   }
 
   @Override
diff --git a/server/sonar-server/src/main/java/org/sonar/server/authentication/CredentialsAuthentication.java b/server/sonar-server/src/main/java/org/sonar/server/authentication/CredentialsAuthentication.java
index 2d9d6667bef..7376693a05e 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/authentication/CredentialsAuthentication.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/authentication/CredentialsAuthentication.java
@@ -58,7 +58,7 @@ public class CredentialsAuthentication {
   private UserDto authenticate(DbSession dbSession, Credentials credentials, HttpServletRequest request, Method method) {
     UserDto localUser = dbClient.userDao().selectActiveUserByLogin(dbSession, credentials.getLogin());
     if (localUser != null && localUser.isLocal()) {
-      localAuthentication.authenticate(dbSession, localUser, credentials.getPassword(), method);
+      localAuthentication.authenticate(dbSession, localUser, credentials.getPassword().orElse(null), method);
       dbSession.commit();
       authenticationEvent.loginSuccess(request, localUser.getLogin(), Source.local(method));
       return localUser;
diff --git a/server/sonar-server/src/main/java/org/sonar/server/authentication/CredentialsExternalAuthentication.java b/server/sonar-server/src/main/java/org/sonar/server/authentication/CredentialsExternalAuthentication.java
index 97ba8d33aa4..30b2937a058 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/authentication/CredentialsExternalAuthentication.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/authentication/CredentialsExternalAuthentication.java
@@ -67,7 +67,7 @@ public class CredentialsExternalAuthentication implements Startable {
   private ExternalGroupsProvider externalGroupsProvider;
 
   public CredentialsExternalAuthentication(Configuration config, SecurityRealmFactory securityRealmFactory,
-                                           UserRegistrar userRegistrar, AuthenticationEvent authenticationEvent) {
+    UserRegistrar userRegistrar, AuthenticationEvent authenticationEvent) {
     this.config = config;
     this.securityRealmFactory = securityRealmFactory;
     this.userRegistrar = userRegistrar;
@@ -102,7 +102,7 @@ public class CredentialsExternalAuthentication implements Startable {
           .setMessage("No user details")
           .build();
       }
-      Authenticator.Context authenticatorContext = new Authenticator.Context(credentials.getLogin(), credentials.getPassword(), request);
+      Authenticator.Context authenticatorContext = new Authenticator.Context(credentials.getLogin(), credentials.getPassword().orElse(null), request);
       boolean status = authenticator.doAuthenticate(authenticatorContext);
       if (!status) {
         throw AuthenticationException.newBuilder()
@@ -155,7 +155,7 @@ public class CredentialsExternalAuthentication implements Startable {
 
   private Credentials fixCase(Credentials credentials) {
     if (config.getBoolean("sonar.authenticator.downcase").orElse(false)) {
-      return new Credentials(credentials.getLogin().toLowerCase(Locale.ENGLISH), credentials.getPassword());
+      return new Credentials(credentials.getLogin().toLowerCase(Locale.ENGLISH), credentials.getPassword().orElse(null));
     }
     return credentials;
   }
diff --git a/server/sonar-server/src/main/java/org/sonar/server/authentication/CredentialsLocalAuthentication.java b/server/sonar-server/src/main/java/org/sonar/server/authentication/CredentialsLocalAuthentication.java
index e28a278d4c9..66eef7bc815 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/authentication/CredentialsLocalAuthentication.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/authentication/CredentialsLocalAuthentication.java
@@ -21,6 +21,7 @@
 package org.sonar.server.authentication;
 
 import java.security.SecureRandom;
+import javax.annotation.Nullable;
 import org.apache.commons.codec.digest.DigestUtils;
 import org.mindrot.jbcrypt.BCrypt;
 import org.sonar.db.DbClient;
@@ -55,7 +56,7 @@ public class CredentialsLocalAuthentication {
    * If the password must be updated because an old algorithm is used, the UserDto is updated but the session
    * is not committed
    */
-  public void authenticate(DbSession session, UserDto user, String password, Method method) {
+  public void authenticate(DbSession session, UserDto user, @Nullable String password, Method method) {
     if (user.getHashMethod() == null) {
       throw AuthenticationException.newBuilder()
         .setSource(Source.local(method))
diff --git a/server/sonar-server/src/test/java/org/sonar/server/authentication/CredentialsTest.java b/server/sonar-server/src/test/java/org/sonar/server/authentication/CredentialsTest.java
index 03c5dedeea7..e3d255f1d0c 100644
--- a/server/sonar-server/src/test/java/org/sonar/server/authentication/CredentialsTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/authentication/CredentialsTest.java
@@ -22,22 +22,39 @@ package org.sonar.server.authentication;
 import org.junit.Test;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.catchThrowable;
 
 public class CredentialsTest {
 
   @Test
-  public void password_can_be_empty_but_not_null() {
+  public void login_cant_be_empty() {
+    Throwable thrown = catchThrowable(() -> new Credentials("", "bar"));
+    assertThat(thrown)
+      .isInstanceOf(IllegalArgumentException.class)
+      .hasMessage("login must not be null nor empty");
+
+    thrown = catchThrowable(() -> new Credentials(null, "bar"));
+    assertThat(thrown)
+      .isInstanceOf(IllegalArgumentException.class)
+      .hasMessage("login must not be null nor empty");
+
+    Credentials underTest = new Credentials("foo", "bar");
+    assertThat(underTest.getLogin()).isEqualTo("foo");
+  }
+
+  @Test
+  public void password_cant_be_empty_string() {
     Credentials underTest = new Credentials("foo", "");
-    assertThat(underTest.getPassword()).isEqualTo("");
+    assertThat(underTest.getPassword()).isEmpty();
 
     underTest = new Credentials("foo", null);
-    assertThat(underTest.getPassword()).isEqualTo("");
+    assertThat(underTest.getPassword()).isEmpty();
 
     underTest = new Credentials("foo", "   ");
-    assertThat(underTest.getPassword()).isEqualTo("   ");
+    assertThat(underTest.getPassword()).hasValue("   ");
 
     underTest = new Credentials("foo", "bar");
-    assertThat(underTest.getPassword()).isEqualTo("bar");
+    assertThat(underTest.getPassword()).hasValue("bar");
   }
 
   @Test