From b80e9666aa4f4b451e7bb8f4c7eaabffbd26ed02 Mon Sep 17 00:00:00 2001 From: Mohamed Karray Date: Thu, 20 Sep 2018 11:51:10 +0200 Subject: [PATCH] review --- .../ChangePasswordNotAllowedException.java | 2 ++ .../scm/user/InvalidPasswordException.java | 2 ++ .../main/java/sonia/scm/user/UserManager.java | 4 +++- .../test/java/sonia/scm/it/UserITCase.java | 21 ++++++++++--------- .../InvalidPasswordExceptionMapper.java | 2 +- .../scm/api/v2/resources/MeResource.java | 4 +++- .../api/v2/resources/MeToUserDtoMapper.java | 1 + .../api/v2/resources/PasswordChangeDto.java | 10 +-------- .../scm/api/v2/resources/ResourceLinks.java | 4 ++-- .../api/v2/resources/UserDtoToUserMapper.java | 18 +++++++++------- .../scm/api/v2/resources/UserResource.java | 2 +- .../api/v2/resources/UserToUserDtoMapper.java | 5 ----- .../scm/api/v2/resources/MeResourceTest.java | 5 ++--- 13 files changed, 40 insertions(+), 40 deletions(-) diff --git a/scm-core/src/main/java/sonia/scm/user/ChangePasswordNotAllowedException.java b/scm-core/src/main/java/sonia/scm/user/ChangePasswordNotAllowedException.java index b2925cf65c..49e4714e80 100644 --- a/scm-core/src/main/java/sonia/scm/user/ChangePasswordNotAllowedException.java +++ b/scm-core/src/main/java/sonia/scm/user/ChangePasswordNotAllowedException.java @@ -2,6 +2,8 @@ package sonia.scm.user; public class ChangePasswordNotAllowedException extends RuntimeException { + public static final String WRON_USER_TYPE = "User of type {0} are not allowed to change password"; + public ChangePasswordNotAllowedException(String message) { super(message); } diff --git a/scm-core/src/main/java/sonia/scm/user/InvalidPasswordException.java b/scm-core/src/main/java/sonia/scm/user/InvalidPasswordException.java index beda2c83fb..8d25e01a1e 100644 --- a/scm-core/src/main/java/sonia/scm/user/InvalidPasswordException.java +++ b/scm-core/src/main/java/sonia/scm/user/InvalidPasswordException.java @@ -2,6 +2,8 @@ package sonia.scm.user; public class InvalidPasswordException extends RuntimeException { + public static final String PASSWORD_NOT_MATCHED = "The given Password does not match with the stored one."; + public InvalidPasswordException(String message) { super(message); } diff --git a/scm-core/src/main/java/sonia/scm/user/UserManager.java b/scm-core/src/main/java/sonia/scm/user/UserManager.java index 90dbcd74b3..8b46fcc893 100644 --- a/scm-core/src/main/java/sonia/scm/user/UserManager.java +++ b/scm-core/src/main/java/sonia/scm/user/UserManager.java @@ -41,6 +41,8 @@ import sonia.scm.search.Searchable; import java.text.MessageFormat; import java.util.function.Consumer; +import static sonia.scm.user.ChangePasswordNotAllowedException.WRON_USER_TYPE; + /** * The central class for managing {@link User} objects. * This class is a singleton and is available via injection. @@ -79,7 +81,7 @@ public interface UserManager default Consumer getUserTypeChecker() { return user -> { if (!isTypeDefault(user)) { - throw new ChangePasswordNotAllowedException(MessageFormat.format("It is not possible to change password for User of type {0}", user.getType())); + throw new ChangePasswordNotAllowedException(MessageFormat.format(WRON_USER_TYPE, user.getType())); } }; } diff --git a/scm-it/src/test/java/sonia/scm/it/UserITCase.java b/scm-it/src/test/java/sonia/scm/it/UserITCase.java index 65ed6b71bb..67fe23dcbc 100644 --- a/scm-it/src/test/java/sonia/scm/it/UserITCase.java +++ b/scm-it/src/test/java/sonia/scm/it/UserITCase.java @@ -17,12 +17,15 @@ public class UserITCase { @Test public void adminShouldChangeOwnPassword() { - String newPassword = TestData.USER_SCM_ADMIN + "1"; + String newUser = "user"; + String password = "pass"; + TestData.createUser(newUser, password, true, "xml"); + String newPassword = "new_password"; // admin change the own password ScmRequests.start() .given() - .url(TestData.getUserUrl(TestData.USER_SCM_ADMIN)) - .usernameAndPassword(TestData.USER_SCM_ADMIN, TestData.USER_SCM_ADMIN) + .url(TestData.getUserUrl(newUser)) + .usernameAndPassword(newUser, password) .getUserResource() .assertStatusCode(200) .usingUserResponse() @@ -30,18 +33,16 @@ public class UserITCase { .assertPassword(Assert::assertNull) .requestChangePassword(newPassword) // the oldPassword is not needed in the user resource .assertStatusCode(204); - // assert password is changed -> login with the new Password than undo changes + // assert password is changed -> login with the new Password ScmRequests.start() .given() - .url(TestData.getUserUrl(TestData.USER_SCM_ADMIN)) - .usernameAndPassword(TestData.USER_SCM_ADMIN, newPassword) + .url(TestData.getUserUrl(newUser)) + .usernameAndPassword(newUser, newPassword) .getUserResource() .assertStatusCode(200) .usingUserResponse() - .assertAdmin(aBoolean -> assertThat(aBoolean).isEqualTo(Boolean.TRUE)) - .assertPassword(Assert::assertNull) - .requestChangePassword(TestData.USER_SCM_ADMIN) - .assertStatusCode(204); + .assertAdmin(isAdmin -> assertThat(isAdmin).isEqualTo(Boolean.TRUE)) + .assertPassword(Assert::assertNull); } diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/InvalidPasswordExceptionMapper.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/InvalidPasswordExceptionMapper.java index eda850d6ce..7c5364ba03 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/InvalidPasswordExceptionMapper.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/InvalidPasswordExceptionMapper.java @@ -10,7 +10,7 @@ import javax.ws.rs.ext.Provider; public class InvalidPasswordExceptionMapper implements ExceptionMapper { @Override public Response toResponse(InvalidPasswordException exception) { - return Response.status(Response.Status.UNAUTHORIZED) + return Response.status(Response.Status.BAD_REQUEST) .entity(exception.getMessage()) .build(); } diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/MeResource.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/MeResource.java index da9489a4bb..0100728220 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/MeResource.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/MeResource.java @@ -24,6 +24,8 @@ import javax.ws.rs.core.Response; import javax.ws.rs.core.UriInfo; import java.util.function.Consumer; +import static sonia.scm.user.InvalidPasswordException.PASSWORD_NOT_MATCHED; + /** * RESTful Web Service Resource to get currently logged in users. @@ -87,7 +89,7 @@ public class MeResource { private Consumer getOldOriginalPasswordChecker(String oldPassword) { return user -> { if (!user.getPassword().equals(passwordService.encryptPassword(oldPassword))) { - throw new InvalidPasswordException("The password is invalid"); + throw new InvalidPasswordException(PASSWORD_NOT_MATCHED); } }; } diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/MeToUserDtoMapper.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/MeToUserDtoMapper.java index a4586fab36..2a872eadd9 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/MeToUserDtoMapper.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/MeToUserDtoMapper.java @@ -23,6 +23,7 @@ public abstract class MeToUserDtoMapper extends UserToUserDtoMapper{ private ResourceLinks resourceLinks; + @Override @AfterMapping protected void appendLinks(User user, @MappingTarget UserDto target) { Links.Builder linksBuilder = linkingTo().self(resourceLinks.me().self()); diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/PasswordChangeDto.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/PasswordChangeDto.java index 2db96ccdf1..47ad6cd147 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/PasswordChangeDto.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/PasswordChangeDto.java @@ -1,7 +1,5 @@ package sonia.scm.api.v2.resources; -import de.otto.edison.hal.HalRepresentation; -import de.otto.edison.hal.Links; import lombok.Getter; import lombok.Setter; import lombok.ToString; @@ -10,16 +8,10 @@ import org.hibernate.validator.constraints.NotEmpty; @Getter @Setter @ToString -public class PasswordChangeDto extends HalRepresentation { +public class PasswordChangeDto { private String oldPassword; @NotEmpty private String newPassword; - - @Override - @SuppressWarnings("squid:S1185") // We want to have this method available in this package - protected HalRepresentation add(Links links) { - return super.add(links); - } } diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/ResourceLinks.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/ResourceLinks.java index 1301a5bb94..e978de443a 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/ResourceLinks.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/ResourceLinks.java @@ -100,8 +100,8 @@ class ResourceLinks { private final LinkBuilder meLinkBuilder; private UserLinks userLinks; - MeLinks(ScmPathInfo uriInfo, UserLinks user) { - meLinkBuilder = new LinkBuilder(uriInfo, MeResource.class); + MeLinks(ScmPathInfo pathInfo, UserLinks user) { + meLinkBuilder = new LinkBuilder(pathInfo, MeResource.class); userLinks = user; } diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/UserDtoToUserMapper.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/UserDtoToUserMapper.java index 1ea07e4be7..e6608f0804 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/UserDtoToUserMapper.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/UserDtoToUserMapper.java @@ -1,9 +1,10 @@ package sonia.scm.api.v2.resources; +import org.mapstruct.AfterMapping; import org.mapstruct.Context; import org.mapstruct.Mapper; import org.mapstruct.Mapping; -import org.mapstruct.Named; +import org.mapstruct.MappingTarget; import sonia.scm.user.User; // Mapstruct does not support parameterized (i.e. non-default) constructors. Thus, we need to use field injection. @@ -11,21 +12,24 @@ import sonia.scm.user.User; @Mapper public abstract class UserDtoToUserMapper extends BaseDtoMapper { - @Mapping(source = "password", target = "password", qualifiedByName = "getUsedPassword") @Mapping(target = "creationDate", ignore = true) public abstract User map(UserDto userDto, @Context String usedPassword); /** * depends on the use case the right password will be mapped. + * The given Password in the context parameter will be set. + * The mapper consumer have the control of what password should be set. + *

* eg. for update user action the password will be set to the original password * for create user and change password actions the password is the user input * - * @param usedPassword the password to be mapped - * @return the password to be mapped + * @param usedPassword the password to be set + * @param user the target */ - @Named("getUsedPassword") - String getUsedPassword(String password, @Context String usedPassword) { - return usedPassword; + @AfterMapping + void overridePassword(@MappingTarget User user, @Context String usedPassword) { + user.setPassword(usedPassword); } + } diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/UserResource.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/UserResource.java index 714ca432ee..c0bb1e65ad 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/UserResource.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/UserResource.java @@ -120,7 +120,7 @@ public class UserResource { @Consumes(VndMediaType.PASSWORD_CHANGE) @StatusCodes({ @ResponseCode(code = 204, condition = "update success"), - @ResponseCode(code = 400, condition = "Invalid body, e.g. illegal change of id/user name"), + @ResponseCode(code = 400, condition = "Invalid body, e.g. the user type is not xml or the given oldPassword do not match the stored one"), @ResponseCode(code = 401, condition = "not authenticated / invalid credentials"), @ResponseCode(code = 403, condition = "not authorized, the current user does not have the \"user\" privilege"), @ResponseCode(code = 404, condition = "not found, no user with the specified id/name available"), diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/UserToUserDtoMapper.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/UserToUserDtoMapper.java index e2e79e2902..832829883b 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/UserToUserDtoMapper.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/UserToUserDtoMapper.java @@ -31,11 +31,6 @@ public abstract class UserToUserDtoMapper extends BaseMapper { @Inject private ResourceLinks resourceLinks; - @VisibleForTesting - void setResourceLinks(ResourceLinks resourceLinks) { - this.resourceLinks = resourceLinks; - } - @AfterMapping protected void appendLinks(User user, @MappingTarget UserDto target) { Links.Builder linksBuilder = linkingTo().self(resourceLinks.user().self(target.getName())); diff --git a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/MeResourceTest.java b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/MeResourceTest.java index 1dc5049fd9..3a5caa63eb 100644 --- a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/MeResourceTest.java +++ b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/MeResourceTest.java @@ -70,7 +70,6 @@ public class MeResourceTest { when(userManager.isTypeDefault(userCaptor.capture())).thenCallRealMethod(); when(userManager.getUserTypeChecker()).thenCallRealMethod(); when(userManager.getDefaultType()).thenReturn("xml"); - userToDtoMapper.setResourceLinks(resourceLinks); MeResource meResource = new MeResource(userToDtoMapper, userManager, passwordService); when(uriInfo.getApiRestUri()).thenReturn(URI.create("/")); when(scmPathInfoStore.get()).thenReturn(uriInfo); @@ -138,7 +137,7 @@ public class MeResourceTest { @Test @SubjectAware(username = "trillian", password = "secret") - public void shouldGet401OnChangePasswordIfOldPasswordDoesNotMatchOriginalPassword() throws Exception { + public void shouldGet400OnChangePasswordIfOldPasswordDoesNotMatchOriginalPassword() throws Exception { String newPassword = "pwd123"; String oldPassword = "notEncriptedSecret"; String content = String.format("{ \"oldPassword\": \"%s\" , \"newPassword\": \"%s\" }", oldPassword, newPassword); @@ -152,7 +151,7 @@ public class MeResourceTest { dispatcher.invoke(request, response); - assertEquals(HttpServletResponse.SC_UNAUTHORIZED, response.getStatus()); + assertEquals(HttpServletResponse.SC_BAD_REQUEST, response.getStatus()); }