You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-commits@jackrabbit.apache.org by an...@apache.org on 2022/05/20 08:39:47 UTC
[jackrabbit-oak] branch OAK-9775 created (now dfd59663f4)
This is an automated email from the ASF dual-hosted git repository.
angela pushed a change to branch OAK-9775
in repository https://gitbox.apache.org/repos/asf/jackrabbit-oak.git
at dfd59663f4 OAK-9775 : ACEs with unsupported restrictions must be cleared upon editing
This branch includes the following new commits:
new dfd59663f4 OAK-9775 : ACEs with unsupported restrictions must be cleared upon editing
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
[jackrabbit-oak] 01/01: OAK-9775 : ACEs with unsupported restrictions must be cleared upon editing
Posted by an...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
angela pushed a commit to branch OAK-9775
in repository https://gitbox.apache.org/repos/asf/jackrabbit-oak.git
commit dfd59663f4469d65b681294ebcef83198aae9de0
Author: angela <an...@adobe.com>
AuthorDate: Fri May 20 10:39:31 2022 +0200
OAK-9775 : ACEs with unsupported restrictions must be cleared upon editing
---
.../principalbased/impl/EntryCache.java | 6 +-
.../impl/PrincipalBasedAccessControlManager.java | 8 +-
.../principalbased/impl/PrincipalPolicyImpl.java | 10 +-
.../authorization/principalbased/impl/Utils.java | 30 +++
.../principalbased/impl/EntryCacheTest.java | 38 +++-
.../impl/UnsupportedRestrictionTest.java | 184 +++++++++++++++++++
.../accesscontrol/AccessControlManagerImpl.java | 21 ++-
.../restriction/RestrictionProviderImpl.java | 2 +-
.../accesscontrol/UnsupportedRestrictionTest.java | 201 +++++++++++++++++++++
.../restriction/AbstractRestrictionProvider.java | 16 +-
10 files changed, 504 insertions(+), 12 deletions(-)
diff --git a/oak-authorization-principalbased/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/principalbased/impl/EntryCache.java b/oak-authorization-principalbased/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/principalbased/impl/EntryCache.java
index 329dc48265..d167189b4f 100644
--- a/oak-authorization-principalbased/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/principalbased/impl/EntryCache.java
+++ b/oak-authorization-principalbased/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/principalbased/impl/EntryCache.java
@@ -86,7 +86,11 @@ class EntryCache implements Constants {
private PermissionEntryImpl(@NotNull Tree entryTree) {
effectivePath = Strings.emptyToNull(TreeUtil.getString(entryTree, REP_EFFECTIVE_PATH));
privilegeBits = bitsProvider.getBits(entryTree.getProperty(REP_PRIVILEGES).getValue(Type.NAMES));
- pattern = restrictionProvider.getPattern(effectivePath, restrictionProvider.readRestrictions(effectivePath, entryTree));
+ if (Utils.hasRestrictions(entryTree)) {
+ pattern = restrictionProvider.getPattern(effectivePath, restrictionProvider.readRestrictions(effectivePath, entryTree));
+ } else {
+ pattern = RestrictionPattern.EMPTY;
+ }
}
@NotNull
diff --git a/oak-authorization-principalbased/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/principalbased/impl/PrincipalBasedAccessControlManager.java b/oak-authorization-principalbased/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/principalbased/impl/PrincipalBasedAccessControlManager.java
index eaec70919a..54f1dc5d06 100644
--- a/oak-authorization-principalbased/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/principalbased/impl/PrincipalBasedAccessControlManager.java
+++ b/oak-authorization-principalbased/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/principalbased/impl/PrincipalBasedAccessControlManager.java
@@ -395,7 +395,13 @@ class PrincipalBasedAccessControlManager extends AbstractAccessControlManager im
}
String oakPath = Strings.emptyToNull(TreeUtil.getString(entryTree, REP_EFFECTIVE_PATH));
PrivilegeBits bits = privilegeBitsProvider.getBits(entryTree.getProperty(Constants.REP_PRIVILEGES).getValue(Type.NAMES));
- Set<Restriction> restrictions = mgrProvider.getRestrictionProvider().readRestrictions(oakPath, entryTree);
+
+ RestrictionProvider rp = mgrProvider.getRestrictionProvider();
+ if (!Utils.hasValidRestrictions(oakPath, entryTree, rp)) {
+ return null;
+ }
+
+ Set<Restriction> restrictions = rp.readRestrictions(oakPath, entryTree);
NamePathMapper npMapper = getNamePathMapper();
return new AbstractEntry(oakPath, principal, bits, restrictions, npMapper) {
@Override
diff --git a/oak-authorization-principalbased/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/principalbased/impl/PrincipalPolicyImpl.java b/oak-authorization-principalbased/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/principalbased/impl/PrincipalPolicyImpl.java
index 5ecec6ecd4..0968a91025 100644
--- a/oak-authorization-principalbased/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/principalbased/impl/PrincipalPolicyImpl.java
+++ b/oak-authorization-principalbased/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/principalbased/impl/PrincipalPolicyImpl.java
@@ -73,9 +73,13 @@ class PrincipalPolicyImpl extends AbstractAccessControlList implements Principal
boolean addEntry(@NotNull Tree entryTree) throws AccessControlException {
String oakPath = Strings.emptyToNull(TreeUtil.getString(entryTree, REP_EFFECTIVE_PATH));
- PrivilegeBits bits = privilegeBitsProvider.getBits(entryTree.getProperty(Constants.REP_PRIVILEGES).getValue(Type.NAMES));
- Set<Restriction> restrictions = restrictionProvider.readRestrictions(oakPath, entryTree);
- return addEntry(new EntryImpl(oakPath, bits, restrictions));
+ if (Utils.hasValidRestrictions(oakPath, entryTree, restrictionProvider)) {
+ PrivilegeBits bits = privilegeBitsProvider.getBits(entryTree.getProperty(Constants.REP_PRIVILEGES).getValue(Type.NAMES));
+ Set<Restriction> restrictions = restrictionProvider.readRestrictions(oakPath, entryTree);
+ return addEntry(new EntryImpl(oakPath, bits, restrictions));
+ } else {
+ return false;
+ }
}
//------------------------------------------< AbstractAccessControlList >---
diff --git a/oak-authorization-principalbased/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/principalbased/impl/Utils.java b/oak-authorization-principalbased/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/principalbased/impl/Utils.java
index 8294b979c2..1d2e691ebd 100644
--- a/oak-authorization-principalbased/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/principalbased/impl/Utils.java
+++ b/oak-authorization-principalbased/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/principalbased/impl/Utils.java
@@ -26,8 +26,10 @@ import org.apache.jackrabbit.oak.plugins.tree.TreeUtil;
import org.apache.jackrabbit.oak.spi.security.authorization.permission.PermissionProvider;
import org.apache.jackrabbit.oak.spi.security.authorization.permission.Permissions;
import org.apache.jackrabbit.oak.spi.security.authorization.principalbased.Filter;
+import org.apache.jackrabbit.oak.spi.security.authorization.restriction.RestrictionProvider;
import org.apache.jackrabbit.oak.spi.xml.ImportBehavior;
import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -121,4 +123,32 @@ final class Utils implements Constants {
return permissionProvider.isGranted(effectivePath, Permissions.getString(Permissions.MODIFY_ACCESS_CONTROL));
}
}
+
+ /**
+ * Tests if the given ACE tree comes with any restrictions. Since node type {@code rep:PrincipalEntry} requires
+ * restrictions to be stored in a separate {@code rep:restrictions} child node, this can be determined by checking if
+ * a restriction child-node exists.
+ *
+ * @param aceTree The tree defining the principal-based access control entry
+ * @return {@code true} if the given ACE defines restrictions, {@code false} otherwise.
+ */
+ public static boolean hasRestrictions(@NotNull Tree aceTree) {
+ return aceTree.hasChild(REP_RESTRICTIONS);
+ }
+
+ public static boolean hasValidRestrictions(@Nullable String oakPath, @NotNull Tree aceTree, @NotNull RestrictionProvider restrictionProvider) {
+ if (hasRestrictions(aceTree)) {
+ try {
+ restrictionProvider.validateRestrictions(oakPath, aceTree);
+ return true;
+ } catch (RepositoryException e) {
+ log.warn("Access control entry at {} contains unsupported restrictions: {}", oakPath, e.getMessage());
+ return false;
+ }
+ } else {
+ // no restriction tree present -> skip validation as principal-acl does not allow for restriction properties
+ // on the ACE-node itself as the regular rep:policy in the default access control management.
+ return true;
+ }
+ }
}
\ No newline at end of file
diff --git a/oak-authorization-principalbased/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/principalbased/impl/EntryCacheTest.java b/oak-authorization-principalbased/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/principalbased/impl/EntryCacheTest.java
index aca0ee03f1..1872f6b5e2 100644
--- a/oak-authorization-principalbased/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/principalbased/impl/EntryCacheTest.java
+++ b/oak-authorization-principalbased/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/principalbased/impl/EntryCacheTest.java
@@ -24,10 +24,15 @@ import org.apache.jackrabbit.oak.plugins.tree.TreeUtil;
import org.apache.jackrabbit.oak.spi.nodetype.NodeTypeConstants;
import org.apache.jackrabbit.oak.spi.security.authorization.AuthorizationConfiguration;
import org.apache.jackrabbit.oak.spi.security.authorization.restriction.RestrictionProvider;
+import org.junit.After;
import org.junit.Before;
import org.junit.Test;
+import java.util.Collections;
+import java.util.Set;
+
import static org.apache.jackrabbit.oak.spi.nodetype.NodeTypeConstants.NT_OAK_UNSTRUCTURED;
+import static org.apache.jackrabbit.oak.spi.security.authorization.accesscontrol.AccessControlConstants.REP_GLOB;
import static org.apache.jackrabbit.oak.spi.security.authorization.principalbased.impl.Constants.MIX_REP_PRINCIPAL_BASED_MIXIN;
import static org.apache.jackrabbit.oak.spi.security.authorization.principalbased.impl.Constants.NT_REP_PRINCIPAL_ENTRY;
import static org.apache.jackrabbit.oak.spi.security.authorization.principalbased.impl.Constants.NT_REP_PRINCIPAL_POLICY;
@@ -37,6 +42,12 @@ import static org.apache.jackrabbit.oak.spi.security.authorization.principalbase
import static org.apache.jackrabbit.oak.spi.security.privilege.PrivilegeConstants.JCR_READ;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.reset;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoInteractions;
public class EntryCacheTest extends AbstractPrincipalBasedTest {
@@ -48,12 +59,21 @@ public class EntryCacheTest extends AbstractPrincipalBasedTest {
public void before() throws Exception {
super.before();
- restrictionProvider = getConfig(AuthorizationConfiguration.class).getRestrictionProvider();
+ restrictionProvider = spy(getConfig(AuthorizationConfiguration.class).getRestrictionProvider());
accessControlledPath = getNamePathMapper().getOakPath(getTestSystemUser().getPath());
Tree accessControlledTree = root.getTree(accessControlledPath);
TreeUtil.addMixin(accessControlledTree, MIX_REP_PRINCIPAL_BASED_MIXIN, root.getTree(NodeTypeConstants.NODE_TYPES_PATH), "uid");
policyTree = TreeUtil.addChild(accessControlledTree, REP_PRINCIPAL_POLICY, NT_REP_PRINCIPAL_POLICY);
}
+
+ @After
+ public void after() throws Exception {
+ try {
+ reset(restrictionProvider);
+ } finally {
+ super.after();
+ }
+ }
@Test
public void testNoEntries() {
@@ -86,5 +106,21 @@ public class EntryCacheTest extends AbstractPrincipalBasedTest {
EntryCache cache = new EntryCache(root, ImmutableSet.of(accessControlledPath), restrictionProvider);
assertTrue(cache.getEntries(TEST_OAK_PATH).hasNext());
+ verifyNoInteractions(restrictionProvider);
+ }
+
+ @Test
+ public void testEntriesWithRestrictionsForTestPath() throws Exception {
+ Tree entry = TreeUtil.addChild(policyTree, "entry1", NT_REP_PRINCIPAL_ENTRY);
+ entry.setProperty(REP_EFFECTIVE_PATH, TEST_OAK_PATH, Type.PATH);
+ entry.setProperty(REP_PRIVILEGES, ImmutableSet.of(JCR_READ), Type.NAMES);
+ restrictionProvider.writeRestrictions(TEST_OAK_PATH, entry,
+ Collections.singleton(restrictionProvider.createRestriction(TEST_OAK_PATH, REP_GLOB, getValueFactory(root).createValue("test"))));
+
+ EntryCache cache = new EntryCache(root, ImmutableSet.of(accessControlledPath), restrictionProvider);
+ assertTrue(cache.getEntries(TEST_OAK_PATH).hasNext());
+
+ verify(restrictionProvider).readRestrictions(eq(TEST_OAK_PATH), any(Tree.class));
+ verify(restrictionProvider).getPattern(eq(TEST_OAK_PATH), any(Set.class));
}
}
\ No newline at end of file
diff --git a/oak-authorization-principalbased/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/principalbased/impl/UnsupportedRestrictionTest.java b/oak-authorization-principalbased/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/principalbased/impl/UnsupportedRestrictionTest.java
new file mode 100644
index 0000000000..122198ebbb
--- /dev/null
+++ b/oak-authorization-principalbased/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/principalbased/impl/UnsupportedRestrictionTest.java
@@ -0,0 +1,184 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.jackrabbit.oak.spi.security.authorization.principalbased.impl;
+
+import org.apache.jackrabbit.api.security.JackrabbitAccessControlList;
+import org.apache.jackrabbit.api.security.JackrabbitAccessControlManager;
+import org.apache.jackrabbit.api.security.authorization.PrincipalAccessControlList;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.api.Type;
+import org.apache.jackrabbit.oak.security.authorization.restriction.RestrictionProviderImpl;
+import org.apache.jackrabbit.oak.spi.security.ConfigurationParameters;
+import org.apache.jackrabbit.oak.spi.security.authorization.AuthorizationConfiguration;
+import org.apache.jackrabbit.oak.spi.security.authorization.accesscontrol.AccessControlConstants;
+import org.apache.jackrabbit.oak.spi.security.authorization.restriction.AbstractRestrictionProvider;
+import org.apache.jackrabbit.oak.spi.security.authorization.restriction.Restriction;
+import org.apache.jackrabbit.oak.spi.security.authorization.restriction.RestrictionDefinition;
+import org.apache.jackrabbit.oak.spi.security.authorization.restriction.RestrictionDefinitionImpl;
+import org.apache.jackrabbit.oak.spi.security.authorization.restriction.RestrictionPattern;
+import org.apache.jackrabbit.oak.spi.security.authorization.restriction.RestrictionProvider;
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
+import org.junit.Test;
+
+import javax.jcr.RepositoryException;
+import javax.jcr.Value;
+import javax.jcr.ValueFactory;
+import javax.jcr.security.AccessControlPolicy;
+import java.security.Principal;
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+
+import static org.apache.jackrabbit.oak.spi.security.privilege.PrivilegeConstants.JCR_READ;
+import static org.apache.jackrabbit.oak.spi.security.privilege.PrivilegeConstants.JCR_WRITE;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertTrue;
+
+/**
+ * Tests for OAK-9775
+ */
+public class UnsupportedRestrictionTest extends AbstractPrincipalBasedTest {
+
+ private final TestRestrictionProvider rp = new TestRestrictionProvider();
+
+ private JackrabbitAccessControlManager acMgr;
+
+ @Override
+ public void before() throws Exception {
+ super.before();
+
+ assertSame(rp, getConfig(AuthorizationConfiguration.class).getRestrictionProvider());
+
+ setupContentTrees(TEST_OAK_PATH);
+
+ acMgr = getAccessControlManager(root);
+
+ Principal p = getTestSystemUser().getPrincipal();
+ PrincipalPolicyImpl acl = getPrincipalPolicyImpl(p, acMgr);
+ assertTrue(acl.addEntry(testJcrPath, privilegesFromNames(JCR_READ), createRestrictions("value1"), Collections.emptyMap()));
+ assertTrue(acl.addEntry(testJcrPath, privilegesFromNames(JCR_READ), createRestrictions("value2"), Collections.emptyMap()));
+ assertTrue(acl.addEntry(testJcrPath, privilegesFromNames(JCR_WRITE), Collections.emptyMap(), Collections.emptyMap()));
+
+
+ acMgr.setPolicy(acl.getPath(), acl);
+ root.commit();
+
+ acl = getPrincipalPolicyImpl(p, acMgr);
+ assertEquals(3, acl.size());
+
+ // make sure the test-restriction is no longer supported (but not removed from the repo content)
+ rp.disable();
+ RestrictionProvider provider = getConfig(AuthorizationConfiguration.class).getRestrictionProvider();
+ assertSame(rp, provider);
+ assertTrue(rp.base instanceof RestrictionProviderImpl);
+ }
+
+ @NotNull
+ private Map<String, Value> createRestrictions(@NotNull String value) {
+ ValueFactory vf = getValueFactory(root);
+ return Collections.singletonMap(TestRestrictionProvider.NAME, vf.createValue(value));
+ }
+
+ @Override
+ protected ConfigurationParameters getSecurityConfigParameters() {
+ ConfigurationParameters cp = ConfigurationParameters.of(AccessControlConstants.PARAM_RESTRICTION_PROVIDER, rp);
+ return ConfigurationParameters.of(AuthorizationConfiguration.NAME, cp);
+ }
+
+ @Test
+ public void testGetPolicies() throws Exception {
+ Principal testPrincipal = getTestSystemUser().getPrincipal();
+ JackrabbitAccessControlList acl = getPrincipalPolicyImpl(testPrincipal, acMgr);
+ assertEquals(1, acl.size());
+ }
+
+ @Test
+ public void testGetEffectivePoliciesByPath() throws Exception {
+ for (AccessControlPolicy policy : acMgr.getEffectivePolicies(testJcrPath)) {
+ if (policy instanceof PrincipalAccessControlList) {
+ assertEquals(1, ((PrincipalAccessControlList) policy).size());
+ }
+ }
+ }
+
+ private static final class TestRestrictionProvider implements RestrictionProvider {
+
+ private static final String NAME = "testRestriction";
+
+ private RestrictionProvider base;
+
+ public TestRestrictionProvider() {
+ base = new AbstractRestrictionProvider(Collections.singletonMap(NAME, new RestrictionDefinitionImpl(NAME, Type.STRING, false))) {
+ @Override
+ public @NotNull RestrictionPattern getPattern(@Nullable String oakPath, @NotNull Tree tree) {
+ return RestrictionPattern.EMPTY;
+ }
+
+ @Override
+ public @NotNull RestrictionPattern getPattern(@Nullable String oakPath, @NotNull Set<Restriction> restrictions) {
+ return RestrictionPattern.EMPTY;
+ }
+ };
+ }
+
+ void disable() {
+ base = new RestrictionProviderImpl();
+ }
+
+ @Override
+ public @NotNull Set<RestrictionDefinition> getSupportedRestrictions(@Nullable String oakPath) {
+ return base.getSupportedRestrictions(oakPath);
+ }
+
+ @Override
+ public @NotNull Restriction createRestriction(@Nullable String oakPath, @NotNull String oakName, @NotNull Value value) throws RepositoryException {
+ return base.createRestriction(oakPath, oakName, value);
+ }
+
+ @Override
+ public @NotNull Restriction createRestriction(@Nullable String oakPath, @NotNull String oakName, @NotNull Value... values) throws RepositoryException {
+ return base.createRestriction(oakPath, oakName, values);
+ }
+
+ @Override
+ public @NotNull Set<Restriction> readRestrictions(@Nullable String oakPath, @NotNull Tree aceTree) {
+ return base.readRestrictions(oakPath, aceTree);
+ }
+
+ @Override
+ public void writeRestrictions(@Nullable String oakPath, @NotNull Tree aceTree, @NotNull Set<Restriction> restrictions) throws RepositoryException {
+ base.writeRestrictions(oakPath, aceTree, restrictions);
+ }
+
+ @Override
+ public void validateRestrictions(@Nullable String oakPath, @NotNull Tree aceTree) throws RepositoryException {
+ base.validateRestrictions(oakPath, aceTree);
+ }
+
+ @Override
+ public @NotNull RestrictionPattern getPattern(@Nullable String oakPath, @NotNull Tree tree) {
+ return base.getPattern(oakPath, tree);
+ }
+
+ @Override
+ public @NotNull RestrictionPattern getPattern(@Nullable String oakPath, @NotNull Set<Restriction> restrictions) {
+ return base.getPattern(oakPath, restrictions);
+ }
+ }
+}
\ No newline at end of file
diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlManagerImpl.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlManagerImpl.java
index 249cfd4d77..4a040c2125 100644
--- a/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlManagerImpl.java
+++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlManagerImpl.java
@@ -468,7 +468,7 @@ public class AccessControlManagerImpl extends AbstractAccessControlManager imple
List<ACE> entries = new ArrayList<>();
Map<String, Principal> principalMap = new HashMap<>();
for (Tree child : aclTree.getChildren()) {
- if (Util.isACE(child, ntMgr) && predicate.test(child)) {
+ if (Util.isACE(child, ntMgr) && predicate.test(child) && hasValidRestrictions(oakPath, child)) {
ACE ace = createEntryWithPrincipalMap(oakPath, child, restrictionProvider, principalMap);
entries.add(ace);
}
@@ -480,6 +480,25 @@ public class AccessControlManagerImpl extends AbstractAccessControlManager imple
}
}
+
+ /**
+ * Returns true if the restrictions present with the specified ACE tree are valid; false otherwise.
+ *
+ * @param oakPath The path of the access controlled tree or {@code null} if
+ * the target policies applies to the repository level.
+ * @param aceTree The tree corresponding to an ACE.
+ * @return true if the given ACE tree at the given path doesn't contain any invalid restrictions; false otherwise
+ */
+ private boolean hasValidRestrictions(@Nullable String oakPath, @NotNull Tree aceTree) {
+ try {
+ restrictionProvider.validateRestrictions(oakPath, aceTree);
+ return true;
+ } catch (RepositoryException e) {
+ log.warn("Access control entry at {} contains unsupported restrictions: {}", oakPath, e.getMessage());
+ return false;
+ }
+ }
+
@Nullable
private JackrabbitAccessControlList createPrincipalACL(@Nullable String oakPath,
@NotNull Principal principal) throws RepositoryException {
diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/restriction/RestrictionProviderImpl.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/restriction/RestrictionProviderImpl.java
index 29c125a6be..9b55c3871e 100644
--- a/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/restriction/RestrictionProviderImpl.java
+++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/restriction/RestrictionProviderImpl.java
@@ -115,7 +115,7 @@ public class RestrictionProviderImpl extends AbstractRestrictionProvider {
case REP_GLOBS: return new GlobsPattern(path, values);
case REP_SUBTREES: return new SubtreePattern(path, values);
default: {
- log.debug("Ignoring unsupported restriction {}", name);
+ log.debug("Ignoring unsupported restriction {} at {}", name, path);
return RestrictionPattern.EMPTY;
}
}
diff --git a/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/UnsupportedRestrictionTest.java b/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/UnsupportedRestrictionTest.java
new file mode 100644
index 0000000000..98a5ece8c8
--- /dev/null
+++ b/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/UnsupportedRestrictionTest.java
@@ -0,0 +1,201 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.jackrabbit.oak.security.authorization.accesscontrol;
+
+import org.apache.jackrabbit.api.security.JackrabbitAccessControlList;
+import org.apache.jackrabbit.api.security.JackrabbitAccessControlManager;
+import org.apache.jackrabbit.commons.jackrabbit.authorization.AccessControlUtils;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.api.Type;
+import org.apache.jackrabbit.oak.security.authorization.restriction.RestrictionProviderImpl;
+import org.apache.jackrabbit.oak.spi.security.ConfigurationParameters;
+import org.apache.jackrabbit.oak.spi.security.authorization.AuthorizationConfiguration;
+import org.apache.jackrabbit.oak.spi.security.authorization.accesscontrol.AccessControlConstants;
+import org.apache.jackrabbit.oak.spi.security.authorization.restriction.AbstractRestrictionProvider;
+import org.apache.jackrabbit.oak.spi.security.authorization.restriction.Restriction;
+import org.apache.jackrabbit.oak.spi.security.authorization.restriction.RestrictionDefinition;
+import org.apache.jackrabbit.oak.spi.security.authorization.restriction.RestrictionDefinitionImpl;
+import org.apache.jackrabbit.oak.spi.security.authorization.restriction.RestrictionPattern;
+import org.apache.jackrabbit.oak.spi.security.authorization.restriction.RestrictionProvider;
+import org.apache.jackrabbit.oak.spi.security.principal.EveryonePrincipal;
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
+import org.junit.Test;
+
+import javax.jcr.RepositoryException;
+import javax.jcr.Value;
+import javax.jcr.ValueFactory;
+import java.security.Principal;
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+
+import static org.apache.jackrabbit.oak.spi.security.privilege.PrivilegeConstants.JCR_READ;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertTrue;
+
+/**
+ * Tests for OAK-9775
+ */
+public class UnsupportedRestrictionTest extends AbstractAccessControlTest {
+
+ private final TestRestrictionProvider rp = new TestRestrictionProvider();
+
+ private JackrabbitAccessControlManager acMgr;
+
+ @Override
+ public void before() throws Exception {
+ super.before();
+
+ assertSame(rp, getRestrictionProvider());
+
+ acMgr = getAccessControlManager(root);
+
+ JackrabbitAccessControlList acl = AccessControlUtils.getAccessControlList(acMgr, TEST_PATH);
+ assertTrue(acl.addEntry(EveryonePrincipal.getInstance(), privilegesFromNames(JCR_READ), false, createRestrictions("value1")));
+ assertTrue(acl.addEntry(EveryonePrincipal.getInstance(), privilegesFromNames(JCR_READ), false, createRestrictions("value2")));
+
+ acMgr.setPolicy(acl.getPath(), acl);
+ root.commit();
+
+ acl = AccessControlUtils.getAccessControlList(acMgr, TEST_PATH);
+ assertEquals(2, acl.size());
+
+ // make sure the test-restriction is no longer supported (but not removed from the repo content)
+ rp.disable();
+ assertSame(rp, getRestrictionProvider());
+ assertTrue(rp.base instanceof RestrictionProviderImpl);
+ }
+
+ @NotNull
+ private Map<String, Value> createRestrictions(@NotNull String value) {
+ ValueFactory vf = getValueFactory(root);
+ return Collections.singletonMap(TestRestrictionProvider.NAME, vf.createValue(value));
+ }
+
+ @Override
+ protected ConfigurationParameters getSecurityConfigParameters() {
+ ConfigurationParameters cp = ConfigurationParameters.of(AccessControlConstants.PARAM_RESTRICTION_PROVIDER, rp);
+ return ConfigurationParameters.of(AuthorizationConfiguration.NAME, cp);
+ }
+
+ @Test
+ public void testUpdateAclForDifferentPrincipal() throws Exception {
+ // edit the ACL for a different principal and write it back
+ Principal testPrincipal = getTestUser().getPrincipal();
+ JackrabbitAccessControlList acl = AccessControlUtils.getAccessControlList(acMgr, TEST_PATH);
+ assertEquals(0, acl.size());
+
+ acl.addEntry(testPrincipal, privilegesFromNames(JCR_READ), true);
+ assertEquals(1, acl.size());
+
+ acMgr.setPolicy(acl.getPath(), acl);
+ // this root.commit resulted in 'OakAccessControl0013: Duplicate ACE found exception' in AccessControlValidator
+ // which prior to the fix got read into the ACL without restrictions (resulting in 2 identical entries)
+ // adding an entry for a different principal didn't clean it up.
+ root.commit();
+
+ acl = AccessControlUtils.getAccessControlList(acMgr, TEST_PATH);
+ assertEquals(1, acl.size());
+ assertTrue(acMgr.hasPrivileges(TEST_PATH, Collections.singleton(testPrincipal), privilegesFromNames(JCR_READ)));
+ }
+
+ @Test
+ public void testUpdateAclForSamePrincipal() throws Exception {
+ // edit the ACL for the same principal and write it back
+ JackrabbitAccessControlList acl = AccessControlUtils.getAccessControlList(acMgr, TEST_PATH);
+ assertEquals(0, acl.size());
+
+ acl.addEntry(EveryonePrincipal.getInstance(), privilegesFromNames(JCR_READ), true);
+ assertEquals(1, acl.size());
+
+ acMgr.setPolicy(acl.getPath(), acl);
+ // this root.commit passed as adding an entry for the same principal cleared the 2 ACEs from the setup,
+ // which prior to the fix got read into the ACL without restrictions (resulting in 2 identical entries)
+ root.commit();
+
+ acl = AccessControlUtils.getAccessControlList(acMgr, TEST_PATH);
+ assertEquals(1, acl.size());
+ assertTrue(acMgr.hasPrivileges(TEST_PATH, Collections.singleton(EveryonePrincipal.getInstance()), privilegesFromNames(JCR_READ)));
+ }
+
+ private static final class TestRestrictionProvider implements RestrictionProvider {
+
+ private static final String NAME = "testRestriction";
+
+ private RestrictionProvider base;
+
+ public TestRestrictionProvider() {
+ base = new AbstractRestrictionProvider(Collections.singletonMap(NAME, new RestrictionDefinitionImpl(NAME, Type.STRING, false))) {
+ @Override
+ public @NotNull RestrictionPattern getPattern(@Nullable String oakPath, @NotNull Tree tree) {
+ return RestrictionPattern.EMPTY;
+ }
+
+ @Override
+ public @NotNull RestrictionPattern getPattern(@Nullable String oakPath, @NotNull Set<Restriction> restrictions) {
+ return RestrictionPattern.EMPTY;
+ }
+ };
+ }
+
+ void disable() {
+ base = new RestrictionProviderImpl();
+ }
+
+ @Override
+ public @NotNull Set<RestrictionDefinition> getSupportedRestrictions(@Nullable String oakPath) {
+ return base.getSupportedRestrictions(oakPath);
+ }
+
+ @Override
+ public @NotNull Restriction createRestriction(@Nullable String oakPath, @NotNull String oakName, @NotNull Value value) throws RepositoryException {
+ return base.createRestriction(oakPath, oakName, value);
+ }
+
+ @Override
+ public @NotNull Restriction createRestriction(@Nullable String oakPath, @NotNull String oakName, @NotNull Value... values) throws RepositoryException {
+ return base.createRestriction(oakPath, oakName, values);
+ }
+
+ @Override
+ public @NotNull Set<Restriction> readRestrictions(@Nullable String oakPath, @NotNull Tree aceTree) {
+ return base.readRestrictions(oakPath, aceTree);
+ }
+
+ @Override
+ public void writeRestrictions(@Nullable String oakPath, @NotNull Tree aceTree, @NotNull Set<Restriction> restrictions) throws RepositoryException {
+ base.writeRestrictions(oakPath, aceTree, restrictions);
+ }
+
+ @Override
+ public void validateRestrictions(@Nullable String oakPath, @NotNull Tree aceTree) throws RepositoryException {
+ base.validateRestrictions(oakPath, aceTree);
+ }
+
+ @Override
+ public @NotNull RestrictionPattern getPattern(@Nullable String oakPath, @NotNull Tree tree) {
+ return base.getPattern(oakPath, tree);
+ }
+
+ @Override
+ public @NotNull RestrictionPattern getPattern(@Nullable String oakPath, @NotNull Set<Restriction> restrictions) {
+ return base.getPattern(oakPath, restrictions);
+ }
+ }
+}
\ No newline at end of file
diff --git a/oak-security-spi/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/restriction/AbstractRestrictionProvider.java b/oak-security-spi/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/restriction/AbstractRestrictionProvider.java
index 23c2c57de4..7dfd035614 100644
--- a/oak-security-spi/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/restriction/AbstractRestrictionProvider.java
+++ b/oak-security-spi/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/restriction/AbstractRestrictionProvider.java
@@ -40,9 +40,13 @@ import org.apache.jackrabbit.oak.plugins.tree.TreeUtil;
import org.apache.jackrabbit.util.Text;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
public abstract class AbstractRestrictionProvider implements RestrictionProvider, AccessControlConstants {
+ private static final Logger log = LoggerFactory.getLogger(AbstractRestrictionProvider.class);
+
private final Map<String, RestrictionDefinition> supported;
public AbstractRestrictionProvider(@NotNull Map<String, ? extends RestrictionDefinition> definitions) {
@@ -110,10 +114,14 @@ public abstract class AbstractRestrictionProvider implements RestrictionProvider
Set<Restriction> restrictions = new HashSet<>();
for (PropertyState propertyState : getRestrictionsTree(aceTree).getProperties()) {
String propName = propertyState.getName();
- if (isRestrictionProperty(propName) && supported.containsKey(propName)) {
- RestrictionDefinition def = supported.get(propName);
- if (def.getRequiredType() == propertyState.getType()) {
- restrictions.add(createRestriction(propertyState, def));
+ if (isRestrictionProperty(propName)) {
+ if (supported.containsKey(propName)) {
+ RestrictionDefinition def = supported.get(propName);
+ if (def.getRequiredType() == propertyState.getType()) {
+ restrictions.add(createRestriction(propertyState, def));
+ }
+ } else {
+ log.warn("Unsupported restriction '{}' detected at {}. It will be ignored.", propName, oakPath);
}
}
}