Do not expose StoredAssignedPermission

scm-core/src/main/java/sonia/scm/security/AssignedPermissionEvent.java

@@ -51,7 +51,7 @@ import java.io.Serializable;
  * @since 1.31
  */
 @Event
-public final class StoredAssignedPermissionEvent implements Serializable
+public final class AssignedPermissionEvent implements Serializable
 {
 
   /** serial version uid */
@@ -60,14 +60,14 @@ public final class StoredAssignedPermissionEvent implements Serializable
   //~--- constructors ---------------------------------------------------------
 
   /**
-   * Constructs a new StoredAssignedPermissionEvent.
+   * Constructs a new AssignedPermissionEvent.
    *
    *
    * @param type type of the event
    * @param permission permission object which has changed
    */
-  public StoredAssignedPermissionEvent(HandlerEventType type,
-    StoredAssignedPermission permission)
+  public AssignedPermissionEvent(HandlerEventType type,
+                                 AssignedPermission permission)
   {
     this.type = type;
     this.permission = permission;
@@ -91,8 +91,8 @@ public final class StoredAssignedPermissionEvent implements Serializable
       return false;
     }
 
-    final StoredAssignedPermissionEvent other =
-      (StoredAssignedPermissionEvent) obj;
+    final AssignedPermissionEvent other =
+      (AssignedPermissionEvent) obj;
 
     return Objects.equal(type, other.type)
       && Objects.equal(permission, other.permission);
@@ -140,7 +140,7 @@ public final class StoredAssignedPermissionEvent implements Serializable
    *
    * @return changed permission
    */
-  public StoredAssignedPermission getPermission()
+  public AssignedPermission getPermission()
   {
     return permission;
   }
@@ -148,7 +148,7 @@ public final class StoredAssignedPermissionEvent implements Serializable
   //~--- fields ---------------------------------------------------------------
 
   /** changed permission */
-  private StoredAssignedPermission permission;
+  private AssignedPermission permission;
 
   /** type of the event */
   private HandlerEventType type;

scm-core/src/main/java/sonia/scm/security/SecuritySystem.java

@@ -32,14 +32,8 @@
 
 package sonia.scm.security;
 
-//~--- non-JDK imports --------------------------------------------------------
-
-import com.google.common.base.Predicate;
-
-//~--- JDK imports ------------------------------------------------------------
-
 import java.util.Collection;
-import java.util.List;
+import java.util.function.Predicate;
 
 /**
  * The SecuritySystem manages global permissions.
@@ -58,7 +52,7 @@ public interface SecuritySystem
    *
    * @return stored permission
    */
-  public StoredAssignedPermission addPermission(AssignedPermission permission);
+  public void addPermission(AssignedPermission permission);
 
   /**
    * Delete stored permission.
@@ -66,15 +60,7 @@ public interface SecuritySystem
    *
    * @param permission permission to be deleted
    */
-  public void deletePermission(StoredAssignedPermission permission);
-
-  /**
-   * Delete stored permission.
-   *
-   *
-   * @param id id  of the permission
-   */
-  public void deletePermission(String id);
+  public void deletePermission(AssignedPermission permission);
 
   //~--- get methods ----------------------------------------------------------
 
@@ -95,6 +81,6 @@ public interface SecuritySystem
    *
    * @return filtered permissions
    */
-  public Collection<StoredAssignedPermission> getPermissions(
+  public Collection<AssignedPermission> getPermissions(
     Predicate<AssignedPermission> predicate);
 }

scm-core/src/main/java/sonia/scm/security/StoredAssignedPermission.java

@@ -34,6 +34,8 @@ package sonia.scm.security;
 
 //~--- JDK imports ------------------------------------------------------------
 
+import com.google.common.base.Objects;
+
 import javax.xml.bind.annotation.XmlAccessType;
 import javax.xml.bind.annotation.XmlAccessorType;
 import javax.xml.bind.annotation.XmlRootElement;

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

@@ -4,9 +4,8 @@ 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.authc.credential.PasswordService;
-import sonia.scm.security.PermissionDescriptor;
+import sonia.scm.security.AssignedPermission;
 import sonia.scm.security.SecuritySystem;
-import sonia.scm.security.StoredAssignedPermission;
 import sonia.scm.user.User;
 import sonia.scm.user.UserManager;
 import sonia.scm.web.VndMediaType;
@@ -154,7 +153,7 @@ public class UserResource {
     @ResponseCode(code = 500, condition = "internal server error")
   })
   public Response getPermissions(@PathParam("id") String id) {
-    String[] permissions = securitySystem.getPermissions(p -> !p.isGroupPermission() && p.getName().equals(id)).stream().map(StoredAssignedPermission::getPermission).toArray(String[]::new);
+    String[] permissions = securitySystem.getPermissions(p -> !p.isGroupPermission() && p.getName().equals(id)).stream().map(AssignedPermission::getPermission).toArray(String[]::new);
     return Response.ok(new PerminssionListDto(permissions)).build();
   }
 }

