Merged in bugfix/delete_repository_permissions (pull request #207)

Fix deletion of permissions after modification

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) {
+  public boolean removePermission(RepositoryPermission permission) {
-    this.permissions.remove(permission);
+    return this.permissions.remove(permission);
   }
 
   public void setPublicReadable(boolean publicReadable) {

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<String> verbs;
 
   /**
-   * Constructs a new {@link RepositoryPermission}.
+   * This constructor exists for mapstruct and JAXB, only -- <b>do not use this in "normal" code</b>.
-   * This constructor is used by JAXB and mapstruct.
+   *
+   * @deprecated Do not use this for "normal" code.
+   * Use {@link RepositoryPermission#RepositoryPermission(String, Collection, boolean)} instead.
    */
+  @Deprecated
   public RepositoryPermission() {}
 
   public RepositoryPermission(String name, Collection<String> 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<String> 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.
    *
-   *
+   * @deprecated Do not use this for "normal" code.
-   * @param groupPermission true if the permission is a group permission
+   * 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.
    *
-   *
+   * @deprecated Do not use this for "normal" code.
-   * @param name name of the user or group
+   * 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.
    *
-   *
+   * @deprecated Do not use this for "normal" code.
-   * @param verbs verbs of the permission
+   * Use {@link RepositoryPermission#RepositoryPermission(String, Collection, boolean)} instead.
    */
+  @Deprecated
   public void setVerbs(Collection<String> verbs)
   {
+    if (this.verbs != null) {
+      throw new IllegalStateException(REPOSITORY_MODIFIED_EXCEPTION_TEXT);
+    }
     this.verbs = unmodifiableSet(new LinkedHashSet<>(verbs));
   }
 }

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);
+    RepositoryPermission permission2 = new RepositoryPermission("name1", asList("one", "two", "two"), false);
-    permission2.setVerbs(asList("one", "two", "two"));
 
     assertThat(permission1).isEqualTo(permission2);
   }

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

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

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

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);
+    ArrayList<RepositoryPermission> permissions = Lists
-    RepositoryPermission modifiedPermission = TEST_PERMISSIONS.get(0);
+      .newArrayList(
-    // modify the type to owner
+        new RepositoryPermission("user_write", asList("*"), false),
-    modifiedPermission.setVerbs(new ArrayList<>(singletonList("*")));
+        new RepositoryPermission("user_read", singletonList("read"), false),
-    ImmutableList<RepositoryPermission> expectedPermissions = ImmutableList.copyOf(TEST_PERMISSIONS);
+        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<RepositoryPermission> expectedPermissions = ImmutableList.copyOf(permissions);
     assertExpectedRequest(requestPUTPermission
       .content("{\"name\" : \"" + modifiedPermission.getName() + "\" , \"verbs\" : [\"*\"], \"groupPermission\" : false}")
       .path(PATH_OF_ALL_PERMISSIONS + modifiedPermission.getName())