diff --git a/java/com.sap.sse.security.common/src/com/sap/sse/security/shared/PermissionChecker.java b/java/com.sap.sse.security.common/src/com/sap/sse/security/shared/PermissionChecker.java index ceda03597f3..dd038138cd4 100644 --- a/java/com.sap.sse.security.common/src/com/sap/sse/security/shared/PermissionChecker.java +++ b/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, 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. + *

+ * + * Relevant ACLs are determined on several constraints: *

+ * + * @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 resolveAcls(O ownershipOrNull, String type, Iterable objectIdentifiersAsStringOrNull); + Iterable resolveAclsAndCheckIfAnyMatches(O ownershipOrNull, String type, Iterable objectIdentifiersAsStringOrNull, + Predicate filterCondition, Iterable allAclsForTypeAndObjectIdsOrNull); } private static final BiFunction impliesChecker = WildcardPermission::implies; @@ -101,7 +127,6 @@ public class PermissionChecker { Iterable groupsOfWhichUserIsMember, U allUser, Iterable 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 groupsOfUser = getGroupsOfUser(user); final Iterable 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, O>, Iterable> 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 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, O> collationKeyBasedOnTypeAndObjectIdAndOwnership = new Triple<>(type, identifiers, ownership); + final Iterable aclsFromPreviousRunForSameTypeAndObjectIds = + typesAndIdsAndOwnershipsWithFullSetOfAcls.get(collationKeyBasedOnTypeAndObjectIdAndOwnership); + final Iterable 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 diff --git a/java/com.sap.sse.security.test/src/com/sap/sse/security/test/PermissionCheckerTest.java b/java/com.sap.sse.security.test/src/com/sap/sse/security/test/PermissionCheckerTest.java index ed9ab55d4aa..5866e33e46e 100644 --- a/java/com.sap.sse.security.test/src/com/sap/sse/security/test/PermissionCheckerTest.java +++ b/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 noopAclResolver = new AclResolver() { @Override - public Iterable resolveAcls(Ownership ownership, String type, - Iterable objectIdentifiersAsString) { - return Collections.emptySet(); + public Iterable resolveAclsAndCheckIfAnyMatches(Ownership ownership, String type, + Iterable objectIdentifiersAsString, Predicate check, + Iterable allAclsForTypeAndObjectIdsOrNull) { + return Collections.emptySet(); // assuming an empty ACL set } }; private final UUID eventId = UUID.randomUUID(); diff --git a/java/com.sap.sse.security/src/com/sap/sse/security/impl/SecurityServiceAclResolver.java b/java/com.sap.sse.security/src/com/sap/sse/security/impl/SecurityServiceAclResolver.java index 90dbb6d6728..5e401c76a41 100755 --- a/java/com.sap.sse.security/src/com/sap/sse/security/impl/SecurityServiceAclResolver.java +++ b/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 { private final AccessControlStore accessControlStore; @@ -22,9 +36,26 @@ public class SecurityServiceAclResolver implements AclResolver resolveAcls( - Ownership ownership, String type, Iterable objectIdentifiersAsString) { - final Set result = new HashSet<>(); + public Iterable resolveAclsAndCheckIfAnyMatches( + Ownership ownership, String type, Iterable objectIdentifiersAsString, + Predicate filterCondition, Iterable allAclsForTypeAndObjectIdsOrNull) { + final Iterable 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 enumerateAndCheckApplicableAcls(Ownership ownership, String type, + Iterable objectIdentifiersAsString, Predicate filterCondition) { + final List 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