add permission to modify the own password over the me and the user endpoints

This commit is contained in:
Mohamed Karray
2018-10-12 15:20:58 +02:00
parent 67b3630c16
commit 023b362f68
19 changed files with 191 additions and 91 deletions

View File

@@ -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<T extends TypedObject>
* @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<T, PermissionCheck> permissionChecker) throws NotFoundException{
modify(object);
}
}

View File

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

View File

@@ -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

View File

@@ -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<User> getUserTypeChecker() {
default Consumer<User> getChangePasswordChecker() {
return user -> {
UserPermissions.changeOwnPassword().check();
if (!isTypeDefault(user)) {
throw new ChangePasswordNotAllowedException(MessageFormat.format(WRONG_USER_TYPE, user.getType()));
}

View File

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

View File

@@ -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() {

View File

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

View File

@@ -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<T extends ModelObject> {
return response;
}
/**
* Method description
*
*
*
*
* @param name
* @param item
*
*
* @return
*/
public Response update(String name, T item)
{
public Response update(T item, Function<T, PermissionCheck> permissionChecker ){
Consumer<Manager> updateAction = mng -> mng.modify(item, permissionChecker);
return applyUpdate(item, updateAction);
}
public Response update(String name, T item) {
Consumer<Manager> updateAction = mng -> mng.modify(item);
return applyUpdate(item, updateAction);
}
public Response applyUpdate(T item, Consumer<Manager> updateAction) {
Response response = null;
preUpdate(item);
try
{
manager.modify(item);
updateAction.accept(manager);
response = Response.noContent().build();
}
catch (AuthorizationException ex)

View File

@@ -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<MODEL_OBJECT extends ModelObject,
return singleAdapter.get(loadBy(id), mapToDto);
}
public Response update(String id, Function<MODEL_OBJECT, MODEL_OBJECT> applyChanges, Consumer<MODEL_OBJECT> 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<User, PermissionCheck> 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<MODEL_OBJECT, MODEL_OBJECT> applyChanges, Consumer<MODEL_OBJECT> checker, Function<MODEL_OBJECT, PermissionCheck> permissionCheck) throws NotFoundException, ConcurrentModificationException {
return singleAdapter.changePassword(
loadBy(id),
applyChanges,
idStaysTheSame(id),
checker
);
checker,
permissionCheck);
}
public Response update(String id, Function<MODEL_OBJECT, MODEL_OBJECT> applyChanges) throws NotFoundException, ConcurrentModificationException {

View File

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

View File

@@ -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<MODEL_OBJECT extends ModelObject,
.map(Response.ResponseBuilder::build)
.orElseThrow(NotFoundException::new);
}
public Response update(Supplier<Optional<MODEL_OBJECT>> reader, Function<MODEL_OBJECT, MODEL_OBJECT> applyChanges, Predicate<MODEL_OBJECT> hasSameKey, Consumer<MODEL_OBJECT> checker) throws NotFoundException, ConcurrentModificationException {
public Response changePassword(Supplier<Optional<MODEL_OBJECT>> reader, Function<MODEL_OBJECT, MODEL_OBJECT> applyChanges, Predicate<MODEL_OBJECT> hasSameKey, Consumer<MODEL_OBJECT> checker, Function<MODEL_OBJECT, PermissionCheck> 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<MODEL_OBJECT extends ModelObject,
public Response update(Supplier<Optional<MODEL_OBJECT>> reader, Function<MODEL_OBJECT, MODEL_OBJECT> applyChanges, Predicate<MODEL_OBJECT> 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<MODEL_OBJECT> 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) {

View File

@@ -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.
*
* <strong>Note:</strong> This method requires "user:modify" privilege.
* <strong>Note:</strong> This method requires "user:modify" privilege to modify the password of other users.
* <strong>Note:</strong> 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));
}
}

View File

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

View File

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

View File

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

View File

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

View File

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

View File

@@ -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;
@@ -124,8 +123,7 @@ 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");
@@ -155,14 +153,13 @@ 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());
}
@@ -173,8 +170,7 @@ 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");
@@ -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();
@@ -209,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", "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"));
}
/**
@@ -230,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" ));
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) {