scm-webapp/src/main/java/sonia/scm/security/AuthorizationChangedEventProducer.java

@@ -189,9 +189,9 @@ public class AuthorizationChangedEventProducer {
    * @param event permission event
    */
   @Subscribe
-  public void onEvent(StoredAssignedPermissionEvent event) {
+  public void onEvent(AssignedPermissionEvent event) {
     if (event.getEventType().isPost()) {
-      StoredAssignedPermission permission = event.getPermission();
+      AssignedPermission permission = event.getPermission();
       if (permission.isGroupPermission()) {
         handleGroupPermissionChange(permission);
       } else {
@@ -200,18 +200,18 @@ public class AuthorizationChangedEventProducer {
     }
   }
   
-  private void handleGroupPermissionChange(StoredAssignedPermission permission) {
+  private void handleGroupPermissionChange(AssignedPermission permission) {
     logger.debug(
-      "fire authorization changed event, because global group permission {} has changed", 
-      permission.getId()
+      "fire authorization changed event for group {}, because permission {} has changed",
+      permission.getName(), permission.getPermission()
     );
     fireEventForEveryUser();
   }
   
-  private void handleUserPermissionChange(StoredAssignedPermission permission) {
+  private void handleUserPermissionChange(AssignedPermission permission) {
     logger.debug(
         "fire authorization changed event for user {}, because permission {} has changed",
-        permission.getName(), permission.getId()
+        permission.getName(), permission.getPermission()
     );
     fireEventForUser(permission.getName());    
   }

scm-webapp/src/main/java/sonia/scm/security/DefaultAuthorizationCollector.java

@@ -175,10 +175,10 @@ public class DefaultAuthorizationCollector implements AuthorizationCollector
   private void collectGlobalPermissions(Builder<String> builder,
     final User user, final GroupNames groups)
   {
-    Collection<StoredAssignedPermission> globalPermissions =
+    Collection<AssignedPermission> globalPermissions =
       securitySystem.getPermissions((AssignedPermission input) -> isUserPermitted(user, groups, input));
 
-    for (StoredAssignedPermission gp : globalPermissions)
+    for (AssignedPermission gp : globalPermissions)
     {
       String permission = gp.getPermission();
 

scm-webapp/src/main/java/sonia/scm/security/DefaultSecuritySystem.java

@@ -36,8 +36,8 @@ package sonia.scm.security;
 //~--- non-JDK imports --------------------------------------------------------
 
 import com.github.legman.Subscribe;
+import com.google.common.base.Objects;
 import com.google.common.base.Preconditions;
-import com.google.common.base.Predicate;
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableSet.Builder;
 import com.google.common.collect.ImmutableSet;
@@ -67,6 +67,7 @@ import java.util.Collections;
 import java.util.Enumeration;
 import java.util.List;
 import java.util.Map.Entry;
+import java.util.function.Predicate;
 
 //~--- JDK imports ------------------------------------------------------------
 
@@ -123,7 +124,7 @@ public class DefaultSecuritySystem implements SecuritySystem
    * @return
    */
   @Override
-  public StoredAssignedPermission addPermission(AssignedPermission permission)
+  public void addPermission(AssignedPermission permission)
   {
     assertIsAdmin();
     validatePermission(permission);
@@ -134,11 +135,9 @@ public class DefaultSecuritySystem implements SecuritySystem
 
     //J-
     ScmEventBus.getInstance().post(
-      new StoredAssignedPermissionEvent(HandlerEventType.CREATE, sap)
+      new AssignedPermissionEvent(HandlerEventType.CREATE, permission)
     );
     //J+
-
-    return sap;
   }
 
   /**
@@ -148,33 +147,16 @@ public class DefaultSecuritySystem implements SecuritySystem
    * @param permission
    */
   @Override
-  public void deletePermission(StoredAssignedPermission permission)
+  public void deletePermission(AssignedPermission permission)
   {
     assertIsAdmin();
-    store.remove(permission.getId());
-    //J-
+    boolean deleted = deletePermissions(sap -> Objects.equal(sap.getName(), permission.getName())
+      && Objects.equal(sap.isGroupPermission(), permission.isGroupPermission())
+      && Objects.equal(sap.getPermission(), permission.getPermission()));
+    if (deleted) {
       ScmEventBus.getInstance().post(
-      new StoredAssignedPermissionEvent(HandlerEventType.CREATE, permission)
+        new AssignedPermissionEvent(HandlerEventType.DELETE, permission)
       );
-    //J+
-  }
-
-  /**
-   * Method description
-   *
-   *
-   * @param id
-   */
-  @Override
-  public void deletePermission(String id)
-  {
-    assertIsAdmin();
-
-    AssignedPermission ap = store.get(id);
-
-    if (ap != null)
-    {
-      deletePermission(new StoredAssignedPermission(id, ap));
     }
   }
 
@@ -189,16 +171,8 @@ public class DefaultSecuritySystem implements SecuritySystem
   {
     if (event.getEventType() == HandlerEventType.DELETE)
     {
-      deletePermissions(new Predicate<AssignedPermission>()
-      {
-
-        @Override
-        public boolean apply(AssignedPermission p)
-        {
-          return !p.isGroupPermission()
-            && event.getItem().getName().equals(p.getName());
-        }
-      });
+      deletePermissions(p -> !p.isGroupPermission()
+        && event.getItem().getName().equals(p.getName()));
     }
   }
 
@@ -213,16 +187,8 @@ public class DefaultSecuritySystem implements SecuritySystem
   {
     if (event.getEventType() == HandlerEventType.DELETE)
     {
-      deletePermissions(new Predicate<AssignedPermission>()
-      {
-
-        @Override
-        public boolean apply(AssignedPermission p)
-        {
-          return p.isGroupPermission()
-            && event.getItem().getName().equals(p.getName());
-        }
-      });
+      deletePermissions(p -> p.isGroupPermission()
+        && event.getItem().getName().equals(p.getName()));
     }
   }
 
@@ -251,13 +217,13 @@ public class DefaultSecuritySystem implements SecuritySystem
    * @return
    */
   @Override
-  public Collection<StoredAssignedPermission> getPermissions(Predicate<AssignedPermission> predicate)
+  public Collection<AssignedPermission> getPermissions(Predicate<AssignedPermission> predicate)
   {
-    Builder<StoredAssignedPermission> permissions = ImmutableSet.builder();
+    Builder<AssignedPermission> permissions = ImmutableSet.builder();
 
     for (Entry<String, AssignedPermission> e : store.getAll().entrySet())
     {
-      if ((predicate == null) || predicate.apply(e.getValue()))
+      if ((predicate == null) || predicate.test(e.getValue()))
       {
         permissions.add(new StoredAssignedPermission(e.getKey(), e.getValue()));
       }
@@ -283,15 +249,17 @@ public class DefaultSecuritySystem implements SecuritySystem
    *
    * @param predicate
    */
-  private void deletePermissions(Predicate<AssignedPermission> predicate)
+  private boolean deletePermissions(Predicate<AssignedPermission> predicate)
   {
-    Collection<StoredAssignedPermission> permissions = getPermissions(predicate);
-
-    for (StoredAssignedPermission permission : permissions)
-    {
-      deletePermission(permission);
+    boolean found = false;
+    for (Entry<String, AssignedPermission> e : store.getAll().entrySet()) {
+      if ((predicate == null) || predicate.test(e.getValue())) {
+        store.remove(e.getKey());
+        found = true;
       }
     }
+    return found;
+  }
 
   /**
    * Method description

scm-webapp/src/test/java/sonia/scm/security/AuthorizationChangedEventProducerTest.java

@@ -214,7 +214,7 @@ public class AuthorizationChangedEventProducerTest {
   }
   
   /**
-   * Tests {@link AuthorizationChangedEventProducer#onEvent(sonia.scm.security.StoredAssignedPermissionEvent)}.
+   * Tests {@link AuthorizationChangedEventProducer#onEvent(AssignedPermissionEvent)}.
    */
   @Test
   public void testOnStoredAssignedPermissionEvent()
@@ -222,10 +222,10 @@ public class AuthorizationChangedEventProducerTest {
     StoredAssignedPermission groupPermission = new StoredAssignedPermission(
       "123", new AssignedPermission("_authenticated", true, "repository:read:*")
     );
-    producer.onEvent(new StoredAssignedPermissionEvent(HandlerEventType.BEFORE_CREATE, groupPermission));
+    producer.onEvent(new AssignedPermissionEvent(HandlerEventType.BEFORE_CREATE, groupPermission));
     assertEventIsNotFired();
     
-    producer.onEvent(new StoredAssignedPermissionEvent(HandlerEventType.CREATE, groupPermission));
+    producer.onEvent(new AssignedPermissionEvent(HandlerEventType.CREATE, groupPermission));
     assertGlobalEventIsFired();
     
     resetStoredEvent();
@@ -233,12 +233,12 @@ public class AuthorizationChangedEventProducerTest {
     StoredAssignedPermission userPermission = new StoredAssignedPermission(
       "123", new AssignedPermission("trillian", false, "repository:read:*")
     );
-    producer.onEvent(new StoredAssignedPermissionEvent(HandlerEventType.BEFORE_CREATE, userPermission));
+    producer.onEvent(new AssignedPermissionEvent(HandlerEventType.BEFORE_CREATE, userPermission));
     assertEventIsNotFired();
     
     resetStoredEvent();
     
-    producer.onEvent(new StoredAssignedPermissionEvent(HandlerEventType.CREATE, userPermission));
+    producer.onEvent(new AssignedPermissionEvent(HandlerEventType.CREATE, userPermission));
     assertUserEventIsFired("trillian");
   }
   

scm-webapp/src/test/java/sonia/scm/security/DefaultSecuritySystemTest.java

@@ -32,6 +32,7 @@
 
 package sonia.scm.security;
 
+import com.google.common.base.Objects;
 import org.apache.shiro.authz.UnauthorizedException;
 import org.apache.shiro.mgt.DefaultSecurityManager;
 import org.apache.shiro.realm.SimpleAccountRealm;
@@ -46,7 +47,6 @@ import sonia.scm.util.ClassLoaders;
 import sonia.scm.util.MockUtil;
 
 import java.util.Collection;
-import java.util.List;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.Assert.assertEquals;
@@ -92,8 +92,7 @@ public class DefaultSecuritySystemTest extends AbstractTestBase
   {
     setAdminSubject();
 
-    StoredAssignedPermission sap = createPermission("trillian", false,
-                                     "repository:*:READ");
+    AssignedPermission sap = createPermission("trillian", false, "repository:*:READ");
 
     assertEquals("trillian", sap.getName());
     assertEquals("repository:*:READ", sap.getPermission());
@@ -124,7 +123,7 @@ public class DefaultSecuritySystemTest extends AbstractTestBase
   {
     setAdminSubject();
 
-    StoredAssignedPermission sap = createPermission("trillian", false,
+    AssignedPermission sap = createPermission("trillian", false,
                                      "repository:*:READ");
 
     securitySystem.deletePermission(sap);
@@ -141,14 +140,14 @@ public class DefaultSecuritySystemTest extends AbstractTestBase
   {
     setAdminSubject();
 
-    StoredAssignedPermission trillian = createPermission("trillian", false,
+    AssignedPermission trillian = createPermission("trillian", false,
                                           "repository:*:READ");
-    StoredAssignedPermission dent = createPermission("dent", false,
+    AssignedPermission dent = createPermission("dent", false,
                                       "repository:*:READ");
-    StoredAssignedPermission marvin = createPermission("marvin", false,
+    AssignedPermission marvin = createPermission("marvin", false,
                                         "repository:*:READ");
 
-    List<StoredAssignedPermission> all = securitySystem.getPermissions(p -> true);
+    Collection<AssignedPermission> all = securitySystem.getPermissions(p -> true);
 
     assertEquals(3, all.size());
     assertThat(all).contains(trillian, dent, marvin);
@@ -163,10 +162,10 @@ public class DefaultSecuritySystemTest extends AbstractTestBase
   {
     setAdminSubject();
 
-    StoredAssignedPermission sap = createPermission("trillian", false,
+    AssignedPermission sap = createPermission("trillian", false,
                                      "repository:*:READ");
 
-    List<StoredAssignedPermission> other = securitySystem.getPermissions(p -> p.getName().equals("trillian"));
+    Collection<AssignedPermission> other = securitySystem.getPermissions(p -> p.getName().equals("trillian"));
 
     assertThat(other).containsExactly(sap);
   }
@@ -180,14 +179,14 @@ public class DefaultSecuritySystemTest extends AbstractTestBase
   {
     setAdminSubject();
 
-    StoredAssignedPermission trillian = createPermission("trillian", false,
+    AssignedPermission trillian = createPermission("trillian", false,
                                           "repository:*:READ");
-    StoredAssignedPermission dent = createPermission("dent", false,
+    AssignedPermission dent = createPermission("dent", false,
                                       "repository:*:READ");
 
     createPermission("hitchhiker", true, "repository:*:READ");
 
-    List<StoredAssignedPermission> filtered =
+    Collection<AssignedPermission> filtered =
       securitySystem.getPermissions(p -> !p.isGroupPermission());
 
     assertThat(filtered)
@@ -215,7 +214,7 @@ public class DefaultSecuritySystemTest extends AbstractTestBase
   {
     setAdminSubject();
 
-    StoredAssignedPermission sap = createPermission("trillian", false,
+    AssignedPermission sap = createPermission("trillian", false,
                                      "repository:*:READ");
 
     setUserSubject();
@@ -231,7 +230,7 @@ public class DefaultSecuritySystemTest extends AbstractTestBase
   {
     setAdminSubject();
 
-    StoredAssignedPermission sap = createPermission("trillian", false,
+    createPermission("trillian", false,
                                      "repository:*:READ");
 
     setUserSubject();
@@ -248,17 +247,16 @@ public class DefaultSecuritySystemTest extends AbstractTestBase
    *
    * @return
    */
-  private StoredAssignedPermission createPermission(String name,
+  private AssignedPermission createPermission(String name,
     boolean groupPermission, String value)
   {
     AssignedPermission ap = new AssignedPermission(name, groupPermission,
                               value);
-    StoredAssignedPermission sap = securitySystem.addPermission(ap);
+    securitySystem.addPermission(ap);
 
-    assertNotNull(sap);
-    assertNotNull(sap.getId());
-
-    return sap;
+    return securitySystem.getPermissions(permission -> Objects.equal(name, permission.getName())
+      && Objects.equal(groupPermission, permission.isGroupPermission())
+      && Objects.equal(value, permission.getPermission())).stream().findAny().orElseThrow(() -> new AssertionError("created permission not found"));
   }
 
   //~--- set methods ----------------------------------------------------------