OAK-9782 : CompositeRestrictionProvider must call validate on aggrega…

…ted providers

oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/UnsupportedRestrictionTest.java

@@ -26,6 +26,8 @@
 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.AggregationAware;
+import org.apache.jackrabbit.oak.spi.security.authorization.restriction.CompositeRestrictionProvider;
 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;
@@ -35,15 +37,20 @@
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
 
 import javax.jcr.RepositoryException;
 import javax.jcr.Value;
 import javax.jcr.ValueFactory;
 import java.security.Principal;
+import java.util.Arrays;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.Map;
 import java.util.Set;
 
+import static org.apache.jackrabbit.oak.api.Type.STRINGS;
 import static org.apache.jackrabbit.oak.spi.security.privilege.PrivilegeConstants.JCR_READ;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertSame;
@@ -52,17 +59,41 @@
 /**
  * Tests for OAK-9775
  */
+@RunWith(Parameterized.class)
 public class UnsupportedRestrictionTest extends AbstractAccessControlTest {
-
-    private final TestRestrictionProvider rp = new TestRestrictionProvider();
 
+    private static final String RESTRICTION_NAME = "testRestriction";
+    private static final String RESTRICTION_NAME_2 = "mvTestRestriction";
+
+    private final boolean useCompositeRestrictionProvider;
+    private final TestRestrictionProvider testRestrictionProvider;
+    private final RestrictionProvider rp;
+
     private JackrabbitAccessControlManager acMgr;
+
+    @Parameterized.Parameters(name = "name={1}")
+    public static Collection<Object[]> parameters() {
+        return Arrays.asList(
+                new Object[]{false, "Singular restriction provider"},
+                new Object[]{true, "Composite restriction provider"});
+    }
+
+    public UnsupportedRestrictionTest(boolean useCompositeRestrictionProvider, @NotNull String name) {
+        this.useCompositeRestrictionProvider = useCompositeRestrictionProvider;
+        this.testRestrictionProvider = new TestRestrictionProvider(new RestrictionDefinitionImpl(RESTRICTION_NAME, Type.STRING, false));
+
+        if (useCompositeRestrictionProvider) {
+            rp = CompositeRestrictionProvider.newInstance(testRestrictionProvider, new TestRestrictionProvider(new RestrictionDefinitionImpl(RESTRICTION_NAME_2, STRINGS, false)));
+        } else {
+            rp = testRestrictionProvider;
+        }
+    }
 
     @Override
     public void before() throws Exception {
         super.before();
 
-        assertSame(rp, getRestrictionProvider());
+        assertRestrictionProvider();
 
         acMgr = getAccessControlManager(root);
 
@@ -76,16 +107,24 @@ public void before() throws Exception {
         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);
+        // make sure the test-restriction is no longer supported (but restrictions not removed from the repo content)
+        testRestrictionProvider.disable();
+        assertRestrictionProvider();
+        assertTrue(testRestrictionProvider.base instanceof RestrictionProviderImpl);
     }
 
+    private void assertRestrictionProvider() {
+        if (useCompositeRestrictionProvider) {
+            assertTrue(rp instanceof CompositeRestrictionProvider);
+        } else {
+            assertSame(rp, getRestrictionProvider());
+        }
+    }
+
     @NotNull
     private Map<String, Value> createRestrictions(@NotNull String value) {
         ValueFactory vf = getValueFactory(root);
-        return Collections.singletonMap(TestRestrictionProvider.NAME, vf.createValue(value));
+        return Collections.singletonMap(RESTRICTION_NAME, vf.createValue(value));
     }
 
     @Override
@@ -134,14 +173,12 @@ public void testUpdateAclForSamePrincipal() throws Exception {
         assertTrue(acMgr.hasPrivileges(TEST_PATH, Collections.singleton(EveryonePrincipal.getInstance()), privilegesFromNames(JCR_READ)));
     }
 
-    private static final class TestRestrictionProvider implements RestrictionProvider {
+    private static final class TestRestrictionProvider implements RestrictionProvider, AggregationAware {
 
-        private static final String NAME = "testRestriction";
-
-        private RestrictionProvider base;
+        private AbstractRestrictionProvider base;
 
-        public TestRestrictionProvider() {
-            base = new AbstractRestrictionProvider(Collections.singletonMap(NAME, new RestrictionDefinitionImpl(NAME, Type.STRING, false))) {
+        public TestRestrictionProvider(@NotNull RestrictionDefinition definition) {
+            base = new AbstractRestrictionProvider(Collections.singletonMap(definition.getName(), definition)) {
                 @Override
                 public @NotNull RestrictionPattern getPattern(@Nullable String oakPath, @NotNull Tree tree) {
                     return RestrictionPattern.EMPTY;
@@ -197,5 +234,10 @@ public void validateRestrictions(@Nullable String oakPath, @NotNull Tree aceTree
         public @NotNull RestrictionPattern getPattern(@Nullable String oakPath, @NotNull Set<Restriction> restrictions) {
             return base.getPattern(oakPath, restrictions);
         }
+
+        @Override
+        public void setComposite(@NotNull CompositeRestrictionProvider composite) {
+            base.setComposite(composite);
+        }
     }
 }

oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/restriction/RestrictionProviderImplTest.java

@@ -29,18 +29,25 @@
 import org.apache.jackrabbit.oak.plugins.tree.TreeUtil;
 import org.apache.jackrabbit.oak.spi.security.authorization.accesscontrol.AccessControlConstants;
 import org.apache.jackrabbit.oak.spi.security.authorization.restriction.CompositePattern;
+import org.apache.jackrabbit.oak.spi.security.authorization.restriction.CompositeRestrictionProvider;
 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.RestrictionImpl;
 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.privilege.PrivilegeConstants;
 import org.junit.Before;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
 
 import javax.jcr.Value;
 import javax.jcr.ValueFactory;
 import javax.jcr.security.AccessControlException;
 import javax.jcr.security.AccessControlManager;
+import java.util.Arrays;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
@@ -58,15 +65,35 @@
 /**
  * Tests for {@link RestrictionProviderImpl}
  */
+@RunWith(Parameterized.class)
 public class RestrictionProviderImplTest extends AbstractSecurityTest implements AccessControlConstants {
 
-    private RestrictionProviderImpl provider;
+    private static final String TEST_RESTR_NAME = "test";
+
+    private final boolean asComposite;
+    private RestrictionProvider provider;
+
+    @Parameterized.Parameters(name = "name={1}")
+    public static Collection<Object[]> parameters() {
+        return Arrays.asList(
+                new Object[]{false, "RestrictionProviderImpl as singular provider"},
+                new Object[]{true, "RestrictionProviderImpl as part of a composite restriction provider"});
+    }
+
+    public RestrictionProviderImplTest(boolean asComposite, String name) {
+        this.asComposite = asComposite;
+    }
 
     @Before
     public void before() throws Exception {
         super.before();
 
-        provider = new RestrictionProviderImpl();
+        RestrictionProviderImpl rp = new RestrictionProviderImpl();
+        if (asComposite) {
+            provider = CompositeRestrictionProvider.newInstance(rp, new TestProvider(Collections.singletonMap(TEST_RESTR_NAME, new RestrictionDefinitionImpl("test", Type.STRING, false))));
+        } else {
+            provider = rp;
+        }
     }
 
     @Test
@@ -75,7 +102,8 @@ public void testGetSupportedDefinitions() {
 
         Set<RestrictionDefinition> defs = provider.getSupportedRestrictions("/testPath");
         assertNotNull(defs);
-        assertEquals(7, defs.size());
+        int expectedSize = (asComposite) ? 8 : 7;
+        assertEquals(expectedSize, defs.size());
 
         Set<String> stringsPropNames = ImmutableSet.of(REP_PREFIXES, REP_CURRENT, REP_GLOBS, REP_SUBTREES);
         for (RestrictionDefinition def : defs) {
@@ -92,7 +120,11 @@ public void testGetSupportedDefinitions() {
                 assertEquals(Type.STRINGS, def.getRequiredType());
                 assertFalse(def.isMandatory());
             } else {
-                fail("unexpected restriction " + def.getName());
+                if (asComposite) {
+                    assertEquals(TEST_RESTR_NAME, def.getName());
+                } else {
+                    fail("unexpected restriction " + def.getName());
+                }
             }
         }
     }

oak-security-spi/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/restriction/AbstractRestrictionProvider.java

@@ -43,16 +43,23 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-public abstract class AbstractRestrictionProvider implements RestrictionProvider, AccessControlConstants {
+public abstract class AbstractRestrictionProvider implements RestrictionProvider, AggregationAware, AccessControlConstants {
 
     private static final Logger log = LoggerFactory.getLogger(AbstractRestrictionProvider.class);
 
     private final Map<String, RestrictionDefinition> supported;
+    private CompositeRestrictionProvider composite = null;
 
     public AbstractRestrictionProvider(@NotNull Map<String, ? extends RestrictionDefinition> definitions) {
         this.supported = ImmutableMap.copyOf(definitions);
     }
 
+    //---------------------------------------------------< AggregationAware >---
+    @Override
+    public void setComposite(@NotNull CompositeRestrictionProvider composite) {
+        this.composite = composite;
+    }
+
     //------------------------------------------------< RestrictionProvider >---
     @NotNull
     @Override
@@ -115,13 +122,20 @@ public Set<Restriction> readRestrictions(@Nullable String oakPath, @NotNull Tree
             for (PropertyState propertyState : getRestrictionsTree(aceTree).getProperties()) {
                 String propName = propertyState.getName();
                 if (isRestrictionProperty(propName)) {
-                    if (supported.containsKey(propName)) {
-                        RestrictionDefinition def = supported.get(propName);
+                    RestrictionDefinition def = supported.get(propName);
+                    if (def != null) {
+                        // supported by this provider -> verify that type matches.
                         if (def.getRequiredType() == propertyState.getType()) {
                             restrictions.add(createRestriction(propertyState, def));
+                        } else {
+                            log.warn("Found restriction '{}' with type mismatch: expected '{}', found '{}'. It will be ignored.", propName, def.getRequiredType(), propertyState.getType());
                         }
                     } else {
-                        log.warn("Unsupported restriction '{}' detected at {}. It will be ignored.", propName, oakPath);
+                        // not supported by this provider. to properly deal with aggregation of multiple providers
+                        // only log a warning if it is not supported by any other provider
+                        if (!isSupportedByAnotherProvider(oakPath, propName)) {
+                            log.warn("Unsupported restriction '{}' detected at {}. It will be ignored.", propName, oakPath);
+                        }
                     }
                 }
             }
@@ -154,12 +168,18 @@ public void validateRestrictions(@Nullable String oakPath, @NotNull Tree aceTree
             for (Map.Entry<String, PropertyState> entry : restrictionProperties.entrySet()) {
                 String restrName = entry.getKey();
                 RestrictionDefinition def = supported.get(restrName);
-                if (def == null) {
-                    throw new AccessControlException("Unsupported restriction: " + restrName);
-                }
-                Type<?> type = entry.getValue().getType();
-                if (type != def.getRequiredType()) {
-                    throw new AccessControlException("Invalid restriction type '" + type + "' for " + restrName + ". Expected " + def.getRequiredType());
+                if (def != null) {
+                    // supported by this provider => verify matching type
+                    Type<?> type = entry.getValue().getType();
+                    if (type != def.getRequiredType()) {
+                        throw new AccessControlException("Invalid restriction type '" + type + "' for " + restrName + ". Expected " + def.getRequiredType());
+                    }
+                } else {
+                    // not supported by this provider. to properly deal with aggregation of multiple providers
+                    // only throw exception if it is not supported by any other provider
+                    if (!isSupportedByAnotherProvider(oakPath, restrName)) {
+                        throw new AccessControlException("Unsupported restriction: " + restrName);
+                    }
                 }
             }
             for (RestrictionDefinition def : supported.values()) {
@@ -213,6 +233,25 @@ private RestrictionDefinition getDefinition(@Nullable String oakPath, @NotNull S
         return definition;
     }
 
+    /**
+     * Evaluate if a restriction with the given name at the given path is supported by another restriction provider 
+     * in case this instance is used in a {@link CompositeRestrictionProvider}.
+     * 
+     * @param oakPath The oak path
+     * @param oakName The name of the restriction
+     * @return {@code true} if this provider is used inside a composite {@code RestrictionProvider} which 
+     * supports restrictions with the given name; {@code false} otherwise or if this provider instance is not aggregated 
+     * inside a composite.
+     */
+    private boolean isSupportedByAnotherProvider(@Nullable String oakPath, @NotNull String oakName) {
+        if (composite != null) {
+            return composite.getSupportedRestrictions(oakPath).stream().anyMatch(restrictionDefinition -> restrictionDefinition.getName().equals(oakName));
+        } else {
+            // not used inside a composite -> therefore it's not supported
+            return false;
+        }
+    }
+
     @NotNull
     private static Restriction createRestriction(@NotNull PropertyState propertyState, @NotNull RestrictionDefinition definition) {
         return new RestrictionImpl(propertyState, definition);

oak-security-spi/src/main/java/org/apache/jackrabbit/oak/spi/security/authorization/restriction/AggregationAware.java

@@ -0,0 +1,28 @@
+/*
+ * 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.restriction;
+
+import org.jetbrains.annotations.NotNull;
+
+/**
+ * Marker interface intended to extend a {@link RestrictionProvider} to make it aware of it's aggregated 
+ * nature in a composite when it comes to evaluate the validity of restrictions.
+ */
+public interface AggregationAware {
+
+    void setComposite(@NotNull CompositeRestrictionProvider composite);
+}