From 023b362f6851c5255b5df4fbcf2bd22603f4a1d3 Mon Sep 17 00:00:00 2001 From: Mohamed Karray Date: Fri, 12 Oct 2018 15:20:58 +0200 Subject: [PATCH 1/8] add permission to modify the own password over the me and the user endpoints --- .../src/main/java/sonia/scm/HandlerBase.java | 14 ++++ scm-core/src/main/java/sonia/scm/Manager.java | 6 +- .../java/sonia/scm/NotFoundException.java | 2 +- .../src/main/java/sonia/scm/user/User.java | 2 +- .../main/java/sonia/scm/user/UserManager.java | 9 ++- .../sonia/scm/util/AuthenticationUtil.java | 12 +++ .../test/java/sonia/scm/it/UserITCase.java | 27 +++++++ .../rest/IllegalArgumentExceptionMapper.java | 2 +- .../resources/AbstractManagerResource.java | 30 +++---- .../resources/IdResourceManagerAdapter.java | 31 ++++++- .../scm/api/v2/resources/MeResource.java | 3 +- .../SingleResourceManagerAdapter.java | 17 ++-- .../scm/api/v2/resources/UserResource.java | 6 +- .../DefaultAuthorizationCollector.java | 5 ++ .../sonia/scm/user/DefaultUserManager.java | 10 ++- .../scm/api/v2/resources/DispatcherMock.java | 2 + .../scm/api/v2/resources/MeResourceTest.java | 19 +++-- .../v2/resources/UserRootResourceTest.java | 4 +- .../DefaultAuthorizationCollectorTest.java | 81 +++++++++---------- 19 files changed, 191 insertions(+), 91 deletions(-) create mode 100644 scm-core/src/main/java/sonia/scm/util/AuthenticationUtil.java diff --git a/scm-core/src/main/java/sonia/scm/HandlerBase.java b/scm-core/src/main/java/sonia/scm/HandlerBase.java index bbe78c8cf7..42c159a767 100644 --- a/scm-core/src/main/java/sonia/scm/HandlerBase.java +++ b/scm-core/src/main/java/sonia/scm/HandlerBase.java @@ -35,8 +35,11 @@ package sonia.scm; //~--- JDK imports ------------------------------------------------------------ +import com.github.sdorra.ssp.PermissionCheck; + import java.io.Closeable; import java.io.IOException; +import java.util.function.Function; /** * The base class of all handlers. @@ -75,4 +78,15 @@ public interface HandlerBase * @throws IOException */ void modify(T object) throws NotFoundException; + + /** + * Modifies a persistent object after checking with the given permission checker. + * + * @param object to modify + * @param permissionChecker check permission before to modify + * @throws IOException + */ + default void modify(T object, Function permissionChecker) throws NotFoundException{ + modify(object); + } } diff --git a/scm-core/src/main/java/sonia/scm/Manager.java b/scm-core/src/main/java/sonia/scm/Manager.java index 14c458d923..20b3a16a8e 100644 --- a/scm-core/src/main/java/sonia/scm/Manager.java +++ b/scm-core/src/main/java/sonia/scm/Manager.java @@ -99,14 +99,14 @@ public interface Manager * @param limit parameter * * @since 1.4 - * @return objects from the store which are starts at the given + * @return objects from the store which are starts at the given * start parameter */ Collection getAll(int start, int limit); /** * Returns objects from the store which are starts at the given start - * parameter sorted by the given {@link java.util.Comparator}. + * parameter sorted by the given {@link java.util.Comparator}. * The objects returned are limited by the limit parameter. * * @@ -115,7 +115,7 @@ public interface Manager * @param limit parameter * * @since 1.4 - * @return objects from the store which are starts at the given + * @return objects from the store which are starts at the given * start parameter */ Collection getAll(Comparator comparator, int start, int limit); diff --git a/scm-core/src/main/java/sonia/scm/NotFoundException.java b/scm-core/src/main/java/sonia/scm/NotFoundException.java index 8a7ae642bd..37546be0b8 100644 --- a/scm-core/src/main/java/sonia/scm/NotFoundException.java +++ b/scm-core/src/main/java/sonia/scm/NotFoundException.java @@ -1,6 +1,6 @@ package sonia.scm; -public class NotFoundException extends Exception { +public class NotFoundException extends RuntimeException { public NotFoundException(String type, String id) { super(type + " with id '" + id + "' not found"); } diff --git a/scm-core/src/main/java/sonia/scm/user/User.java b/scm-core/src/main/java/sonia/scm/user/User.java index e9dc277e5f..d9d2336387 100644 --- a/scm-core/src/main/java/sonia/scm/user/User.java +++ b/scm-core/src/main/java/sonia/scm/user/User.java @@ -56,7 +56,7 @@ import java.security.Principal; * * @author Sebastian Sdorra */ -@StaticPermissions(value = "user", globalPermissions = {"create", "list", "autocomplete"}) +@StaticPermissions(value = "user", globalPermissions = {"create", "list", "autocomplete", "changeOwnPassword"}) @XmlRootElement(name = "users") @XmlAccessorType(XmlAccessType.FIELD) public class User extends BasicPropertiesAware implements Principal, ModelObject, PermissionObject, ReducedModelObject 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 7829cd9974..f786b6ac41 100644 --- a/scm-core/src/main/java/sonia/scm/user/UserManager.java +++ b/scm-core/src/main/java/sonia/scm/user/UserManager.java @@ -77,10 +77,15 @@ public interface UserManager /** - * Only account of the default type "xml" can change their password + * Check if a user can modify the password + * + * 1 - the permission changeOwnPassword should be checked + * 2 - Only account of the default type "xml" can change their password + * */ - default Consumer getUserTypeChecker() { + default Consumer getChangePasswordChecker() { return user -> { + UserPermissions.changeOwnPassword().check(); if (!isTypeDefault(user)) { throw new ChangePasswordNotAllowedException(MessageFormat.format(WRONG_USER_TYPE, user.getType())); } diff --git a/scm-core/src/main/java/sonia/scm/util/AuthenticationUtil.java b/scm-core/src/main/java/sonia/scm/util/AuthenticationUtil.java new file mode 100644 index 0000000000..a0c021fa3c --- /dev/null +++ b/scm-core/src/main/java/sonia/scm/util/AuthenticationUtil.java @@ -0,0 +1,12 @@ +package sonia.scm.util; + +import org.apache.shiro.SecurityUtils; + +public class AuthenticationUtil { + + public static String getAuthenticatedUsername() { + Object subject = SecurityUtils.getSubject().getPrincipal(); + AssertUtil.assertIsNotNull(subject); + return subject.toString(); + } +} 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 33fbe0cc5d..7e7ceae4f7 100644 --- a/scm-it/src/test/java/sonia/scm/it/UserITCase.java +++ b/scm-it/src/test/java/sonia/scm/it/UserITCase.java @@ -74,6 +74,33 @@ public class UserITCase { } + @Test + public void nonAdminUserShouldChangeOwnPassword() { + String newUser = "user"; + String password = "pass"; + TestData.createUser(newUser, password, false, "xml"); + String newPassword = "new_password"; + ScmRequests.start() + .given() + .url(TestData.getUserUrl(newUser)) + .usernameAndPassword(newUser, password) + .getUserResource() + .assertStatusCode(200) + .usingUserResponse() + .assertAdmin(aBoolean -> assertThat(aBoolean).isEqualTo(Boolean.FALSE)) + .requestChangePassword(newPassword) // the oldPassword is not needed in the user resource + .assertStatusCode(204); + // assert password is changed -> login with the new Password + ScmRequests.start() + .given() + .url(TestData.getUserUrl(newUser)) + .usernameAndPassword(newUser, newPassword) + .getUserResource() + .assertStatusCode(200) + .usingUserResponse() + .assertAdmin(isAdmin -> assertThat(isAdmin).isEqualTo(Boolean.FALSE)) + .assertPassword(Assert::assertNull); + } @Test public void shouldHidePasswordLinkIfUserTypeIsNotXML() { diff --git a/scm-webapp/src/main/java/sonia/scm/api/rest/IllegalArgumentExceptionMapper.java b/scm-webapp/src/main/java/sonia/scm/api/rest/IllegalArgumentExceptionMapper.java index 44d4d9194d..ff29a770cf 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/rest/IllegalArgumentExceptionMapper.java +++ b/scm-webapp/src/main/java/sonia/scm/api/rest/IllegalArgumentExceptionMapper.java @@ -63,6 +63,6 @@ public class IllegalArgumentExceptionMapper public Response toResponse(IllegalArgumentException exception) { log.info("caught IllegalArgumentException -- mapping to bad request", exception); - return Response.status(Status.BAD_REQUEST).build(); + return Response.status(Status.BAD_REQUEST).entity(exception.getMessage()).build(); } } diff --git a/scm-webapp/src/main/java/sonia/scm/api/rest/resources/AbstractManagerResource.java b/scm-webapp/src/main/java/sonia/scm/api/rest/resources/AbstractManagerResource.java index f00072cdcb..794f11893b 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/rest/resources/AbstractManagerResource.java +++ b/scm-webapp/src/main/java/sonia/scm/api/rest/resources/AbstractManagerResource.java @@ -35,6 +35,7 @@ package sonia.scm.api.rest.resources; //~--- non-JDK imports -------------------------------------------------------- +import com.github.sdorra.ssp.PermissionCheck; import org.apache.commons.beanutils.BeanComparator; import org.apache.shiro.authz.AuthorizationException; import org.slf4j.Logger; @@ -63,6 +64,8 @@ import java.util.Arrays; import java.util.Collection; import java.util.Comparator; import java.util.Date; +import java.util.function.Consumer; +import java.util.function.Function; //~--- JDK imports ------------------------------------------------------------ @@ -196,27 +199,24 @@ public abstract class AbstractManagerResource { return response; } - /** - * Method description - * - * - * - * - * @param name - * @param item - * - * - * @return - */ - public Response update(String name, T item) - { + public Response update(T item, Function permissionChecker ){ + Consumer updateAction = mng -> mng.modify(item, permissionChecker); + return applyUpdate(item, updateAction); + } + + public Response update(String name, T item) { + Consumer updateAction = mng -> mng.modify(item); + return applyUpdate(item, updateAction); + } + + public Response applyUpdate(T item, Consumer updateAction) { Response response = null; preUpdate(item); try { - manager.modify(item); + updateAction.accept(manager); response = Response.noContent().build(); } catch (AuthorizationException ex) diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/IdResourceManagerAdapter.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/IdResourceManagerAdapter.java index 9b890eb174..d58a870d6a 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/IdResourceManagerAdapter.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/IdResourceManagerAdapter.java @@ -1,5 +1,6 @@ package sonia.scm.api.v2.resources; +import com.github.sdorra.ssp.PermissionCheck; import de.otto.edison.hal.HalRepresentation; import sonia.scm.AlreadyExistsException; import sonia.scm.ConcurrentModificationException; @@ -7,6 +8,10 @@ import sonia.scm.Manager; import sonia.scm.ModelObject; import sonia.scm.NotFoundException; import sonia.scm.PageResult; +import sonia.scm.user.User; +import sonia.scm.user.UserPermissions; +import sonia.scm.util.AssertUtil; +import sonia.scm.util.AuthenticationUtil; import javax.ws.rs.core.Response; import java.util.Optional; @@ -38,13 +43,31 @@ class IdResourceManagerAdapter applyChanges, Consumer checker) throws NotFoundException, ConcurrentModificationException { - return singleAdapter.update( + + /** + * If the authenticated user is the same user that want to change password than return the changeOwnPassword verification function + * if the authenticated user is different he should have the modify permission to be able to modify passwords of other users + * + * @param usernameToChangePassword the user name of the user we want to change password + * @return function to verify permission + */ + public Function getChangePasswordPermission(String usernameToChangePassword) { + AssertUtil.assertIsNotEmpty(usernameToChangePassword); + return user -> { + if (usernameToChangePassword.equals(AuthenticationUtil.getAuthenticatedUsername())) { + return UserPermissions.changeOwnPassword(); + } + return UserPermissions.modify(user); + }; + } + + public Response changePassword(String id, Function applyChanges, Consumer checker, Function permissionCheck) throws NotFoundException, ConcurrentModificationException { + return singleAdapter.changePassword( loadBy(id), applyChanges, idStaysTheSame(id), - checker - ); + checker, + permissionCheck); } public Response update(String id, Function applyChanges) throws NotFoundException, ConcurrentModificationException { 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 e684bc25db..4c99b62618 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 @@ -10,6 +10,7 @@ import sonia.scm.NotFoundException; import sonia.scm.user.InvalidPasswordException; import sonia.scm.user.User; import sonia.scm.user.UserManager; +import sonia.scm.user.UserPermissions; import sonia.scm.web.VndMediaType; import javax.inject.Inject; @@ -80,7 +81,7 @@ public class MeResource { @Consumes(VndMediaType.PASSWORD_CHANGE) public Response changePassword(PasswordChangeDto passwordChangeDto) throws NotFoundException, ConcurrentModificationException { String name = (String) SecurityUtils.getSubject().getPrincipals().getPrimaryPrincipal(); - return adapter.update(name, user -> user.changePassword(passwordService.encryptPassword(passwordChangeDto.getNewPassword())), userManager.getUserTypeChecker().andThen(getOldOriginalPasswordChecker(passwordChangeDto.getOldPassword()))); + return adapter.changePassword(name, user -> user.clone().changePassword(passwordService.encryptPassword(passwordChangeDto.getNewPassword())), userManager.getChangePasswordChecker().andThen(getOldOriginalPasswordChecker(passwordChangeDto.getOldPassword())), user -> UserPermissions.changeOwnPassword()); } /** diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/SingleResourceManagerAdapter.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/SingleResourceManagerAdapter.java index f2bc93d47e..743d9f9710 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/SingleResourceManagerAdapter.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/SingleResourceManagerAdapter.java @@ -1,5 +1,6 @@ package sonia.scm.api.v2.resources; +import com.github.sdorra.ssp.PermissionCheck; import de.otto.edison.hal.HalRepresentation; import sonia.scm.ConcurrentModificationException; import sonia.scm.Manager; @@ -16,8 +17,6 @@ import java.util.function.Function; import java.util.function.Predicate; import java.util.function.Supplier; -import static javax.ws.rs.core.Response.Status.BAD_REQUEST; - /** * Adapter from resource http endpoints to managers, for Single resources (e.g. {@code /user/name}). * @@ -54,10 +53,12 @@ class SingleResourceManagerAdapter> reader, Function applyChanges, Predicate hasSameKey, Consumer checker) throws NotFoundException, ConcurrentModificationException { + public Response changePassword(Supplier> reader, Function applyChanges, Predicate hasSameKey, Consumer checker, Function permissionCheck) throws NotFoundException, ConcurrentModificationException { MODEL_OBJECT existingModelObject = reader.get().orElseThrow(NotFoundException::new); + MODEL_OBJECT changedModelObject = applyChanges.apply(existingModelObject); + checkForUpdate(hasSameKey, existingModelObject, changedModelObject); checker.accept(existingModelObject); - return update(reader,applyChanges,hasSameKey); + return update(changedModelObject, permissionCheck); } /** @@ -67,13 +68,17 @@ class SingleResourceManagerAdapter> reader, Function applyChanges, Predicate hasSameKey) throws NotFoundException, ConcurrentModificationException { MODEL_OBJECT existingModelObject = reader.get().orElseThrow(NotFoundException::new); MODEL_OBJECT changedModelObject = applyChanges.apply(existingModelObject); + checkForUpdate(hasSameKey, existingModelObject, changedModelObject); + return update(getId(existingModelObject), changedModelObject); + } + + public void checkForUpdate(Predicate hasSameKey, MODEL_OBJECT existingModelObject, MODEL_OBJECT changedModelObject) throws ConcurrentModificationException { if (!hasSameKey.test(changedModelObject)) { - return Response.status(BAD_REQUEST).entity("illegal change of id").build(); + throw new IllegalArgumentException("illegal change of id"); } else if (modelObjectWasModifiedConcurrently(existingModelObject, changedModelObject)) { throw new ConcurrentModificationException(); } - return update(getId(existingModelObject), changedModelObject); } private boolean modelObjectWasModifiedConcurrently(MODEL_OBJECT existing, MODEL_OBJECT updated) { 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 c0bb1e65ad..42f9274d74 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 @@ -111,7 +111,9 @@ public class UserResource { * The oldPassword property of the DTO is not needed here. it will be ignored. * The oldPassword property is needed in the MeResources when the actual user change the own password. * - * Note: This method requires "user:modify" privilege. + * Note: This method requires "user:modify" privilege to modify the password of other users. + * Note: This method requires "user:changeOwnPassword" privilege to modify the own password. + * * @param name name of the user to be modified * @param passwordChangeDto change password object to modify password. the old password is here not required */ @@ -128,7 +130,7 @@ public class UserResource { }) @TypeHint(TypeHint.NO_CONTENT.class) public Response changePassword(@PathParam("id") String name, @Valid PasswordChangeDto passwordChangeDto) throws NotFoundException, ConcurrentModificationException { - return adapter.update(name, user -> user.changePassword(passwordService.encryptPassword(passwordChangeDto.getNewPassword())), userManager.getUserTypeChecker()); + return adapter.changePassword(name, user -> user.changePassword(passwordService.encryptPassword(passwordChangeDto.getNewPassword())), userManager.getChangePasswordChecker(), adapter.getChangePasswordPermission(name)); } } diff --git a/scm-webapp/src/main/java/sonia/scm/security/DefaultAuthorizationCollector.java b/scm-webapp/src/main/java/sonia/scm/security/DefaultAuthorizationCollector.java index 36b7f4089d..265e163c23 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/DefaultAuthorizationCollector.java +++ b/scm-webapp/src/main/java/sonia/scm/security/DefaultAuthorizationCollector.java @@ -260,6 +260,7 @@ public class DefaultAuthorizationCollector implements AuthorizationCollector builder.add(canReadOwnUser(user)); builder.add(getUserAutocompletePermission()); builder.add(getGroupAutocompletePermission()); + builder.add(getChangeOwnPasswordPermission()); permissions = builder.build(); } @@ -272,6 +273,10 @@ public class DefaultAuthorizationCollector implements AuthorizationCollector return GroupPermissions.autocomplete().asShiroString(); } + private String getChangeOwnPasswordPermission() { + return UserPermissions.changeOwnPassword().asShiroString(); + } + private String getUserAutocompletePermission() { return UserPermissions.autocomplete().asShiroString(); } diff --git a/scm-webapp/src/main/java/sonia/scm/user/DefaultUserManager.java b/scm-webapp/src/main/java/sonia/scm/user/DefaultUserManager.java index 19802902b8..c9821575d2 100644 --- a/scm-webapp/src/main/java/sonia/scm/user/DefaultUserManager.java +++ b/scm-webapp/src/main/java/sonia/scm/user/DefaultUserManager.java @@ -36,6 +36,7 @@ package sonia.scm.user; //~--- non-JDK imports -------------------------------------------------------- import com.github.sdorra.ssp.PermissionActionCheck; +import com.github.sdorra.ssp.PermissionCheck; import com.google.inject.Inject; import com.google.inject.Singleton; import org.slf4j.Logger; @@ -63,6 +64,7 @@ import java.util.Collection; import java.util.Collections; import java.util.Comparator; import java.util.List; +import java.util.function.Function; //~--- JDK imports ------------------------------------------------------------ @@ -194,11 +196,15 @@ public class DefaultUserManager extends AbstractUserManager */ @Override public void modify(User user) throws NotFoundException { - logger.info("modify user {} of type {}", user.getName(), user.getType()); + modify(user,UserPermissions::modify); + } + @Override + public void modify(User user, Function permissionChecker) throws NotFoundException { + logger.info("modify user {} of type {}", user.getName(), user.getType()); managerDaoAdapter.modify( user, - UserPermissions::modify, + permissionChecker, notModified -> fireEvent(HandlerEventType.BEFORE_MODIFY, user, notModified), notModified -> fireEvent(HandlerEventType.MODIFY, user, notModified)); } diff --git a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/DispatcherMock.java b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/DispatcherMock.java index a1abdb6ff4..8a71abb670 100644 --- a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/DispatcherMock.java +++ b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/DispatcherMock.java @@ -5,6 +5,7 @@ import org.jboss.resteasy.mock.MockDispatcherFactory; import sonia.scm.api.rest.AlreadyExistsExceptionMapper; import sonia.scm.api.rest.AuthorizationExceptionMapper; import sonia.scm.api.rest.ConcurrentModificationExceptionMapper; +import sonia.scm.api.rest.IllegalArgumentExceptionMapper; public class DispatcherMock { public static Dispatcher createDispatcher(Object resource) { @@ -17,6 +18,7 @@ public class DispatcherMock { dispatcher.getProviderFactory().registerProvider(InternalRepositoryExceptionMapper.class); dispatcher.getProviderFactory().registerProvider(ChangePasswordNotAllowedExceptionMapper.class); dispatcher.getProviderFactory().registerProvider(InvalidPasswordExceptionMapper.class); + dispatcher.getProviderFactory().registerProvider(IllegalArgumentExceptionMapper.class); return dispatcher; } } 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 c7b040172e..8aec18af01 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 @@ -17,6 +17,7 @@ import sonia.scm.user.User; import sonia.scm.user.UserManager; import sonia.scm.web.VndMediaType; +import javax.lang.model.util.Types; import javax.servlet.http.HttpServletResponse; import java.net.URI; import java.net.URISyntaxException; @@ -69,7 +70,7 @@ public class MeResourceTest { doNothing().when(userManager).modify(userCaptor.capture()); doNothing().when(userManager).delete(userCaptor.capture()); when(userManager.isTypeDefault(userCaptor.capture())).thenCallRealMethod(); - when(userManager.getUserTypeChecker()).thenCallRealMethod(); + when(userManager.getChangePasswordChecker()).thenCallRealMethod(); when(userManager.getDefaultType()).thenReturn("xml"); MeResource meResource = new MeResource(userToDtoMapper, userManager, passwordService); when(uriInfo.getApiRestUri()).thenReturn(URI.create("/")); @@ -97,21 +98,23 @@ public class MeResourceTest { public void shouldEncryptPasswordBeforeChanging() throws Exception { String newPassword = "pwd123"; String encryptedNewPassword = "encrypted123"; - String oldPassword = "notEncriptedSecret"; + String oldPassword = "secret"; String content = String.format("{ \"oldPassword\": \"%s\" , \"newPassword\": \"%s\" }", oldPassword, newPassword); MockHttpRequest request = MockHttpRequest .put("/" + MeResource.ME_PATH_V2 + "password") .contentType(VndMediaType.PASSWORD_CHANGE) .content(content.getBytes()); MockHttpResponse response = new MockHttpResponse(); - when(passwordService.encryptPassword(eq(newPassword))).thenReturn(encryptedNewPassword); - when(passwordService.encryptPassword(eq(oldPassword))).thenReturn("secret"); + when(passwordService.encryptPassword(newPassword)).thenReturn(encryptedNewPassword); + when(passwordService.encryptPassword(oldPassword)).thenReturn("secret"); + ArgumentCaptor modifyUserCaptor = ArgumentCaptor.forClass(User.class); + doNothing().when(userManager).modify(modifyUserCaptor.capture(), any()); dispatcher.invoke(request, response); assertEquals(HttpServletResponse.SC_NO_CONTENT, response.getStatus()); - verify(userManager).modify(any(User.class)); - User updatedUser = userCaptor.getValue(); + verify(userManager).modify(any(), any()); + User updatedUser = modifyUserCaptor.getValue(); assertEquals(encryptedNewPassword, updatedUser.getPassword()); } @@ -120,14 +123,14 @@ public class MeResourceTest { public void shouldGet400OnChangePasswordOfUserWithNonDefaultType() throws Exception { originalUser.setType("not an xml type"); String newPassword = "pwd123"; - String oldPassword = "notEncriptedSecret"; + String oldPassword = "secret"; String content = String.format("{ \"oldPassword\": \"%s\" , \"newPassword\": \"%s\" }", oldPassword, newPassword); MockHttpRequest request = MockHttpRequest .put("/" + MeResource.ME_PATH_V2 + "password") .contentType(VndMediaType.PASSWORD_CHANGE) .content(content.getBytes()); MockHttpResponse response = new MockHttpResponse(); - when(passwordService.encryptPassword(newPassword)).thenReturn("encrypted123"); + when(passwordService.encryptPassword(eq(newPassword))).thenReturn("encrypted123"); when(passwordService.encryptPassword(eq(oldPassword))).thenReturn("secret"); dispatcher.invoke(request, response); diff --git a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/UserRootResourceTest.java b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/UserRootResourceTest.java index 3d33fa0eb6..69f5d65fd6 100644 --- a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/UserRootResourceTest.java +++ b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/UserRootResourceTest.java @@ -69,7 +69,7 @@ public class UserRootResourceTest { originalUser = createDummyUser("Neo"); when(userManager.create(userCaptor.capture())).thenAnswer(invocation -> invocation.getArguments()[0]); when(userManager.isTypeDefault(userCaptor.capture())).thenCallRealMethod(); - when(userManager.getUserTypeChecker()).thenCallRealMethod(); + when(userManager.getChangePasswordChecker()).thenCallRealMethod(); doNothing().when(userManager).modify(userCaptor.capture()); doNothing().when(userManager).delete(userCaptor.capture()); when(userManager.getDefaultType()).thenReturn("xml"); @@ -151,7 +151,7 @@ public class UserRootResourceTest { dispatcher.invoke(request, response); assertEquals(HttpServletResponse.SC_NO_CONTENT, response.getStatus()); - verify(userManager).modify(any(User.class)); + verify(userManager).modify(any(), any()); User updatedUser = userCaptor.getValue(); assertEquals("encrypted123", updatedUser.getPassword()); } diff --git a/scm-webapp/src/test/java/sonia/scm/security/DefaultAuthorizationCollectorTest.java b/scm-webapp/src/test/java/sonia/scm/security/DefaultAuthorizationCollectorTest.java index 9155af3b56..a85d03991d 100644 --- a/scm-webapp/src/test/java/sonia/scm/security/DefaultAuthorizationCollectorTest.java +++ b/scm-webapp/src/test/java/sonia/scm/security/DefaultAuthorizationCollectorTest.java @@ -1,10 +1,10 @@ /** * Copyright (c) 2014, Sebastian Sdorra * All rights reserved. - * + * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are met: - * + * * 1. Redistributions of source code must retain the above copyright notice, * this list of conditions and the following disclaimer. * 2. Redistributions in binary form must reproduce the above copyright notice, @@ -13,7 +13,7 @@ * 3. Neither the name of SCM-Manager; nor the names of its * contributors may be used to endorse or promote products derived from this * software without specific prior written permission. - * + * * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE @@ -24,9 +24,9 @@ * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. - * + * * http://bitbucket.org/sdorra/scm-manager - * + * */ package sonia.scm.security; @@ -57,7 +57,6 @@ import sonia.scm.repository.RepositoryTestData; import sonia.scm.user.User; import sonia.scm.user.UserTestData; -import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.nullValue; @@ -70,7 +69,7 @@ import static org.mockito.Mockito.when; /** * Unit tests for {@link AuthorizationCollector}. - * + * * @author Sebastian Sdorra */ @SuppressWarnings("unchecked") @@ -79,28 +78,28 @@ public class DefaultAuthorizationCollectorTest { @Mock private Cache cache; - + @Mock private CacheManager cacheManager; - + @Mock private RepositoryDAO repositoryDAO; @Mock private SecuritySystem securitySystem; - + private DefaultAuthorizationCollector collector; - + @Rule public ShiroRule shiro = new ShiroRule(); - + /** * Set up object to test. */ @Before public void setUp(){ when(cacheManager.getCache(Mockito.any(String.class))).thenReturn(cache); - + collector = new DefaultAuthorizationCollector(cacheManager, repositoryDAO, securitySystem); } @@ -116,7 +115,7 @@ public class DefaultAuthorizationCollectorTest { assertThat(authInfo.getStringPermissions(), nullValue()); assertThat(authInfo.getObjectPermissions(), nullValue()); } - + /** * Tests {@link AuthorizationCollector#collect()} from cache. */ @@ -124,16 +123,15 @@ public class DefaultAuthorizationCollectorTest { @SubjectAware( configuration = "classpath:sonia/scm/shiro-001.ini" ) - public void testCollectFromCache() - { + public void testCollectFromCache() { AuthorizationInfo info = new SimpleAuthorizationInfo(); when(cache.get(anyObject())).thenReturn(info); authenticate(UserTestData.createTrillian(), "main"); - + AuthorizationInfo authInfo = collector.collect(); assertSame(info, authInfo); } - + /** * Tests {@link AuthorizationCollector#collect()} with cache. */ @@ -141,13 +139,13 @@ public class DefaultAuthorizationCollectorTest { @SubjectAware( configuration = "classpath:sonia/scm/shiro-001.ini" ) - public void testCollectWithCache(){ + public void testCollectWithCache() { authenticate(UserTestData.createTrillian(), "main"); - + AuthorizationInfo authInfo = collector.collect(); verify(cache).put(any(), any()); } - + /** * Tests {@link AuthorizationCollector#collect()} without permissions. */ @@ -155,17 +153,16 @@ public class DefaultAuthorizationCollectorTest { @SubjectAware( configuration = "classpath:sonia/scm/shiro-001.ini" ) - public void testCollectWithoutPermissions() - { + public void testCollectWithoutPermissions() { authenticate(UserTestData.createTrillian(), "main"); - + AuthorizationInfo authInfo = collector.collect(); assertThat(authInfo.getRoles(), Matchers.contains(Role.USER)); - assertThat(authInfo.getStringPermissions(), hasSize(3)); - assertThat(authInfo.getStringPermissions(), containsInAnyOrder("user:autocomplete", "group:autocomplete", "user:read:trillian")); + assertThat(authInfo.getStringPermissions(), hasSize(4)); + assertThat(authInfo.getStringPermissions(), containsInAnyOrder("user:autocomplete", "group:autocomplete", "user:changeOwnPassword", "user:read:trillian")); assertThat(authInfo.getObjectPermissions(), nullValue()); } - + /** * Tests {@link AuthorizationCollector#collect()} as admin. */ @@ -173,18 +170,17 @@ public class DefaultAuthorizationCollectorTest { @SubjectAware( configuration = "classpath:sonia/scm/shiro-001.ini" ) - public void testCollectAsAdmin() - { + public void testCollectAsAdmin() { User trillian = UserTestData.createTrillian(); trillian.setAdmin(true); authenticate(trillian, "main"); - + AuthorizationInfo authInfo = collector.collect(); assertThat(authInfo.getRoles(), Matchers.containsInAnyOrder(Role.USER, Role.ADMIN)); assertThat(authInfo.getObjectPermissions(), nullValue()); assertThat(authInfo.getStringPermissions(), Matchers.contains("*")); } - + /** * Tests {@link AuthorizationCollector#collect()} with repository permissions. */ @@ -192,8 +188,7 @@ public class DefaultAuthorizationCollectorTest { @SubjectAware( configuration = "classpath:sonia/scm/shiro-001.ini" ) - public void testCollectWithRepositoryPermissions() - { + public void testCollectWithRepositoryPermissions() { String group = "heart-of-gold-crew"; authenticate(UserTestData.createTrillian(), group); Repository heartOfGold = RepositoryTestData.createHeartOfGold(); @@ -204,14 +199,14 @@ public class DefaultAuthorizationCollectorTest { sonia.scm.repository.Permission permission = new sonia.scm.repository.Permission(group, PermissionType.WRITE, true); puzzle42.setPermissions(Lists.newArrayList(permission)); when(repositoryDAO.getAll()).thenReturn(Lists.newArrayList(heartOfGold, puzzle42)); - + // execute and assert AuthorizationInfo authInfo = collector.collect(); assertThat(authInfo.getRoles(), Matchers.containsInAnyOrder(Role.USER)); assertThat(authInfo.getObjectPermissions(), nullValue()); - assertThat(authInfo.getStringPermissions(), containsInAnyOrder("user:autocomplete", "group:autocomplete", "repository:read,pull:one", "repository:read,pull,push:two", "user:read:trillian")); + assertThat(authInfo.getStringPermissions(), containsInAnyOrder("user:autocomplete", "group:autocomplete", "user:changeOwnPassword", "repository:read,pull:one", "repository:read,pull,push:two", "user:read:trillian")); } - + /** * Tests {@link AuthorizationCollector#collect()} with global permissions. */ @@ -219,20 +214,20 @@ public class DefaultAuthorizationCollectorTest { @SubjectAware( configuration = "classpath:sonia/scm/shiro-001.ini" ) - public void testCollectWithGlobalPermissions(){ + public void testCollectWithGlobalPermissions() { authenticate(UserTestData.createTrillian(), "main"); - + StoredAssignedPermission p1 = new StoredAssignedPermission("one", new AssignedPermission("one", "one:one")); StoredAssignedPermission p2 = new StoredAssignedPermission("two", new AssignedPermission("two", "two:two")); when(securitySystem.getPermissions(Mockito.any(Predicate.class))).thenReturn(Lists.newArrayList(p1, p2)); - + // execute and assert AuthorizationInfo authInfo = collector.collect(); assertThat(authInfo.getRoles(), Matchers.containsInAnyOrder(Role.USER)); assertThat(authInfo.getObjectPermissions(), nullValue()); - assertThat(authInfo.getStringPermissions(), containsInAnyOrder("one:one", "two:two", "user:read:trillian", "user:autocomplete" , "group:autocomplete" )); + assertThat(authInfo.getStringPermissions(), containsInAnyOrder("one:one", "two:two", "user:read:trillian", "user:autocomplete", "group:autocomplete", "user:changeOwnPassword")); } - + private void authenticate(User user, String group, String... groups) { SimplePrincipalCollection spc = new SimplePrincipalCollection(); spc.add(user.getName(), "unit"); @@ -249,9 +244,9 @@ public class DefaultAuthorizationCollectorTest { public void testInvalidateCache() { collector.invalidateCache(AuthorizationChangedEvent.createForEveryUser()); verify(cache).clear(); - + collector.invalidateCache(AuthorizationChangedEvent.createForUser("dent")); verify(cache).removeAll(Mockito.any(Predicate.class)); } - + } From f94922837b997183cf3749e6d5e7ae5ca3af42af Mon Sep 17 00:00:00 2001 From: Mohamed Karray Date: Tue, 16 Oct 2018 09:15:35 +0200 Subject: [PATCH 2/8] apply permission from adapter --- .../scm/api/v2/resources/IdResourceManagerAdapter.java | 9 +++++---- .../main/java/sonia/scm/api/v2/resources/MeResource.java | 2 +- .../java/sonia/scm/api/v2/resources/UserResource.java | 2 +- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/IdResourceManagerAdapter.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/IdResourceManagerAdapter.java index d58a870d6a..f3ff20b010 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/IdResourceManagerAdapter.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/IdResourceManagerAdapter.java @@ -51,9 +51,10 @@ class IdResourceManagerAdapter getChangePasswordPermission(String usernameToChangePassword) { + public Function getChangePasswordPermission(String usernameToChangePassword) { AssertUtil.assertIsNotEmpty(usernameToChangePassword); - return user -> { + return model -> { + User user = (User) model; if (usernameToChangePassword.equals(AuthenticationUtil.getAuthenticatedUsername())) { return UserPermissions.changeOwnPassword(); } @@ -61,13 +62,13 @@ class IdResourceManagerAdapter applyChanges, Consumer checker, Function permissionCheck) throws NotFoundException, ConcurrentModificationException { + public Response changePassword(String id, Function applyChanges, Consumer checker ) throws NotFoundException, ConcurrentModificationException { return singleAdapter.changePassword( loadBy(id), applyChanges, idStaysTheSame(id), checker, - permissionCheck); + getChangePasswordPermission(id)); } public Response update(String id, Function applyChanges) throws NotFoundException, ConcurrentModificationException { 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 4c99b62618..7ebbf7ebb2 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 @@ -81,7 +81,7 @@ public class MeResource { @Consumes(VndMediaType.PASSWORD_CHANGE) public Response changePassword(PasswordChangeDto passwordChangeDto) throws NotFoundException, ConcurrentModificationException { String name = (String) SecurityUtils.getSubject().getPrincipals().getPrimaryPrincipal(); - return adapter.changePassword(name, user -> user.clone().changePassword(passwordService.encryptPassword(passwordChangeDto.getNewPassword())), userManager.getChangePasswordChecker().andThen(getOldOriginalPasswordChecker(passwordChangeDto.getOldPassword())), user -> UserPermissions.changeOwnPassword()); + return adapter.changePassword(name, user -> user.clone().changePassword(passwordService.encryptPassword(passwordChangeDto.getNewPassword())), userManager.getChangePasswordChecker().andThen(getOldOriginalPasswordChecker(passwordChangeDto.getOldPassword()))); } /** 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 42f9274d74..8c87d1b2d7 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 @@ -130,7 +130,7 @@ public class UserResource { }) @TypeHint(TypeHint.NO_CONTENT.class) public Response changePassword(@PathParam("id") String name, @Valid PasswordChangeDto passwordChangeDto) throws NotFoundException, ConcurrentModificationException { - return adapter.changePassword(name, user -> user.changePassword(passwordService.encryptPassword(passwordChangeDto.getNewPassword())), userManager.getChangePasswordChecker(), adapter.getChangePasswordPermission(name)); + return adapter.changePassword(name, user -> user.changePassword(passwordService.encryptPassword(passwordChangeDto.getNewPassword())), userManager.getChangePasswordChecker()); } } From d2dbccb80c16e1ea7f37b9f03e66cc39571febc8 Mon Sep 17 00:00:00 2001 From: Mohamed Karray Date: Tue, 16 Oct 2018 10:47:52 +0200 Subject: [PATCH 3/8] remove throws NotFoundException and fix modify password --- .../main/java/sonia/scm/ManagerDecorator.java | 6 ++-- .../ChangePasswordNotAllowedException.java | 1 + .../scm/security/SyncingRealmHelperTest.java | 4 +-- .../src/test/java/sonia/scm/it/MeITCase.java | 29 +++++++++++++++++++ .../test/java/sonia/scm/it/UserITCase.java | 21 ++++++++++++-- .../java/sonia/scm/it/utils/ScmRequests.java | 4 +++ .../sonia/scm/user/UserManagerTestBase.java | 4 +-- .../java/sonia/scm/ManagerDaoAdapter.java | 4 +-- .../resources/ChangePasswordResource.java | 2 +- .../api/v2/resources/DiffRootResource.java | 2 +- .../scm/api/v2/resources/GroupResource.java | 4 +-- .../resources/IdResourceManagerAdapter.java | 6 ++-- .../scm/api/v2/resources/MeResource.java | 10 ++++--- .../v2/resources/PermissionRootResource.java | 6 ++-- .../api/v2/resources/RepositoryResource.java | 4 +-- .../SingleResourceManagerAdapter.java | 6 ++-- .../api/v2/resources/SourceRootResource.java | 2 +- .../scm/api/v2/resources/UserResource.java | 13 ++++++--- .../sonia/scm/group/DefaultGroupManager.java | 6 ++-- .../repository/DefaultRepositoryManager.java | 4 +-- .../sonia/scm/repository/HealthChecker.java | 6 ++-- .../sonia/scm/user/DefaultUserManager.java | 8 ++--- .../DefaultRepositoryManagerTest.java | 12 ++++---- 23 files changed, 110 insertions(+), 54 deletions(-) diff --git a/scm-core/src/main/java/sonia/scm/ManagerDecorator.java b/scm-core/src/main/java/sonia/scm/ManagerDecorator.java index 7b3f03ee8c..ef20a374cb 100644 --- a/scm-core/src/main/java/sonia/scm/ManagerDecorator.java +++ b/scm-core/src/main/java/sonia/scm/ManagerDecorator.java @@ -71,7 +71,7 @@ public class ManagerDecorator implements Manager { } @Override - public void delete(T object) throws NotFoundException { + public void delete(T object){ decorated.delete(object); } @@ -82,12 +82,12 @@ public class ManagerDecorator implements Manager { } @Override - public void modify(T object) throws NotFoundException { + public void modify(T object){ decorated.modify(object); } @Override - public void refresh(T object) throws NotFoundException { + public void refresh(T object){ decorated.refresh(object); } 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 19c609ba30..caa0c918fc 100644 --- a/scm-core/src/main/java/sonia/scm/user/ChangePasswordNotAllowedException.java +++ b/scm-core/src/main/java/sonia/scm/user/ChangePasswordNotAllowedException.java @@ -3,6 +3,7 @@ package sonia.scm.user; public class ChangePasswordNotAllowedException extends RuntimeException { public static final String WRONG_USER_TYPE = "User of type {0} are not allowed to change password"; + public static final String OLD_PASSWORD_REQUIRED = "the old password is required."; public ChangePasswordNotAllowedException(String message) { super(message); diff --git a/scm-core/src/test/java/sonia/scm/security/SyncingRealmHelperTest.java b/scm-core/src/test/java/sonia/scm/security/SyncingRealmHelperTest.java index 27ca923902..93b0752766 100644 --- a/scm-core/src/test/java/sonia/scm/security/SyncingRealmHelperTest.java +++ b/scm-core/src/test/java/sonia/scm/security/SyncingRealmHelperTest.java @@ -154,7 +154,7 @@ public class SyncingRealmHelperTest { * Tests {@link SyncingRealmHelper#store(Group)} with an existing group. */ @Test - public void testStoreGroupModify() throws NotFoundException { + public void testStoreGroupModify(){ Group group = new Group("unit-test", "heartOfGold"); when(groupManager.get("heartOfGold")).thenReturn(group); @@ -191,7 +191,7 @@ public class SyncingRealmHelperTest { * Tests {@link SyncingRealmHelper#store(User)} with an existing user. */ @Test - public void testStoreUserModify() throws NotFoundException { + public void testStoreUserModify(){ when(userManager.contains("tricia")).thenReturn(Boolean.TRUE); User user = new User("tricia"); diff --git a/scm-it/src/test/java/sonia/scm/it/MeITCase.java b/scm-it/src/test/java/sonia/scm/it/MeITCase.java index 64f06765ea..203b748ade 100644 --- a/scm-it/src/test/java/sonia/scm/it/MeITCase.java +++ b/scm-it/src/test/java/sonia/scm/it/MeITCase.java @@ -44,6 +44,35 @@ public class MeITCase { .assertStatusCode(204); } + @Test + public void nonAdminUserShouldChangeOwnPassword() { + String newPassword = "pass1"; + String username = "user1"; + String password = "pass"; + TestData.createUser(username, password,false,"xml"); + // user change the own password + ScmRequests.start() + .given() + .url(TestData.getMeUrl()) + .usernameAndPassword(username, password) + .getMeResource() + .assertStatusCode(200) + .usingMeResponse() + .assertAdmin(aBoolean -> assertThat(aBoolean).isEqualTo(Boolean.FALSE)) + .assertPassword(Assert::assertNull) + .assertType(s -> assertThat(s).isEqualTo("xml")) + .requestChangePassword(password, newPassword) + .assertStatusCode(204); + // assert password is changed -> login with the new Password than undo changes + ScmRequests.start() + .given() + .url(TestData.getMeUrl()) + .usernameAndPassword(username, newPassword) + .getMeResource() + .assertStatusCode(200); + + } + @Test public void shouldHidePasswordLinkIfUserTypeIsNotXML() { String newUser = "user"; 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 7e7ceae4f7..2860d2f3fa 100644 --- a/scm-it/src/test/java/sonia/scm/it/UserITCase.java +++ b/scm-it/src/test/java/sonia/scm/it/UserITCase.java @@ -31,7 +31,7 @@ public class UserITCase { .usingUserResponse() .assertAdmin(aBoolean -> assertThat(aBoolean).isEqualTo(Boolean.TRUE)) .assertPassword(Assert::assertNull) - .requestChangePassword(newPassword) // the oldPassword is not needed in the user resource + .requestChangePassword(password, newPassword) // the oldPassword is needed when the own password should be changed .assertStatusCode(204); // assert password is changed -> login with the new Password ScmRequests.start() @@ -60,7 +60,7 @@ public class UserITCase { .getUserResource() .assertStatusCode(200) .usingUserResponse() - .assertAdmin(aBoolean -> assertThat(aBoolean).isEqualTo(Boolean.TRUE)) // the user anonymous is not an admin + .assertAdmin(aBoolean -> assertThat(aBoolean).isEqualTo(Boolean.TRUE)) .assertPassword(Assert::assertNull) .requestChangePassword(newPassword) // the oldPassword is not needed in the user resource .assertStatusCode(204); @@ -75,6 +75,21 @@ public class UserITCase { } @Test + public void nonAdminUserShouldNotChangePasswordOfOtherUser() { + String user = "user"; + String password = "pass"; + TestData.createUser(user, password, false, "xml"); + String user2 = "user2"; + TestData.createUser(user2, password, false, "xml"); + ScmRequests.start() + .given() + .url(TestData.getUserUrl(user2)) + .usernameAndPassword(user, password) + .getUserResource() + .assertStatusCode(403); + } + + @Test public void nonAdminUserShouldChangeOwnPassword() { String newUser = "user"; String password = "pass"; @@ -88,7 +103,7 @@ public class UserITCase { .assertStatusCode(200) .usingUserResponse() .assertAdmin(aBoolean -> assertThat(aBoolean).isEqualTo(Boolean.FALSE)) - .requestChangePassword(newPassword) // the oldPassword is not needed in the user resource + .requestChangePassword(password, newPassword) // the oldPassword is needed when the own password should be changed .assertStatusCode(204); // assert password is changed -> login with the new Password ScmRequests.start() diff --git a/scm-it/src/test/java/sonia/scm/it/utils/ScmRequests.java b/scm-it/src/test/java/sonia/scm/it/utils/ScmRequests.java index 41fd9a1290..d93b223404 100644 --- a/scm-it/src/test/java/sonia/scm/it/utils/ScmRequests.java +++ b/scm-it/src/test/java/sonia/scm/it/utils/ScmRequests.java @@ -406,6 +406,10 @@ public class ScmRequests { return new AppliedChangePasswordRequest(applyPUTRequestFromLink(super.response, LINKS_PASSWORD_HREF, VndMediaType.PASSWORD_CHANGE, createPasswordChangeJson(null, newPassword))); } + public AppliedChangePasswordRequest requestChangePassword(String oldPassword , String newPassword) { + return new AppliedChangePasswordRequest(applyPUTRequestFromLink(super.response, LINKS_PASSWORD_HREF, VndMediaType.PASSWORD_CHANGE, createPasswordChangeJson(oldPassword, newPassword))); + } + } diff --git a/scm-test/src/main/java/sonia/scm/user/UserManagerTestBase.java b/scm-test/src/main/java/sonia/scm/user/UserManagerTestBase.java index 9f1c50c797..dddce344ce 100644 --- a/scm-test/src/main/java/sonia/scm/user/UserManagerTestBase.java +++ b/scm-test/src/main/java/sonia/scm/user/UserManagerTestBase.java @@ -196,7 +196,7 @@ public abstract class UserManagerTestBase extends ManagerTestBase { } @Test(expected = NotFoundException.class) - public void testModifyNotExisting() throws NotFoundException, ConcurrentModificationException { + public void testModifyNotExisting() { manager.modify(UserTestData.createZaphod()); } @@ -249,7 +249,7 @@ public abstract class UserManagerTestBase extends ManagerTestBase { } @Test(expected = NotFoundException.class) - public void testRefreshNotFound() throws NotFoundException { + public void testRefreshNotFound(){ manager.refresh(UserTestData.createDent()); } diff --git a/scm-webapp/src/main/java/sonia/scm/ManagerDaoAdapter.java b/scm-webapp/src/main/java/sonia/scm/ManagerDaoAdapter.java index e2a1f29a33..7979eca0e7 100644 --- a/scm-webapp/src/main/java/sonia/scm/ManagerDaoAdapter.java +++ b/scm-webapp/src/main/java/sonia/scm/ManagerDaoAdapter.java @@ -15,7 +15,7 @@ public class ManagerDaoAdapter { this.dao = dao; } - public void modify(T object, Function permissionCheck, AroundHandler beforeUpdate, AroundHandler afterUpdate) throws NotFoundException { + public void modify(T object, Function permissionCheck, AroundHandler beforeUpdate, AroundHandler afterUpdate) { T notModified = dao.get(object.getId()); if (notModified != null) { permissionCheck.apply(notModified).check(); @@ -51,7 +51,7 @@ public class ManagerDaoAdapter { return newObject; } - public void delete(T toDelete, Supplier permissionCheck, AroundHandler beforeDelete, AroundHandler afterDelete) throws NotFoundException { + public void delete(T toDelete, Supplier permissionCheck, AroundHandler beforeDelete, AroundHandler afterDelete) { permissionCheck.get().check(); if (dao.contains(toDelete)) { beforeDelete.handle(toDelete); diff --git a/scm-webapp/src/main/java/sonia/scm/api/rest/resources/ChangePasswordResource.java b/scm-webapp/src/main/java/sonia/scm/api/rest/resources/ChangePasswordResource.java index 95f729c2a3..6592bfab8b 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/rest/resources/ChangePasswordResource.java +++ b/scm-webapp/src/main/java/sonia/scm/api/rest/resources/ChangePasswordResource.java @@ -109,7 +109,7 @@ public class ChangePasswordResource @ResponseCode(code = 500, condition = "internal server error") }) @Produces({ MediaType.APPLICATION_JSON, MediaType.APPLICATION_XML }) - public Response changePassword(@FormParam("old-password") String oldPassword, @FormParam("new-password") String newPassword) throws NotFoundException, ConcurrentModificationException { + public Response changePassword(@FormParam("old-password") String oldPassword, @FormParam("new-password") String newPassword) { AssertUtil.assertIsNotEmpty(oldPassword); AssertUtil.assertIsNotEmpty(newPassword); diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/DiffRootResource.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/DiffRootResource.java index 99a996b02f..cb78c961a2 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/DiffRootResource.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/DiffRootResource.java @@ -50,7 +50,7 @@ public class DiffRootResource { @ResponseCode(code = 404, condition = "not found, no revision with the specified param for the repository available or repository not found"), @ResponseCode(code = 500, condition = "internal server error") }) - public Response get(@PathParam("namespace") String namespace, @PathParam("name") String name, @PathParam("revision") String revision) throws NotFoundException { + public Response get(@PathParam("namespace") String namespace, @PathParam("name") String name, @PathParam("revision") String revision){ HttpUtil.checkForCRLFInjection(revision); try (RepositoryService repositoryService = serviceFactory.create(new NamespaceAndName(namespace, name))) { StreamingOutput responseEntry = output -> { diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/GroupResource.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/GroupResource.java index ffa9a9ec08..789a9847cb 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/GroupResource.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/GroupResource.java @@ -53,7 +53,7 @@ public class GroupResource { @ResponseCode(code = 404, condition = "not found, no group with the specified id/name available"), @ResponseCode(code = 500, condition = "internal server error") }) - public Response get(@PathParam("id") String id) throws NotFoundException { + public Response get(@PathParam("id") String id){ return adapter.get(id, groupToGroupDtoMapper::map); } @@ -98,7 +98,7 @@ public class GroupResource { @ResponseCode(code = 500, condition = "internal server error") }) @TypeHint(TypeHint.NO_CONTENT.class) - public Response update(@PathParam("id") String name, @Valid GroupDto groupDto) throws NotFoundException, ConcurrentModificationException { + public Response update(@PathParam("id") String name, @Valid GroupDto groupDto) throws ConcurrentModificationException { return adapter.update(name, existing -> dtoToGroupMapper.map(groupDto)); } } diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/IdResourceManagerAdapter.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/IdResourceManagerAdapter.java index f3ff20b010..197d089eb1 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/IdResourceManagerAdapter.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/IdResourceManagerAdapter.java @@ -39,7 +39,7 @@ class IdResourceManagerAdapter(manager, type); } - Response get(String id, Function mapToDto) throws NotFoundException { + Response get(String id, Function mapToDto) { return singleAdapter.get(loadBy(id), mapToDto); } @@ -62,7 +62,7 @@ class IdResourceManagerAdapter applyChanges, Consumer checker ) throws NotFoundException, ConcurrentModificationException { + public Response changePassword(String id, Function applyChanges, Consumer checker ) throws ConcurrentModificationException { return singleAdapter.changePassword( loadBy(id), applyChanges, @@ -71,7 +71,7 @@ class IdResourceManagerAdapter applyChanges) throws NotFoundException, ConcurrentModificationException { + public Response update(String id, Function applyChanges) throws ConcurrentModificationException { return singleAdapter.update( loadBy(id), applyChanges, 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 7ebbf7ebb2..b4fa21feaa 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 @@ -6,11 +6,10 @@ import com.webcohesion.enunciate.metadata.rs.TypeHint; import org.apache.shiro.SecurityUtils; import org.apache.shiro.authc.credential.PasswordService; import sonia.scm.ConcurrentModificationException; -import sonia.scm.NotFoundException; +import sonia.scm.user.ChangePasswordNotAllowedException; import sonia.scm.user.InvalidPasswordException; import sonia.scm.user.User; import sonia.scm.user.UserManager; -import sonia.scm.user.UserPermissions; import sonia.scm.web.VndMediaType; import javax.inject.Inject; @@ -61,7 +60,7 @@ public class MeResource { @ResponseCode(code = 401, condition = "not authenticated / invalid credentials"), @ResponseCode(code = 500, condition = "internal server error") }) - public Response get(@Context Request request, @Context UriInfo uriInfo) throws NotFoundException { + public Response get(@Context Request request, @Context UriInfo uriInfo) { String id = (String) SecurityUtils.getSubject().getPrincipals().getPrimaryPrincipal(); return adapter.get(id, meToUserDtoMapper::map); @@ -79,8 +78,11 @@ public class MeResource { }) @TypeHint(TypeHint.NO_CONTENT.class) @Consumes(VndMediaType.PASSWORD_CHANGE) - public Response changePassword(PasswordChangeDto passwordChangeDto) throws NotFoundException, ConcurrentModificationException { + public Response changePassword(PasswordChangeDto passwordChangeDto) throws ConcurrentModificationException { String name = (String) SecurityUtils.getSubject().getPrincipals().getPrimaryPrincipal(); + if (passwordChangeDto.getOldPassword() == null){ + throw new ChangePasswordNotAllowedException(ChangePasswordNotAllowedException.OLD_PASSWORD_REQUIRED); + } return adapter.changePassword(name, user -> user.clone().changePassword(passwordService.encryptPassword(passwordChangeDto.getNewPassword())), userManager.getChangePasswordChecker().andThen(getOldOriginalPasswordChecker(passwordChangeDto.getOldPassword()))); } diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/PermissionRootResource.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/PermissionRootResource.java index af1ba3bf64..b1ea912acf 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/PermissionRootResource.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/PermissionRootResource.java @@ -100,7 +100,7 @@ public class PermissionRootResource { @Produces(VndMediaType.PERMISSION) @TypeHint(PermissionDto.class) @Path("{permission-name}") - public Response get(@PathParam("namespace") String namespace, @PathParam("name") String name, @PathParam("permission-name") String permissionName) throws NotFoundException { + public Response get(@PathParam("namespace") String namespace, @PathParam("name") String name, @PathParam("permission-name") String permissionName) { Repository repository = load(namespace, name); RepositoryPermissions.permissionRead(repository).check(); return Response.ok( @@ -158,7 +158,7 @@ public class PermissionRootResource { public Response update(@PathParam("namespace") String namespace, @PathParam("name") String name, @PathParam("permission-name") String permissionName, - @Valid PermissionDto permission) throws NotFoundException, AlreadyExistsException { + @Valid PermissionDto permission) throws AlreadyExistsException { log.info("try to update the permission with name: {}. the modified permission is: {}", permissionName, permission); Repository repository = load(namespace, name); RepositoryPermissions.permissionWrite(repository).check(); @@ -198,7 +198,7 @@ public class PermissionRootResource { @Path("{permission-name}") public Response delete(@PathParam("namespace") String namespace, @PathParam("name") String name, - @PathParam("permission-name") String permissionName) throws NotFoundException { + @PathParam("permission-name") String permissionName) { log.info("try to delete the permission with name: {}.", permissionName); Repository repository = load(namespace, name); RepositoryPermissions.modify(repository).check(); diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/RepositoryResource.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/RepositoryResource.java index f9328525a9..56bb50d4e6 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/RepositoryResource.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/RepositoryResource.java @@ -91,7 +91,7 @@ public class RepositoryResource { @ResponseCode(code = 404, condition = "not found, no repository with the specified name available in the namespace"), @ResponseCode(code = 500, condition = "internal server error") }) - public Response get(@PathParam("namespace") String namespace, @PathParam("name") String name) throws NotFoundException { + public Response get(@PathParam("namespace") String namespace, @PathParam("name") String name){ return adapter.get(loadBy(namespace, name), repositoryToDtoMapper::map); } @@ -138,7 +138,7 @@ public class RepositoryResource { @ResponseCode(code = 500, condition = "internal server error") }) @TypeHint(TypeHint.NO_CONTENT.class) - public Response update(@PathParam("namespace") String namespace, @PathParam("name") String name, @Valid RepositoryDto repositoryDto) throws NotFoundException, ConcurrentModificationException { + public Response update(@PathParam("namespace") String namespace, @PathParam("name") String name, @Valid RepositoryDto repositoryDto) throws ConcurrentModificationException { return adapter.update( loadBy(namespace, name), existing -> processUpdate(repositoryDto, existing), diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/SingleResourceManagerAdapter.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/SingleResourceManagerAdapter.java index 743d9f9710..b5763c4ff0 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/SingleResourceManagerAdapter.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/SingleResourceManagerAdapter.java @@ -46,14 +46,14 @@ class SingleResourceManagerAdapter> reader, Function mapToDto) throws NotFoundException { + Response get(Supplier> reader, Function mapToDto) { return reader.get() .map(mapToDto) .map(Response::ok) .map(Response.ResponseBuilder::build) .orElseThrow(NotFoundException::new); } - public Response changePassword(Supplier> reader, Function applyChanges, Predicate hasSameKey, Consumer checker, Function permissionCheck) throws NotFoundException, ConcurrentModificationException { + public Response changePassword(Supplier> reader, Function applyChanges, Predicate hasSameKey, Consumer checker, Function permissionCheck) throws ConcurrentModificationException { MODEL_OBJECT existingModelObject = reader.get().orElseThrow(NotFoundException::new); MODEL_OBJECT changedModelObject = applyChanges.apply(existingModelObject); checkForUpdate(hasSameKey, existingModelObject, changedModelObject); @@ -65,7 +65,7 @@ class SingleResourceManagerAdapter> reader, Function applyChanges, Predicate hasSameKey) throws NotFoundException, ConcurrentModificationException { + public Response update(Supplier> reader, Function applyChanges, Predicate hasSameKey) throws ConcurrentModificationException { MODEL_OBJECT existingModelObject = reader.get().orElseThrow(NotFoundException::new); MODEL_OBJECT changedModelObject = applyChanges.apply(existingModelObject); checkForUpdate(hasSameKey, existingModelObject, changedModelObject); diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/SourceRootResource.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/SourceRootResource.java index 874ce4f568..e3cf17d3a4 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/SourceRootResource.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/SourceRootResource.java @@ -47,7 +47,7 @@ public class SourceRootResource { @GET @Produces(VndMediaType.SOURCE) @Path("{revision}/{path: .*}") - public Response get(@PathParam("namespace") String namespace, @PathParam("name") String name, @PathParam("revision") String revision, @PathParam("path") String path) throws NotFoundException, IOException { + public Response get(@PathParam("namespace") String namespace, @PathParam("name") String name, @PathParam("revision") String revision, @PathParam("path") String path) throws IOException { return getSource(namespace, name, path, revision); } 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 8c87d1b2d7..e8d2b8617d 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 @@ -3,9 +3,10 @@ package sonia.scm.api.v2.resources; import com.webcohesion.enunciate.metadata.rs.ResponseCode; import com.webcohesion.enunciate.metadata.rs.StatusCodes; import com.webcohesion.enunciate.metadata.rs.TypeHint; +import org.apache.shiro.SecurityUtils; import org.apache.shiro.authc.credential.PasswordService; import sonia.scm.ConcurrentModificationException; -import sonia.scm.NotFoundException; +import sonia.scm.user.ChangePasswordNotAllowedException; import sonia.scm.user.User; import sonia.scm.user.UserManager; import sonia.scm.web.VndMediaType; @@ -57,7 +58,7 @@ public class UserResource { @ResponseCode(code = 404, condition = "not found, no user with the specified id/name available"), @ResponseCode(code = 500, condition = "internal server error") }) - public Response get(@PathParam("id") String id) throws NotFoundException { + public Response get(@PathParam("id") String id) { return adapter.get(id, userToDtoMapper::map); } @@ -102,7 +103,7 @@ public class UserResource { @ResponseCode(code = 500, condition = "internal server error") }) @TypeHint(TypeHint.NO_CONTENT.class) - public Response update(@PathParam("id") String name, @Valid UserDto userDto) throws NotFoundException, ConcurrentModificationException { + public Response update(@PathParam("id") String name, @Valid UserDto userDto) throws ConcurrentModificationException { return adapter.update(name, existing -> dtoToUserMapper.map(userDto, existing.getPassword())); } @@ -129,7 +130,11 @@ public class UserResource { @ResponseCode(code = 500, condition = "internal server error") }) @TypeHint(TypeHint.NO_CONTENT.class) - public Response changePassword(@PathParam("id") String name, @Valid PasswordChangeDto passwordChangeDto) throws NotFoundException, ConcurrentModificationException { + public Response changePassword(@PathParam("id") String name, @Valid PasswordChangeDto passwordChangeDto) throws ConcurrentModificationException { + String currentUserName = (String) SecurityUtils.getSubject().getPrincipals().getPrimaryPrincipal(); + if (currentUserName.equals(name) && passwordChangeDto.getOldPassword() == null){ + throw new ChangePasswordNotAllowedException(ChangePasswordNotAllowedException.OLD_PASSWORD_REQUIRED); + } return adapter.changePassword(name, user -> user.changePassword(passwordService.encryptPassword(passwordChangeDto.getNewPassword())), userManager.getChangePasswordChecker()); } diff --git a/scm-webapp/src/main/java/sonia/scm/group/DefaultGroupManager.java b/scm-webapp/src/main/java/sonia/scm/group/DefaultGroupManager.java index 327415ec22..a0ef2dbad5 100644 --- a/scm-webapp/src/main/java/sonia/scm/group/DefaultGroupManager.java +++ b/scm-webapp/src/main/java/sonia/scm/group/DefaultGroupManager.java @@ -125,7 +125,7 @@ public class DefaultGroupManager extends AbstractGroupManager } @Override - public void delete(Group group) throws NotFoundException { + public void delete(Group group){ logger.info("delete group {} of type {}", group.getName(), group.getType()); managerDaoAdapter.delete( group, @@ -145,7 +145,7 @@ public class DefaultGroupManager extends AbstractGroupManager public void init(SCMContextProvider context) {} @Override - public void modify(Group group) throws NotFoundException { + public void modify(Group group){ logger.info("modify group {} of type {}", group.getName(), group.getType()); managerDaoAdapter.modify( @@ -160,7 +160,7 @@ public class DefaultGroupManager extends AbstractGroupManager } @Override - public void refresh(Group group) throws NotFoundException { + public void refresh(Group group){ String name = group.getName(); if (logger.isInfoEnabled()) { diff --git a/scm-webapp/src/main/java/sonia/scm/repository/DefaultRepositoryManager.java b/scm-webapp/src/main/java/sonia/scm/repository/DefaultRepositoryManager.java index 8cb325b818..c2e8bc3ad8 100644 --- a/scm-webapp/src/main/java/sonia/scm/repository/DefaultRepositoryManager.java +++ b/scm-webapp/src/main/java/sonia/scm/repository/DefaultRepositoryManager.java @@ -151,7 +151,7 @@ public class DefaultRepositoryManager extends AbstractRepositoryManager { } @Override - public void delete(Repository repository) throws NotFoundException { + public void delete(Repository repository){ logger.info("delete repository {}/{} of type {}", repository.getNamespace(), repository.getName(), repository.getType()); managerDaoAdapter.delete( repository, @@ -179,7 +179,7 @@ public class DefaultRepositoryManager extends AbstractRepositoryManager { } @Override - public void modify(Repository repository) throws NotFoundException { + public void modify(Repository repository){ logger.info("modify repository {}/{} of type {}", repository.getNamespace(), repository.getName(), repository.getType()); managerDaoAdapter.modify( diff --git a/scm-webapp/src/main/java/sonia/scm/repository/HealthChecker.java b/scm-webapp/src/main/java/sonia/scm/repository/HealthChecker.java index 52dea4223b..f943cc7151 100644 --- a/scm-webapp/src/main/java/sonia/scm/repository/HealthChecker.java +++ b/scm-webapp/src/main/java/sonia/scm/repository/HealthChecker.java @@ -55,7 +55,7 @@ public final class HealthChecker { this.repositoryManager = repositoryManager; } - public void check(String id) throws NotFoundException { + public void check(String id){ RepositoryPermissions.healthCheck(id).check(); Repository repository = repositoryManager.get(id); @@ -68,7 +68,7 @@ public final class HealthChecker { } public void check(Repository repository) - throws NotFoundException, ConcurrentModificationException { + { RepositoryPermissions.healthCheck(repository).check(); doCheck(repository); @@ -94,7 +94,7 @@ public final class HealthChecker { } } - private void doCheck(Repository repository) throws NotFoundException { + private void doCheck(Repository repository){ logger.info("start health check for repository {}", repository.getName()); HealthCheckResult result = HealthCheckResult.healthy(); diff --git a/scm-webapp/src/main/java/sonia/scm/user/DefaultUserManager.java b/scm-webapp/src/main/java/sonia/scm/user/DefaultUserManager.java index c9821575d2..9c9d89d341 100644 --- a/scm-webapp/src/main/java/sonia/scm/user/DefaultUserManager.java +++ b/scm-webapp/src/main/java/sonia/scm/user/DefaultUserManager.java @@ -159,7 +159,7 @@ public class DefaultUserManager extends AbstractUserManager } @Override - public void delete(User user) throws NotFoundException { + public void delete(User user) { logger.info("delete user {} of type {}", user.getName(), user.getType()); managerDaoAdapter.delete( user, @@ -195,12 +195,12 @@ public class DefaultUserManager extends AbstractUserManager * @throws IOException */ @Override - public void modify(User user) throws NotFoundException { + public void modify(User user) { modify(user,UserPermissions::modify); } @Override - public void modify(User user, Function permissionChecker) throws NotFoundException { + public void modify(User user, Function permissionChecker) { logger.info("modify user {} of type {}", user.getName(), user.getType()); managerDaoAdapter.modify( user, @@ -218,7 +218,7 @@ public class DefaultUserManager extends AbstractUserManager * @throws IOException */ @Override - public void refresh(User user) throws NotFoundException { + public void refresh(User user) { if (logger.isInfoEnabled()) { logger.info("refresh user {} of type {}", user.getName(), user.getType()); diff --git a/scm-webapp/src/test/java/sonia/scm/repository/DefaultRepositoryManagerTest.java b/scm-webapp/src/test/java/sonia/scm/repository/DefaultRepositoryManagerTest.java index a67c275bc0..4a28c3bc83 100644 --- a/scm-webapp/src/test/java/sonia/scm/repository/DefaultRepositoryManagerTest.java +++ b/scm-webapp/src/test/java/sonia/scm/repository/DefaultRepositoryManagerTest.java @@ -152,7 +152,7 @@ public class DefaultRepositoryManagerTest extends ManagerTestBase { } @Test(expected = NotFoundException.class) - public void testDeleteNotFound() throws NotFoundException { + public void testDeleteNotFound(){ manager.delete(createRepositoryWithId()); } @@ -304,7 +304,7 @@ public class DefaultRepositoryManagerTest extends ManagerTestBase { } @Test - public void testModify() throws NotFoundException, AlreadyExistsException { + public void testModify() throws AlreadyExistsException { Repository heartOfGold = createTestRepository(); heartOfGold.setDescription("prototype ship"); @@ -328,12 +328,12 @@ public class DefaultRepositoryManagerTest extends ManagerTestBase { } @Test(expected = NotFoundException.class) - public void testModifyNotFound() throws NotFoundException { + public void testModifyNotFound(){ manager.modify(createRepositoryWithId()); } @Test - public void testRefresh() throws NotFoundException, AlreadyExistsException { + public void testRefresh() throws AlreadyExistsException { Repository heartOfGold = createTestRepository(); String description = heartOfGold.getDescription(); @@ -354,7 +354,7 @@ public class DefaultRepositoryManagerTest extends ManagerTestBase { } @Test(expected = RepositoryNotFoundException.class) - public void testRefreshNotFound() throws NotFoundException { + public void testRefreshNotFound(){ manager.refresh(createRepositoryWithId()); } @@ -495,7 +495,7 @@ public class DefaultRepositoryManagerTest extends ManagerTestBase { return createRepository(RepositoryTestData.createHeartOfGold()); } - private void delete(Manager manager, Repository repository) throws NotFoundException { + private void delete(Manager manager, Repository repository){ String id = repository.getId(); From c731ecd14d4235c9a75e17ffb2d5001fa7a941a5 Mon Sep 17 00:00:00 2001 From: Mohamed Karray Date: Tue, 16 Oct 2018 12:20:10 +0200 Subject: [PATCH 4/8] fix throw exception --- .../src/main/java/sonia/scm/repository/HealthChecker.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scm-webapp/src/main/java/sonia/scm/repository/HealthChecker.java b/scm-webapp/src/main/java/sonia/scm/repository/HealthChecker.java index f943cc7151..e9c415c4fa 100644 --- a/scm-webapp/src/main/java/sonia/scm/repository/HealthChecker.java +++ b/scm-webapp/src/main/java/sonia/scm/repository/HealthChecker.java @@ -83,7 +83,7 @@ public final class HealthChecker { if (check.isPermitted(repository)) { try { check(repository); - } catch (ConcurrentModificationException | NotFoundException ex) { + } catch (NotFoundException ex) { logger.error("health check ends with exception", ex); } } else { From f0c69d4f30ac418baa9407f3015e7ce32c8f8841 Mon Sep 17 00:00:00 2001 From: Mohamed Karray Date: Tue, 16 Oct 2018 13:18:59 +0200 Subject: [PATCH 5/8] disable sonar validation with id squid:S2068 --- .../java/sonia/scm/user/ChangePasswordNotAllowedException.java | 1 + 1 file changed, 1 insertion(+) 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 caa0c918fc..a942943881 100644 --- a/scm-core/src/main/java/sonia/scm/user/ChangePasswordNotAllowedException.java +++ b/scm-core/src/main/java/sonia/scm/user/ChangePasswordNotAllowedException.java @@ -3,6 +3,7 @@ package sonia.scm.user; public class ChangePasswordNotAllowedException extends RuntimeException { public static final String WRONG_USER_TYPE = "User of type {0} are not allowed to change password"; + @SuppressWarnings("squid:S2068") public static final String OLD_PASSWORD_REQUIRED = "the old password is required."; public ChangePasswordNotAllowedException(String message) { From 9bfb2cdadbd1240edfc594fd680f7350b14efdde Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Pfeuffer?= Date: Wed, 17 Oct 2018 11:58:37 +0200 Subject: [PATCH 6/8] Move password logic to manager --- .../ChangePasswordNotAllowedException.java | 8 +-- .../scm/user/InvalidPasswordException.java | 6 +- .../src/main/java/sonia/scm/user/User.java | 26 ++----- .../main/java/sonia/scm/user/UserManager.java | 16 +++-- .../sonia/scm/user/UserManagerDecorator.java | 11 ++- .../main/java/sonia/scm/web/VndMediaType.java | 2 + .../test/java/sonia/scm/it/UserITCase.java | 24 ------- .../java/sonia/scm/it/utils/ScmRequests.java | 17 +++-- .../resources/AbstractManagerResource.java | 30 ++++---- .../resources/IdResourceManagerAdapter.java | 48 ------------- .../scm/api/v2/resources/MeResource.java | 17 ++--- .../api/v2/resources/PasswordChangeDto.java | 1 + .../v2/resources/PasswordOverwriteDto.java | 14 ++++ .../scm/api/v2/resources/ResourceLinks.java | 2 +- .../SingleResourceManagerAdapter.java | 19 ++--- .../scm/api/v2/resources/UserResource.java | 16 ++--- .../api/v2/resources/UserToUserDtoMapper.java | 6 +- .../DefaultAuthorizationCollector.java | 6 +- .../sonia/scm/user/DefaultUserManager.java | 44 ++++++++---- .../scm/api/v2/resources/MeResourceTest.java | 49 +++++++++---- .../v2/resources/UserRootResourceTest.java | 52 ++++++++++++-- .../v2/resources/UserToUserDtoMapperTest.java | 13 ++-- .../DefaultAuthorizationCollectorTest.java | 6 +- .../scm/user/DefaultUserManagerTest.java | 70 ++++++++++++++++++- 24 files changed, 285 insertions(+), 218 deletions(-) create mode 100644 scm-webapp/src/main/java/sonia/scm/api/v2/resources/PasswordOverwriteDto.java 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 a942943881..fda5e69323 100644 --- a/scm-core/src/main/java/sonia/scm/user/ChangePasswordNotAllowedException.java +++ b/scm-core/src/main/java/sonia/scm/user/ChangePasswordNotAllowedException.java @@ -2,12 +2,10 @@ package sonia.scm.user; public class ChangePasswordNotAllowedException extends RuntimeException { - public static final String WRONG_USER_TYPE = "User of type {0} are not allowed to change password"; - @SuppressWarnings("squid:S2068") - public static final String OLD_PASSWORD_REQUIRED = "the old password is required."; + public static final String WRONG_USER_TYPE = "User of type %s are not allowed to change password"; - public ChangePasswordNotAllowedException(String message) { - super(message); + public ChangePasswordNotAllowedException(String type) { + super(String.format(WRONG_USER_TYPE, type)); } } 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 e06191a8f2..870430a1bb 100644 --- a/scm-core/src/main/java/sonia/scm/user/InvalidPasswordException.java +++ b/scm-core/src/main/java/sonia/scm/user/InvalidPasswordException.java @@ -2,9 +2,7 @@ package sonia.scm.user; public class InvalidPasswordException extends RuntimeException { - public static final String INVALID_MATCHING = "The given Password does not match with the stored one."; - - public InvalidPasswordException(String message) { - super(message); + public InvalidPasswordException() { + super("The given Password does not match with the stored one."); } } diff --git a/scm-core/src/main/java/sonia/scm/user/User.java b/scm-core/src/main/java/sonia/scm/user/User.java index b125561043..cae383a402 100644 --- a/scm-core/src/main/java/sonia/scm/user/User.java +++ b/scm-core/src/main/java/sonia/scm/user/User.java @@ -50,15 +50,16 @@ import javax.xml.bind.annotation.XmlAccessorType; import javax.xml.bind.annotation.XmlRootElement; import java.security.Principal; -import static sonia.scm.user.InvalidPasswordException.INVALID_MATCHING; - //~--- JDK imports ------------------------------------------------------------ /** * * @author Sebastian Sdorra */ -@StaticPermissions(value = "user", globalPermissions = {"create", "list", "autocomplete", "changeOwnPassword"}) +@StaticPermissions( + value = "user", + globalPermissions = {"create", "list", "autocomplete"}, + permissions = {"read", "modify", "delete", "changePassword"}) @XmlRootElement(name = "users") @XmlAccessorType(XmlAccessType.FIELD) public class User extends BasicPropertiesAware implements Principal, ModelObject, PermissionObject, ReducedModelObject @@ -276,25 +277,6 @@ public class User extends BasicPropertiesAware implements Principal, ModelObject //J+ } - public User changePassword(String password){ - setPassword(password); - return this; - } - - /** - * Match given old password from the dto with the stored password before updating - * - * @param newPassword the new password - * @param oldPassword the old password - * @return this - */ - public User changePassword(String newPassword, String oldPassword){ - if (!getPassword().equals(oldPassword)) { - throw new InvalidPasswordException(INVALID_MATCHING); - } - setPassword(newPassword); - return this; - } //~--- get methods ---------------------------------------------------------- /** 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 55f6a17d3f..f301c1f2b1 100644 --- a/scm-core/src/main/java/sonia/scm/user/UserManager.java +++ b/scm-core/src/main/java/sonia/scm/user/UserManager.java @@ -38,11 +38,7 @@ package sonia.scm.user; import sonia.scm.Manager; import sonia.scm.search.Searchable; -import java.text.MessageFormat; import java.util.Collection; -import java.util.function.Consumer; - -import static sonia.scm.user.ChangePasswordNotAllowedException.WRONG_USER_TYPE; /** * The central class for managing {@link User} objects. @@ -87,5 +83,17 @@ public interface UserManager */ Collection autocomplete(String filter); + /** + * Changes the password of the logged in user. + * @param oldPassword The current encrypted password of the user. + * @param newPassword The new encrypted password of the user. + */ + void changePasswordForLoggedInUser(String oldPassword, String newPassword); + /** + * Overwrites the password for the given user id. This needs user write privileges. + * @param userId The id of the user to change the password for. + * @param newPassword The new encrypted password. + */ + void overwritePassword(String userId, String newPassword); } diff --git a/scm-core/src/main/java/sonia/scm/user/UserManagerDecorator.java b/scm-core/src/main/java/sonia/scm/user/UserManagerDecorator.java index f291e325c1..0384fe1b52 100644 --- a/scm-core/src/main/java/sonia/scm/user/UserManagerDecorator.java +++ b/scm-core/src/main/java/sonia/scm/user/UserManagerDecorator.java @@ -126,7 +126,16 @@ public class UserManagerDecorator extends ManagerDecorator return decorated.autocomplete(filter); } - //~--- fields --------------------------------------------------------------- + @Override + public void changePasswordForLoggedInUser(String oldPassword, String newPassword) { + decorated.changePasswordForLoggedInUser(oldPassword, newPassword); + } + + @Override + public void overwritePassword(String userId, String newPassword) { + decorated.overwritePassword(userId, newPassword); + } +//~--- fields --------------------------------------------------------------- /** Field description */ private final UserManager decorated; diff --git a/scm-core/src/main/java/sonia/scm/web/VndMediaType.java b/scm-core/src/main/java/sonia/scm/web/VndMediaType.java index b6f2210d80..7b6d5cb039 100644 --- a/scm-core/src/main/java/sonia/scm/web/VndMediaType.java +++ b/scm-core/src/main/java/sonia/scm/web/VndMediaType.java @@ -39,6 +39,8 @@ public class VndMediaType { public static final String UI_PLUGIN_COLLECTION = PREFIX + "uiPluginCollection" + SUFFIX; @SuppressWarnings("squid:S2068") public static final String PASSWORD_CHANGE = PREFIX + "passwordChange" + SUFFIX; + @SuppressWarnings("squid:S2068") + public static final String PASSWORD_OVERWRITE = PREFIX + "passwordOverwrite" + SUFFIX; public static final String ME = PREFIX + "me" + SUFFIX; public static final String SOURCE = PREFIX + "source" + SUFFIX; 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 b646fea0b1..f517123dcc 100644 --- a/scm-it/src/test/java/sonia/scm/it/UserITCase.java +++ b/scm-it/src/test/java/sonia/scm/it/UserITCase.java @@ -85,30 +85,6 @@ public class UserITCase { .assertStatusCode(403); } - @Test - public void nonAdminUserShouldChangeOwnPassword() { - String newUser = "user1"; - String password = "pass"; - TestData.createUser(newUser, password, false, "xml", "em@l.de"); - String newPassword = "new_password"; - ScmRequests.start() - .requestIndexResource(newUser, password) - .assertUsersLinkDoesNotExists(); - // use the users/password endpoint bypassed the index resource - ScmRequests.start() - .requestUser(newUser, password, newUser) - .assertStatusCode(200) - .assertAdmin(aBoolean -> assertThat(aBoolean).isEqualTo(Boolean.FALSE)) - .requestChangePassword(password, newPassword) // the oldPassword is needed when the own password should be changed - .assertStatusCode(204); -// // assert password is changed -> login with the new Password - ScmRequests.start() - .requestUser(newUser, newPassword, newUser) - .assertStatusCode(200) - .assertAdmin(isAdmin -> assertThat(isAdmin).isEqualTo(Boolean.FALSE)) - .assertPassword(Assert::assertNull); - } - @Test public void shouldHidePasswordLinkIfUserTypeIsNotXML() { String newUser = "user"; diff --git a/scm-it/src/test/java/sonia/scm/it/utils/ScmRequests.java b/scm-it/src/test/java/sonia/scm/it/utils/ScmRequests.java index 6fc8c85140..d15cdc9b7f 100644 --- a/scm-it/src/test/java/sonia/scm/it/utils/ScmRequests.java +++ b/scm-it/src/test/java/sonia/scm/it/utils/ScmRequests.java @@ -2,6 +2,8 @@ package sonia.scm.it.utils; import io.restassured.RestAssured; import io.restassured.response.Response; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import sonia.scm.web.VndMediaType; import java.util.List; @@ -24,7 +26,8 @@ import static sonia.scm.it.utils.TestData.createPasswordChangeJson; */ public class ScmRequests { - private String url; + private static final Logger LOG = LoggerFactory.getLogger(ScmRequests.class); + private String username; private String password; @@ -47,7 +50,7 @@ public class ScmRequests { public ChangePasswordResponse requestUserChangePassword(String username, String password, String userPathParam, String newPassword) { setUsername(username); setPassword(password); - return new ChangePasswordResponse<>(applyPUTRequest(RestUtil.REST_BASE_URL.resolve("users/"+userPathParam+"/password").toString(), VndMediaType.PASSWORD_CHANGE, TestData.createPasswordChangeJson(password,newPassword)), null); + return new ChangePasswordResponse<>(applyPUTRequest(RestUtil.REST_BASE_URL.resolve("users/"+userPathParam+"/password").toString(), VndMediaType.PASSWORD_OVERWRITE, TestData.createPasswordChangeJson(password,newPassword)), null); } @@ -85,6 +88,7 @@ public class ScmRequests { * @return the response of the GET request using the given url */ private Response applyGETRequestWithQueryParams(String url, String params) { + LOG.info("GET {}", url); return RestAssured.given() .auth().preemptive().basic(username, password) .when() @@ -127,6 +131,7 @@ public class ScmRequests { * @return the response of the PUT request using the given url */ private Response applyPUTRequest(String url, String mediaType, String body) { + LOG.info("PUT {}", url); return RestAssured.given() .auth().preemptive().basic(username, password) .when() @@ -144,8 +149,6 @@ public class ScmRequests { this.password = password; } - - public class IndexResponse extends ModelResponse { public static final String LINK_AUTOCOMPLETE_USERS = "_links.autocomplete.find{it.name=='users'}.href"; public static final String LINK_AUTOCOMPLETE_GROUPS = "_links.autocomplete.find{it.name=='groups'}.href"; @@ -292,7 +295,9 @@ public class ScmRequests { super(response, previousResponse); } - + public ChangePasswordResponse requestChangePassword(String oldPassword, String newPassword) { + return new ChangePasswordResponse<>(applyPUTRequestFromLink(super.response, LINKS_PASSWORD_HREF, VndMediaType.PASSWORD_CHANGE, createPasswordChangeJson(oldPassword, newPassword)), this); + } } public class UserResponse extends ModelResponse, PREV> { @@ -328,7 +333,7 @@ public class ScmRequests { } public ChangePasswordResponse requestChangePassword(String oldPassword, String newPassword) { - return new ChangePasswordResponse<>(applyPUTRequestFromLink(super.response, LINKS_PASSWORD_HREF, VndMediaType.PASSWORD_CHANGE, createPasswordChangeJson(oldPassword, newPassword)), this); + return new ChangePasswordResponse<>(applyPUTRequestFromLink(super.response, LINKS_PASSWORD_HREF, VndMediaType.PASSWORD_OVERWRITE, createPasswordChangeJson(oldPassword, newPassword)), this); } } diff --git a/scm-webapp/src/main/java/sonia/scm/api/rest/resources/AbstractManagerResource.java b/scm-webapp/src/main/java/sonia/scm/api/rest/resources/AbstractManagerResource.java index 794f11893b..f00072cdcb 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/rest/resources/AbstractManagerResource.java +++ b/scm-webapp/src/main/java/sonia/scm/api/rest/resources/AbstractManagerResource.java @@ -35,7 +35,6 @@ package sonia.scm.api.rest.resources; //~--- non-JDK imports -------------------------------------------------------- -import com.github.sdorra.ssp.PermissionCheck; import org.apache.commons.beanutils.BeanComparator; import org.apache.shiro.authz.AuthorizationException; import org.slf4j.Logger; @@ -64,8 +63,6 @@ import java.util.Arrays; import java.util.Collection; import java.util.Comparator; import java.util.Date; -import java.util.function.Consumer; -import java.util.function.Function; //~--- JDK imports ------------------------------------------------------------ @@ -199,24 +196,27 @@ public abstract class AbstractManagerResource { return response; } - public Response update(T item, Function permissionChecker ){ - Consumer updateAction = mng -> mng.modify(item, permissionChecker); - return applyUpdate(item, updateAction); - } - - public Response update(String name, T item) { - Consumer updateAction = mng -> mng.modify(item); - return applyUpdate(item, updateAction); - } - - public Response applyUpdate(T item, Consumer updateAction) { + /** + * Method description + * + * + * + * + * @param name + * @param item + * + * + * @return + */ + public Response update(String name, T item) + { Response response = null; preUpdate(item); try { - updateAction.accept(manager); + manager.modify(item); response = Response.noContent().build(); } catch (AuthorizationException ex) diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/IdResourceManagerAdapter.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/IdResourceManagerAdapter.java index 5abc209463..af2b4217e9 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/IdResourceManagerAdapter.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/IdResourceManagerAdapter.java @@ -47,54 +47,6 @@ class IdResourceManagerAdapter getChangePasswordPermission(String usernameToChangePassword) { - AssertUtil.assertIsNotEmpty(usernameToChangePassword); - return model -> { - User user = (User) model; - if (usernameToChangePassword.equals(AuthenticationUtil.getAuthenticatedUsername())) { - return UserPermissions.changeOwnPassword(); - } - return UserPermissions.modify(user); - }; - } - - - /** - * Check if a user can modify the password - * - * 1 - the permission changeOwnPassword should be checked - * 2 - Only account of the default type "xml" can change their password - * - */ - private Consumer getChangePasswordChecker() { - return model -> { - User user = (User) model; - UserPermissions.changeOwnPassword().check(); - UserManager userManager = (UserManager) manager; - if (!userManager.isTypeDefault(user)) { - throw new ChangePasswordNotAllowedException(MessageFormat.format(WRONG_USER_TYPE, user.getType())); - } - }; - } - - - public Response changePassword(String id, Function applyChanges ) throws ConcurrentModificationException { - return singleAdapter.changePassword( - loadBy(id), - applyChanges, - idStaysTheSame(id), - getChangePasswordChecker(), - getChangePasswordPermission(id)); - } - public Response update(String id, Function applyChanges) throws ConcurrentModificationException { return singleAdapter.update( loadBy(id), 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 e30b589c6f..f616aff852 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 @@ -5,14 +5,12 @@ import com.webcohesion.enunciate.metadata.rs.StatusCodes; import com.webcohesion.enunciate.metadata.rs.TypeHint; import org.apache.shiro.SecurityUtils; import org.apache.shiro.authc.credential.PasswordService; -import sonia.scm.ConcurrentModificationException; -import sonia.scm.user.ChangePasswordNotAllowedException; -import sonia.scm.user.InvalidPasswordException; import sonia.scm.user.User; import sonia.scm.user.UserManager; import sonia.scm.web.VndMediaType; import javax.inject.Inject; +import javax.validation.Valid; import javax.ws.rs.Consumes; import javax.ws.rs.GET; import javax.ws.rs.PUT; @@ -22,9 +20,6 @@ import javax.ws.rs.core.Context; import javax.ws.rs.core.Request; import javax.ws.rs.core.Response; import javax.ws.rs.core.UriInfo; -import java.util.function.Consumer; - -import static sonia.scm.user.InvalidPasswordException.INVALID_MATCHING; /** @@ -78,12 +73,8 @@ public class MeResource { }) @TypeHint(TypeHint.NO_CONTENT.class) @Consumes(VndMediaType.PASSWORD_CHANGE) - public Response changePassword(PasswordChangeDto passwordChangeDto) throws ConcurrentModificationException { - String name = (String) SecurityUtils.getSubject().getPrincipals().getPrimaryPrincipal(); - if (passwordChangeDto.getOldPassword() == null){ - throw new ChangePasswordNotAllowedException(ChangePasswordNotAllowedException.OLD_PASSWORD_REQUIRED); - } - return adapter.changePassword(name, user -> user.clone().changePassword(passwordService.encryptPassword(passwordChangeDto.getNewPassword()), passwordService.encryptPassword(passwordChangeDto.getOldPassword()))); + public Response changePassword(@Valid PasswordChangeDto passwordChangeDto) { + userManager.changePasswordForLoggedInUser(passwordService.encryptPassword(passwordChangeDto.getOldPassword()), passwordService.encryptPassword(passwordChangeDto.getNewPassword())); + return Response.noContent().build(); } - } 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 47ad6cd147..8a69c58e86 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 @@ -10,6 +10,7 @@ import org.hibernate.validator.constraints.NotEmpty; @ToString public class PasswordChangeDto { + @NotEmpty private String oldPassword; @NotEmpty diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/PasswordOverwriteDto.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/PasswordOverwriteDto.java new file mode 100644 index 0000000000..0570ed81e1 --- /dev/null +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/PasswordOverwriteDto.java @@ -0,0 +1,14 @@ +package sonia.scm.api.v2.resources; + +import lombok.Getter; +import lombok.Setter; +import lombok.ToString; +import org.hibernate.validator.constraints.NotEmpty; + +@Getter +@Setter +@ToString +public class PasswordOverwriteDto { + @NotEmpty + private String newPassword; +} 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 820e722814..2b37eda9b7 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 @@ -87,7 +87,7 @@ class ResourceLinks { } public String passwordChange(String name) { - return userLinkBuilder.method("getUserResource").parameters(name).method("changePassword").parameters().href(); + return userLinkBuilder.method("getUserResource").parameters(name).method("overwritePassword").parameters().href(); } } diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/SingleResourceManagerAdapter.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/SingleResourceManagerAdapter.java index b5763c4ff0..d484567bf8 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/SingleResourceManagerAdapter.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/SingleResourceManagerAdapter.java @@ -1,6 +1,5 @@ package sonia.scm.api.v2.resources; -import com.github.sdorra.ssp.PermissionCheck; import de.otto.edison.hal.HalRepresentation; import sonia.scm.ConcurrentModificationException; import sonia.scm.Manager; @@ -17,6 +16,8 @@ import java.util.function.Function; import java.util.function.Predicate; import java.util.function.Supplier; +import static javax.ws.rs.core.Response.Status.BAD_REQUEST; + /** * Adapter from resource http endpoints to managers, for Single resources (e.g. {@code /user/name}). * @@ -53,32 +54,26 @@ class SingleResourceManagerAdapter> reader, Function applyChanges, Predicate hasSameKey, Consumer checker, Function permissionCheck) throws ConcurrentModificationException { + public Response update(Supplier> reader, Function applyChanges, Predicate hasSameKey, Consumer checker) throws NotFoundException, ConcurrentModificationException { MODEL_OBJECT existingModelObject = reader.get().orElseThrow(NotFoundException::new); - MODEL_OBJECT changedModelObject = applyChanges.apply(existingModelObject); - checkForUpdate(hasSameKey, existingModelObject, changedModelObject); checker.accept(existingModelObject); - return update(changedModelObject, permissionCheck); + return update(reader,applyChanges,hasSameKey); } /** * Update the model object for the given id according to the given function and returns a corresponding http response. * This handles all corner cases, eg. no matching object for the id or missing privileges. */ - public Response update(Supplier> reader, Function applyChanges, Predicate hasSameKey) throws ConcurrentModificationException { + public Response update(Supplier> reader, Function applyChanges, Predicate hasSameKey) throws NotFoundException, ConcurrentModificationException { MODEL_OBJECT existingModelObject = reader.get().orElseThrow(NotFoundException::new); MODEL_OBJECT changedModelObject = applyChanges.apply(existingModelObject); - checkForUpdate(hasSameKey, existingModelObject, changedModelObject); - return update(getId(existingModelObject), changedModelObject); - } - - public void checkForUpdate(Predicate hasSameKey, MODEL_OBJECT existingModelObject, MODEL_OBJECT changedModelObject) throws ConcurrentModificationException { if (!hasSameKey.test(changedModelObject)) { - throw new IllegalArgumentException("illegal change of id"); + return Response.status(BAD_REQUEST).entity("illegal change of id").build(); } else if (modelObjectWasModifiedConcurrently(existingModelObject, changedModelObject)) { throw new ConcurrentModificationException(); } + return update(getId(existingModelObject), changedModelObject); } private boolean modelObjectWasModifiedConcurrently(MODEL_OBJECT existing, MODEL_OBJECT updated) { 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 5912184f4e..ef4b8a6eb4 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 @@ -3,10 +3,8 @@ package sonia.scm.api.v2.resources; import com.webcohesion.enunciate.metadata.rs.ResponseCode; import com.webcohesion.enunciate.metadata.rs.StatusCodes; import com.webcohesion.enunciate.metadata.rs.TypeHint; -import org.apache.shiro.SecurityUtils; import org.apache.shiro.authc.credential.PasswordService; import sonia.scm.ConcurrentModificationException; -import sonia.scm.user.ChangePasswordNotAllowedException; import sonia.scm.user.User; import sonia.scm.user.UserManager; import sonia.scm.web.VndMediaType; @@ -116,11 +114,11 @@ public class UserResource { * Note: This method requires "user:changeOwnPassword" privilege to modify the own password. * * @param name name of the user to be modified - * @param passwordChangeDto change password object to modify password. the old password is here not required + * @param passwordOverwriteDto change password object to modify password. the old password is here not required */ @PUT @Path("password") - @Consumes(VndMediaType.PASSWORD_CHANGE) + @Consumes(VndMediaType.PASSWORD_OVERWRITE) @StatusCodes({ @ResponseCode(code = 204, condition = "update success"), @ResponseCode(code = 400, condition = "Invalid body, e.g. the user type is not xml or the given oldPassword do not match the stored one"), @@ -130,12 +128,8 @@ public class UserResource { @ResponseCode(code = 500, condition = "internal server error") }) @TypeHint(TypeHint.NO_CONTENT.class) - public Response changePassword(@PathParam("id") String name, @Valid PasswordChangeDto passwordChangeDto) throws ConcurrentModificationException { - String currentUserName = (String) SecurityUtils.getSubject().getPrincipals().getPrimaryPrincipal(); - if (currentUserName.equals(name) && passwordChangeDto.getOldPassword() == null){ - throw new ChangePasswordNotAllowedException(ChangePasswordNotAllowedException.OLD_PASSWORD_REQUIRED); - } - return adapter.changePassword(name, user -> user.changePassword(passwordService.encryptPassword(passwordChangeDto.getNewPassword()))); + public Response overwritePassword(@PathParam("id") String name, @Valid PasswordOverwriteDto passwordOverwriteDto) { + userManager.overwritePassword(name, passwordService.encryptPassword(passwordOverwriteDto.getNewPassword())); + return Response.noContent().build(); } - } 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 832829883b..eb49ff5f3d 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 @@ -39,9 +39,9 @@ public abstract class UserToUserDtoMapper extends BaseMapper { } if (UserPermissions.modify(user).isPermitted()) { linksBuilder.single(link("update", resourceLinks.user().update(target.getName()))); - } - if (userManager.isTypeDefault(user)) { - linksBuilder.single(link("password", resourceLinks.user().passwordChange(target.getName()))); + if (userManager.isTypeDefault(user)) { + linksBuilder.single(link("password", resourceLinks.user().passwordChange(target.getName()))); + } } target.add(linksBuilder.build()); } diff --git a/scm-webapp/src/main/java/sonia/scm/security/DefaultAuthorizationCollector.java b/scm-webapp/src/main/java/sonia/scm/security/DefaultAuthorizationCollector.java index 265e163c23..faa7208c66 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/DefaultAuthorizationCollector.java +++ b/scm-webapp/src/main/java/sonia/scm/security/DefaultAuthorizationCollector.java @@ -260,7 +260,7 @@ public class DefaultAuthorizationCollector implements AuthorizationCollector builder.add(canReadOwnUser(user)); builder.add(getUserAutocompletePermission()); builder.add(getGroupAutocompletePermission()); - builder.add(getChangeOwnPasswordPermission()); + builder.add(getChangeOwnPasswordPermission(user)); permissions = builder.build(); } @@ -273,8 +273,8 @@ public class DefaultAuthorizationCollector implements AuthorizationCollector return GroupPermissions.autocomplete().asShiroString(); } - private String getChangeOwnPasswordPermission() { - return UserPermissions.changeOwnPassword().asShiroString(); + private String getChangeOwnPasswordPermission(User user) { + return UserPermissions.changePassword(user).asShiroString(); } private String getUserAutocompletePermission() { diff --git a/scm-webapp/src/main/java/sonia/scm/user/DefaultUserManager.java b/scm-webapp/src/main/java/sonia/scm/user/DefaultUserManager.java index 2e3981b2cf..4efaaa1d1c 100644 --- a/scm-webapp/src/main/java/sonia/scm/user/DefaultUserManager.java +++ b/scm-webapp/src/main/java/sonia/scm/user/DefaultUserManager.java @@ -33,12 +33,10 @@ package sonia.scm.user; -//~--- non-JDK imports -------------------------------------------------------- - import com.github.sdorra.ssp.PermissionActionCheck; -import com.github.sdorra.ssp.PermissionCheck; import com.google.inject.Inject; import com.google.inject.Singleton; +import org.apache.shiro.SecurityUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import sonia.scm.AlreadyExistsException; @@ -64,9 +62,6 @@ import java.util.Collection; import java.util.Collections; import java.util.Comparator; import java.util.List; -import java.util.function.Function; - -//~--- JDK imports ------------------------------------------------------------ /** * @@ -196,15 +191,10 @@ public class DefaultUserManager extends AbstractUserManager */ @Override public void modify(User user) { - modify(user,UserPermissions::modify); - } - - @Override - public void modify(User user, Function permissionChecker) { logger.info("modify user {} of type {}", user.getName(), user.getType()); managerDaoAdapter.modify( user, - permissionChecker, + UserPermissions::modify, notModified -> fireEvent(HandlerEventType.BEFORE_MODIFY, user, notModified), notModified -> fireEvent(HandlerEventType.MODIFY, user, notModified)); } @@ -408,6 +398,36 @@ public class DefaultUserManager extends AbstractUserManager //~--- methods -------------------------------------------------------------- + @Override + public void changePasswordForLoggedInUser(String oldPassword, String newPassword) { + User user = get((String) SecurityUtils.getSubject().getPrincipals().getPrimaryPrincipal()); + + if (!user.getPassword().equals(oldPassword)) { + throw new InvalidPasswordException(); + } + + user.setPassword(newPassword); + + managerDaoAdapter.modify( + user, + UserPermissions::changePassword, + notModified -> fireEvent(HandlerEventType.BEFORE_MODIFY, user, notModified), + notModified -> fireEvent(HandlerEventType.MODIFY, user, notModified)); + } + + @Override + public void overwritePassword(String userId, String newPassword) { + User user = get(userId); + if (user == null) { + throw new NotFoundException(); + } + if (!isTypeDefault(user)) { + throw new ChangePasswordNotAllowedException(user.getType()); + } + user.setPassword(newPassword); + this.modify(user); + } + /** * Method description * 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 9da9b1dd3d..143b469650 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 @@ -13,6 +13,7 @@ import org.junit.Test; import org.mockito.ArgumentCaptor; import org.mockito.InjectMocks; import org.mockito.Mock; +import sonia.scm.user.InvalidPasswordException; import sonia.scm.user.User; import sonia.scm.user.UserManager; import sonia.scm.web.VndMediaType; @@ -27,6 +28,7 @@ import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.mockito.MockitoAnnotations.initMocks; @@ -97,6 +99,7 @@ public class MeResourceTest { public void shouldEncryptPasswordBeforeChanging() throws Exception { String newPassword = "pwd123"; String encryptedNewPassword = "encrypted123"; + String encryptedOldPassword = "encryptedOld"; String oldPassword = "secret"; String content = String.format("{ \"oldPassword\": \"%s\" , \"newPassword\": \"%s\" }", oldPassword, newPassword); MockHttpRequest request = MockHttpRequest @@ -104,33 +107,32 @@ public class MeResourceTest { .contentType(VndMediaType.PASSWORD_CHANGE) .content(content.getBytes()); MockHttpResponse response = new MockHttpResponse(); + when(passwordService.encryptPassword(newPassword)).thenReturn(encryptedNewPassword); - when(passwordService.encryptPassword(oldPassword)).thenReturn("secret"); - ArgumentCaptor modifyUserCaptor = ArgumentCaptor.forClass(User.class); - doNothing().when(userManager).modify(modifyUserCaptor.capture(), any()); + when(passwordService.encryptPassword(oldPassword)).thenReturn(encryptedOldPassword); + + ArgumentCaptor encryptedOldPasswordCaptor = ArgumentCaptor.forClass(String.class); + ArgumentCaptor encryptedNewPasswordCaptor = ArgumentCaptor.forClass(String.class); + doNothing().when(userManager).changePasswordForLoggedInUser(encryptedOldPasswordCaptor.capture(), encryptedNewPasswordCaptor.capture()); dispatcher.invoke(request, response); assertEquals(HttpServletResponse.SC_NO_CONTENT, response.getStatus()); - verify(userManager).modify(any(), any()); - User updatedUser = modifyUserCaptor.getValue(); - assertEquals(encryptedNewPassword, updatedUser.getPassword()); + assertEquals(encryptedNewPassword, encryptedNewPasswordCaptor.getValue()); + assertEquals(encryptedOldPassword, encryptedOldPasswordCaptor.getValue()); } @Test @SubjectAware(username = "trillian", password = "secret") - public void shouldGet400OnChangePasswordOfUserWithNonDefaultType() throws Exception { + public void shouldGet400OnMissingOldPassword() throws Exception { originalUser.setType("not an xml type"); String newPassword = "pwd123"; - String oldPassword = "secret"; - String content = String.format("{ \"oldPassword\": \"%s\" , \"newPassword\": \"%s\" }", oldPassword, newPassword); + String content = String.format("{ \"newPassword\": \"%s\" }", newPassword); MockHttpRequest request = MockHttpRequest .put("/" + MeResource.ME_PATH_V2 + "password") .contentType(VndMediaType.PASSWORD_CHANGE) .content(content.getBytes()); MockHttpResponse response = new MockHttpResponse(); - when(passwordService.encryptPassword(eq(newPassword))).thenReturn("encrypted123"); - when(passwordService.encryptPassword(eq(oldPassword))).thenReturn("secret"); dispatcher.invoke(request, response); @@ -139,17 +141,34 @@ public class MeResourceTest { @Test @SubjectAware(username = "trillian", password = "secret") - public void shouldGet400OnChangePasswordIfOldPasswordDoesNotMatchOriginalPassword() throws Exception { + public void shouldGet400OnMissingEmptyPassword() throws Exception { String newPassword = "pwd123"; - String oldPassword = "notEncriptedSecret"; + String oldPassword = ""; String content = String.format("{ \"oldPassword\": \"%s\" , \"newPassword\": \"%s\" }", oldPassword, newPassword); MockHttpRequest request = MockHttpRequest .put("/" + MeResource.ME_PATH_V2 + "password") .contentType(VndMediaType.PASSWORD_CHANGE) .content(content.getBytes()); MockHttpResponse response = new MockHttpResponse(); - when(passwordService.encryptPassword(newPassword)).thenReturn("encrypted123"); - when(passwordService.encryptPassword(eq(oldPassword))).thenReturn("differentThanSecret"); + + dispatcher.invoke(request, response); + + assertEquals(HttpServletResponse.SC_BAD_REQUEST, response.getStatus()); + } + + @Test + @SubjectAware(username = "trillian", password = "secret") + public void shouldMapExceptionFromManager() throws Exception { + String newPassword = "pwd123"; + String oldPassword = "secret"; + String content = String.format("{ \"oldPassword\": \"%s\" , \"newPassword\": \"%s\" }", oldPassword, newPassword); + MockHttpRequest request = MockHttpRequest + .put("/" + MeResource.ME_PATH_V2 + "password") + .contentType(VndMediaType.PASSWORD_CHANGE) + .content(content.getBytes()); + MockHttpResponse response = new MockHttpResponse(); + + doThrow(InvalidPasswordException.class).when(userManager).changePasswordForLoggedInUser(any(), any()); dispatcher.invoke(request, response); diff --git a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/UserRootResourceTest.java b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/UserRootResourceTest.java index bced4e6545..b363b66bfb 100644 --- a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/UserRootResourceTest.java +++ b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/UserRootResourceTest.java @@ -14,7 +14,9 @@ import org.junit.Test; import org.mockito.ArgumentCaptor; import org.mockito.InjectMocks; import org.mockito.Mock; +import sonia.scm.NotFoundException; import sonia.scm.PageResult; +import sonia.scm.user.ChangePasswordNotAllowedException; import sonia.scm.user.User; import sonia.scm.user.UserManager; import sonia.scm.web.VndMediaType; @@ -31,6 +33,7 @@ import static org.junit.Assert.assertTrue; import static org.mockito.Matchers.any; import static org.mockito.Matchers.eq; import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -142,7 +145,7 @@ public class UserRootResourceTest { String content = String.format("{\"newPassword\": \"%s\"}", newPassword); MockHttpRequest request = MockHttpRequest .put("/" + UserRootResource.USERS_PATH_V2 + "Neo/password") - .contentType(VndMediaType.PASSWORD_CHANGE) + .contentType(VndMediaType.PASSWORD_OVERWRITE) .content(content.getBytes()); MockHttpResponse response = new MockHttpResponse(); when(passwordService.encryptPassword(newPassword)).thenReturn("encrypted123"); @@ -150,26 +153,61 @@ public class UserRootResourceTest { dispatcher.invoke(request, response); assertEquals(HttpServletResponse.SC_NO_CONTENT, response.getStatus()); - verify(userManager).modify(any(), any()); - User updatedUser = userCaptor.getValue(); - assertEquals("encrypted123", updatedUser.getPassword()); + verify(userManager).overwritePassword("Neo", "encrypted123"); } @Test - public void shouldGet400OnChangePasswordOfUserWithNonDefaultType() throws Exception { + public void shouldGet400OnOverwritePasswordWhenManagerThrowsNotAllowed() throws Exception { originalUser.setType("not an xml type"); String newPassword = "pwd123"; String content = String.format("{\"newPassword\": \"%s\"}", newPassword); MockHttpRequest request = MockHttpRequest .put("/" + UserRootResource.USERS_PATH_V2 + "Neo/password") - .contentType(VndMediaType.PASSWORD_CHANGE) + .contentType(VndMediaType.PASSWORD_OVERWRITE) + .content(content.getBytes()); + MockHttpResponse response = new MockHttpResponse(); + + doThrow(ChangePasswordNotAllowedException.class).when(userManager).overwritePassword(any(), any()); + + dispatcher.invoke(request, response); + + assertEquals(HttpServletResponse.SC_BAD_REQUEST, response.getStatus()); + } + + @Test + public void shouldGet404OnOverwritePasswordWhenNotFound() throws Exception { + originalUser.setType("not an xml type"); + String newPassword = "pwd123"; + String content = String.format("{\"newPassword\": \"%s\"}", newPassword); + MockHttpRequest request = MockHttpRequest + .put("/" + UserRootResource.USERS_PATH_V2 + "Neo/password") + .contentType(VndMediaType.PASSWORD_OVERWRITE) + .content(content.getBytes()); + MockHttpResponse response = new MockHttpResponse(); + + doThrow(NotFoundException.class).when(userManager).overwritePassword(any(), any()); + + dispatcher.invoke(request, response); + + assertEquals(HttpServletResponse.SC_NOT_FOUND, response.getStatus()); + } + + @Test + public void shouldEncryptPasswordOnOverwritePassword() throws Exception { + originalUser.setType("not an xml type"); + String newPassword = "pwd123"; + String content = String.format("{\"newPassword\": \"%s\"}", newPassword); + MockHttpRequest request = MockHttpRequest + .put("/" + UserRootResource.USERS_PATH_V2 + "Neo/password") + .contentType(VndMediaType.PASSWORD_OVERWRITE) .content(content.getBytes()); MockHttpResponse response = new MockHttpResponse(); when(passwordService.encryptPassword(newPassword)).thenReturn("encrypted123"); dispatcher.invoke(request, response); - assertEquals(HttpServletResponse.SC_BAD_REQUEST, response.getStatus()); + assertEquals(HttpServletResponse.SC_NO_CONTENT, response.getStatus()); + verify(userManager).overwritePassword("Neo", "encrypted123"); } @Test diff --git a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/UserToUserDtoMapperTest.java b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/UserToUserDtoMapperTest.java index 7570a3f162..dfff933f19 100644 --- a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/UserToUserDtoMapperTest.java +++ b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/UserToUserDtoMapperTest.java @@ -65,7 +65,7 @@ public class UserToUserDtoMapperTest { } @Test - public void shouldGetPasswordLinkOnlyForDefaultUserType() { + public void shouldGetPasswordLinkForAdmin() { User user = createDefaultUser(); when(subject.isPermitted("user:modify:abc")).thenReturn(true); when(userManager.isTypeDefault(eq(user))).thenReturn(true); @@ -73,14 +73,15 @@ public class UserToUserDtoMapperTest { UserDto userDto = mapper.map(user); assertEquals("expected password link with modify permission", expectedBaseUri.resolve("abc/password").toString(), userDto.getLinks().getLinkBy("password").get().getHref()); + } - when(subject.isPermitted("user:modify:abc")).thenReturn(false); - userDto = mapper.map(user); - assertEquals("expected password link on mission modify permission", expectedBaseUri.resolve("abc/password").toString(), userDto.getLinks().getLinkBy("password").get().getHref()); - + @Test + public void shouldGetPasswordLinkOnlyForDefaultUserType() { + User user = createDefaultUser(); + when(subject.isPermitted("user:modify:abc")).thenReturn(true); when(userManager.isTypeDefault(eq(user))).thenReturn(false); - userDto = mapper.map(user); + UserDto userDto = mapper.map(user); assertFalse("expected no password link", userDto.getLinks().getLinkBy("password").isPresent()); } diff --git a/scm-webapp/src/test/java/sonia/scm/security/DefaultAuthorizationCollectorTest.java b/scm-webapp/src/test/java/sonia/scm/security/DefaultAuthorizationCollectorTest.java index a85d03991d..2f455469dd 100644 --- a/scm-webapp/src/test/java/sonia/scm/security/DefaultAuthorizationCollectorTest.java +++ b/scm-webapp/src/test/java/sonia/scm/security/DefaultAuthorizationCollectorTest.java @@ -159,7 +159,7 @@ public class DefaultAuthorizationCollectorTest { AuthorizationInfo authInfo = collector.collect(); assertThat(authInfo.getRoles(), Matchers.contains(Role.USER)); assertThat(authInfo.getStringPermissions(), hasSize(4)); - assertThat(authInfo.getStringPermissions(), containsInAnyOrder("user:autocomplete", "group:autocomplete", "user:changeOwnPassword", "user:read:trillian")); + assertThat(authInfo.getStringPermissions(), containsInAnyOrder("user:autocomplete", "group:autocomplete", "user:changePassword:trillian", "user:read:trillian")); assertThat(authInfo.getObjectPermissions(), nullValue()); } @@ -204,7 +204,7 @@ public class DefaultAuthorizationCollectorTest { AuthorizationInfo authInfo = collector.collect(); assertThat(authInfo.getRoles(), Matchers.containsInAnyOrder(Role.USER)); assertThat(authInfo.getObjectPermissions(), nullValue()); - assertThat(authInfo.getStringPermissions(), containsInAnyOrder("user:autocomplete", "group:autocomplete", "user:changeOwnPassword", "repository:read,pull:one", "repository:read,pull,push:two", "user:read:trillian")); + assertThat(authInfo.getStringPermissions(), containsInAnyOrder("user:autocomplete", "group:autocomplete", "user:changePassword:trillian", "repository:read,pull:one", "repository:read,pull,push:two", "user:read:trillian")); } /** @@ -225,7 +225,7 @@ public class DefaultAuthorizationCollectorTest { AuthorizationInfo authInfo = collector.collect(); assertThat(authInfo.getRoles(), Matchers.containsInAnyOrder(Role.USER)); assertThat(authInfo.getObjectPermissions(), nullValue()); - assertThat(authInfo.getStringPermissions(), containsInAnyOrder("one:one", "two:two", "user:read:trillian", "user:autocomplete", "group:autocomplete", "user:changeOwnPassword")); + assertThat(authInfo.getStringPermissions(), containsInAnyOrder("one:one", "two:two", "user:read:trillian", "user:autocomplete", "group:autocomplete", "user:changePassword:trillian")); } private void authenticate(User user, String group, String... groups) { diff --git a/scm-webapp/src/test/java/sonia/scm/user/DefaultUserManagerTest.java b/scm-webapp/src/test/java/sonia/scm/user/DefaultUserManagerTest.java index eb892c7e89..df32528e5e 100644 --- a/scm-webapp/src/test/java/sonia/scm/user/DefaultUserManagerTest.java +++ b/scm-webapp/src/test/java/sonia/scm/user/DefaultUserManagerTest.java @@ -39,9 +39,13 @@ import com.github.sdorra.shiro.ShiroRule; import com.github.sdorra.shiro.SubjectAware; import com.google.common.collect.Lists; +import org.assertj.core.api.Assertions; +import org.junit.Assert; import org.junit.Before; import org.junit.Test; +import org.mockito.ArgumentCaptor; +import sonia.scm.NotFoundException; import sonia.scm.store.JAXBConfigurationStoreFactory; import sonia.scm.user.xml.XmlUserDAO; import sonia.scm.util.MockUtil; @@ -70,6 +74,10 @@ public class DefaultUserManagerTest extends UserManagerTestBase @Rule public ShiroRule shiro = new ShiroRule(); + + private UserDAO userDAO = mock(UserDAO.class); + private User trillian; + /** * Method description * @@ -82,6 +90,16 @@ public class DefaultUserManagerTest extends UserManagerTestBase return new DefaultUserManager(createXmlUserDAO()); } + @Before + public void initDao() { + trillian = UserTestData.createTrillian(); + trillian.setPassword("oldEncrypted"); + + userDAO = mock(UserDAO.class); + when(userDAO.getType()).thenReturn("xml"); + when(userDAO.get("trillian")).thenReturn(trillian); + } + /** * Method description * @@ -89,7 +107,6 @@ public class DefaultUserManagerTest extends UserManagerTestBase @Test public void testDefaultAccountAfterFristStart() { - UserDAO userDAO = mock(UserDAO.class); List users = Lists.newArrayList(new User("tuser")); when(userDAO.getAll()).thenReturn(users); @@ -108,8 +125,6 @@ public class DefaultUserManagerTest extends UserManagerTestBase @SuppressWarnings("unchecked") public void testDefaultAccountCreation() { - UserDAO userDAO = mock(UserDAO.class); - when(userDAO.getAll()).thenReturn(Collections.EMPTY_LIST); UserManager userManager = new DefaultUserManager(userDAO); @@ -118,6 +133,55 @@ public class DefaultUserManagerTest extends UserManagerTestBase verify(userDAO, times(2)).add(any(User.class)); } + @Test(expected = InvalidPasswordException.class) + public void shouldFailChangePasswordForWrongOldPassword() { + UserManager userManager = new DefaultUserManager(userDAO); + + userManager.changePasswordForLoggedInUser("wrongPassword", "---"); + } + + @Test + public void shouldSucceedChangePassword() { + ArgumentCaptor userCaptor = ArgumentCaptor.forClass(User.class); + + doNothing().when(userDAO).modify(userCaptor.capture()); + + UserManager userManager = new DefaultUserManager(userDAO); + + userManager.changePasswordForLoggedInUser("oldEncrypted", "newEncrypted"); + + Assertions.assertThat(userCaptor.getValue().getPassword()).isEqualTo("newEncrypted"); + } + + @Test(expected = ChangePasswordNotAllowedException.class) + public void shouldFailOverwritePasswordForWrongType() { + trillian.setType("wrongType"); + + UserManager userManager = new DefaultUserManager(userDAO); + + userManager.overwritePassword("trillian", "---"); + } + + @Test(expected = NotFoundException.class) + public void shouldFailOverwritePasswordForMissingUser() { + UserManager userManager = new DefaultUserManager(userDAO); + + userManager.overwritePassword("notExisting", "---"); + } + + @Test + public void shouldSucceedOverwritePassword() { + ArgumentCaptor userCaptor = ArgumentCaptor.forClass(User.class); + + doNothing().when(userDAO).modify(userCaptor.capture()); + + UserManager userManager = new DefaultUserManager(userDAO); + + userManager.overwritePassword("trillian", "newEncrypted"); + + Assertions.assertThat(userCaptor.getValue().getPassword()).isEqualTo("newEncrypted"); + } + //~--- methods -------------------------------------------------------------- private XmlUserDAO createXmlUserDAO() { From 3c7df066f0e8f11e40edd735e7f5531809490ef3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Pfeuffer?= Date: Wed, 17 Oct 2018 13:25:07 +0200 Subject: [PATCH 7/8] Cleanup --- .../src/main/java/sonia/scm/HandlerBase.java | 14 ---------- .../sonia/scm/util/AuthenticationUtil.java | 12 --------- .../test/java/sonia/scm/it/UserITCase.java | 2 +- .../java/sonia/scm/it/utils/ScmRequests.java | 26 ++++++++----------- .../resources/IdResourceManagerAdapter.java | 11 -------- 5 files changed, 12 insertions(+), 53 deletions(-) delete mode 100644 scm-core/src/main/java/sonia/scm/util/AuthenticationUtil.java diff --git a/scm-core/src/main/java/sonia/scm/HandlerBase.java b/scm-core/src/main/java/sonia/scm/HandlerBase.java index 42c159a767..bbe78c8cf7 100644 --- a/scm-core/src/main/java/sonia/scm/HandlerBase.java +++ b/scm-core/src/main/java/sonia/scm/HandlerBase.java @@ -35,11 +35,8 @@ package sonia.scm; //~--- JDK imports ------------------------------------------------------------ -import com.github.sdorra.ssp.PermissionCheck; - import java.io.Closeable; import java.io.IOException; -import java.util.function.Function; /** * The base class of all handlers. @@ -78,15 +75,4 @@ public interface HandlerBase * @throws IOException */ void modify(T object) throws NotFoundException; - - /** - * Modifies a persistent object after checking with the given permission checker. - * - * @param object to modify - * @param permissionChecker check permission before to modify - * @throws IOException - */ - default void modify(T object, Function permissionChecker) throws NotFoundException{ - modify(object); - } } diff --git a/scm-core/src/main/java/sonia/scm/util/AuthenticationUtil.java b/scm-core/src/main/java/sonia/scm/util/AuthenticationUtil.java deleted file mode 100644 index a0c021fa3c..0000000000 --- a/scm-core/src/main/java/sonia/scm/util/AuthenticationUtil.java +++ /dev/null @@ -1,12 +0,0 @@ -package sonia.scm.util; - -import org.apache.shiro.SecurityUtils; - -public class AuthenticationUtil { - - public static String getAuthenticatedUsername() { - Object subject = SecurityUtils.getSubject().getPrincipal(); - AssertUtil.assertIsNotNull(subject); - return subject.toString(); - } -} 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 f517123dcc..4c7e3b2cac 100644 --- a/scm-it/src/test/java/sonia/scm/it/UserITCase.java +++ b/scm-it/src/test/java/sonia/scm/it/UserITCase.java @@ -29,7 +29,7 @@ public class UserITCase { .assertStatusCode(200) .assertAdmin(aBoolean -> assertThat(aBoolean).isEqualTo(Boolean.TRUE)) .assertPassword(Assert::assertNull) - .requestChangePassword(password, newPassword) // the oldPassword is needed when the own password should be changed + .requestChangePassword(newPassword) .assertStatusCode(204); // assert password is changed -> login with the new Password ScmRequests.start() diff --git a/scm-it/src/test/java/sonia/scm/it/utils/ScmRequests.java b/scm-it/src/test/java/sonia/scm/it/utils/ScmRequests.java index d15cdc9b7f..69c79c37bf 100644 --- a/scm-it/src/test/java/sonia/scm/it/utils/ScmRequests.java +++ b/scm-it/src/test/java/sonia/scm/it/utils/ScmRequests.java @@ -4,6 +4,7 @@ import io.restassured.RestAssured; import io.restassured.response.Response; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import sonia.scm.user.User; import sonia.scm.web.VndMediaType; import java.util.List; @@ -41,7 +42,7 @@ public class ScmRequests { return new IndexResponse(applyGETRequest(RestUtil.REST_BASE_URL.toString())); } - public UserResponse requestUser(String username, String password, String pathParam) { + public , T extends ModelResponse> UserResponse requestUser(String username, String password, String pathParam) { setUsername(username); setPassword(password); return new UserResponse<>(applyGETRequest(RestUtil.REST_BASE_URL.resolve("users/"+pathParam).toString()), null); @@ -176,7 +177,7 @@ public class ScmRequests { return new MeResponse<>(applyGETRequestFromLink(response, LINK_ME), this); } - public UserResponse requestUser(String username) { + public UserResponse requestUser(String username) { return new UserResponse<>(applyGETRequestFromLinkWithParams(response, LINK_USERS, username), this); } @@ -288,7 +289,7 @@ public class ScmRequests { } - public class MeResponse extends UserResponse { + public class MeResponse extends UserResponse, PREV> { public MeResponse(Response response, PREV previousResponse) { @@ -300,7 +301,7 @@ public class ScmRequests { } } - public class UserResponse extends ModelResponse, PREV> { + public class UserResponse, PREV extends ModelResponse> extends ModelResponse { public static final String LINKS_PASSWORD_HREF = "_links.password.href"; @@ -308,34 +309,29 @@ public class ScmRequests { super(response, previousResponse); } - public UserResponse assertPassword(Consumer assertPassword) { + public SELF assertPassword(Consumer assertPassword) { return super.assertSingleProperty(assertPassword, "password"); } - public UserResponse assertType(Consumer assertType) { + public SELF assertType(Consumer assertType) { return assertSingleProperty(assertType, "type"); } - public UserResponse assertAdmin(Consumer assertAdmin) { + public SELF assertAdmin(Consumer assertAdmin) { return assertSingleProperty(assertAdmin, "admin"); } - public UserResponse assertPasswordLinkDoesNotExists() { + public SELF assertPasswordLinkDoesNotExists() { return assertPropertyPathDoesNotExists(LINKS_PASSWORD_HREF); } - public UserResponse assertPasswordLinkExists() { + public SELF assertPasswordLinkExists() { return assertPropertyPathExists(LINKS_PASSWORD_HREF); } public ChangePasswordResponse requestChangePassword(String newPassword) { - return requestChangePassword(null, newPassword); + return new ChangePasswordResponse<>(applyPUTRequestFromLink(super.response, LINKS_PASSWORD_HREF, VndMediaType.PASSWORD_OVERWRITE, createPasswordChangeJson(null, newPassword)), this); } - - public ChangePasswordResponse requestChangePassword(String oldPassword, String newPassword) { - return new ChangePasswordResponse<>(applyPUTRequestFromLink(super.response, LINKS_PASSWORD_HREF, VndMediaType.PASSWORD_OVERWRITE, createPasswordChangeJson(oldPassword, newPassword)), this); - } - } diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/IdResourceManagerAdapter.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/IdResourceManagerAdapter.java index af2b4217e9..9fe3d8284d 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/IdResourceManagerAdapter.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/IdResourceManagerAdapter.java @@ -1,29 +1,18 @@ package sonia.scm.api.v2.resources; -import com.github.sdorra.ssp.PermissionCheck; import de.otto.edison.hal.HalRepresentation; import sonia.scm.AlreadyExistsException; import sonia.scm.ConcurrentModificationException; import sonia.scm.Manager; import sonia.scm.ModelObject; import sonia.scm.PageResult; -import sonia.scm.user.ChangePasswordNotAllowedException; -import sonia.scm.user.User; -import sonia.scm.user.UserManager; -import sonia.scm.user.UserPermissions; -import sonia.scm.util.AssertUtil; -import sonia.scm.util.AuthenticationUtil; import javax.ws.rs.core.Response; -import java.text.MessageFormat; import java.util.Optional; -import java.util.function.Consumer; import java.util.function.Function; import java.util.function.Predicate; import java.util.function.Supplier; -import static sonia.scm.user.ChangePasswordNotAllowedException.WRONG_USER_TYPE; - /** * Facade for {@link SingleResourceManagerAdapter} and {@link CollectionResourceManagerAdapter} * for model objects handled by a single id. From b775ac12b2d5beb81b13b718f498d4a2540495c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Pfeuffer?= Date: Wed, 17 Oct 2018 11:48:27 +0000 Subject: [PATCH 8/8] Close branch feature/modify_password_for_user_v2