bug5239: cache ACLs in meta-permission check based on ownership, type and object IDs once fully enumerated; do shortcut evaluation

java/com.sap.sse.security.common/src/com/sap/sse/security/shared/PermissionChecker.java

@@ -9,9 +9,13 @@ import java.util.Map;
 import java.util.Set;
 import java.util.function.BiFunction;
 import java.util.function.Function;
+import java.util.function.Predicate;
 
+import com.google.gwt.aria.client.Roles;
 import com.sap.sse.common.Util;
+import com.sap.sse.common.Util.Triple;
 import com.sap.sse.security.shared.HasPermissions.Action;
+import com.sap.sse.security.shared.impl.AccessControlList;
 import com.sap.sse.security.shared.impl.Ownership;
 import com.sap.sse.security.shared.impl.Role;
 import com.sap.sse.security.shared.impl.UserGroup;
@@ -39,7 +43,16 @@ public class PermissionChecker {
      */
     public interface AclResolver<A extends SecurityAccessControlList<?>, O extends AbstractOwnership<?, ?>> {
         /**
-         * Resolved all relevant ACLs during meta permission check. Relevant ACLs are determined on several constraints:
+         * Resolves all relevant ACLs during meta permission check and searches for the first ACL to match the
+         * {@code filterCondition}. If and only if at least one such ACL is found, {@code null} is returned. As this
+         * could happen before having discovered all applicable ACLs for the {@code type} /
+         * {@code objectIdentifiersAsStringOrNull} combination, the full set of ACLs isn't known in this case. However,
+         * if no ACL matched the {@code filterCondition}, all ACLs applicable to the {@code type} /
+         * {@code objectIdentifiersAsStringOrNull} combination have been scanned and can be returned for caching, to be
+         * passed in for future invocations for the same combination of type and object ID.
+         * <p>
+         * 
+         * Relevant ACLs are determined on several constraints:
          * <ul>
          * <li>The security type is required to be given. Due to the fact that the meta permission check expands the
          * permission to distinct permissions regarding the type and action part, the effective check always takes place
@@ -54,8 +67,21 @@ public class PermissionChecker {
          * check includes distinct identifiers, we can enumerate the objects based on the type (we always have this one)
          * and identifiers, which means, a direct lookup for the ACLs by {@link QualifiedObjectIdentifier} is possible.
          * </ul>
+         * 
+         * @param filterCondition
+         *            if any of the ACLs found matches this predicate, {@code true} is returned; {@code false} otherwise
+         * 
+         * @param allAclsForTypeAndObjectIdsOrNull
+         *            if not {@code null} these ACLs are assumed to be the full set of ACLs applicable to the combination
+         *            of {@code type} and {@code objectIdentifiersAsStringOrNull}. In this case a potentially expensive
+         *            computation to determine those can be avoided. If {@code null}, the ACLs need to be determined here.
+         * 
+         * @return {@code null} if any of the ACLs found passed the {@code filterCondition}; the set of
+         *         {@link AccessControlList}s relevant for the combination of {@code type} and
+         *         {@code objectIdentifiersAsStringOrNull} otherwise
          */
-        Iterable<A> resolveAcls(O ownershipOrNull, String type, Iterable<String> objectIdentifiersAsStringOrNull);
+        Iterable<AccessControlList> resolveAclsAndCheckIfAnyMatches(O ownershipOrNull, String type, Iterable<String> objectIdentifiersAsStringOrNull,
+                Predicate<A> filterCondition, Iterable<AccessControlList> allAclsForTypeAndObjectIdsOrNull);
     }
     
     private static final BiFunction<WildcardPermission, WildcardPermission, Boolean> impliesChecker = WildcardPermission::implies;
@@ -101,7 +127,6 @@ public class PermissionChecker {
             Iterable<G> groupsOfWhichUserIsMember, U allUser,
             Iterable<G> groupsOfWhichAllUserIsMember, O ownership, A acl) {
         PermissionState result = checkAcl(permission, groupsOfWhichUserIsMember, groupsOfWhichAllUserIsMember, acl);
-
         // anonymous can only grant it if not already decided by acl
         if (result == PermissionState.NONE) {
             PermissionState anonymous = checkUserPermissions(permission, allUser, groupsOfWhichAllUserIsMember,
@@ -266,6 +291,9 @@ public class PermissionChecker {
                 permission, allPermissionTypes, /* expand multiple to single object IDs */ true);
         final Iterable<G> groupsOfUser = getGroupsOfUser(user);
         final Iterable<G> groupsOfAllUser = getGroupsOfUser(allUser);
+        // for resolveAcls, all effectivePermissionsToCheck can be collated based on their types and object IDs; the action can be ignored;
+        // this map remembers for which type/object-id combinations the check has already been performed
+        final Map<Triple<String, Set<String>, O>, Iterable<AccessControlList>> typesAndIdsAndOwnershipsWithFullSetOfAcls = new HashMap<>();
         for (WildcardPermission effectiveWildcardPermissionToCheck : effectivePermissionsToCheck) {
             // Ownership may be null in case of an unqualified meta permission check
             // That's the case when e.g. assigning an unqualified role to a user
@@ -282,13 +310,18 @@ public class PermissionChecker {
                 identifiers = Collections.emptySet();
                 identifierWildcard = true;
             }
-            // TODO bug5239 for resolveAcls, all effectivePermissionsToCheck can be collated based on their types and object IDs; the action can be ignored; no need to repeat this for different actions
-            final Iterable<A> acls = aclResolver.resolveAcls(ownership, type, identifierWildcard ? null : identifiers);
-            // TODO bug5239: it would probably be more efficient to stop at the first ACL that revokes any permission 
-            for (A acl : acls) {
-                if (checkAcl(effectiveWildcardPermissionToCheck, groupsOfUser, groupsOfAllUser, acl) == PermissionState.REVOKED) {
-                    return false;
-                }
+            final Triple<String, Set<String>, O> collationKeyBasedOnTypeAndObjectIdAndOwnership = new Triple<>(type, identifiers, ownership);
+            final Iterable<AccessControlList> aclsFromPreviousRunForSameTypeAndObjectIds =
+                    typesAndIdsAndOwnershipsWithFullSetOfAcls.get(collationKeyBasedOnTypeAndObjectIdAndOwnership);
+            final Iterable<AccessControlList> acls = aclResolver.resolveAclsAndCheckIfAnyMatches(ownership, type,
+                    identifierWildcard ? null : identifiers,
+                    acl->checkAcl(effectiveWildcardPermissionToCheck, groupsOfUser, groupsOfAllUser, acl) == PermissionState.REVOKED,
+                    aclsFromPreviousRunForSameTypeAndObjectIds);
+            if (acls == null) { // this means an ACL was found that REVOKED the permission
+                return false;
+            } else if (aclsFromPreviousRunForSameTypeAndObjectIds == null) {
+                // now we have the full scan of ACLs for the type/object-id combination; cache it
+                typesAndIdsAndOwnershipsWithFullSetOfAcls.put(collationKeyBasedOnTypeAndObjectIdAndOwnership, acls);
             }
             if (checkUserPermissions(effectiveWildcardPermissionToCheck, user, groupsOfUser, ownership,
                     impliesChecker, /* matchOnlyNonQualifiedRolesIfNoOwnershipIsGiven */ true) != PermissionState.GRANTED

java/com.sap.sse.security.test/src/com/sap/sse/security/test/PermissionCheckerTest.java

@@ -13,6 +13,7 @@ import java.util.Set;
 import java.util.UUID;
 import java.util.function.BooleanSupplier;
 import java.util.function.Function;
+import java.util.function.Predicate;
 import java.util.function.Supplier;
 
 import org.apache.shiro.subject.PrincipalCollection;
@@ -27,8 +28,8 @@ import com.sap.sse.security.interfaces.UserStore;
 import com.sap.sse.security.shared.AdminRole;
 import com.sap.sse.security.shared.HasPermissions;
 import com.sap.sse.security.shared.HasPermissions.DefaultActions;
-import com.sap.sse.security.shared.PermissionChecker.AclResolver;
 import com.sap.sse.security.shared.PermissionChecker;
+import com.sap.sse.security.shared.PermissionChecker.AclResolver;
 import com.sap.sse.security.shared.QualifiedObjectIdentifier;
 import com.sap.sse.security.shared.RoleDefinition;
 import com.sap.sse.security.shared.RoleDefinitionImpl;
@@ -48,9 +49,10 @@ import com.sap.sse.security.userstore.mongodb.UserStoreImpl;
 public class PermissionCheckerTest {
     private final AclResolver<AccessControlList, Ownership> noopAclResolver = new AclResolver<AccessControlList, Ownership>() {
         @Override
-        public Iterable<AccessControlList> resolveAcls(Ownership ownership, String type,
-                Iterable<String> objectIdentifiersAsString) {
-            return Collections.emptySet();
+        public Iterable<AccessControlList> resolveAclsAndCheckIfAnyMatches(Ownership ownership, String type,
+                Iterable<String> objectIdentifiersAsString, Predicate<AccessControlList> check,
+                Iterable<AccessControlList> allAclsForTypeAndObjectIdsOrNull) {
+            return Collections.emptySet(); // assuming an empty ACL set
         }
     };
     private final UUID eventId = UUID.randomUUID();

java/com.sap.sse.security/src/com/sap/sse/security/impl/SecurityServiceAclResolver.java

@@ -1,9 +1,11 @@
 package com.sap.sse.security.impl;
 
-import java.util.HashSet;
-import java.util.Set;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.function.Predicate;
 
 import com.sap.sse.common.Util;
+import com.sap.sse.security.SecurityService;
 import com.sap.sse.security.interfaces.AccessControlStore;
 import com.sap.sse.security.shared.AccessControlListAnnotation;
 import com.sap.sse.security.shared.OwnershipAnnotation;
@@ -14,6 +16,18 @@ import com.sap.sse.security.shared.impl.User;
 import com.sap.sse.security.shared.impl.UserGroup;
 
 // TODO Bug 5239: a more efficient implementation is possible using reverse mappings in AccessControlStore
+/**
+ * An {@link AclResolver} implementation backed by the {@link SecurityService}'s {@link AccessControlStore}. It makes
+ * use of an explicit ACL set passed, e.g., from a cache, or otherwise will use the {@link AccessControlStore} to
+ * determine the ACLs that apply based on the object type, optional object IDs, as well as ownerships. Shortcut
+ * evaluation is applied in both cases (provided or computed ACLs), looking for the first ACL that matches a predicate.
+ * If such an ACL is found, {@code null} is returned. Otherwise, in the provided case, the ACL set provided is returned
+ * unmodified, whereas in the computed case, the full ACL set for the context of ownership, type and object IDs computed
+ * is returned.
+ * 
+ * @author Axel Uhl (D043530)
+ *
+ */
 public class SecurityServiceAclResolver implements AclResolver<AccessControlList, Ownership> {
     private final AccessControlStore accessControlStore;
 
@@ -22,9 +36,26 @@ public class SecurityServiceAclResolver implements AclResolver<AccessControlList
     }
 
     @Override
-    public Iterable<AccessControlList> resolveAcls(
-            Ownership ownership, String type, Iterable<String> objectIdentifiersAsString) {
-        final Set<AccessControlList> result = new HashSet<>();
+    public Iterable<AccessControlList> resolveAclsAndCheckIfAnyMatches(
+            Ownership ownership, String type, Iterable<String> objectIdentifiersAsString,
+            Predicate<AccessControlList> filterCondition, Iterable<AccessControlList> allAclsForTypeAndObjectIdsOrNull) {
+        final Iterable<AccessControlList> result;
+        if (allAclsForTypeAndObjectIdsOrNull == null) {
+            result = enumerateAndCheckApplicableAcls(ownership, type, objectIdentifiersAsString, filterCondition);
+        } else {
+            for (AccessControlList acl : allAclsForTypeAndObjectIdsOrNull) {
+                if (filterCondition.test(acl)) {
+                    return null;
+                }
+            }
+            result = allAclsForTypeAndObjectIdsOrNull;
+        }
+        return result;
+    }
+    
+    private Iterable<AccessControlList> enumerateAndCheckApplicableAcls(Ownership ownership, String type,
+            Iterable<String> objectIdentifiersAsString, Predicate<AccessControlList> filterCondition) {
+        final List<AccessControlList> result = new LinkedList<>();
         for (AccessControlListAnnotation annotation : accessControlStore.getAccessControlLists()) {
             // TODO bug5239: introduce shadow hash maps for ACLs that map by object type
             if (!annotation.getIdOfAnnotatedObject().getTypeIdentifier().equals(type)) {
@@ -49,7 +80,11 @@ public class SecurityServiceAclResolver implements AclResolver<AccessControlList
                     }
                 }
             }
-            result.add(annotation.getAnnotation());
+            final AccessControlList acl = annotation.getAnnotation();
+            if (filterCondition.test(acl)) {
+                return null;
+            }
+            result.add(acl);
         }
         return result;
     }