From: Simon Brandhof Date: Mon, 10 Dec 2018 13:31:04 +0000 (+0100) Subject: SONARCLOUD-220 refactor the class Credentials X-Git-Tag: 7.5~19 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=c03ee773c65f63da493dcc299b194b61f7b29b28;p=sonarqube.git SONARCLOUD-220 refactor the class Credentials --- 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 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