SONAR-23021 Enforce password complexity in the backend

2 files changed, 74 insertions, 20 deletions

diff --git a/server/sonar-webserver-webapi/src/it/java/org/sonar/server/user/ws/ChangePasswordActionIT.java b/server/sonar-webserver-webapi/src/it/java/org/sonar/server/user/ws/ChangePasswordActionIT.java
index 5650b35f9b0..773b5022b17 100644
--- a/server/sonar-webserver-webapi/src/it/java/org/sonar/server/user/ws/ChangePasswordActionIT.java
+++ b/server/sonar-webserver-webapi/src/it/java/org/sonar/server/user/ws/ChangePasswordActionIT.java
@@ -19,14 +19,18 @@
  */
 package org.sonar.server.user.ws;
 
+import jakarta.servlet.ServletOutputStream;
+import jakarta.servlet.WriteListener;
 import java.io.IOException;
 import java.util.Optional;
+import java.util.stream.Stream;
 import javax.annotation.Nullable;
-import jakarta.servlet.ServletOutputStream;
-import jakarta.servlet.WriteListener;
-import org.junit.Before;
-import org.junit.Rule;
-import org.junit.Test;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
 import org.mockito.ArgumentCaptor;
 import org.sonar.api.config.internal.MapSettings;
 import org.sonar.api.server.http.HttpRequest;
