From 5e364e1043b37a663a0736f1f02fc1979ea81ce9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Pfeuffer?= Date: Wed, 16 Jan 2019 16:03:02 +0100 Subject: [PATCH] Do not expose StoredAssignedPermission --- ...vent.java => AssignedPermissionEvent.java} | 16 ++-- .../sonia/scm/security/SecuritySystem.java | 22 +---- .../security/StoredAssignedPermission.java | 2 + .../scm/api/v2/resources/UserResource.java | 5 +- .../AuthorizationChangedEventProducer.java | 16 ++-- .../DefaultAuthorizationCollector.java | 4 +- .../scm/security/DefaultSecuritySystem.java | 86 ++++++------------- ...AuthorizationChangedEventProducerTest.java | 12 +-- .../security/DefaultSecuritySystemTest.java | 40 ++++----- 9 files changed, 78 insertions(+), 125 deletions(-) rename scm-core/src/main/java/sonia/scm/security/{StoredAssignedPermissionEvent.java => AssignedPermissionEvent.java} (90%) diff --git a/scm-core/src/main/java/sonia/scm/security/StoredAssignedPermissionEvent.java b/scm-core/src/main/java/sonia/scm/security/AssignedPermissionEvent.java similarity index 90% rename from scm-core/src/main/java/sonia/scm/security/StoredAssignedPermissionEvent.java rename to scm-core/src/main/java/sonia/scm/security/AssignedPermissionEvent.java index ad93bf25a9..9ebea488a5 100644 --- a/scm-core/src/main/java/sonia/scm/security/StoredAssignedPermissionEvent.java +++ b/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; diff --git a/scm-core/src/main/java/sonia/scm/security/SecuritySystem.java b/scm-core/src/main/java/sonia/scm/security/SecuritySystem.java index f99625a5a9..49f27be3e8 100644 --- a/scm-core/src/main/java/sonia/scm/security/SecuritySystem.java +++ b/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 getPermissions( + public Collection getPermissions( Predicate predicate); } diff --git a/scm-core/src/main/java/sonia/scm/security/StoredAssignedPermission.java b/scm-core/src/main/java/sonia/scm/security/StoredAssignedPermission.java index 903f86df90..4b2e46b665 100644 --- a/scm-core/src/main/java/sonia/scm/security/StoredAssignedPermission.java +++ b/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; 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 70381c3304..be97af2677 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 @@ -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(); } } diff --git a/scm-webapp/src/main/java/sonia/scm/security/AuthorizationChangedEventProducer.java b/scm-webapp/src/main/java/sonia/scm/security/AuthorizationChangedEventProducer.java index cf4c980625..0586db2bb3 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/AuthorizationChangedEventProducer.java +++ b/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() + "fire authorization changed event for user {}, because permission {} has changed", + permission.getName(), permission.getPermission() ); fireEventForUser(permission.getName()); } 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 0a347a1a03..903445df3a 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/DefaultAuthorizationCollector.java +++ b/scm-webapp/src/main/java/sonia/scm/security/DefaultAuthorizationCollector.java @@ -175,10 +175,10 @@ public class DefaultAuthorizationCollector implements AuthorizationCollector private void collectGlobalPermissions(Builder builder, final User user, final GroupNames groups) { - Collection globalPermissions = + Collection globalPermissions = securitySystem.getPermissions((AssignedPermission input) -> isUserPermitted(user, groups, input)); - for (StoredAssignedPermission gp : globalPermissions) + for (AssignedPermission gp : globalPermissions) { String permission = gp.getPermission(); diff --git a/scm-webapp/src/main/java/sonia/scm/security/DefaultSecuritySystem.java b/scm-webapp/src/main/java/sonia/scm/security/DefaultSecuritySystem.java index 1869558345..a8a9cc7f8b 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/DefaultSecuritySystem.java +++ b/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- - ScmEventBus.getInstance().post( - new StoredAssignedPermissionEvent(HandlerEventType.CREATE, 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)); + 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 AssignedPermissionEvent(HandlerEventType.DELETE, permission) + ); } } @@ -189,16 +171,8 @@ public class DefaultSecuritySystem implements SecuritySystem { if (event.getEventType() == HandlerEventType.DELETE) { - deletePermissions(new Predicate() - { - - @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() - { - - @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 getPermissions(Predicate predicate) + public Collection getPermissions(Predicate predicate) { - Builder permissions = ImmutableSet.builder(); + Builder permissions = ImmutableSet.builder(); for (Entry 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,14 +249,16 @@ public class DefaultSecuritySystem implements SecuritySystem * * @param predicate */ - private void deletePermissions(Predicate predicate) + private boolean deletePermissions(Predicate predicate) { - Collection permissions = getPermissions(predicate); - - for (StoredAssignedPermission permission : permissions) - { - deletePermission(permission); + boolean found = false; + for (Entry e : store.getAll().entrySet()) { + if ((predicate == null) || predicate.test(e.getValue())) { + store.remove(e.getKey()); + found = true; + } } + return found; } /** diff --git a/scm-webapp/src/test/java/sonia/scm/security/AuthorizationChangedEventProducerTest.java b/scm-webapp/src/test/java/sonia/scm/security/AuthorizationChangedEventProducerTest.java index b45e0d72e9..17e0a8ed52 100644 --- a/scm-webapp/src/test/java/sonia/scm/security/AuthorizationChangedEventProducerTest.java +++ b/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"); } @@ -253,4 +253,4 @@ public class AuthorizationChangedEventProducerTest { } -} \ No newline at end of file +} diff --git a/scm-webapp/src/test/java/sonia/scm/security/DefaultSecuritySystemTest.java b/scm-webapp/src/test/java/sonia/scm/security/DefaultSecuritySystemTest.java index ed6bac7bea..74214376ec 100644 --- a/scm-webapp/src/test/java/sonia/scm/security/DefaultSecuritySystemTest.java +++ b/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 all = securitySystem.getPermissions(p -> true); + Collection 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 other = securitySystem.getPermissions(p -> p.getName().equals("trillian")); + Collection 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 filtered = + Collection 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 ----------------------------------------------------------