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 2019/06/06 17:20:27 UTC

svn commit: r1860730 [1/3] - in /jackrabbit/oak/trunk/oak-core: ./ src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/ src/test/java/org/apache/jackrabbit/oak/ src/test/java/org/apache/jackrabbit/oak/security/authorization/acc...

Author: angela
Date: Thu Jun  6 17:20:27 2019
New Revision: 1860730

URL: http://svn.apache.org/viewvc?rev=1860730&view=rev
Log:
OAK-8384 : Improve tests for o.a.j.oak.security.authorization.accesscontrol
minor improvement: fix indention with AccessControlValidator

Added:
    jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlManagerLimitedPermissionsTest.java   (with props)
    jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/NodeACLTest.java   (with props)
    jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/TestUtility.java   (with props)
Modified:
    jackrabbit/oak/trunk/oak-core/pom.xml
    jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidator.java
    jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/AbstractSecurityTest.java
    jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/ACLTest.java
    jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AbstractAccessControlTest.java
    jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlImporterAbortTest.java
    jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlImporterBaseTest.java
    jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlImporterBesteffortTest.java
    jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlImporterIgnoreTest.java
    jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlManagerImplTest.java
    jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidatorTest.java
    jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AdminPrincipalsBaseTest.java
    jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/EntryTest.java
    jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/UtilTest.java

Modified: jackrabbit/oak/trunk/oak-core/pom.xml
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/pom.xml?rev=1860730&r1=1860729&r2=1860730&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/pom.xml (original)
+++ jackrabbit/oak/trunk/oak-core/pom.xml Thu Jun  6 17:20:27 2019
@@ -180,6 +180,7 @@
                                     <include>org.apache.jackrabbit.oak.security.authorization.permission</include>
                                     <include>org.apache.jackrabbit.oak.security.user</include>
                                     <include>org.apache.jackrabbit.oak.security.authentication.token</include>
+                                    <include>org.apache.jackrabbit.oak.security.authorization.accesscontrol</include>
                                     <include>org.apache.jackrabbit.oak.security.authorization.restriction</include>
                                 </includes>
                                 <excludes>
@@ -199,7 +200,6 @@
                                     <include>org.apache.jackrabbit.oak.security.authentication</include>
                                     <include>org.apache.jackrabbit.oak.security.user.query</include>
                                     <include>org.apache.jackrabbit.oak.security.privilege</include>
-                                    <include>org.apache.jackrabbit.oak.security.authorization.accesscontrol</include>
                                 </includes>
                                 <excludes>
                                     <exclude>*Test</exclude>

Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidator.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidator.java?rev=1860730&r1=1860729&r2=1860730&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidator.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlValidator.java Thu Jun  6 17:20:27 2019
@@ -309,8 +309,8 @@ class AccessControlValidator extends Def
         return new CommitFailedException(ACCESS_CONTROL, code, message);
     }
 
-     @NotNull
-     private ValidationEntry createAceEntry(@Nullable String path, @NotNull Tree aceTree) throws CommitFailedException {
+    @NotNull
+    private ValidationEntry createAceEntry(@Nullable String path, @NotNull Tree aceTree) throws CommitFailedException {
         String principalName = checkValidPrincipal(aceTree);
         PrivilegeBits privilegeBits = privilegeBitsProvider.getBits(getPrivilegeNames(aceTree));
         boolean isAllow = NT_REP_GRANT_ACE.equals(TreeUtil.getPrimaryTypeName(aceTree));

Modified: jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/AbstractSecurityTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/AbstractSecurityTest.java?rev=1860730&r1=1860729&r2=1860730&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/AbstractSecurityTest.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/AbstractSecurityTest.java Thu Jun  6 17:20:27 2019
@@ -16,8 +16,10 @@
  */
 package org.apache.jackrabbit.oak;
 
+import java.security.Principal;
 import java.util.Arrays;
 import java.util.List;
+import java.util.Set;
 import java.util.UUID;
 import javax.jcr.Credentials;
 import javax.jcr.NoSuchWorkspaceException;
@@ -212,7 +214,7 @@ public abstract class AbstractSecurityTe
         }
     }
 
-    protected Privilege[] privilegesFromNames(String... privilegeNames) throws RepositoryException {
+    protected Privilege[] privilegesFromNames(@NotNull String... privilegeNames) throws RepositoryException {
         return privilegesFromNames(Arrays.asList(privilegeNames));
     }
 
@@ -273,4 +275,9 @@ public abstract class AbstractSecurityTe
     public TreeProvider getTreeProvider() {
         return treeProvider;
     }
+
+    @NotNull
+    public static Set<Principal> getPrincipals(@NotNull ContentSession session) {
+        return session.getAuthInfo().getPrincipals();
+    }
 }

Modified: jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/ACLTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/ACLTest.java?rev=1860730&r1=1860729&r2=1860730&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/ACLTest.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/ACLTest.java Thu Jun  6 17:20:27 2019
@@ -41,6 +41,7 @@ import com.google.common.collect.Sets;
 import org.apache.jackrabbit.JcrConstants;
 import org.apache.jackrabbit.api.security.JackrabbitAccessControlEntry;
 import org.apache.jackrabbit.api.security.JackrabbitAccessControlList;
+import org.apache.jackrabbit.api.security.authorization.PrivilegeManager;
 import org.apache.jackrabbit.oak.api.Tree;
 import org.apache.jackrabbit.oak.api.Type;
 import org.apache.jackrabbit.oak.namepath.impl.GlobalNameMapper;
@@ -72,21 +73,24 @@ import static org.junit.Assert.assertNot
 import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
 
 /**
  * Test {@code ACL} implementation.
  */
 public class ACLTest extends AbstractAccessControlTest implements PrivilegeConstants, AccessControlConstants {
 
-    private static void assertACE(JackrabbitAccessControlEntry ace, boolean isAllow, Privilege... privileges) {
+    private static void assertACE(@NotNull JackrabbitAccessControlEntry ace, boolean isAllow, @NotNull Privilege... privileges) {
         assertEquals(isAllow, ace.isAllow());
         assertEquals(Sets.newHashSet(privileges), Sets.newHashSet(ace.getPrivileges()));
     }
 
     @Test
-    public void testGetNamePathMapper() throws Exception {
+    public void testGetNamePathMapper() {
         assertSame(getNamePathMapper(), createEmptyACL().getNamePathMapper());
-        assertSame(NamePathMapper.DEFAULT, createACL(TEST_PATH, ImmutableList.<ACE>of(), NamePathMapper.DEFAULT).getNamePathMapper());
+        assertSame(NamePathMapper.DEFAULT, createACL(TEST_PATH, ImmutableList.of(), NamePathMapper.DEFAULT).getNamePathMapper());
     }
 
     @Test
@@ -96,7 +100,7 @@ public class ACLTest extends AbstractAcc
         NamePathMapper npMapper = new NamePathMapperImpl(nameMapper);
 
         // map of jcr-path to standard jcr-path
-        Map<String, String> paths = new HashMap<String, String>();
+        Map<String, String> paths = new HashMap<>();
         paths.put(null, null);
         paths.put(TEST_PATH, TEST_PATH);
         paths.put("/", "/");
@@ -104,7 +108,7 @@ public class ACLTest extends AbstractAcc
         paths.put("/{http://jackrabbit.apache.org}testPath", "/jr:testPath");
 
         for (String path : paths.keySet()) {
-            AbstractAccessControlList acl = createACL(path, Collections.<ACE>emptyList(), npMapper);
+            AbstractAccessControlList acl = createACL(path, Collections.emptyList(), npMapper);
             assertEquals(paths.get(path), acl.getPath());
         }
     }
@@ -115,7 +119,7 @@ public class ACLTest extends AbstractAcc
                 singletonMap("oak", "http://jackrabbit.apache.org"),
                 singletonMap("jcr", "http://jackrabbit.apache.org")));
         // map of jcr-path to oak path
