diff --git a/scm-core/src/main/java/sonia/scm/repository/Repository.java b/scm-core/src/main/java/sonia/scm/repository/Repository.java index 18613c1a12..665f63487d 100644 --- a/scm-core/src/main/java/sonia/scm/repository/Repository.java +++ b/scm-core/src/main/java/sonia/scm/repository/Repository.java @@ -307,8 +307,8 @@ public class Repository extends BasicPropertiesAware implements ModelObject, Per this.permissions.add(newPermission); } - public void removePermission(RepositoryPermission permission) { - this.permissions.remove(permission); + public boolean removePermission(RepositoryPermission permission) { + return this.permissions.remove(permission); } public void setPublicReadable(boolean publicReadable) { diff --git a/scm-core/src/main/java/sonia/scm/repository/RepositoryPermission.java b/scm-core/src/main/java/sonia/scm/repository/RepositoryPermission.java index 54163e0393..4a458a6e6a 100644 --- a/scm-core/src/main/java/sonia/scm/repository/RepositoryPermission.java +++ b/scm-core/src/main/java/sonia/scm/repository/RepositoryPermission.java @@ -45,6 +45,7 @@ import javax.xml.bind.annotation.XmlElement; import javax.xml.bind.annotation.XmlRootElement; import java.io.Serializable; import java.util.Collection; +import java.util.Collections; import java.util.LinkedHashSet; import java.util.Set; @@ -55,6 +56,8 @@ import static java.util.Collections.unmodifiableSet; /** * Permissions controls the access to {@link Repository}. + * This object should be immutable, but could not be due to mapstruct. Do not modify instances of this because this + * would change the hash code and therefor make it undeletable in a repository. * * @author Sebastian Sdorra */ @@ -64,22 +67,26 @@ public class RepositoryPermission implements PermissionObject, Serializable { private static final long serialVersionUID = -2915175031430884040L; + public static final String REPOSITORY_MODIFIED_EXCEPTION_TEXT = "repository permission must not be modified"; - private boolean groupPermission = false; + private Boolean groupPermission; private String name; @XmlElement(name = "verb") private Set verbs; /** - * Constructs a new {@link RepositoryPermission}. - * This constructor is used by JAXB and mapstruct. + * This constructor exists for mapstruct and JAXB, only -- do not use this in "normal" code. + * + * @deprecated Do not use this for "normal" code. + * Use {@link RepositoryPermission#RepositoryPermission(String, Collection, boolean)} instead. */ + @Deprecated public RepositoryPermission() {} public RepositoryPermission(String name, Collection verbs, boolean groupPermission) { this.name = name; - this.verbs = unmodifiableSet(new LinkedHashSet<>(verbs)); + this.verbs = new LinkedHashSet<>(verbs); this.groupPermission = groupPermission; } @@ -163,7 +170,7 @@ public class RepositoryPermission implements PermissionObject, Serializable */ public Collection getVerbs() { - return verbs == null? emptyList(): verbs; + return verbs == null ? emptyList() : Collections.unmodifiableSet(verbs); } /** @@ -181,35 +188,50 @@ public class RepositoryPermission implements PermissionObject, Serializable //~--- set methods ---------------------------------------------------------- /** - * Sets true if the permission is a group permission. + * Use this for creation only. This will throw an {@link IllegalStateException} when modified. + * @throws IllegalStateException when modified after the value has been set once. * - * - * @param groupPermission true if the permission is a group permission + * @deprecated Do not use this for "normal" code. + * Use {@link RepositoryPermission#RepositoryPermission(String, Collection, boolean)} instead. */ + @Deprecated public void setGroupPermission(boolean groupPermission) { + if (this.groupPermission != null) { + throw new IllegalStateException(REPOSITORY_MODIFIED_EXCEPTION_TEXT); + } this.groupPermission = groupPermission; } /** - * The name of the user or group. + * Use this for creation only. This will throw an {@link IllegalStateException} when modified. + * @throws IllegalStateException when modified after the value has been set once. * - * - * @param name name of the user or group + * @deprecated Do not use this for "normal" code. + * Use {@link RepositoryPermission#RepositoryPermission(String, Collection, boolean)} instead. */ + @Deprecated public void setName(String name) { + if (this.name != null) { + throw new IllegalStateException(REPOSITORY_MODIFIED_EXCEPTION_TEXT); + } this.name = name; } /** - * Sets the verb of the permission. + * Use this for creation only. This will throw an {@link IllegalStateException} when modified. + * @throws IllegalStateException when modified after the value has been set once. * - * - * @param verbs verbs of the permission + * @deprecated Do not use this for "normal" code. + * Use {@link RepositoryPermission#RepositoryPermission(String, Collection, boolean)} instead. */ + @Deprecated public void setVerbs(Collection verbs) { + if (this.verbs != null) { + throw new IllegalStateException(REPOSITORY_MODIFIED_EXCEPTION_TEXT); + } this.verbs = unmodifiableSet(new LinkedHashSet<>(verbs)); } } diff --git a/scm-core/src/test/java/sonia/scm/repository/RepositoryPermissionTest.java b/scm-core/src/test/java/sonia/scm/repository/RepositoryPermissionTest.java index d65358a66e..c7f05c5f3c 100644 --- a/scm-core/src/test/java/sonia/scm/repository/RepositoryPermissionTest.java +++ b/scm-core/src/test/java/sonia/scm/repository/RepositoryPermissionTest.java @@ -50,8 +50,7 @@ class RepositoryPermissionTest { @Test void shouldBeEqualWithRedundantVerbs() { RepositoryPermission permission1 = new RepositoryPermission("name1", asList("one", "two"), false); - RepositoryPermission permission2 = new RepositoryPermission("name1", asList("one", "two"), false); - permission2.setVerbs(asList("one", "two", "two")); + RepositoryPermission permission2 = new RepositoryPermission("name1", asList("one", "two", "two"), false); assertThat(permission1).isEqualTo(permission2); } diff --git a/scm-ui/src/repos/permissions/modules/permissions.js b/scm-ui/src/repos/permissions/modules/permissions.js index cb6b013c61..8c4161e907 100644 --- a/scm-ui/src/repos/permissions/modules/permissions.js +++ b/scm-ui/src/repos/permissions/modules/permissions.js @@ -451,7 +451,10 @@ function deletePermissionFromState( ) { let newPermission = []; for (let i = 0; i < oldPermissions.length; i++) { - if (oldPermissions[i] !== permission) { + if ( + oldPermissions[i].name !== permission.name || + oldPermissions[i].groupPermission !== permission.groupPermission + ) { newPermission.push(oldPermissions[i]); } } diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/RepositoryPermissionDtoToRepositoryPermissionMapper.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/RepositoryPermissionDtoToRepositoryPermissionMapper.java index 8e015e8b60..881e9251f0 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/RepositoryPermissionDtoToRepositoryPermissionMapper.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/RepositoryPermissionDtoToRepositoryPermissionMapper.java @@ -5,18 +5,8 @@ import org.mapstruct.Mapper; import org.mapstruct.MappingTarget; import sonia.scm.repository.RepositoryPermission; -@Mapper(collectionMappingStrategy = CollectionMappingStrategy.TARGET_IMMUTABLE) +@Mapper( collectionMappingStrategy = CollectionMappingStrategy.TARGET_IMMUTABLE) public abstract class RepositoryPermissionDtoToRepositoryPermissionMapper { public abstract RepositoryPermission map(RepositoryPermissionDto permissionDto); - - /** - * this method is needed to modify an existing permission object - * - * @param target the target permission - * @param repositoryPermissionDto the source dto - * @return the mapped target permission object - */ - public abstract void modify(@MappingTarget RepositoryPermission target, RepositoryPermissionDto repositoryPermissionDto); - } diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/RepositoryPermissionRootResource.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/RepositoryPermissionRootResource.java index dd66e6e5f2..d149f55401 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/RepositoryPermissionRootResource.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/RepositoryPermissionRootResource.java @@ -177,7 +177,11 @@ public class RepositoryPermissionRootResource { .filter(filterPermission(permissionName)) .findFirst() .orElseThrow(() -> notFound(entity(RepositoryPermission.class, namespace).in(Repository.class, namespace + "/" + name))); - dtoToModelMapper.modify(existingPermission, permission); + RepositoryPermission newPermission = dtoToModelMapper.map(permission); + if (!repository.removePermission(existingPermission)) { + throw new IllegalStateException(String.format("could not delete modified permission %s from repository %s/%s", existingPermission, namespace, name)); + } + repository.addPermission(newPermission); manager.modify(repository); log.info("the permission with name: {} is updated.", permissionName); return Response.noContent().build(); @@ -210,7 +214,7 @@ public class RepositoryPermissionRootResource { .findFirst() .ifPresent(repository::removePermission); manager.modify(repository); - log.info("the permission with name: {} is updated.", permissionName); + log.info("the permission with name: {} is deleted.", permissionName); return Response.noContent().build(); } diff --git a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/RepositoryPermissionRootResourceTest.java b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/RepositoryPermissionRootResourceTest.java index 4472acb2c5..fb533ad280 100644 --- a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/RepositoryPermissionRootResourceTest.java +++ b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/RepositoryPermissionRootResourceTest.java @@ -301,11 +301,18 @@ public class RepositoryPermissionRootResourceTest extends RepositoryTestBase { @Test public void shouldGetUpdatedPermissions() throws URISyntaxException { - createUserWithRepositoryAndPermissions(TEST_PERMISSIONS, PERMISSION_WRITE); - RepositoryPermission modifiedPermission = TEST_PERMISSIONS.get(0); - // modify the type to owner - modifiedPermission.setVerbs(new ArrayList<>(singletonList("*"))); - ImmutableList expectedPermissions = ImmutableList.copyOf(TEST_PERMISSIONS); + ArrayList permissions = Lists + .newArrayList( + new RepositoryPermission("user_write", asList("*"), false), + new RepositoryPermission("user_read", singletonList("read"), false), + new RepositoryPermission("user_owner", singletonList("*"), false), + new RepositoryPermission("group_read", singletonList("read"), true), + new RepositoryPermission("group_write", asList("read", "modify"), true), + new RepositoryPermission("group_owner", singletonList("*"), true) + ); + createUserWithRepositoryAndPermissions(permissions, PERMISSION_WRITE); + RepositoryPermission modifiedPermission = permissions.get(0); + ImmutableList expectedPermissions = ImmutableList.copyOf(permissions); assertExpectedRequest(requestPUTPermission .content("{\"name\" : \"" + modifiedPermission.getName() + "\" , \"verbs\" : [\"*\"], \"groupPermission\" : false}") .path(PATH_OF_ALL_PERMISSIONS + modifiedPermission.getName())