review

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);
   }

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);
   }

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<User> 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()));
       }
     };
   }

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))
+      .url(TestData.getUserUrl(newUser))
-      .usernameAndPassword(TestData.USER_SCM_ADMIN, TestData.USER_SCM_ADMIN)
+      .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))
+      .url(TestData.getUserUrl(newUser))
-      .usernameAndPassword(TestData.USER_SCM_ADMIN, newPassword)
+      .usernameAndPassword(newUser, newPassword)
       .getUserResource()
       .assertStatusCode(200)
       .usingUserResponse()
-      .assertAdmin(aBoolean -> assertThat(aBoolean).isEqualTo(Boolean.TRUE))
+      .assertAdmin(isAdmin -> assertThat(isAdmin).isEqualTo(Boolean.TRUE))
-      .assertPassword(Assert::assertNull)
+      .assertPassword(Assert::assertNull);
-      .requestChangePassword(TestData.USER_SCM_ADMIN)
-      .assertStatusCode(204);
 
   }
 

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<InvalidPasswordException> {
   @Override
   public Response toResponse(InvalidPasswordException exception) {
-    return Response.status(Response.Status.UNAUTHORIZED)
+    return Response.status(Response.Status.BAD_REQUEST)
       .entity(exception.getMessage())
       .build();
   }

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<User> 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);
       }
     };
   }

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());

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);
-  }
 }

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) {
+    MeLinks(ScmPathInfo pathInfo, UserLinks user) {
-      meLinkBuilder = new LinkBuilder(uriInfo, MeResource.class);
+      meLinkBuilder = new LinkBuilder(pathInfo, MeResource.class);
       userLinks = user;
     }
 

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.
+   * </p>
    * 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
+   * @param usedPassword the password to be set
-   * @return the password to be mapped
+   * @param user         the target
    */
-  @Named("getUsedPassword")
+  @AfterMapping
-  String getUsedPassword(String password, @Context String usedPassword) {
+  void overridePassword(@MappingTarget User user, @Context String usedPassword) {
-    return usedPassword;
+    user.setPassword(usedPassword);
   }
+
 }

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"),

scm-webapp/src/main/java/sonia/scm/api/v2/resources/UserToUserDtoMapper.java

@@ -31,11 +31,6 @@ public abstract class UserToUserDtoMapper extends BaseMapper<User, UserDto> {
   @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()));

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());
   }