-        Map<String, String> paths = new HashMap();
+        Map<String, String> paths = new HashMap<>();
         paths.put(null, null);
         paths.put(TEST_PATH, TEST_PATH);
         paths.put("/", "/");
@@ -127,19 +131,19 @@ public class ACLTest extends AbstractAcc
 
         // test if oak-path is properly set.
         for (String path : paths.keySet()) {
-            AbstractAccessControlList acl = createACL(path, Collections.<ACE>emptyList(), npMapper);
+            AbstractAccessControlList acl = createACL(path, Collections.emptyList(), npMapper);
             assertEquals(paths.get(path), acl.getOakPath());
         }
     }
 
     @Test
-    public void testEmptyAcl() throws RepositoryException {
+    public void testEmptyAcl() {
         AbstractAccessControlList acl = createEmptyACL();
 
         assertNotNull(acl.getAccessControlEntries());
         assertNotNull(acl.getEntries());
 
-        assertTrue(acl.getAccessControlEntries().length == 0);
+        assertEquals(0, acl.getAccessControlEntries().length);
         assertEquals(acl.getAccessControlEntries().length, acl.getEntries().size());
         assertEquals(0, acl.size());
         assertTrue(acl.isEmpty());
@@ -147,13 +151,13 @@ public class ACLTest extends AbstractAcc
 
     @Test
     public void testSize() throws RepositoryException {
-        AbstractAccessControlList acl = createACL(createTestEntries());
+        AbstractAccessControlList acl = createACL(TEST_PATH, createTestEntries(), getNamePathMapper());
         assertEquals(3, acl.size());
     }
 
     @Test
     public void testIsEmpty() throws RepositoryException {
-        AbstractAccessControlList acl = createACL(createTestEntries());
+        AbstractAccessControlList acl = createACL(TEST_PATH, createTestEntries(), getNamePathMapper());
         assertFalse(acl.isEmpty());
     }
 
@@ -172,7 +176,7 @@ public class ACLTest extends AbstractAcc
     }
 
     @Test
-    public void testGetRestrictionNames() throws RepositoryException {
+    public void testGetRestrictionNames() {
         AbstractAccessControlList acl = createEmptyACL();
 
         String[] restrNames = acl.getRestrictionNames();
@@ -185,7 +189,7 @@ public class ACLTest extends AbstractAcc
     }
 
     @Test
-    public void testGetRestrictionType() throws RepositoryException {
+    public void testGetRestrictionType() {
         AbstractAccessControlList acl = createEmptyACL();
         for (RestrictionDefinition def : getRestrictionProvider().getSupportedRestrictions(TEST_PATH)) {
             int reqType = acl.getRestrictionType(getNamePathMapper().getJcrName(def.getName()));
@@ -196,7 +200,7 @@ public class ACLTest extends AbstractAcc
     }
 
     @Test
-    public void testGetRestrictionTypeForUnknownName() throws RepositoryException {
+    public void testGetRestrictionTypeForUnknownName() {
         AbstractAccessControlList acl = createEmptyACL();
         // for backwards compatibility getRestrictionType(String) must return
         // UNDEFINED for a unknown restriction name:
@@ -221,37 +225,20 @@ public class ACLTest extends AbstractAcc
         acl.addAccessControlEntry(internal, privilegesFromNames(JCR_READ));
     }
 
-    @Test
+    @Test(expected = AccessControlException.class)
     public void testNullPrincipal() throws Exception {
-
-        try {
-            acl.addAccessControlEntry(null, privilegesFromNames(JCR_READ));
-            fail("Adding an ACE with null principal should fail");
-        } catch (AccessControlException e) {
-            // success
-        }
+        acl.addAccessControlEntry(null, privilegesFromNames(JCR_READ));
     }
 
-    @Test
+    @Test(expected = AccessControlException.class)
     public void testEmptyPrincipal() throws Exception {
-
-        try {
-            acl.addAccessControlEntry(new PrincipalImpl(""), privilegesFromNames(JCR_READ));
-            fail("Adding an ACE with empty-named principal should fail");
-        } catch (AccessControlException e) {
-            // success
-        }
+        acl.addAccessControlEntry(new PrincipalImpl(""), privilegesFromNames(JCR_READ));
     }
 
     @Test
-    public void testAddEntriesWithCustomPrincipal()  throws Exception {
+    public void testAddEntriesWithCustomPrincipal() throws Exception {
         Principal oakPrincipal = new PrincipalImpl("anonymous");
-        Principal principal = new Principal() {
-            @Override
-            public String getName() {
-                return "anonymous";
-            }
-        };
+        Principal principal = () -> "anonymous";
 
         assertTrue(acl.addAccessControlEntry(oakPrincipal, privilegesFromNames(JCR_READ)));
         assertTrue(acl.addAccessControlEntry(principal, privilegesFromNames(JCR_READ_ACCESS_CONTROL)));
@@ -262,30 +249,29 @@ public class ACLTest extends AbstractAcc
         assertArrayEquals(privilegesFromNames(JCR_READ_ACCESS_CONTROL), acl.getAccessControlEntries()[0].getPrivileges());
     }
 
-    @Test
+    @Test(expected = AccessControlException.class)
     public void testAddEntryWithoutPrivilege() throws Exception {
-        try {
-            acl.addAccessControlEntry(testPrincipal, new Privilege[0]);
-            fail("Adding an ACE with empty privilege array should fail.");
-        } catch (AccessControlException e) {
-            // success
-        }
-        try {
-            acl.addAccessControlEntry(testPrincipal, null);
-            fail("Adding an ACE with null privileges should fail.");
-        } catch (AccessControlException e) {
-            // success
-        }
+        acl.addAccessControlEntry(testPrincipal, new Privilege[0]);
     }
 
-    @Test
+    @Test(expected = AccessControlException.class)
+    public void testAddEntryWithNullPrivilege() throws Exception {
+        acl.addAccessControlEntry(testPrincipal, null);
+    }
+
+    @Test(expected = AccessControlException.class)
     public void testAddEntryWithInvalidPrivilege() throws Exception {
-        try {
-            acl.addAccessControlEntry(testPrincipal, new Privilege[]{new InvalidPrivilege()});
-            fail("Adding an ACE with invalid privileges should fail.");
-        } catch (AccessControlException e) {
-            // success
-        }
+        Privilege invalid = when(mock(Privilege.class).getName()).thenReturn("invalid").getMock();
+        acl.addAccessControlEntry(testPrincipal, new Privilege[]{invalid});
+    }
+
+    @Test(expected = AccessControlException.class)
+    public void testAddEntryWithAbstractPrivilege() throws Exception {
+        Privilege abstractPriv = when(mock(Privilege.class).isAbstract()).thenReturn(true).getMock();
+        when(abstractPriv.getName()).thenReturn("privName");
+        PrivilegeManager privMgr = when(mock(PrivilegeManager.class).getPrivilege(anyString())).thenReturn(abstractPriv).getMock();
+        ACL list = createACL(TEST_PATH, Collections.emptyList(), getNamePathMapper(), getRestrictionProvider(), privMgr);
+        list.addAccessControlEntry(testPrincipal, new Privilege[] {abstractPriv});
     }
 
     @Test
@@ -302,14 +288,14 @@ public class ACLTest extends AbstractAcc
 
     @Test
     public void testAddEntry2() throws Exception {
-        assertTrue(acl.addEntry(testPrincipal, testPrivileges, true, Collections.<String, Value>emptyMap()));
+        assertTrue(acl.addEntry(testPrincipal, testPrivileges, true, Collections.emptyMap()));
         assertFalse(acl.isEmpty());
     }
 
     @Test
     public void testAddEntryTwice() throws Exception {
-        acl.addEntry(testPrincipal, testPrivileges, true, Collections.<String, Value>emptyMap());
-        assertFalse(acl.addEntry(testPrincipal, testPrivileges, true, Collections.<String, Value>emptyMap()));
+        acl.addEntry(testPrincipal, testPrivileges, true, Collections.emptyMap());
+        assertFalse(acl.addEntry(testPrincipal, testPrivileges, true, Collections.emptyMap()));
     }
 
     @Test
@@ -328,48 +314,38 @@ public class ACLTest extends AbstractAcc
         assertTrue(acl.isEmpty());
     }
 
-    @Test
+    @Test(expected = AccessControlException.class)
     public void testRemoveInvalidEntry() throws Exception {
-        try {
-            acl.removeAccessControlEntry(new JackrabbitAccessControlEntry() {
-                public boolean isAllow() {
-                    return false;
-                }
+        acl.removeAccessControlEntry(new JackrabbitAccessControlEntry() {
+            public boolean isAllow() {
+                return false;
+            }
 
-                public String[] getRestrictionNames() {
-                    return new String[0];
-                }
+            public String[] getRestrictionNames() {
+                return new String[0];
+            }
 
-                public Value getRestriction(String restrictionName) {
-                    return null;
-                }
+            public Value getRestriction(String restrictionName) {
+                return null;
+            }
 
-                public Value[] getRestrictions(String restrictionName) {
-                    return null;
-                }
+            public Value[] getRestrictions(String restrictionName) {
+                return null;
+            }
 
-                public Principal getPrincipal() {
-                    return testPrincipal;
-                }
+            public Principal getPrincipal() {
+                return testPrincipal;
+            }
 
-                public Privilege[] getPrivileges() {
-                    return testPrivileges;
-                }
-            });
-            fail("Passing an unknown ACE should fail");
-        } catch (AccessControlException e) {
-            // success
-        }
+            public Privilege[] getPrivileges() {
+                return testPrivileges;
+            }
+        });
     }
 
-    @Test
+    @Test(expected = AccessControlException.class)
     public void testRemoveNonExisting() throws Exception {
-        try {
-            acl.removeAccessControlEntry(createEntry(testPrincipal, testPrivileges, true));
-            fail("Removing a non-existing ACE should fail.");
-        } catch (AccessControlException e) {
-            // success
-        }
+        acl.removeAccessControlEntry(createEntry(testPrincipal, testPrivileges, true));
     }
 
     @Test
@@ -422,27 +398,37 @@ public class ACLTest extends AbstractAcc
         assertEquals(first, acl.getEntries().get(2));
     }
 
+    @Test(expected = AccessControlException.class)
+    public void testReorderInvalidSourceEntry() throws Exception {
+        acl.addAccessControlEntry(testPrincipal, privilegesFromNames(JCR_READ, JCR_READ_ACCESS_CONTROL));
+        acl.addAccessControlEntry(EveryonePrincipal.getInstance(), privilegesFromNames(JCR_WRITE));
+
+        AccessControlEntry invalid = createEntry(testPrincipal, false, null, JCR_WRITE);
+        acl.orderBefore(invalid, acl.getEntries().get(0));
+    }
+
+    @Test(expected = AccessControlException.class)
+    public void testReorderInvalidSourcDestEntry() throws Exception {
+        acl.addAccessControlEntry(testPrincipal, privilegesFromNames(JCR_READ, JCR_READ_ACCESS_CONTROL));
+        acl.addAccessControlEntry(EveryonePrincipal.getInstance(), privilegesFromNames(JCR_WRITE));
+
+        AccessControlEntry invalid = createEntry(testPrincipal, false, null, JCR_MODIFY_PROPERTIES);
+        acl.orderBefore(acl.getEntries().get(0), invalid);
+    }
+
     @Test
-    public void testReorderInvalidEntries() throws Exception {
-        Privilege[] read = privilegesFromNames(JCR_READ, JCR_READ_ACCESS_CONTROL);
-        Privilege[] write = privilegesFromNames(JCR_WRITE);
+    public void testReorderSourceSameAsDest() throws Exception {
+        AbstractAccessControlList acl = createEmptyACL();
+        acl.addAccessControlEntry(testPrincipal, privilegesFromNames(JCR_READ, JCR_READ_ACCESS_CONTROL));
+        acl.addEntry(testPrincipal, privilegesFromNames(JCR_WRITE), false);
+        acl.addAccessControlEntry(EveryonePrincipal.getInstance(), privilegesFromNames(JCR_WRITE));
 
-        acl.addAccessControlEntry(testPrincipal, read);
-        acl.addAccessControlEntry(EveryonePrincipal.getInstance(), write);
+        AccessControlEntry[] entries = acl.getAccessControlEntries();
+        assertEquals(3, entries.length);
 
-        AccessControlEntry invalid = createEntry(testPrincipal, false, null, JCR_WRITE);
-        try {
-            acl.orderBefore(invalid, acl.getEntries().get(0));
-            fail("src entry not contained in list -> reorder should fail.");
-        } catch (AccessControlException e) {
-            // success
-        }
-        try {
-            acl.orderBefore(acl.getEntries().get(0), invalid);
-            fail("dest entry not contained in list -> reorder should fail.");
-        } catch (AccessControlException e) {
-            // success
-        }
+        acl.orderBefore(entries[1], entries[1]);
+
+        assertArrayEquals(entries, acl.getAccessControlEntries());
     }
 
     @Test
@@ -455,7 +441,7 @@ public class ACLTest extends AbstractAcc
         assertTrue(acl.addEntry(testPrincipal, privileges, true));
 
         // expected: only a single allow-entry with both privileges
-        assertTrue(acl.size() == 1);
+        assertEquals(1, acl.size());
         assertACE(acl.getEntries().get(0), true, privileges);
     }
 
@@ -469,7 +455,7 @@ public class ACLTest extends AbstractAcc
         assertFalse(acl.addEntry(testPrincipal, achPrivs, true));
 
         // expected: only a single allow-entry with add_child_nodes + read privilege
-        assertTrue(acl.size() == 1);
+        assertEquals(1, acl.size());
         assertACE(acl.getEntries().get(0), true, privileges);
     }
 
@@ -496,7 +482,7 @@ public class ACLTest extends AbstractAcc
         assertTrue(acl.addEntry(testPrincipal, privileges, false));
 
         // expected: 2 entries one allowing ADD_CHILD_NODES, the other denying READ
-        assertTrue(acl.size() == 2);
+        assertEquals(2, acl.size());
 
         assertACE(acl.getEntries().get(0), true, privilegesFromNames(JCR_ADD_CHILD_NODES));
         assertACE(acl.getEntries().get(1), false, privilegesFromNames(JCR_READ));
@@ -512,7 +498,7 @@ public class ACLTest extends AbstractAcc
         assertTrue(acl.addEntry(testPrincipal, modProperties, false));
 
         // expected: 2 entries with the allow entry being adjusted
-        assertTrue(acl.size() == 2);
+        assertEquals(2, acl.size());
 
         Privilege[] expected = privilegesFromNames(JCR_ADD_CHILD_NODES, JCR_REMOVE_CHILD_NODES, JCR_REMOVE_NODE, JCR_NODE_TYPE_MANAGEMENT);
         assertACE(acl.getEntries().get(0), true, expected);
@@ -543,7 +529,7 @@ public class ACLTest extends AbstractAcc
         // add same privileges for another principal -> must modify as well.
         assertTrue(acl.addAccessControlEntry(everyone, privs));
         // .. 2 entries must be present.
-        assertTrue(acl.getAccessControlEntries().length == 2);
+        assertEquals(2, acl.getAccessControlEntries().length);
         assertEquals(everyone, acl.getAccessControlEntries()[1].getPrincipal());
     }
 
@@ -614,11 +600,11 @@ public class ACLTest extends AbstractAcc
         Privilege[] grPriv = privilegesFromNames(REP_WRITE);
         Privilege[] dePriv = privilegesFromNames(JCR_REMOVE_CHILD_NODES);
 
-        acl.addEntry(everyone, grPriv, true, Collections.<String, Value>emptyMap());
-        acl.addEntry(everyone, dePriv, false, Collections.<String, Value>emptyMap());
+        acl.addEntry(everyone, grPriv, true, Collections.emptyMap());
+        acl.addEntry(everyone, dePriv, false, Collections.emptyMap());
 
-        Set<Privilege> allows = new HashSet<Privilege>();
-        Set<Privilege> denies = new HashSet<Privilege>();
+        Set<Privilege> allows = new HashSet<>();
+        Set<Privilege> denies = new HashSet<>();
         AccessControlEntry[] entries = acl.getAccessControlEntries();
         for (AccessControlEntry en : entries) {
             if (everyone.equals(en.getPrincipal()) && en instanceof JackrabbitAccessControlEntry) {
@@ -637,7 +623,7 @@ public class ACLTest extends AbstractAcc
         assertEquals(ImmutableSet.copyOf(expected), allows);
 
         assertEquals(1, denies.size());
-        assertArrayEquals(privilegesFromNames(JCR_REMOVE_CHILD_NODES), denies.toArray(new Privilege[denies.size()]));
+        assertArrayEquals(privilegesFromNames(JCR_REMOVE_CHILD_NODES), denies.toArray(new Privilege[0]));
     }
 
     @Test
@@ -664,11 +650,7 @@ public class ACLTest extends AbstractAcc
         acl.addEntry(testPrincipal, readPriv, false);
 
         assertFalse(acl.addEntry(new PrincipalImpl(testPrincipal.getName()), readPriv, false));
-        assertFalse(acl.addEntry(new Principal() {
-            public String getName() {
-                return testPrincipal.getName();
-            }
-        }, readPriv, false));
+        assertFalse(acl.addEntry(() -> testPrincipal.getName(), readPriv, false));
     }
 
     @Test
@@ -804,37 +786,27 @@ public class ACLTest extends AbstractAcc
         }
     }
 
