From e49184b8add39647359ac9776ec85b38ed4a71d5 Mon Sep 17 00:00:00 2001 From: Dimitris Kavvathas Date: Tue, 28 Feb 2023 11:43:10 +0100 Subject: [PATCH] SONAR-18372 Return 400 Bad Request with message on changePasswordAction when user input is incorrect. --- .../server/user/ws/ChangePasswordAction.java | 87 +++++++++++-- .../user/ws/ChangePasswordActionTest.java | 118 +++++++++++------- 2 files changed, 152 insertions(+), 53 deletions(-) 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 376832647ba..c437c68568e 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 @@ -19,15 +19,21 @@ */ package org.sonar.server.user.ws; -import java.io.IOException; +import com.google.gson.Gson; +import com.google.gson.GsonBuilder; +import com.google.gson.stream.JsonWriter; +import java.io.OutputStream; +import java.io.OutputStreamWriter; +import java.util.Optional; import javax.servlet.FilterChain; -import javax.servlet.ServletException; import javax.servlet.ServletRequest; import javax.servlet.ServletResponse; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import org.sonar.api.server.ws.Change; import org.sonar.api.server.ws.WebService; +import org.sonar.api.utils.log.Logger; +import org.sonar.api.utils.log.Loggers; import org.sonar.api.web.ServletFilter; import org.sonar.db.DbClient; import org.sonar.db.DbSession; @@ -46,7 +52,10 @@ import org.sonar.server.ws.ServletFilterHandler; import static java.lang.String.format; import static java.net.HttpURLConnection.HTTP_BAD_REQUEST; import static java.net.HttpURLConnection.HTTP_NO_CONTENT; -import static org.sonarqube.ws.WsUtils.checkArgument; +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.sonar.server.user.ws.ChangePasswordAction.PasswordMessage.NEW_PASSWORD_SAME_AS_OLD; +import static org.sonar.server.user.ws.ChangePasswordAction.PasswordMessage.OLD_PASSWORD_INCORRECT; +import static org.sonarqube.ws.MediaTypes.JSON; import static org.sonarqube.ws.WsUtils.isNullOrEmpty; import static org.sonarqube.ws.client.user.UsersWsParameters.PARAM_LOGIN; import static org.sonarqube.ws.client.user.UsersWsParameters.PARAM_PASSWORD; @@ -54,6 +63,8 @@ import static org.sonarqube.ws.client.user.UsersWsParameters.PARAM_PREVIOUS_PASS public class ChangePasswordAction extends ServletFilter implements BaseUsersWsAction { + private static final Logger LOG = Loggers.get(ChangePasswordAction.class); + private static final String CHANGE_PASSWORD = "change_password"; private static final String CHANGE_PASSWORD_URL = "/" + UsersWs.API_USERS + "/" + CHANGE_PASSWORD; private static final String MSG_PARAMETER_MISSING = "The '%s' parameter is missing"; @@ -106,7 +117,7 @@ public class ChangePasswordAction extends ServletFilter implements BaseUsersWsAc } @Override - public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException { + public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) { userSession.checkLoggedIn(); try (DbSession dbSession = dbClient.openSession(false)) { String login = getParamOrThrow(request, PARAM_LOGIN); @@ -117,7 +128,7 @@ public class ChangePasswordAction extends ServletFilter implements BaseUsersWsAc user = getUserOrThrow(dbSession, login); String previousPassword = getParamOrThrow(request, PARAM_PREVIOUS_PASSWORD); checkPreviousPassword(dbSession, user, previousPassword); - checkArgument(!previousPassword.equals(newPassword), "Password must be different from old password"); + checkNewPasswordSameAsOld(newPassword, previousPassword); deleteTokensAndRefreshSession(request, response, dbSession, user); } else { userSession.checkIsSystemAdministrator(); @@ -128,20 +139,34 @@ public class ChangePasswordAction extends ServletFilter implements BaseUsersWsAc setResponseStatus(response, HTTP_NO_CONTENT); } catch (BadRequestException badRequestException) { setResponseStatus(response, HTTP_BAD_REQUEST); + LOG.debug(badRequestException.getMessage(), badRequestException); + } catch (PasswordException passwordException) { + LOG.debug(passwordException.getMessage(), passwordException); + setResponseStatus(response, HTTP_BAD_REQUEST); + String message = passwordException.getPasswordMessage().map(pm -> pm.key).orElseGet(passwordException::getMessage); + writeJsonResponse(message, response); } } - private static String getParamOrThrow(ServletRequest request, String key) { + private static String getParamOrThrow(ServletRequest request, String key) throws PasswordException { String value = request.getParameter(key); - checkArgument(!isNullOrEmpty(value), MSG_PARAMETER_MISSING, key); + if (isNullOrEmpty(value)) { + throw new PasswordException(format(MSG_PARAMETER_MISSING, key)); + } return value; } - private void checkPreviousPassword(DbSession dbSession, UserDto user, String password) { + private void checkPreviousPassword(DbSession dbSession, UserDto user, String password) throws PasswordException { try { localAuthentication.authenticate(dbSession, user, password, AuthenticationEvent.Method.BASIC); } catch (AuthenticationException ex) { - throw new IllegalArgumentException("Incorrect password"); + throw new PasswordException(OLD_PASSWORD_INCORRECT, "Incorrect password"); + } + } + + private static void checkNewPasswordSameAsOld(String newPassword, String previousPassword) throws PasswordException { + if (previousPassword.equals(newPassword)) { + throw new PasswordException(NEW_PASSWORD_SAME_AS_OLD, "Password must be different from old password"); } } @@ -174,4 +199,48 @@ public class ChangePasswordAction extends ServletFilter implements BaseUsersWsAc private static void setResponseStatus(ServletResponse response, int newStatusCode) { ((HttpServletResponse) response).setStatus(newStatusCode); } + + private static void writeJsonResponse(String msg, ServletResponse response) { + Gson gson = new GsonBuilder() + .disableHtmlEscaping() + .create(); + try (OutputStream output = response.getOutputStream(); + JsonWriter writer = gson.newJsonWriter(new OutputStreamWriter(output, UTF_8))) { + response.setContentType(JSON); + writer.beginObject() + .name("result").value(msg); + writer.endObject(); + } catch (Exception e) { + throw new IllegalStateException("Error while writing message", e); + } + } + + enum PasswordMessage { + OLD_PASSWORD_INCORRECT("old_password_incorrect"), + NEW_PASSWORD_SAME_AS_OLD("new_password_same_as_old"); + + final String key; + + PasswordMessage(String key) { + this.key = key; + } + } + + private static class PasswordException extends Exception { + private final PasswordMessage passwordMessage; + public PasswordException(PasswordMessage passwordMessage, String message) { + super(message); + this.passwordMessage = passwordMessage; + } + + public PasswordException(String message) { + super(message); + this.passwordMessage = null; + } + + public Optional getPasswordMessage() { + return Optional.ofNullable(passwordMessage); + } + } + } diff --git a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/user/ws/ChangePasswordActionTest.java b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/user/ws/ChangePasswordActionTest.java index 9de0426be61..6a6b9842356 100644 --- a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/user/ws/ChangePasswordActionTest.java +++ b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/user/ws/ChangePasswordActionTest.java @@ -23,7 +23,8 @@ import java.io.IOException; import java.util.Optional; import javax.annotation.Nullable; import javax.servlet.FilterChain; -import javax.servlet.ServletException; +import javax.servlet.ServletOutputStream; +import javax.servlet.WriteListener; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import org.junit.Before; @@ -56,6 +57,7 @@ import static java.net.HttpURLConnection.HTTP_NO_CONTENT; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; @@ -92,17 +94,20 @@ public class ChangePasswordActionTest { private final JwtHttpHandler jwtHttpHandler = mock(JwtHttpHandler.class); private final ChangePasswordAction changePasswordAction = new ChangePasswordAction(db.getDbClient(), userUpdater, userSessionRule, localAuthentication, jwtHttpHandler); + private ServletOutputStream responseOutputStream; @Before - public void setUp() { + public void setUp() throws IOException { db.users().insertDefaultGroup(); + responseOutputStream = new StringOutputStream(); + doReturn(responseOutputStream).when(response).getOutputStream(); } @Test - public void a_user_can_update_his_password() throws ServletException, IOException { + public void a_user_can_update_his_password() { UserTestData user = createLocalUser(OLD_PASSWORD); String oldCryptedPassword = findEncryptedPassword(user.getLogin()); - userSessionRule.logIn(user.getUserDto()); + userSessionRule.logIn(user.userDto()); executeTest(user.getLogin(), OLD_PASSWORD, NEW_PASSWORD); @@ -116,9 +121,9 @@ public class ChangePasswordActionTest { } @Test - public void system_administrator_can_update_password_of_user() throws ServletException, IOException { + public void system_administrator_can_update_password_of_user() { UserTestData admin = createLocalUser(); - userSessionRule.logIn(admin.getUserDto()).setSystemAdministrator(); + userSessionRule.logIn(admin.userDto()).setSystemAdministrator(); UserTestData user = createLocalUser(); String originalPassword = findEncryptedPassword(user.getLogin()); db.commit(); @@ -142,7 +147,7 @@ public class ChangePasswordActionTest { } @Test - public void fail_to_update_someone_else_password_if_not_admin() throws ServletException, IOException { + public void fail_to_update_someone_else_password_if_not_admin() { UserTestData user = createLocalUser(); userSessionRule.logIn(user.getLogin()); UserTestData anotherLocalUser = createLocalUser(); @@ -155,7 +160,7 @@ public class ChangePasswordActionTest { } @Test - public void fail_to_update_someone_else_password_if_not_admin_and_user_doesnt_exist() throws ServletException, IOException { + public void fail_to_update_someone_else_password_if_not_admin_and_user_doesnt_exist() { UserTestData user = createLocalUser(); userSessionRule.logIn(user.getLogin()); @@ -170,7 +175,7 @@ public class ChangePasswordActionTest { @Test public void fail_to_update_unknown_user() { UserTestData admin = createLocalUser(); - userSessionRule.logIn(admin.getUserDto()).setSystemAdministrator(); + userSessionRule.logIn(admin.userDto()).setSystemAdministrator(); assertThatThrownBy(() -> executeTest("polop", null, "polop")) .isInstanceOf(NotFoundException.class) @@ -195,20 +200,20 @@ public class ChangePasswordActionTest { when(request.getParameter(PARAM_PASSWORD)).thenReturn("new password"); when(request.getParameter(PARAM_PREVIOUS_PASSWORD)).thenReturn(NEW_PASSWORD); - assertThatThrownBy(() -> executeTest(null, OLD_PASSWORD, NEW_PASSWORD)) - .isInstanceOf(IllegalArgumentException.class) - .hasMessage("The 'login' parameter is missing"); + executeTest(null, OLD_PASSWORD, NEW_PASSWORD); + verify(response).setStatus(HTTP_BAD_REQUEST); + assertThat(responseOutputStream).hasToString("{\"result\":\"The 'login' parameter is missing\"}"); verifyNoInteractions(jwtHttpHandler); } @Test public void fail_to_update_password_on_self_without_old_password() { UserTestData user = createLocalUser(); - userSessionRule.logIn(user.getUserDto()); + userSessionRule.logIn(user.userDto()); - assertThatThrownBy(() -> executeTest(user.getLogin(), null, NEW_PASSWORD)) - .isInstanceOf(IllegalArgumentException.class) - .hasMessage("The 'previousPassword' parameter is missing"); + executeTest(user.getLogin(), null, NEW_PASSWORD); + verify(response).setStatus(HTTP_BAD_REQUEST); + assertThat(responseOutputStream).hasToString("{\"result\":\"The 'previousPassword' parameter is missing\"}"); assertThat(findSessionTokenDto(db.getSession(), user.getSessionTokenUuid())).isPresent(); verifyNoInteractions(jwtHttpHandler); } @@ -216,11 +221,13 @@ public class ChangePasswordActionTest { @Test public void fail_to_update_password_on_self_without_new_password() { UserTestData user = createLocalUser(); - userSessionRule.logIn(user.getUserDto()); + userSessionRule.logIn(user.userDto()); + - assertThatThrownBy(() -> executeTest(user.getLogin(), OLD_PASSWORD, null)) - .isInstanceOf(IllegalArgumentException.class) - .hasMessage("The 'password' parameter is missing"); + executeTest(user.getLogin(), OLD_PASSWORD, null); + verify(response).setStatus(HTTP_BAD_REQUEST); + assertThat(responseOutputStream).hasToString("{\"result\":\"The 'password' parameter is missing\"}"); + assertThat(findSessionTokenDto(db.getSession(), user.getSessionTokenUuid())).isPresent(); assertThat(findSessionTokenDto(db.getSession(), user.getSessionTokenUuid())).isPresent(); verifyNoInteractions(jwtHttpHandler); } @@ -228,17 +235,17 @@ public class ChangePasswordActionTest { @Test public void fail_to_update_password_on_self_with_bad_old_password() { UserTestData user = createLocalUser(); - userSessionRule.logIn(user.getUserDto()); + userSessionRule.logIn(user.userDto()); - assertThatThrownBy(() -> executeTest(user.getLogin(), "I dunno", NEW_PASSWORD)) - .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Incorrect password"); + executeTest(user.getLogin(), "I dunno", NEW_PASSWORD); + verify(response).setStatus(HTTP_BAD_REQUEST); + assertThat(responseOutputStream).hasToString("{\"result\":\"old_password_incorrect\"}"); assertThat(findSessionTokenDto(db.getSession(), user.getSessionTokenUuid())).isPresent(); verifyNoInteractions(jwtHttpHandler); } @Test - public void fail_to_update_password_on_external_auth() throws ServletException, IOException { + public void fail_to_update_password_on_external_auth() { UserDto admin = db.users().insertUser(); userSessionRule.logIn(admin).setSystemAdministrator(); UserDto user = db.users().insertUser(u -> u.setLocal(false)); @@ -250,11 +257,11 @@ public class ChangePasswordActionTest { @Test public void fail_to_update_to_same_password() { UserTestData user = createLocalUser(OLD_PASSWORD); - userSessionRule.logIn(user.getUserDto()); + userSessionRule.logIn(user.userDto()); - assertThatThrownBy(() -> executeTest(user.getLogin(), OLD_PASSWORD, OLD_PASSWORD)) - .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Password must be different from old password"); + executeTest(user.getLogin(), OLD_PASSWORD, OLD_PASSWORD); + verify(response).setStatus(HTTP_BAD_REQUEST); + assertThat(responseOutputStream).hasToString("{\"result\":\"new_password_same_as_old\"}"); assertThat(findSessionTokenDto(db.getSession(), user.getSessionTokenUuid())).isPresent(); verifyNoInteractions(jwtHttpHandler); } @@ -277,7 +284,7 @@ public class ChangePasswordActionTest { .containsExactlyInAnyOrder(PARAM_LOGIN, PARAM_PASSWORD, PARAM_PREVIOUS_PASSWORD); } - private void executeTest(@Nullable String login, @Nullable String oldPassword, @Nullable String newPassword) throws IOException, ServletException { + private void executeTest(@Nullable String login, @Nullable String oldPassword, @Nullable String newPassword) { when(request.getParameter(PARAM_LOGIN)).thenReturn(login); when(request.getParameter(PARAM_PREVIOUS_PASSWORD)).thenReturn(oldPassword); when(request.getParameter(PARAM_PASSWORD)).thenReturn(newPassword); @@ -286,8 +293,8 @@ public class ChangePasswordActionTest { private UserTestData createLocalUser(String password) { UserTestData userTestData = createLocalUser(); - localAuthentication.storeHashPassword(userTestData.getUserDto(), password); - db.getDbClient().userDao().update(db.getSession(), userTestData.getUserDto()); + localAuthentication.storeHashPassword(userTestData.userDto(), password); + db.getDbClient().userDao().update(db.getSession(), userTestData.userDto()); db.commit(); return userTestData; } @@ -304,18 +311,7 @@ public class ChangePasswordActionTest { return db.getDbClient().sessionTokensDao().insert(db.getSession(), userTokenDto); } - private static class UserTestData { - private final UserDto userDto; - private final SessionTokenDto sessionTokenDto; - - private UserTestData(UserDto userDto, SessionTokenDto sessionTokenDto) { - this.userDto = userDto; - this.sessionTokenDto = sessionTokenDto; - } - - UserDto getUserDto() { - return userDto; - } + private record UserTestData(UserDto userDto, SessionTokenDto sessionTokenDto) { String getLogin() { return userDto.getLogin(); @@ -326,4 +322,38 @@ public class ChangePasswordActionTest { } } + static class StringOutputStream extends ServletOutputStream { + private final StringBuilder buf = new StringBuilder(); + + StringOutputStream() { + } + + @Override + public boolean isReady() { + return false; + } + + @Override + public void setWriteListener(WriteListener listener) { + + } + + public void write(byte[] b) { + this.buf.append(new String(b)); + } + + public void write(byte[] b, int off, int len) { + this.buf.append(new String(b, off, len)); + } + + public void write(int b) { + byte[] bytes = new byte[] {(byte) b}; + this.buf.append(new String(bytes)); + } + + public String toString() { + return this.buf.toString(); + } + } + } -- 2.39.5