SONAR-14320 Do not throw an exception if password is NULL and hash is BCrypt

2 files changed, 84 insertions, 56 deletions

diff --git a/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/CredentialsLocalAuthentication.java b/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/CredentialsLocalAuthentication.java
index e54b106c156..c7aa4d5acae 100644
--- a/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/CredentialsLocalAuthentication.java
+++ b/server/sonar-webserver-auth/src/main/java/org/sonar/server/authentication/CredentialsLocalAuthentication.java
@@ -49,6 +49,12 @@ public class CredentialsLocalAuthentication {
   private static final SecureRandom SECURE_RANDOM = new SecureRandom();
   private static final HashMethod DEFAULT = HashMethod.PBKDF2;
   private static final String PBKDF2_ITERATIONS_PROP = "sonar.internal.pbkdf2.iterations";
+  public static final String ERROR_NULL_HASH_METHOD = "null hash method";
+  public static final String ERROR_NULL_PASSWORD_IN_DB = "null password in DB";
+  public static final String ERROR_NULL_SALT = "null salt";
+  public static final String ERROR_WRONG_PASSWORD = "wrong password";
+  public static final String ERROR_PASSWORD_CANNOT_BE_NULL = "Password cannot be null";
+  public static final String ERROR_UNKNOWN_HASH_METHOD = "Unknown hash method [%s]";
 
   private final DbClient dbClient;
   private final EnumMap<HashMethod, HashFunction> hashFunctions = new EnumMap<>(HashMethod.class);
@@ -76,7 +82,7 @@ public class CredentialsLocalAuthentication {
       throw AuthenticationException.newBuilder()
         .setSource(Source.local(method))
         .setLogin(user.getLogin())
-        .setMessage("null hash method")
+        .setMessage(ERROR_NULL_HASH_METHOD)
         .build();
     }
 
@@ -87,7 +93,7 @@ public class CredentialsLocalAuthentication {
       throw AuthenticationException.newBuilder()
         .setSource(Source.local(method))
         .setLogin(user.getLogin())
-        .setMessage(format("Unknown hash method [%s]", user.getHashMethod()))
+        .setMessage(format(ERROR_UNKNOWN_HASH_METHOD, user.getHashMethod()))
         .build();
     }
 
@@ -159,20 +165,20 @@ public class CredentialsLocalAuthentication {
     @Override
     public AuthenticationResult checkCredentials(UserDto user, String password) {
       if (user.getCryptedPassword() == null) {
-        return new AuthenticationResult(false, "null password in DB");
+        return new AuthenticationResult(false, ERROR_NULL_PASSWORD_IN_DB);
       }
       if (user.getSalt() == null) {
-        return new AuthenticationResult(false, "null salt");
+        return new AuthenticationResult(false, ERROR_NULL_SALT);
       }
       if (!user.getCryptedPassword().equals(hash(user.getSalt(), password))) {
-        return new AuthenticationResult(false, "wrong password");
+        return new AuthenticationResult(false, ERROR_WRONG_PASSWORD);
       }
       return new AuthenticationResult(true, "");
     }
 
     @Override
     public void storeHashPassword(UserDto user, String password) {
-      requireNonNull(password, "Password cannot be null");
+      requireNonNull(password, ERROR_PASSWORD_CANNOT_BE_NULL);
       byte[] saltRandom = new byte[20];
       SECURE_RANDOM.nextBytes(saltRandom);
       String salt = DigestUtils.sha1Hex(saltRandom);
@@ -191,6 +197,7 @@ public class CredentialsLocalAuthentication {
     private static final int DEFAULT_ITERATIONS = 100_000;
     private static final String ALGORITHM = "PBKDF2WithHmacSHA512";
     private static final int KEY_LEN = 512;
+    public static final String ERROR_INVALID_HASH_STORED = "invalid hash stored";
     private final int gen_iterations;
 
     public PBKDF2Function(@Nullable Integer gen_iterations) {
@@ -200,27 +207,27 @@ public class CredentialsLocalAuthentication {
     @Override
     public AuthenticationResult checkCredentials(UserDto user, String password) {
       if (user.getCryptedPassword() == null) {
-        return new AuthenticationResult(false, "null password in DB");
+        return new AuthenticationResult(false, ERROR_NULL_PASSWORD_IN_DB);
       }
       if (user.getSalt() == null) {
-        return new AuthenticationResult(false, "null salt");
+        return new AuthenticationResult(false, ERROR_NULL_SALT);
       }
 
       int pos = user.getCryptedPassword().indexOf('$');
       if (pos < 1) {
-        return new AuthenticationResult(false, "invalid hash stored");
+        return new AuthenticationResult(false, ERROR_INVALID_HASH_STORED);
       }
       int iterations;
       try {
         iterations = Integer.parseInt(user.getCryptedPassword().substring(0, pos));
       } catch (NumberFormatException e) {
-        return new AuthenticationResult(false, "invalid hash stored");
+        return new AuthenticationResult(false, ERROR_INVALID_HASH_STORED);
       }
       String hash = user.getCryptedPassword().substring(pos + 1);
       byte[] salt = Base64.getDecoder().decode(user.getSalt());
 
       if (!hash.equals(hash(salt, password, iterations))) {
-        return new AuthenticationResult(false, "wrong password");
+        return new AuthenticationResult(false, ERROR_WRONG_PASSWORD);
       }
       boolean needsUpdate = iterations != gen_iterations;
       return new AuthenticationResult(true, "", needsUpdate);
@@ -255,16 +262,20 @@ public class CredentialsLocalAuthentication {
   private static final class BcryptFunction implements HashFunction {
     @Override
     public AuthenticationResult checkCredentials(UserDto user, String password) {
-      // This behavior is overridden in most of integration tests for performance reasons, any changes to BCrypt calls should be propagated to Byteman classes
+      if (user.getCryptedPassword() == null) {
+        return new AuthenticationResult(false, ERROR_NULL_PASSWORD_IN_DB);
+      }
+      // This behavior is overridden in most of integration tests for performance reasons, any changes to BCrypt calls should be propagated to
+      // Byteman classes
       if (!BCrypt.checkpw(password, user.getCryptedPassword())) {
-        return new AuthenticationResult(false, "wrong password");
+        return new AuthenticationResult(false, ERROR_WRONG_PASSWORD);
       }
       return new AuthenticationResult(true, "");
     }
 
     @Override
     public void storeHashPassword(UserDto user, String password) {
-      requireNonNull(password, "Password cannot be null");
+      requireNonNull(password, ERROR_PASSWORD_CANNOT_BE_NULL);
       user.setHashMethod(HashMethod.BCRYPT.name())
         .setCryptedPassword(BCrypt.hashpw(password, BCrypt.gensalt(12)))
         .setSalt(null);
diff --git a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/CredentialsLocalAuthenticationTest.java b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/CredentialsLocalAuthenticationTest.java
index e7cafffc61a..7cac4072035 100644
--- a/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/CredentialsLocalAuthenticationTest.java
+++ b/server/sonar-webserver-auth/src/test/java/org/sonar/server/authentication/CredentialsLocalAuthenticationTest.java
@@ -25,16 +25,18 @@ import org.apache.commons.codec.digest.DigestUtils;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
-import org.junit.rules.ExpectedException;
 import org.mindrot.jbcrypt.BCrypt;
 import org.sonar.api.config.internal.MapSettings;
+import org.sonar.db.DbSession;
 import org.sonar.db.DbTester;
 import org.sonar.db.user.UserDto;
 import org.sonar.server.authentication.event.AuthenticationEvent;
 import org.sonar.server.authentication.event.AuthenticationException;
 
+import static java.lang.String.format;
 import static org.apache.commons.lang.RandomStringUtils.randomAlphanumeric;
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.sonar.db.user.UserTesting.newUserDto;
 import static org.sonar.server.authentication.CredentialsLocalAuthentication.HashMethod.BCRYPT;
 import static org.sonar.server.authentication.CredentialsLocalAuthentication.HashMethod.PBKDF2;
@@ -42,8 +44,6 @@ import static org.sonar.server.authentication.CredentialsLocalAuthentication.Has
 
 public class CredentialsLocalAuthenticationTest {
   @Rule
-  public ExpectedException expectedException = ExpectedException.none();
-  @Rule
   public DbTester db = DbTester.create();
 
   private static final Random RANDOM = new Random();
@@ -58,23 +58,23 @@ public class CredentialsLocalAuthenticationTest {
 
   @Test
   public void incorrect_hash_should_throw_AuthenticationException() {
+    DbSession dbSession = db.getSession();
     UserDto user = newUserDto()
       .setHashMethod("ALGON2");
 
-    expectedException.expect(AuthenticationException.class);
-    expectedException.expectMessage("Unknown hash method [ALGON2]");
-
-    underTest.authenticate(db.getSession(), user, "whatever", AuthenticationEvent.Method.BASIC);
+    assertThatThrownBy(() -> underTest.authenticate(dbSession, user, "whatever", AuthenticationEvent.Method.BASIC))
+      .isInstanceOf(AuthenticationException.class)
+      .hasMessage(format(CredentialsLocalAuthentication.ERROR_UNKNOWN_HASH_METHOD, "ALGON2"));
   }
 
   @Test
   public void null_hash_should_throw_AuthenticationException() {
+    DbSession dbSession = db.getSession();
     UserDto user = newUserDto();
 
-    expectedException.expect(AuthenticationException.class);
-    expectedException.expectMessage("null hash method");
-
-    underTest.authenticate(db.getSession(), user, "whatever", AuthenticationEvent.Method.BASIC);
+    assertThatThrownBy(() -> underTest.authenticate(dbSession, user, "whatever", AuthenticationEvent.Method.BASIC))
+      .isInstanceOf(AuthenticationException.class)
+      .hasMessage(CredentialsLocalAuthentication.ERROR_NULL_HASH_METHOD);
   }
 
   @Test
@@ -107,6 +107,7 @@ public class CredentialsLocalAuthenticationTest {
   @Test
   public void authentication_with_sha1_with_incorrect_password_should_throw_AuthenticationException() {
     String password = randomAlphanumeric(60);
+    DbSession dbSession = db.getSession();
 
     byte[] saltRandom = new byte[20];
     RANDOM.nextBytes(saltRandom);
@@ -117,14 +118,14 @@ public class CredentialsLocalAuthenticationTest {
       .setCryptedPassword(DigestUtils.sha1Hex("--" + salt + "--" + password + "--"))
       .setSalt(salt);
 
-    expectedException.expect(AuthenticationException.class);
-    expectedException.expectMessage("wrong password");
-
-    underTest.authenticate(db.getSession(), user, "WHATEVER", AuthenticationEvent.Method.BASIC);
+    assertThatThrownBy(() -> underTest.authenticate(dbSession, user, "WHATEVER", AuthenticationEvent.Method.BASIC))
+      .isInstanceOf(AuthenticationException.class)
+      .hasMessage(CredentialsLocalAuthentication.ERROR_WRONG_PASSWORD);
   }
 
   @Test
   public void authentication_with_sha1_with_empty_password_should_throw_AuthenticationException() {
+    DbSession dbSession = db.getSession();
     byte[] saltRandom = new byte[20];
     RANDOM.nextBytes(saltRandom);
     String salt = DigestUtils.sha1Hex(saltRandom);
@@ -134,14 +135,14 @@ public class CredentialsLocalAuthenticationTest {
       .setHashMethod(SHA1.name())
       .setSalt(salt);
 
-    expectedException.expect(AuthenticationException.class);
-    expectedException.expectMessage("null password in DB");
-
-    underTest.authenticate(db.getSession(), user, "WHATEVER", AuthenticationEvent.Method.BASIC);
+    assertThatThrownBy(() -> underTest.authenticate(dbSession, user, "WHATEVER", AuthenticationEvent.Method.BASIC))
+      .isInstanceOf(AuthenticationException.class)
+      .hasMessage(CredentialsLocalAuthentication.ERROR_NULL_PASSWORD_IN_DB);
   }
 
   @Test
   public void authentication_with_sha1_with_empty_salt_should_throw_AuthenticationException() {
+    DbSession dbSession = db.getSession();
     String password = randomAlphanumeric(60);
 
     UserDto user = newUserDto()
@@ -149,24 +150,35 @@ public class CredentialsLocalAuthenticationTest {
       .setCryptedPassword(DigestUtils.sha1Hex("--0242b0b4c0a93ddfe09dd886de50bc25ba000b51--" + password + "--"))
       .setSalt(null);
 
-    expectedException.expect(AuthenticationException.class);
-    expectedException.expectMessage("null salt");
-
-    underTest.authenticate(db.getSession(), user, "WHATEVER", AuthenticationEvent.Method.BASIC);
+    assertThatThrownBy(() -> underTest.authenticate(dbSession, user, "WHATEVER", AuthenticationEvent.Method.BASIC))
+      .isInstanceOf(AuthenticationException.class)
+      .hasMessage(CredentialsLocalAuthentication.ERROR_NULL_SALT);
   }
 
   @Test
   public void authentication_with_bcrypt_with_incorrect_password_should_throw_AuthenticationException() {
+    DbSession dbSession = db.getSession();
     String password = randomAlphanumeric(60);
 
     UserDto user = newUserDto()
       .setHashMethod(BCRYPT.name())
       .setCryptedPassword(BCrypt.hashpw(password, BCrypt.gensalt(12)));
 
-    expectedException.expect(AuthenticationException.class);
-    expectedException.expectMessage("wrong password");
+    assertThatThrownBy(() -> underTest.authenticate(dbSession, user, "WHATEVER", AuthenticationEvent.Method.BASIC))
+      .isInstanceOf(AuthenticationException.class)
+      .hasMessage(CredentialsLocalAuthentication.ERROR_WRONG_PASSWORD);
+  }
 
-    underTest.authenticate(db.getSession(), user, "WHATEVER", AuthenticationEvent.Method.BASIC);
+  @Test
+  public void authentication_with_bcrypt_with_empty_password_should_throw_AuthenticationException() {
+    DbSession dbSession = db.getSession();
+    UserDto user = newUserDto()
+      .setCryptedPassword(null)
+      .setHashMethod(BCRYPT.name());
+
+    assertThatThrownBy(() -> underTest.authenticate(dbSession, user, "WHATEVER", AuthenticationEvent.Method.BASIC))
+      .isInstanceOf(AuthenticationException.class)
+      .hasMessage(CredentialsLocalAuthentication.ERROR_NULL_PASSWORD_IN_DB);
   }
 
   @Test
@@ -273,34 +285,39 @@ public class CredentialsLocalAuthenticationTest {
 
   @Test
   public void authentication_with_pbkdf2_with_incorrect_password_should_throw_AuthenticationException() {
+    DbSession dbSession = db.getSession();
     UserDto user = newUserDto()
       .setHashMethod(PBKDF2.name())
       .setCryptedPassword("1$hash")
       .setSalt("salt");
 
-    expectedException.expect(AuthenticationException.class);
-    expectedException.expectMessage("wrong password");
-
-    underTest.authenticate(db.getSession(), user, "WHATEVER", AuthenticationEvent.Method.BASIC);
+    assertThatThrownBy(() -> underTest.authenticate(dbSession, user, "WHATEVER", AuthenticationEvent.Method.BASIC))
+      .isInstanceOf(AuthenticationException.class)
+      .hasMessage(CredentialsLocalAuthentication.ERROR_WRONG_PASSWORD);
   }
 
   @Test
   public void authentication_with_pbkdf2_with_invalid_password_should_throw_AuthenticationException() {
+    DbSession dbSession = db.getSession();
     String password = randomAlphanumeric(60);
 
     byte[] saltRandom = new byte[20];
     RANDOM.nextBytes(saltRandom);
     String salt = DigestUtils.sha1Hex(saltRandom);
 
-    UserDto user = newUserDto()
+    UserDto userInvalidHash = newUserDto()
       .setHashMethod(PBKDF2.name())
       .setCryptedPassword(DigestUtils.sha1Hex("--" + salt + "--" + password + "--"))
       .setSalt(salt);
 
-    expectedException.expect(AuthenticationException.class);
-    expectedException.expectMessage("invalid hash stored");
+    UserDto userInvalidIterations = newUserDto()
+      .setHashMethod(PBKDF2.name())
+      .setCryptedPassword("$$")
+      .setSalt(salt);
 
-    underTest.authenticate(db.getSession(), user, "WHATEVER", AuthenticationEvent.Method.BASIC);
+    assertThatThrownBy(() -> underTest.authenticate(dbSession, userInvalidIterations, "WHATEVER", AuthenticationEvent.Method.BASIC))
+      .isInstanceOf(AuthenticationException.class)
+      .hasMessage("invalid hash stored");
   }
 
   @Test
@@ -308,30 +325,30 @@ public class CredentialsLocalAuthenticationTest {
     byte[] saltRandom = new byte[20];
     RANDOM.nextBytes(saltRandom);
     String salt = DigestUtils.sha1Hex(saltRandom);
+    DbSession dbSession = db.getSession();
 
     UserDto user = newUserDto()
       .setCryptedPassword(null)
       .setHashMethod(PBKDF2.name())
       .setSalt(salt);
 
-    expectedException.expect(AuthenticationException.class);
-    expectedException.expectMessage("null password in DB");
-
-    underTest.authenticate(db.getSession(), user, "WHATEVER", AuthenticationEvent.Method.BASIC);
+    assertThatThrownBy(() -> underTest.authenticate(dbSession, user, "WHATEVER", AuthenticationEvent.Method.BASIC))
+      .isInstanceOf(AuthenticationException.class)
+      .hasMessage(CredentialsLocalAuthentication.ERROR_NULL_PASSWORD_IN_DB);
   }
 
   @Test
   public void authentication_with_pbkdf2_with_empty_salt_should_throw_AuthenticationException() {
     String password = randomAlphanumeric(60);
+    DbSession dbSession = db.getSession();
 
     UserDto user = newUserDto()
       .setHashMethod(PBKDF2.name())
       .setCryptedPassword(DigestUtils.sha1Hex("--0242b0b4c0a93ddfe09dd886de50bc25ba000b51--" + password + "--"))
       .setSalt(null);
 
-    expectedException.expect(AuthenticationException.class);
-    expectedException.expectMessage("null salt");
-
-    underTest.authenticate(db.getSession(), user, "WHATEVER", AuthenticationEvent.Method.BASIC);
+    assertThatThrownBy(() -> underTest.authenticate(dbSession, user, "WHATEVER", AuthenticationEvent.Method.BASIC))
+      .isInstanceOf(AuthenticationException.class)
+      .hasMessage(CredentialsLocalAuthentication.ERROR_NULL_SALT);
   }
 }