-    @Test
+    @Test(expected = AccessControlException.class)
     public void testUnsupportedRestrictions2() throws Exception {
         RestrictionProvider rp = new TestRestrictionProvider("restr", Type.NAME, false);
 
-        JackrabbitAccessControlList acl = createACL(TEST_PATH, new ArrayList(), namePathMapper, rp);
-        try {
-            acl.addEntry(testPrincipal, testPrivileges, false, Collections.<String, Value>singletonMap("unsupported", getValueFactory().createValue("value")));
-            fail("Unsupported restriction must be detected.");
-        } catch (AccessControlException e) {
-            // mandatory restriction missing -> success
-        }
+        JackrabbitAccessControlList acl = createACL(TEST_PATH, new ArrayList<>(), namePathMapper, rp);
+        acl.addEntry(testPrincipal, testPrivileges, false, Collections.singletonMap("unsupported", getValueFactory().createValue("value")));
     }
 
-    @Test
+    @Test(expected = AccessControlException.class)
     public void testInvalidRestrictionType() throws Exception {
         RestrictionProvider rp = new TestRestrictionProvider("restr", Type.NAME, false);
 
-        JackrabbitAccessControlList acl = createACL(TEST_PATH, new ArrayList(), namePathMapper, rp);
-        try {
-            acl.addEntry(testPrincipal, testPrivileges, false, Collections.<String, Value>singletonMap("restr", getValueFactory().createValue(true)));
-            fail("Invalid restriction type.");
-        } catch (AccessControlException e) {
-            // mandatory restriction missing -> success
-        }
+        JackrabbitAccessControlList acl = createACL(TEST_PATH, new ArrayList<>(), namePathMapper, rp);
+        acl.addEntry(testPrincipal, testPrivileges, false, Collections.singletonMap("restr", getValueFactory().createValue(true)));
     }
 
     @Test(expected = AccessControlException.class)
     public void testMandatoryRestrictions() throws Exception {
         RestrictionProvider rp = new TestRestrictionProvider("mandatory", Type.NAME, true);
 
-        JackrabbitAccessControlList acl = createACL(TEST_PATH, new ArrayList(), namePathMapper, rp);
+        JackrabbitAccessControlList acl = createACL(TEST_PATH, new ArrayList<>(), namePathMapper, rp);
         acl.addEntry(testPrincipal, testPrivileges, false, Collections.emptyMap(), Collections.emptyMap());
     }
 
