This is an automated email from the ASF dual-hosted git repository.
thomasm pushed a commit to branch OAK-9780
in repository https://gitbox.apache.org/repos/asf/jackrabbit-oak.git
commit 29a860f388ec56b6bec089300013e98d4ce01727
Author: angela <anch...@adobe.com>
AuthorDate: Thu Jun 2 15:53:06 2022 +0200
OAK-9791 : Missing check for restriction node being present
---
.../impl/PrincipalBasedAccessControlManager.java | 2 +-
.../principalbased/impl/PrincipalPolicyImpl.java | 2 +-
.../authorization/principalbased/impl/Utils.java | 19 +++++++++
.../principalbased/impl/UtilsTest.java | 47 ++++++++++++++++++----
4 files changed, 61 insertions(+), 9 deletions(-)
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 54f1dc5d06..21876483c2 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
@@ -401,7 +401,7 @@ class PrincipalBasedAccessControlManager extends
AbstractAccessControlManager im
return null;
}
- Set<Restriction> restrictions = rp.readRestrictions(oakPath,
entryTree);
+ Set<Restriction> restrictions = Utils.readRestrictions(rp, 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 0968a91025..c89b1f4dd1 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
@@ -75,7 +75,7 @@ class PrincipalPolicyImpl extends AbstractAccessControlList
implements Principal
String oakPath = Strings.emptyToNull(TreeUtil.getString(entryTree,
REP_EFFECTIVE_PATH));
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);
+ Set<Restriction> restrictions =
Utils.readRestrictions(restrictionProvider, oakPath, entryTree);
return addEntry(new EntryImpl(oakPath, bits, restrictions));
} else {
return false;
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 1d2e691ebd..03bc40dce4 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,6 +26,7 @@ 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.Restriction;
import
org.apache.jackrabbit.oak.spi.security.authorization.restriction.RestrictionProvider;
import org.apache.jackrabbit.oak.spi.xml.ImportBehavior;
import org.jetbrains.annotations.NotNull;
@@ -151,4 +152,22 @@ final class Utils implements Constants {
return true;
}
}
+
+ /**
+ * Utility method that conditionally reads restrictions from provider if
the given {@code aceTree} has restriction
+ * child tree, i.e. combining {@link #hasRestrictions(Tree)} with {@link
RestrictionProvider#readRestrictions(String, Tree)}.
+ *
+ * @param provider The restriction provider
+ * @param effectivePath The effective path
+ * @param aceTree The ace tree
+ * @return restrictions as read from the provider if the given {@code
aceTree} has a rep:restriction child. otherwise,
+ * an empty set without calling the provider.
+ */
+ public static Set<Restriction> readRestrictions(@NotNull
RestrictionProvider provider, @Nullable String effectivePath, @NotNull Tree
aceTree) {
+ if (hasRestrictions(aceTree)) {
+ return provider.readRestrictions(effectivePath, aceTree);
+ } else {
+ return Collections.emptySet();
+ }
+ }
}
\ No newline at end of file
diff --git
a/oak-authorization-principalbased/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/principalbased/impl/UtilsTest.java
b/oak-authorization-principalbased/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/principalbased/impl/UtilsTest.java
index d7a76df75d..b111edb75f 100644
---
a/oak-authorization-principalbased/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/principalbased/impl/UtilsTest.java
+++
b/oak-authorization-principalbased/src/test/java/org/apache/jackrabbit/oak/spi/security/authorization/principalbased/impl/UtilsTest.java
@@ -21,12 +21,13 @@ import
org.apache.jackrabbit.api.security.authorization.PrivilegeManager;
import org.apache.jackrabbit.oak.api.Tree;
import org.apache.jackrabbit.oak.namepath.NamePathMapper;
import
org.apache.jackrabbit.oak.spi.security.authorization.principalbased.Filter;
+import
org.apache.jackrabbit.oak.spi.security.authorization.restriction.AbstractRestrictionProvider;
+import
org.apache.jackrabbit.oak.spi.security.authorization.restriction.RestrictionProvider;
import org.apache.jackrabbit.oak.spi.security.principal.PrincipalImpl;
import org.apache.jackrabbit.oak.spi.xml.ImportBehavior;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import org.junit.Test;
-import org.mockito.Mockito;
import org.mockito.stubbing.Answer;
import javax.jcr.RepositoryException;
@@ -39,8 +40,13 @@ import java.util.Set;
import static org.junit.Assert.assertArrayEquals;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertSame;
import static org.junit.Assert.assertTrue;
import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoInteractions;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.mockito.Mockito.when;
public class UtilsTest implements Constants {
@@ -53,15 +59,15 @@ public class UtilsTest implements Constants {
}
private static Filter mockFilter(boolean canHandle) {
- Filter filter = Mockito.mock(Filter.class);
+ Filter filter = mock(Filter.class);
when(filter.canHandle(any(Set.class))).thenReturn(canHandle);
return filter;
}
private static PrivilegeManager mockPrivilegeManager() throws
RepositoryException {
- PrivilegeManager privilegeManager =
Mockito.mock(PrivilegeManager.class);
+ PrivilegeManager privilegeManager = mock(PrivilegeManager.class);
when(privilegeManager.getPrivilege(any(String.class))).then((Answer<Privilege>)
invocationOnMock -> {
- Privilege p = Mockito.mock(Privilege.class);
+ Privilege p = mock(Privilege.class);
when(p.getName()).thenReturn(invocationOnMock.getArgument(0));
return p;
});
@@ -100,7 +106,7 @@ public class UtilsTest implements Constants {
@Test(expected = AccessControlException.class)
public void testCanHandleNullName() throws Exception {
- Utils.canHandle(Mockito.mock(Principal.class), mockFilter(true),
ImportBehavior.ABORT);
+ Utils.canHandle(mock(Principal.class), mockFilter(true),
ImportBehavior.ABORT);
}
@Test(expected = AccessControlException.class)
@@ -137,7 +143,7 @@ public class UtilsTest implements Constants {
@Test
public void testPrivilegesFromNamesEmptyNames() {
- assertArrayEquals(new Privilege[0],
Utils.privilegesFromOakNames(Collections.emptySet(),
Mockito.mock(PrivilegeManager.class), NamePathMapper.DEFAULT));
+ assertArrayEquals(new Privilege[0],
Utils.privilegesFromOakNames(Collections.emptySet(),
mock(PrivilegeManager.class), NamePathMapper.DEFAULT));
}
@Test
@@ -147,7 +153,7 @@ public class UtilsTest implements Constants {
@Test
public void testPrivilegesFromNamesRemapped() throws Exception {
- NamePathMapper mapper = Mockito.mock(NamePathMapper.class);
+ NamePathMapper mapper = mock(NamePathMapper.class);
when(mapper.getJcrName(any())).thenReturn("c");
Privilege[] privs = Utils.privilegesFromOakNames(Sets.newHashSet("a",
"b"), mockPrivilegeManager(), mapper);
@@ -156,4 +162,31 @@ public class UtilsTest implements Constants {
assertEquals("c", p.getName());
}
}
+
+ @Test
+ public void testReadRestrictionsNoRestrictionTree() {
+ Tree entryTree = mock(Tree.class);
+
+ RestrictionProvider rp = mock(AbstractRestrictionProvider.class);
+
+ when(entryTree.hasChild(REP_RESTRICTIONS)).thenReturn(false);
+ assertSame(Collections.emptySet(), Utils.readRestrictions(rp,
"/test/path", entryTree));
+ verifyNoInteractions(rp);
+ }
+
+ @Test
+ public void testReadRestrictions() {
+ Tree restTree = mock(Tree.class);
+ Tree entryTree = mock(Tree.class);
+
+ RestrictionProvider rp = mock(AbstractRestrictionProvider.class);
+
+ when(entryTree.hasChild(REP_RESTRICTIONS)).thenReturn(true);
+ when(entryTree.getChild(REP_RESTRICTIONS)).thenReturn(restTree);
+ when(restTree.getProperties()).thenReturn(Collections.emptyList());
+
+ Utils.readRestrictions(rp, "/test/path", entryTree);
+ verify(rp).readRestrictions("/test/path", entryTree);
+ verifyNoMoreInteractions(rp);
+ }
}
\ No newline at end of file