@@ -68,13 +72,14 @@ import static org.sonarqube.ws.client.user.UsersWsParameters.PARAM_PREVIOUS_PASS
 
 public class ChangePasswordActionIT {
 
-  private static final String OLD_PASSWORD = "1234";
-  private static final String NEW_PASSWORD = "12345";
+  private static final String OLD_PASSWORD = "1234567890_aA";
+  private static final String NEW_PASSWORD = "1234567890_bB";
 
-  @Rule
-  public DbTester db = DbTester.create();
-  @Rule
-  public UserSessionRule userSessionRule = UserSessionRule.standalone().logIn();
+  @RegisterExtension
+  private final DbTester db = DbTester.create();
+
+  @RegisterExtension
+  private final UserSessionRule userSessionRule = UserSessionRule.standalone().logIn();
 
   private final ArgumentCaptor<UserDto> userDtoCaptor = ArgumentCaptor.forClass(UserDto.class);
 
@@ -93,10 +98,11 @@ public class ChangePasswordActionIT {
 
   private final ManagedInstanceChecker managedInstanceChecker = mock(ManagedInstanceChecker.class);
 
-  private final ChangePasswordAction underTest = new ChangePasswordAction(db.getDbClient(), userUpdater, userSessionRule, localAuthentication, jwtHttpHandler, managedInstanceChecker);
+  private final ChangePasswordAction underTest = new ChangePasswordAction(db.getDbClient(), userUpdater, userSessionRule, localAuthentication, jwtHttpHandler,
+    managedInstanceChecker);
   private ServletOutputStream responseOutputStream;
 
-  @Before
+  @BeforeEach
   public void setUp() throws IOException {
     db.users().insertDefaultGroup();
     responseOutputStream = new StringOutputStream();
@@ -178,7 +184,7 @@ public class ChangePasswordActionIT {
     UserTestData admin = createLocalUser();
     userSessionRule.logIn(admin.userDto()).setSystemAdministrator();
 
-    assertThatThrownBy(() -> executeTest("polop", null, "polop"))
+    assertThatThrownBy(() -> executeTest("polop", null, NEW_PASSWORD))
       .isInstanceOf(NotFoundException.class)
       .hasMessage("User with login 'polop' has not been found");
     assertThat(findSessionTokenDto(db.getSession(), admin.getSessionTokenUuid())).isPresent();
@@ -191,7 +197,7 @@ public class ChangePasswordActionIT {
     userSessionRule.logIn(user);
 
     String userLogin = user.getLogin();
-    assertThatThrownBy(() -> executeTest(userLogin, null, "polop"))
+    assertThatThrownBy(() -> executeTest(userLogin, null, NEW_PASSWORD))
       .isInstanceOf(NotFoundException.class)
       .hasMessage(format("User with login '%s' has not been found", userLogin));
     verifyNoInteractions(jwtHttpHandler);
@@ -268,6 +274,28 @@ public class ChangePasswordActionIT {
     verifyNoInteractions(jwtHttpHandler);
   }
 
+  public static Stream<Arguments> invalidPasswords() {
+    return Stream.of(
+      Arguments.of("12345678901", "Password must be at least 12 characters long"),
+      Arguments.of("123456789012", "Password must contain at least one uppercase character"),
+      Arguments.of("12345678901A", "Password must contain at least one lowercase character"),
+      Arguments.of("1234567890aA", "Password must contain at least one special character"),
+      Arguments.of("abcdefghiaA%", "Password must contain at least one digit")
+    );
+  }
+
+  @ParameterizedTest
+  @MethodSource("invalidPasswords")
+  void changePassword_whenPasswordDoesNotMatchSecurityRequirements_shouldThrowWithExpectedMessage(String newPassword, String expectedMessage) {
+    UserTestData user = createLocalUser(OLD_PASSWORD);
+    userSessionRule.logIn(user.userDto());
+
+    executeTest(user.getLogin(), OLD_PASSWORD, newPassword);
+
+    verify(response).setStatus(HTTP_BAD_REQUEST);
+    assertThat(responseOutputStream).hasToString("{\"result\":\"" + expectedMessage + "\"}");
+  }
+
   @Test
   public void changePassword_whenInstanceIsManagedAndUserUpdate_shouldThrow() {
     doThrow(BadRequestException.create("Operation not allowed when the instance is externally managed.")).when(managedInstanceChecker).throwIfInstanceIsManaged();
diff --git a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/user/ws/ChangePasswordAction.java b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/user/ws/ChangePasswordAction.java
index 2ab8ac9ccf2..0623d7e8a7e 100644
--- a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/user/ws/ChangePasswordAction.java
+++ b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/user/ws/ChangePasswordAction.java
@@ -25,6 +25,7 @@ import com.google.gson.stream.JsonWriter;
 import java.io.OutputStream;
 import java.io.OutputStreamWriter;
 import java.util.Optional;
+import java.util.regex.Pattern;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.sonar.api.server.http.HttpRequest;
@@ -49,6 +50,7 @@ import org.sonar.server.user.UserSession;
 import org.sonar.server.user.UserUpdater;
 import org.sonar.server.ws.ServletFilterHandler;
 
+import static com.google.common.base.Preconditions.checkArgument;
 import static java.lang.String.format;
 import static java.net.HttpURLConnection.HTTP_BAD_REQUEST;
 import static java.net.HttpURLConnection.HTTP_NO_CONTENT;
@@ -69,6 +71,13 @@ public class ChangePasswordAction extends HttpFilter implements BaseUsersWsActio
   private static final String CHANGE_PASSWORD_URL = "/" + UsersWs.API_USERS + "/" + CHANGE_PASSWORD;
   private static final String MSG_PARAMETER_MISSING = "The '%s' parameter is missing";
 
+  private static final Pattern UPPERCASE_PATTERN = Pattern.compile("[A-Z]");
+  private static final Pattern LOWERCASE_PATTERN = Pattern.compile("[a-z]");
+  private static final Pattern DIGIT_PATTERN = Pattern.compile("\\d");
+  private static final Pattern SPECIAL_CHARACTER_PATTERN = Pattern.compile("[^a-zA-Z0-9]");
+
+  private static final int MIN_PASSWORD_LENGTH = 12;
+
   private final DbClient dbClient;
   private final UserUpdater userUpdater;
   private final UserSession userSession;
@@ -95,8 +104,8 @@ public class ChangePasswordAction extends HttpFilter implements BaseUsersWsActio
   public void define(WebService.NewController controller) {
     WebService.NewAction action = controller.createAction(CHANGE_PASSWORD)
       .setDescription("Update a user's password. Authenticated users can change their own password, " +
-        "provided that the account is not linked to an external authentication system. " +
-        "Administer System permission is required to change another user's password.")
+                      "provided that the account is not linked to an external authentication system. " +
+                      "Administer System permission is required to change another user's password.")
       .setSince("5.2")
       .setPost(true)
       .setHandler(ServletFilterHandler.INSTANCE)
@@ -110,12 +119,15 @@ public class ChangePasswordAction extends HttpFilter implements BaseUsersWsActio
     action.createParam(PARAM_PASSWORD)
       .setDescription("New password")
       .setRequired(true)
-      .setExampleValue("mypassword");
+      .setExampleValue("My_Passw0rd%")
+      .setMinimumLength(12)
+      .setDescription("The password needs to fulfill the following requirements: at least 12 characters " +
+                      "and contain at least one uppercase character, one lowercase character, one digit and one special character.");
 
     action.createParam(PARAM_PREVIOUS_PASSWORD)
       .setDescription("Previous password. Required when changing one's own password.")
       .setRequired(false)
-      .setExampleValue("oldpassword");
+      .setExampleValue("My_Previous_Passw0rd%");
   }
 
   @Override
@@ -124,6 +136,7 @@ public class ChangePasswordAction extends HttpFilter implements BaseUsersWsActio
     try (DbSession dbSession = dbClient.openSession(false)) {
       String login = getParamOrThrow(request, PARAM_LOGIN);
       String newPassword = getParamOrThrow(request, PARAM_PASSWORD);
+      assertPasswordFormatIsValid(newPassword);
       UserDto user;
 
       if (login.equals(userSession.getLogin())) {
@@ -154,6 +167,18 @@ public class ChangePasswordAction extends HttpFilter implements BaseUsersWsActio
     }
   }
 
+  private static void assertPasswordFormatIsValid(String newPassword) throws PasswordException {
+    try {
+      checkArgument(newPassword.length() >= MIN_PASSWORD_LENGTH, "Password must be at least %s characters long", MIN_PASSWORD_LENGTH);
+      checkArgument(UPPERCASE_PATTERN.matcher(newPassword).find(), "Password must contain at least one uppercase character");
+      checkArgument(LOWERCASE_PATTERN.matcher(newPassword).find(), "Password must contain at least one lowercase character");
+      checkArgument(DIGIT_PATTERN.matcher(newPassword).find(), "Password must contain at least one digit");
+      checkArgument(SPECIAL_CHARACTER_PATTERN.matcher(newPassword).find(), "Password must contain at least one special character");
+    } catch (IllegalArgumentException e) {
+      throw new PasswordException(e.getMessage());
+    }
+  }
+
   private static String getParamOrThrow(HttpRequest request, String key) throws PasswordException {
     String value = request.getParameter(key);
     if (isNullOrEmpty(value)) {
@@ -210,7 +235,7 @@ public class ChangePasswordAction extends HttpFilter implements BaseUsersWsActio
       .create();
 
     try (OutputStream output = response.getOutputStream();
-         JsonWriter writer = gson.newJsonWriter(new OutputStreamWriter(output, UTF_8))) {
+      JsonWriter writer = gson.newJsonWriter(new OutputStreamWriter(output, UTF_8))) {
       response.setContentType(JSON);
       writer.beginObject()
         .name("result").value(msg);
@@ -233,6 +258,7 @@ public class ChangePasswordAction extends HttpFilter implements BaseUsersWsActio
 
   private static class PasswordException extends Exception {
     private final PasswordMessage passwordMessage;
+
     public PasswordException(PasswordMessage passwordMessage, String message) {
       super(message);
       this.passwordMessage = passwordMessage;