@@ -842,7 +814,7 @@ public class ACLTest extends AbstractAcc
     public void testMandatoryRestrictionsPresent() throws Exception {
         RestrictionProvider rp = new TestRestrictionProvider("mandatory", Type.NAME, true);
 
-        JackrabbitAccessControlList acl = createACL(TEST_PATH, new ArrayList(), namePathMapper, rp);
+        JackrabbitAccessControlList acl = createACL(TEST_PATH, new ArrayList<>(), namePathMapper, rp);
         acl.addEntry(testPrincipal, testPrivileges, false, Collections.singletonMap("mandatory", getValueFactory(root).createValue("name", PropertyType.NAME)), Collections.emptyMap());
     }
 
@@ -850,7 +822,7 @@ public class ACLTest extends AbstractAcc
     public void testMandatoryRestrictionsPresentAsMV() throws Exception {
         RestrictionProvider rp = new TestRestrictionProvider("mandatory", Type.NAME, true);
 
-        JackrabbitAccessControlList acl = createACL(TEST_PATH, new ArrayList(), namePathMapper, rp);
+        JackrabbitAccessControlList acl = createACL(TEST_PATH, new ArrayList<>(), namePathMapper, rp);
         acl.addEntry(testPrincipal, testPrivileges, false, Collections.emptyMap(), Collections.singletonMap("mandatory", new Value[] {getValueFactory(root).createValue("name", PropertyType.NAME)}));
     }
 
@@ -858,7 +830,7 @@ public class ACLTest extends AbstractAcc
     public void testMandatoryMVRestrictions() throws Exception {
         RestrictionProvider rp = new TestRestrictionProvider("mandatory", Type.NAMES, true);
 
-        JackrabbitAccessControlList acl = createACL(TEST_PATH, new ArrayList(), namePathMapper, rp);
+        JackrabbitAccessControlList acl = createACL(TEST_PATH, new ArrayList<>(), namePathMapper, rp);
         acl.addEntry(testPrincipal, testPrivileges, false, Collections.emptyMap(), Collections.emptyMap());
     }
 
@@ -866,7 +838,7 @@ public class ACLTest extends AbstractAcc
     public void testMandatoryMVRestrictionsPresentAsSingle() throws Exception {
         RestrictionProvider rp = new TestRestrictionProvider("mandatory", Type.NAMES, true);
 
-        JackrabbitAccessControlList acl = createACL(TEST_PATH, new ArrayList(), namePathMapper, rp);
+        JackrabbitAccessControlList acl = createACL(TEST_PATH, new ArrayList<>(), namePathMapper, rp);
         acl.addEntry(testPrincipal, testPrivileges, false, Collections.singletonMap("mandatory", getValueFactory(root).createValue("name", PropertyType.NAME)), Collections.emptyMap());
     }
 
@@ -874,40 +846,12 @@ public class ACLTest extends AbstractAcc
     public void testMandatoryMVRestrictionsPresent() throws Exception {
         RestrictionProvider rp = new TestRestrictionProvider("mandatory", Type.NAMES, true);
 
-        JackrabbitAccessControlList acl = createACL(TEST_PATH, new ArrayList(), namePathMapper, rp);
+        JackrabbitAccessControlList acl = createACL(TEST_PATH, new ArrayList<>(), namePathMapper, rp);
         acl.addEntry(testPrincipal, testPrivileges, false, Collections.emptyMap(), Collections.singletonMap("mandatory", new Value[] {getValueFactory(root).createValue("name", PropertyType.NAME)}));
     }
 
     //--------------------------------------------------------------------------
 
-    private class InvalidPrivilege implements Privilege {
-
-        @Override
-        public String getName() {
-            return "invalidPrivilege";
-        }
-
-        @Override
-        public boolean isAbstract() {
-            return false;
-        }
-
-        @Override
-        public boolean isAggregate() {
-            return false;
-        }
-
-        @Override
-        public Privilege[] getDeclaredAggregatePrivileges() {
-            return new Privilege[0];
-        }
-
-        @Override
-        public Privilege[] getAggregatePrivileges() {
-            return new Privilege[0];
-        }
-    }
-
     private final class TestRestrictionProvider extends AbstractRestrictionProvider {
 
         private TestRestrictionProvider(String name, Type type, boolean isMandatory) {

Modified: jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AbstractAccessControlTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AbstractAccessControlTest.java?rev=1860730&r1=1860729&r2=1860730&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AbstractAccessControlTest.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AbstractAccessControlTest.java Thu Jun  6 17:20:27 2019
@@ -31,8 +31,10 @@ import org.apache.jackrabbit.JcrConstant
 import org.apache.jackrabbit.api.security.authorization.PrivilegeManager;
 import org.apache.jackrabbit.api.security.principal.PrincipalManager;
 import org.apache.jackrabbit.oak.AbstractSecurityTest;
+import org.apache.jackrabbit.oak.commons.PathUtils;
 import org.apache.jackrabbit.oak.namepath.NamePathMapper;
 import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.plugins.tree.TreeUtil;
 import org.apache.jackrabbit.oak.spi.security.authorization.AuthorizationConfiguration;
 import org.apache.jackrabbit.oak.spi.security.authorization.accesscontrol.ACE;
 import org.apache.jackrabbit.oak.spi.security.authorization.restriction.Restriction;
@@ -41,7 +43,6 @@ import org.apache.jackrabbit.oak.spi.sec
 import org.apache.jackrabbit.oak.spi.security.privilege.PrivilegeBits;
 import org.apache.jackrabbit.oak.spi.security.privilege.PrivilegeBitsProvider;
 import org.apache.jackrabbit.oak.spi.security.privilege.PrivilegeConstants;
-import org.apache.jackrabbit.oak.util.NodeUtil;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 import org.junit.Before;
@@ -62,8 +63,7 @@ public abstract class AbstractAccessCont
     public void before() throws Exception {
         super.before();
 
-        NodeUtil rootNode = new NodeUtil(root.getTree("/"), getNamePathMapper());
-        rootNode.addChild("testPath", JcrConstants.NT_UNSTRUCTURED);
+        TreeUtil.addChild(root.getTree(PathUtils.ROOT_PATH), "testPath", JcrConstants.NT_UNSTRUCTURED);
         root.commit();
 
         testPrincipal = getTestUser().getPrincipal();
@@ -89,33 +89,37 @@ public abstract class AbstractAccessCont
         }
     }
 
+    @NotNull
     RestrictionProvider getRestrictionProvider() {
         return getConfig(AuthorizationConfiguration.class).getRestrictionProvider();
     }
 
+    @NotNull
     PrivilegeBitsProvider getBitsProvider() {
         return new PrivilegeBitsProvider(root);
     }
 
+    @NotNull
     List<ACE> createTestEntries() throws RepositoryException {
-        List<ACE> entries = new ArrayList(3);
+        List<ACE> entries = new ArrayList<>(3);
         for (int i = 0; i < 3; i++) {
-            entries.add(createEntry(
-                    new PrincipalImpl("testPrincipal" + i), true, null, PrivilegeConstants.JCR_READ));
+            entries.add(createEntry(new PrincipalImpl("testPrincipal" + i), true, null, PrivilegeConstants.JCR_READ));
         }
         return entries;
     }
 
-    ACE createEntry(Principal principal, boolean isAllow, Set<Restriction> restrictions, String... privilegeNames) throws RepositoryException {
+    @NotNull
+    ACE createEntry(@NotNull Principal principal, boolean isAllow, @Nullable Set<Restriction> restrictions, @NotNull String... privilegeNames) throws RepositoryException {
         return createEntry(principal, privilegesFromNames(privilegeNames), isAllow, restrictions);
     }
 
-    ACE createEntry(Principal principal, Privilege[] privileges, boolean isAllow)
-            throws RepositoryException {
+    @NotNull
+    ACE createEntry(@NotNull Principal principal, @NotNull Privilege[] privileges, boolean isAllow) throws RepositoryException {
         return createEntry(principal, privileges, isAllow, null);
     }
 
-    ACE createEntry(Principal principal, PrivilegeBits bits, boolean isAllow, Set<Restriction> restrictions) throws RepositoryException {
+    @NotNull
+    ACE createEntry(@NotNull Principal principal, @NotNull PrivilegeBits bits, boolean isAllow, @NotNull Set<Restriction> restrictions) throws RepositoryException {
         AccessControlPolicyIterator it = getAccessControlManager(root).getApplicablePolicies(TEST_PATH);
         while (it.hasNext()) {
             AccessControlPolicy policy = it.nextAccessControlPolicy();
@@ -127,30 +131,39 @@ public abstract class AbstractAccessCont
         throw new UnsupportedOperationException();
     }
 
+    @NotNull
     private ACE createEntry(@NotNull Principal principal, @NotNull Privilege[] privileges, boolean isAllow, @Nullable Set<Restriction> restrictions)
             throws RepositoryException {
         ACL acl = createEmptyACL();
         return acl.createACE(principal, getBitsProvider().getBits(privileges, getNamePathMapper()), isAllow, restrictions);
     }
 
+    @NotNull
     ACL createEmptyACL() {
-        return createACL(TEST_PATH, Collections.<ACE>emptyList(), getNamePathMapper(), getRestrictionProvider());
-    }
-
-    ACL createACL(@NotNull List<ACE> entries) {
-        return createACL(TEST_PATH, entries, namePathMapper, getRestrictionProvider());
+        return createACL(TEST_PATH, Collections.emptyList(), getNamePathMapper(), getRestrictionProvider());
     }
 
+    @NotNull
     ACL createACL(@Nullable String jcrPath,
                   @NotNull List<ACE> entries,
                   @NotNull NamePathMapper namePathMapper) {
         return createACL(jcrPath, entries, namePathMapper, getRestrictionProvider());
     }
 
+    @NotNull
+    ACL createACL(@Nullable String jcrPath,
+                  @NotNull List<ACE> entries,
+                  @NotNull NamePathMapper namePathMapper,
+                  @NotNull RestrictionProvider restrictionProvider) {
+        return createACL(jcrPath, entries, namePathMapper, restrictionProvider, privilegeManager);
+    }
+
+    @NotNull
     ACL createACL(@Nullable String jcrPath,
                   @NotNull List<ACE> entries,
                   @NotNull NamePathMapper namePathMapper,
-                  final @NotNull RestrictionProvider restrictionProvider) {
+                  @NotNull RestrictionProvider restrictionProvider,
+                  @NotNull PrivilegeManager privilegeManager) {
         String path = (jcrPath == null) ? null : namePathMapper.getOakPath(jcrPath);
         return new ACL(path, entries, namePathMapper) {
             @NotNull

Modified: jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlImporterAbortTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlImporterAbortTest.java?rev=1860730&r1=1860729&r2=1860730&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlImporterAbortTest.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlImporterAbortTest.java Thu Jun  6 17:20:27 2019
@@ -33,6 +33,6 @@ public class AccessControlImporterAbortT
     public void testStartAceChildInfoUnknownPrincipal() throws Exception {
         init();
         importer.start(aclTree);
-        importer.startChildInfo(aceInfo, ImmutableList.of(unknownPrincipalInfo));
+        importer.startChildInfo(aceGrantInfo, ImmutableList.of(unknownPrincipalInfo));
     }
 }
\ No newline at end of file

Modified: jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlImporterBaseTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlImporterBaseTest.java?rev=1860730&r1=1860729&r2=1860730&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlImporterBaseTest.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlImporterBaseTest.java Thu Jun  6 17:20:27 2019
@@ -26,7 +26,9 @@ import javax.jcr.Value;
 import javax.jcr.ValueFormatException;
 import javax.jcr.nodetype.ConstraintViolationException;
 import javax.jcr.security.AccessControlException;
+import javax.jcr.security.AccessControlList;
 import javax.jcr.security.AccessControlManager;
+import javax.jcr.security.AccessControlPolicy;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
@@ -36,14 +38,18 @@ import org.apache.jackrabbit.api.Jackrab
 import org.apache.jackrabbit.api.security.JackrabbitAccessControlList;
 import org.apache.jackrabbit.commons.jackrabbit.authorization.AccessControlUtils;
 import org.apache.jackrabbit.oak.AbstractSecurityTest;
+import org.apache.jackrabbit.oak.api.Root;
 import org.apache.jackrabbit.oak.api.Tree;
 import org.apache.jackrabbit.oak.api.Type;
 import org.apache.jackrabbit.oak.commons.PathUtils;
+import org.apache.jackrabbit.oak.namepath.NamePathMapper;
 import org.apache.jackrabbit.oak.spi.nodetype.NodeTypeConstants;
 import org.apache.jackrabbit.oak.spi.security.ConfigurationParameters;
+import org.apache.jackrabbit.oak.spi.security.SecurityProvider;
 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.principal.EveryonePrincipal;
+import org.apache.jackrabbit.oak.spi.security.principal.PrincipalConfiguration;
 import org.apache.jackrabbit.oak.spi.security.privilege.PrivilegeConstants;
 import org.apache.jackrabbit.oak.spi.xml.NodeInfo;
 import org.apache.jackrabbit.oak.spi.xml.PropInfo;
@@ -53,16 +59,25 @@ import org.apache.jackrabbit.oak.spi.xml
 import org.apache.jackrabbit.oak.plugins.tree.TreeUtil;
 import org.jetbrains.annotations.NotNull;
 import org.junit.Test;
-import org.mockito.Mockito;
+import org.mockito.invocation.InvocationOnMock;
+import org.mockito.stubbing.Answer;
 
+import static org.apache.jackrabbit.JcrConstants.JCR_PRIMARYTYPE;
+import static org.apache.jackrabbit.oak.spi.nodetype.NodeTypeConstants.NT_OAK_UNSTRUCTURED;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.when;
+import static org.mockito.Mockito.withSettings;
 
 public abstract class AccessControlImporterBaseTest  extends AbstractSecurityTest implements AccessControlConstants {
 
-    final NodeInfo aceInfo = new NodeInfo("anyAceName", NT_REP_GRANT_ACE, ImmutableList.of(), null);
+    final NodeInfo aceGrantInfo = new NodeInfo("grantAceName", NT_REP_GRANT_ACE, ImmutableList.of(), null);
+    final NodeInfo aceDenyInfo = new NodeInfo("denyAceName", NT_REP_DENY_ACE, ImmutableList.of(), null);
     final NodeInfo restrInfo = new NodeInfo("anyRestrName", NT_REP_RESTRICTIONS, ImmutableList.of(), null);
     final PropInfo unknownPrincipalInfo = new PropInfo(REP_PRINCIPAL_NAME, PropertyType.STRING, createTextValue("unknownPrincipal"));
 
@@ -80,7 +95,7 @@ public abstract class AccessControlImpor
         super.before();
 
         Tree t = root.getTree(PathUtils.ROOT_PATH).addChild("testNode");
-        t.setProperty(JcrConstants.JCR_PRIMARYTYPE, NodeTypeConstants.NT_OAK_UNSTRUCTURED, Type.NAME);
+        t.setProperty(JCR_PRIMARYTYPE, NT_OAK_UNSTRUCTURED, Type.NAME);
 
         AccessControlManager acMgr = getAccessControlManager(root);
         JackrabbitAccessControlList acl = AccessControlUtils.getAccessControlList(acMgr, t.getPath());
@@ -120,8 +135,9 @@ public abstract class AccessControlImpor
 
     abstract String getImportBehavior();
 
-    Session mockJackrabbitSession() throws Exception {
-        JackrabbitSession s = Mockito.mock(JackrabbitSession.class);
+    @NotNull
+    private Session mockJackrabbitSession() throws Exception {
+        JackrabbitSession s = mock(JackrabbitSession.class);
         when(s.getPrincipalManager()).thenReturn(getPrincipalManager(root));
         when(s.getAccessControlManager()).thenReturn(getAccessControlManager(root));
         return s;
@@ -131,11 +147,12 @@ public abstract class AccessControlImpor
         return false;
     }
 
-    boolean init() throws Exception {
-        return importer.init(mockJackrabbitSession(), root, getNamePathMapper(), isWorkspaceImport(), ImportUUIDBehavior.IMPORT_UUID_COLLISION_REMOVE_EXISTING, new ReferenceChangeTracker(), getSecurityProvider());
+    void init() throws Exception {
+        importer.init(mockJackrabbitSession(), root, getNamePathMapper(), isWorkspaceImport(), ImportUUIDBehavior.IMPORT_UUID_COLLISION_REMOVE_EXISTING, new ReferenceChangeTracker(), getSecurityProvider());
     }
 
-    TextValue createTextValue(@NotNull String val) {
+    @NotNull
+    private TextValue createTextValue(@NotNull String val) {
         return new TextValue() {
             @Override
             public String getString() {
@@ -156,7 +173,7 @@ public abstract class AccessControlImpor
     }
 
     List<TextValue> createTextValues(@NotNull String... values) {
-        List<TextValue> l = new ArrayList();
+        List<TextValue> l = new ArrayList<>();
         for (String v : values) {
             l.add(createTextValue(v));
         }
@@ -165,8 +182,8 @@ public abstract class AccessControlImpor
 
     //---------------------------------------------------------------< init >---
     @Test
-    public void testInitNoJackrabbitSession() throws Exception {
-        Session s = Mockito.mock(Session.class);
+    public void testInitNoJackrabbitSession() {
+        Session s = mock(Session.class);
         assertFalse(importer.init(s, root, getNamePathMapper(), false, ImportUUIDBehavior.IMPORT_UUID_COLLISION_THROW, new ReferenceChangeTracker(), getSecurityProvider()));
     }
 
@@ -197,10 +214,23 @@ public abstract class AccessControlImpor
         assertTrue(importer.init(mockJackrabbitSession(), root, getNamePathMapper(), isWorkspaceImport(), ImportUUIDBehavior.IMPORT_UUID_CREATE_NEW, new ReferenceChangeTracker(), getSecurityProvider()));
     }
 
+    @Test
+    public void testInitCausesRepositoryException() {
+        JackrabbitSession s = mock(JackrabbitSession.class, withSettings().defaultAnswer(new Answer() {
+
+            @Override
+            public Object answer(InvocationOnMock invocationOnMock) throws Throwable {
+                throw new RepositoryException();
+            }
+        }));
+        // session methods are only invoked for session-imports
+        assertEquals(isWorkspaceImport(), importer.init(s, root, getNamePathMapper(), isWorkspaceImport(), ImportUUIDBehavior.IMPORT_UUID_COLLISION_REPLACE_EXISTING, new ReferenceChangeTracker(), getSecurityProvider()));
+    }
+
     //--------------------------------------------------------------< start >---
     @Test(expected = IllegalStateException.class)
     public void testStartNotInitialized() throws Exception {
-        importer.start(Mockito.mock(Tree.class));
+        importer.start(mock(Tree.class));
     }
 
     @Test
@@ -230,6 +260,13 @@ public abstract class AccessControlImpor
     }
 
     @Test
+    public void testStartAclTreeWrongPrimaryType() throws Exception {
+        init();
+        aclTree.setProperty(JCR_PRIMARYTYPE, NT_OAK_UNSTRUCTURED);
+        assertFalse(importer.start(aclTree));
+    }
+
+    @Test
     public void testStartRepoPolicyTree() throws Exception {
         init();
 
@@ -248,9 +285,7 @@ public abstract class AccessControlImpor
         init();
 
         Tree rootTree = root.getTree(PathUtils.ROOT_PATH);
-        Tree repoPolicy = accessControlledTree.addChild(REP_REPO_POLICY);
-        repoPolicy.setProperty(JcrConstants.JCR_PRIMARYTYPE, NT_REP_ACL, Type.NAME);
-
+        Tree repoPolicy = TreeUtil.addChild(rootTree, REP_REPO_POLICY, NT_REP_ACL);
         assertFalse(importer.start(repoPolicy));
     }
 
@@ -259,16 +294,40 @@ public abstract class AccessControlImpor
         init();
 
         TreeUtil.addMixin(accessControlledTree, MIX_REP_REPO_ACCESS_CONTROLLABLE, root.getTree(NodeTypeConstants.NODE_TYPES_PATH), null);
-        Tree repoPolicy = accessControlledTree.addChild(REP_REPO_POLICY);
-        repoPolicy.setProperty(JcrConstants.JCR_PRIMARYTYPE, NT_REP_ACL, Type.NAME);
+        Tree repoPolicy = TreeUtil.addChild(accessControlledTree, REP_REPO_POLICY, NT_REP_ACL);
+        assertFalse(importer.start(repoPolicy));
+    }
 
+    @Test
+    public void testStartRepoPolicyTreeWrongPrimaryType() throws Exception {
+        init();
+        TreeUtil.addMixin(accessControlledTree, MIX_REP_REPO_ACCESS_CONTROLLABLE, root.getTree(NodeTypeConstants.NODE_TYPES_PATH), null);
+        Tree repoPolicy = TreeUtil.addChild(accessControlledTree, REP_REPO_POLICY, NT_OAK_UNSTRUCTURED);
         assertFalse(importer.start(repoPolicy));
     }
 
+    @Test
+    public void testStartNoJackrabbitAccessControlList() throws Exception {
+        AccessControlList policy = mock(AccessControlList.class);
+        AccessControlManager acMgr = mock(AccessControlManager.class);
+        when(acMgr.getPolicies(anyString())).thenReturn(new AccessControlPolicy[] {policy});
+        JackrabbitSession s = mock(JackrabbitSession.class);
+        when(s.getAccessControlManager()).thenReturn(acMgr);
+        when(s.getPrincipalManager()).thenReturn(getPrincipalManager(root));
+
+        SecurityProvider sp = mock(SecurityProvider.class);
+        AuthorizationConfiguration ac = spy(getConfig(AuthorizationConfiguration.class));
+        when(ac.getAccessControlManager(root, getNamePathMapper())).thenReturn(acMgr);
+        when(sp.getConfiguration(AuthorizationConfiguration.class)).thenReturn(ac);
+        when(sp.getConfiguration(PrincipalConfiguration.class)).thenReturn(getConfig(PrincipalConfiguration.class));
+
+        importer.init(s, root, getNamePathMapper(), isWorkspaceImport(), ImportUUIDBehavior.IMPORT_UUID_COLLISION_REMOVE_EXISTING, new ReferenceChangeTracker(), sp);
+        assertFalse(importer.start(aclTree));
+    }
     //--------------------------------------------------< processReferences >---
 
     @Test
-    public void testProcessReferencesIsNoOp() throws Exception {
+    public void testProcessReferencesIsNoOp() {
         importer.processReferences();
         assertFalse(root.hasPendingChanges());
     }
@@ -277,12 +336,12 @@ public abstract class AccessControlImpor
 
     @Test(expected = IllegalStateException.class)
     public void testStartChildInfoNotInitialized() throws Exception {
-        importer.startChildInfo(Mockito.mock(NodeInfo.class), ImmutableList.of());
+        importer.startChildInfo(mock(NodeInfo.class), ImmutableList.of());
     }
 
     @Test(expected = ConstraintViolationException.class)
     public void testStartChildInfoUnknownType() throws Exception {
-        NodeInfo invalidChildInfo = new NodeInfo("anyName", NodeTypeConstants.NT_OAK_UNSTRUCTURED, ImmutableList.of(), null);
+        NodeInfo invalidChildInfo = new NodeInfo("anyName", NT_OAK_UNSTRUCTURED, ImmutableList.of(), null);
         init();
         importer.start(aclTree);
         importer.startChildInfo(invalidChildInfo, ImmutableList.of());
@@ -292,8 +351,8 @@ public abstract class AccessControlImpor
     public void testStartNestedAceChildInfo() throws Exception {
         init();
         importer.start(aclTree);
-        importer.startChildInfo(aceInfo, ImmutableList.of());
-        importer.startChildInfo(aceInfo, ImmutableList.of());
+        importer.startChildInfo(aceGrantInfo, ImmutableList.of());
+        importer.startChildInfo(aceDenyInfo, ImmutableList.of());
     }
 
     @Test(expected = ConstraintViolationException.class)
@@ -303,10 +362,11 @@ public abstract class AccessControlImpor
         importer.startChildInfo(restrInfo, ImmutableList.of());
     }
 
+    @Test
     public void testStartAceAndRestrictionChildInfo() throws Exception {
         init();
         importer.start(aclTree);
-        importer.startChildInfo(aceInfo, ImmutableList.of());
+        importer.startChildInfo(aceGrantInfo, ImmutableList.of());
         importer.startChildInfo(restrInfo, ImmutableList.of());
     }
 
@@ -315,7 +375,7 @@ public abstract class AccessControlImpor
         init();
         importer.start(aclTree);
         PropInfo invalidPrivInfo = new PropInfo(REP_PRIVILEGES, PropertyType.NAME, createTextValues("jcr:invalidPrivilege"), PropInfo.MultipleStatus.MULTIPLE);
-        importer.startChildInfo(aceInfo, ImmutableList.of(invalidPrivInfo));
+        importer.startChildInfo(aceDenyInfo, ImmutableList.of(invalidPrivInfo));
     }
 
     //-------------------------------------------------------< endChildInfo >---
@@ -336,7 +396,7 @@ public abstract class AccessControlImpor
     public void testEndChildInfoIncompleteAce() throws Exception {
         init();
         importer.start(aclTree);
-        importer.startChildInfo(aceInfo, ImmutableList.of());
+        importer.startChildInfo(aceGrantInfo, ImmutableList.of());
         importer.endChildInfo();
     }
 
@@ -367,7 +427,7 @@ public abstract class AccessControlImpor
     public void testInvalidRestriction() throws Exception {
         init();
         importer.start(aclTree);
-        importer.startChildInfo(aceInfo, ImmutableList.of(principalInfo, privInfo));
+        importer.startChildInfo(aceGrantInfo, ImmutableList.of(principalInfo, privInfo));
 
         PropInfo invalidRestrProp = new PropInfo(REP_GLOB, PropertyType.NAME, createTextValues("glob1", "glob2"), PropInfo.MultipleStatus.MULTIPLE);
         importer.startChildInfo(restrInfo, ImmutableList.of(invalidRestrProp));
@@ -380,7 +440,7 @@ public abstract class AccessControlImpor
     public void testUnknownRestriction() throws Exception {
         init();
         importer.start(aclTree);
-        importer.startChildInfo(aceInfo, ImmutableList.of(principalInfo, privInfo));
+        importer.startChildInfo(aceGrantInfo, ImmutableList.of(principalInfo, privInfo));
 
         PropInfo invalidRestrProp = new PropInfo("unknown", PropertyType.STRING, createTextValue("val"));
         importer.startChildInfo(restrInfo, ImmutableList.of(invalidRestrProp));
@@ -393,7 +453,7 @@ public abstract class AccessControlImpor
     public void testImportSimple() throws Exception {
         init();
         importer.start(aclTree);
-        importer.startChildInfo(aceInfo, ImmutableList.of(principalInfo, privInfo));
+        importer.startChildInfo(aceGrantInfo, ImmutableList.of(principalInfo, privInfo));
         importer.endChildInfo();
         importer.end(aclTree);
 
@@ -418,7 +478,7 @@ public abstract class AccessControlImpor
 
         init();
         importer.start(aclTree);
-        importer.startChildInfo(aceInfo, ImmutableList.of(principalInfo, privInfo, globInfo, ntNamesInfo, itemNamesInfo));
+        importer.startChildInfo(aceGrantInfo, ImmutableList.of(principalInfo, privInfo, globInfo, ntNamesInfo, itemNamesInfo));
         importer.endChildInfo();
         importer.end(aclTree);
 
@@ -436,7 +496,7 @@ public abstract class AccessControlImpor
 
         init();
         importer.start(aclTree);
-        importer.startChildInfo(aceInfo, ImmutableList.of(principalInfo, privInfo));
+        importer.startChildInfo(aceGrantInfo, ImmutableList.of(principalInfo, privInfo));
         importer.startChildInfo(restrInfo, ImmutableList.of(globInfo, ntNamesInfo, itemNamesInfo));
         importer.endChildInfo();
         importer.endChildInfo();

Modified: jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlImporterBesteffortTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlImporterBesteffortTest.java?rev=1860730&r1=1860729&r2=1860730&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlImporterBesteffortTest.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlImporterBesteffortTest.java Thu Jun  6 17:20:27 2019
@@ -39,7 +39,7 @@ public class AccessControlImporterBestef
     public void testStartAceChildInfoUnknownPrincipal() throws Exception {
         init();
         importer.start(aclTree);
-        importer.startChildInfo(aceInfo, ImmutableList.of(unknownPrincipalInfo));
+        importer.startChildInfo(aceGrantInfo, ImmutableList.of(unknownPrincipalInfo));
     }
 
     @Test
@@ -48,7 +48,7 @@ public class AccessControlImporterBestef
         importer.start(aclTree);
 
         PropInfo privs = new PropInfo(REP_PRIVILEGES, PropertyType.NAME, createTextValues(PrivilegeConstants.JCR_READ));
-        importer.startChildInfo(aceInfo, ImmutableList.of(unknownPrincipalInfo, privs));
+        importer.startChildInfo(aceGrantInfo, ImmutableList.of(unknownPrincipalInfo, privs));
         importer.endChildInfo();
 
         importer.end(aclTree);

Modified: jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlImporterIgnoreTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlImporterIgnoreTest.java?rev=1860730&r1=1860729&r2=1860730&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlImporterIgnoreTest.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlImporterIgnoreTest.java Thu Jun  6 17:20:27 2019
@@ -37,7 +37,7 @@ public class AccessControlImporterIgnore
     public void testStartAceChildInfoUnknownPrincipal() throws Exception {
         init();
         importer.start(aclTree);
-        importer.startChildInfo(aceInfo, ImmutableList.of(unknownPrincipalInfo));
+        importer.startChildInfo(aceGrantInfo, ImmutableList.of(unknownPrincipalInfo));
     }
 
     @Test
@@ -46,7 +46,7 @@ public class AccessControlImporterIgnore
         importer.start(aclTree);
 
         PropInfo privs = new PropInfo(REP_PRIVILEGES, PropertyType.NAME, createTextValues(PrivilegeConstants.JCR_READ));
-        importer.startChildInfo(aceInfo, ImmutableList.of(unknownPrincipalInfo, privs));
+        importer.startChildInfo(aceGrantInfo, ImmutableList.of(unknownPrincipalInfo, privs));
         importer.endChildInfo();
 
         importer.end(aclTree);