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/10/31 10:00:57 UTC

svn commit: r1869208 [2/2] - in /jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external: basic/ impl/ impl/jmx/ impl/principal/

Modified: jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/ExternalGroupPrincipalProviderTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/ExternalGroupPrincipalProviderTest.java?rev=1869208&r1=1869207&r2=1869208&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/ExternalGroupPrincipalProviderTest.java (original)
+++ jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/ExternalGroupPrincipalProviderTest.java Thu Oct 31 10:00:56 2019
@@ -16,27 +16,20 @@
  */
 package org.apache.jackrabbit.oak.spi.security.authentication.external.impl.principal;
 
-import java.security.Principal;
-import java.util.Arrays;
-import java.util.Collections;
-import java.util.Comparator;
-import java.util.HashSet;
-import java.util.Iterator;
-import java.util.List;
-import java.util.Set;
 import com.google.common.base.Function;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
-
 import org.apache.jackrabbit.api.security.principal.GroupPrincipal;
 import org.apache.jackrabbit.api.security.principal.PrincipalManager;
 import org.apache.jackrabbit.api.security.user.Authorizable;
 import org.apache.jackrabbit.api.security.user.Group;
 import org.apache.jackrabbit.api.security.user.User;
 import org.apache.jackrabbit.api.security.user.UserManager;
+import org.apache.jackrabbit.oak.api.QueryEngine;
 import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Tree;
 import org.apache.jackrabbit.oak.namepath.NamePathMapper;
 import org.apache.jackrabbit.oak.spi.security.authentication.external.ExternalGroup;
 import org.apache.jackrabbit.oak.spi.security.authentication.external.ExternalIdentity;
@@ -51,11 +44,29 @@ import org.jetbrains.annotations.NotNull
 import org.jetbrains.annotations.Nullable;
 import org.junit.Test;
 
+import java.security.Principal;
+import java.text.ParseException;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import static org.apache.jackrabbit.oak.spi.security.authentication.external.impl.ExternalIdentityConstants.REP_EXTERNAL_ID;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertSame;
 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;
 
 public class ExternalGroupPrincipalProviderTest extends AbstractPrincipalTest {
 
@@ -69,18 +80,15 @@ public class ExternalGroupPrincipalProvi
         root.refresh();
     }
 
+    @NotNull
     Set<Principal> getExpectedGroupPrincipals(@NotNull String userId) throws Exception {
         if (syncConfig.user().getMembershipNestingDepth() == 1) {
-            Set<Principal> principals = ImmutableSet.copyOf(Iterables.transform(idp.getUser(userId).getDeclaredGroups(), new Function<ExternalIdentityRef, Principal>() {
-                @Nullable
-                @Override
-                public Principal apply(ExternalIdentityRef input) {
-                    try {
-                        return new PrincipalImpl(idp.getIdentity(input).getPrincipalName());
-                    } catch (ExternalIdentityException e) {
-                        throw new RuntimeException(e);
-                    }
-                };
+            Set<Principal> principals = ImmutableSet.copyOf(Iterables.transform(idp.getUser(userId).getDeclaredGroups(), (Function<ExternalIdentityRef, Principal>) input -> {
+                try {
+                    return new PrincipalImpl(idp.getIdentity(input).getPrincipalName());
+                } catch (ExternalIdentityException e) {
+                    throw new RuntimeException(e);
+                }
             }));
             return principals;
         } else {
@@ -90,6 +98,11 @@ public class ExternalGroupPrincipalProvi
         }
     }
 
+    @NotNull
+    Set<Principal> getExpectedAllSearchResult(@NotNull String userId) throws Exception {
+        return getExpectedGroupPrincipals(userId);
+    }
+
     private void collectExpectedPrincipals(Set<Principal> grPrincipals, @NotNull Iterable<ExternalIdentityRef> declaredGroups, long depth) throws Exception {
         if (depth <= 0) {
             return;
@@ -316,6 +329,44 @@ public class ExternalGroupPrincipalProvi
     }
 
     @Test
+    public void testGetPrincipalsNonExistingUserTree() throws Exception {
+        Authorizable a = spy(getUserManager(root).getAuthorizable(USER_ID));
+        when(a.getPath()).thenReturn("/path/to/non/existing/item");
+        UserManager um = when(mock(UserManager.class).getAuthorizable(USER_ID)).thenReturn(a).getMock();
+        UserConfiguration uc = when(mock(UserConfiguration.class).getUserManager(root, getNamePathMapper())).thenReturn(um).getMock();
+
+        ExternalGroupPrincipalProvider pp = new ExternalGroupPrincipalProvider(root, uc, getNamePathMapper(), ImmutableMap.of(idp.getName(), getAutoMembership()));
+        assertTrue(pp.getPrincipals(USER_ID).isEmpty());
+    }
+
+    @Test
+    public void testGetPrincipalsForGroupTree() throws Exception {
+        Authorizable group = getUserManager(root).createGroup("testGroup");
+        Authorizable a = spy(getUserManager(root).getAuthorizable(USER_ID));
+        when(a.getPath()).thenReturn(group.getPath());
+        UserManager um = when(mock(UserManager.class).getAuthorizable(USER_ID)).thenReturn(a).getMock();
+        UserConfiguration uc = when(mock(UserConfiguration.class).getUserManager(root, getNamePathMapper())).thenReturn(um).getMock();
+
+        ExternalGroupPrincipalProvider pp = new ExternalGroupPrincipalProvider(root, uc, getNamePathMapper(), ImmutableMap.of(idp.getName(), getAutoMembership()));
+        assertTrue(pp.getPrincipals(USER_ID).isEmpty());
+    }
+
+    @Test
+    public void testGetPrincipalsMissingIdpName() throws Exception {
+        String userPath = getUserManager(root).getAuthorizable(USER_ID).getPath();
+
+        Tree t = root.getTree(userPath);
+        t.removeProperty(REP_EXTERNAL_ID);
+
+        String[] automembership = getAutoMembership();
+        ExternalGroupPrincipalProvider pp = new ExternalGroupPrincipalProvider(root, getUserConfiguration(), getNamePathMapper(), ImmutableMap.of(idp.getName(), automembership));
+
+        Set<? extends Principal> principals = pp.getPrincipals(USER_ID);
+        assertFalse(principals.isEmpty());
+        assertFalse(principals.removeAll(ImmutableSet.copyOf(automembership)));
+    }
+
+    @Test
     public void testFindPrincipalsByHintTypeNotGroup() {
         Iterator<? extends Principal> iter = principalProvider.findPrincipals("a",
                 PrincipalManager.SEARCH_TYPE_NOT_GROUP);
@@ -376,7 +427,7 @@ public class ExternalGroupPrincipalProvi
         Set<? extends Principal> res2 = ImmutableSet
                 .copyOf(principalProvider.findPrincipals("%", false, PrincipalManager.SEARCH_TYPE_ALL, 0, -1));
         assertEquals(expected, res2);
-}
+    }
 
     @Test
     public void testFindPrincipalsByTypeNotGroup() {
@@ -387,16 +438,16 @@ public class ExternalGroupPrincipalProvi
     @Test
     public void testFindPrincipalsByTypeGroup() throws Exception {
         Set<? extends Principal> res = ImmutableSet.copyOf(principalProvider.findPrincipals(PrincipalManager.SEARCH_TYPE_GROUP));
-        assertEquals(getExpectedGroupPrincipals(USER_ID), res);
+        assertEquals(getExpectedAllSearchResult(USER_ID), res);
 
         Set<? extends Principal> res2 = ImmutableSet.copyOf(principalProvider.findPrincipals(null, false, PrincipalManager.SEARCH_TYPE_GROUP, 0, -1));
-        assertEquals(getExpectedGroupPrincipals(USER_ID), res2);
+        assertEquals(getExpectedAllSearchResult(USER_ID), res2);
     }
 
     @Test
     public void testFindPrincipalsByTypeAll() throws Exception {
         Set<? extends Principal> res = ImmutableSet.copyOf(principalProvider.findPrincipals(PrincipalManager.SEARCH_TYPE_ALL));
-        assertEquals(getExpectedGroupPrincipals(USER_ID), res);
+        assertEquals(getExpectedAllSearchResult(USER_ID), res);
     }
 
     @Test
@@ -426,7 +477,7 @@ public class ExternalGroupPrincipalProvi
         List<Principal> in = Arrays.asList(new PrincipalImpl("p3"), new PrincipalImpl("p1"), new PrincipalImpl("p2"));
         ExternalGroupPrincipalProvider p = new ExternalGroupPrincipalProvider(root,
                 getSecurityProvider().getConfiguration(UserConfiguration.class), NamePathMapper.DEFAULT,
-                ImmutableMap.of(idp.getName(), new String[0])) {
+                ImmutableMap.of(idp.getName(), getAutoMembership())) {
             @NotNull
             @Override
             public Iterator<? extends Principal> findPrincipals(@Nullable String nameHint, int searchType) {
@@ -438,6 +489,76 @@ public class ExternalGroupPrincipalProvi
         assertEquals(in, out);
     }
 
+    @Test
+    public void testFindPrincipalsWithOffset() throws Exception {
+        Set<Principal> all = getExpectedAllSearchResult(USER_ID);
+
+        long offset = 2;
+        long expectedSize = (all.size() <= offset) ? 0 : all.size()-offset;
+        Set<? extends Principal> result = ImmutableSet.copyOf(principalProvider.findPrincipals(null, false, PrincipalManager.SEARCH_TYPE_GROUP, offset, -1));
+        assertEquals(expectedSize, result.size());
+    }
+
+    @Test
+    public void testFindPrincipalsWithOffsetEqualsResultSize() throws Exception {
+        Set<Principal> all = getExpectedAllSearchResult(USER_ID);
+
+        Set<? extends Principal> result = ImmutableSet.copyOf(principalProvider.findPrincipals(null, false, PrincipalManager.SEARCH_TYPE_GROUP, all.size(), -1));
+        assertTrue(result.isEmpty());
+    }
+
+    @Test
+    public void testFindPrincipalsWithOffsetExceedsResultSize() throws Exception {
+        Set<Principal> all = getExpectedAllSearchResult(USER_ID);
+
+        Set<? extends Principal> result = ImmutableSet.copyOf(principalProvider.findPrincipals(null, false, PrincipalManager.SEARCH_TYPE_GROUP, all.size()+1, -1));
+        assertTrue(result.isEmpty());
+    }
+
+    @Test
+    public void testFindPrincipalsWithLimit() throws Exception {
+        Set<Principal> all = getExpectedAllSearchResult(USER_ID);
+
+        Set<? extends Principal> result = ImmutableSet.copyOf(principalProvider.findPrincipals(null, false, PrincipalManager.SEARCH_TYPE_GROUP, 0, 1));
+        assertEquals(1, result.size());
+    }
+
+    @Test
+    public void testFindPrincipalsWithLimitExceedsResultSize() throws Exception {
+        Set<Principal> all = getExpectedAllSearchResult(USER_ID);
+
+        Set<? extends Principal> result = ImmutableSet.copyOf(principalProvider.findPrincipals(null, false, PrincipalManager.SEARCH_TYPE_GROUP, 0, all.size()+1));
+        assertEquals(all, result);
+    }
+
+    @Test
+    public void testFindPrincipalsWithZeroLimit() throws Exception {
+        Set<? extends Principal> result = ImmutableSet.copyOf(principalProvider.findPrincipals(null, false, PrincipalManager.SEARCH_TYPE_GROUP, 0, 0));
+        assertTrue(result.isEmpty());
+    }
+
+    @Test
+    public void testFindPrincipalsWithOffsetAndLimit() throws Exception {
+        Set<Principal> all = getExpectedAllSearchResult(USER_ID);
+
+        long offset = all.size()-1;
+        long limit = all.size();
+        Set<? extends Principal> result = ImmutableSet.copyOf(principalProvider.findPrincipals(null, false, PrincipalManager.SEARCH_TYPE_GROUP, offset, limit));
+        assertEquals(1, result.size());
+    }
+
+    @Test
+    public void testFindPrincipalsWithParseException() throws Exception {
+        QueryEngine qe = mock(QueryEngine.class);
+        when(qe.executeQuery(anyString(), anyString(), any(Map.class), any(Map.class))).thenThrow(new ParseException("fail", 0));
+
+        Root r = when(mock(Root.class).getQueryEngine()).thenReturn(qe).getMock();
+        ExternalGroupPrincipalProvider pp = new ExternalGroupPrincipalProvider(r, getUserConfiguration(), getNamePathMapper(), Collections.emptyMap());
+
+        assertNull(pp.getPrincipal("a"));
+        assertFalse(pp.findPrincipals(PrincipalManager.SEARCH_TYPE_GROUP).hasNext());
+    }
+
     private static final class TestUser extends TestIdentityProvider.TestIdentity implements ExternalUser {
 
         private final Iterable<ExternalIdentityRef> declaredGroups;

Modified: jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/ExternalGroupPrincipalTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/ExternalGroupPrincipalTest.java?rev=1869208&r1=1869207&r2=1869208&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/ExternalGroupPrincipalTest.java (original)
+++ jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/ExternalGroupPrincipalTest.java Thu Oct 31 10:00:56 2019
@@ -16,23 +16,40 @@
  */
 package org.apache.jackrabbit.oak.spi.security.authentication.external.impl.principal;
 
-import java.security.Principal;
-import java.util.Enumeration;
-import com.google.common.base.Function;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
-
 import org.apache.jackrabbit.api.security.principal.GroupPrincipal;
+import org.apache.jackrabbit.api.security.principal.ItemBasedPrincipal;
+import org.apache.jackrabbit.api.security.user.Authorizable;
 import org.apache.jackrabbit.api.security.user.Group;
-import org.apache.jackrabbit.oak.spi.security.authentication.external.ExternalGroup;
+import org.apache.jackrabbit.api.security.user.UserManager;
+import org.apache.jackrabbit.oak.api.QueryEngine;
+import org.apache.jackrabbit.oak.api.Root;
+import org.apache.jackrabbit.oak.api.Type;
+import org.apache.jackrabbit.oak.spi.security.authentication.external.ExternalIdentityRef;
 import org.apache.jackrabbit.oak.spi.security.authentication.external.ExternalUser;
 import org.apache.jackrabbit.oak.spi.security.principal.PrincipalImpl;
-import org.jetbrains.annotations.Nullable;
+import org.apache.jackrabbit.oak.spi.security.user.UserConfiguration;
+import org.jetbrains.annotations.NotNull;
 import org.junit.Test;
 
+import javax.jcr.RepositoryException;
+import java.security.Principal;
+import java.text.ParseException;
+import java.util.Enumeration;
+import java.util.Map;
+
+import static org.apache.jackrabbit.oak.spi.security.authentication.external.TestIdentityProvider.ID_SECOND_USER;
+import static org.apache.jackrabbit.oak.spi.security.authentication.external.impl.ExternalIdentityConstants.REP_EXTERNAL_PRINCIPAL_NAMES;
 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;
 
 public class ExternalGroupPrincipalTest extends AbstractPrincipalTest {
 
@@ -46,17 +63,24 @@ public class ExternalGroupPrincipalTest
     }
 
     @Test
+    public void testNotIsMember() throws Exception {
+        GroupPrincipal principal = getGroupPrincipal(idp.getUser(USER_ID).getDeclaredGroups().iterator().next());
+
+        Authorizable notMember = getUserManager(root).getAuthorizable(ID_SECOND_USER);
+        assertFalse(principal.isMember(notMember.getPrincipal()));
+
+        root.getTree(notMember.getPath()).setProperty(REP_EXTERNAL_PRINCIPAL_NAMES, ImmutableList.of("secondGroup"), Type.STRINGS);
+        assertFalse(principal.isMember(notMember.getPrincipal()));
+
+        root.getTree(notMember.getPath()).setProperty(REP_EXTERNAL_PRINCIPAL_NAMES, ImmutableList.of(), Type.STRINGS);
+        assertFalse(principal.isMember(new PrincipalImpl(notMember.getPrincipal().getName())));
+    }
+
+    @Test
     public void testIsMemberExternalGroup() throws Exception {
         GroupPrincipal principal = getGroupPrincipal();
 
-        Iterable<String> exGroupPrincNames = Iterables.transform(ImmutableList.copyOf(idp.listGroups()), new Function<ExternalGroup, String>() {
-            @Nullable
-            @Override
-            public String apply(ExternalGroup input) {
-                return input.getPrincipalName();
-            }
-        });
-
+        Iterable<String> exGroupPrincNames = Iterables.transform(ImmutableList.copyOf(idp.listGroups()), input -> input.getPrincipalName());
         for (String principalName : exGroupPrincNames) {
             assertFalse(principal.isMember(new PrincipalImpl(principalName)));
         }
@@ -75,15 +99,26 @@ public class ExternalGroupPrincipalTest
         Group gr = createTestGroup();
         GroupPrincipal principal = getGroupPrincipal();
 
+        String name = gr.getPrincipal().getName();
         assertFalse(principal.isMember(gr.getPrincipal()));
-        assertFalse(principal.isMember(new PrincipalImpl(gr.getPrincipal().getName())));
+        assertFalse(principal.isMember(new PrincipalImpl(name)));
+        ItemBasedPrincipal ibp = new ItemBasedPrincipal() {
+            @Override
+            public @NotNull String getPath() throws RepositoryException {
+                return gr.getPath();
+            }
+            @Override
+            public String getName() {
+                return name;
+            }
+        };
+        assertFalse(principal.isMember(ibp));
     }
 
     @Test
     public void testMembers() throws Exception {
         GroupPrincipal principal = getGroupPrincipal();
-
-        Principal[] expectedMembers = new Principal[] {
+        Principal[] expectedMembers = new Principal[]{
                 getUserManager(root).getAuthorizable(USER_ID).getPrincipal(),
                 new PrincipalImpl(idp.getUser(USER_ID).getPrincipalName())
         };
@@ -95,4 +130,35 @@ public class ExternalGroupPrincipalTest
             assertFalse(members.hasMoreElements());
         }
     }
+
+    @Test
+    public void testMembersUserLookupFails() throws Exception {
+        UserManager um = spy(getUserManager(root));
+        String userPath = um.getAuthorizable(USER_ID).getPath();
+        when(um.getAuthorizableByPath(userPath)).thenReturn(null);
+
+        UserConfiguration uc = when(mock(UserConfiguration.class).getUserManager(root, getNamePathMapper())).thenReturn(um).getMock();
+        ExternalGroupPrincipalProvider pp = new ExternalGroupPrincipalProvider(root, uc, getNamePathMapper(), ImmutableMap.of(idp.getName(), getAutoMembership()));
+
+        ExternalIdentityRef ref = idp.getUser(USER_ID).getDeclaredGroups().iterator().next();
+        String groupName = idp.getIdentity(ref).getPrincipalName();
+
+        Principal gp = pp.getPrincipal(groupName);
+        assertTrue(gp instanceof GroupPrincipal);
+        assertFalse(((GroupPrincipal)gp).members().hasMoreElements());
+    }
+
+    @Test
+    public void testMembersQueryFails() throws Exception {
+        QueryEngine qe = mock(QueryEngine.class);
+        when(qe.executeQuery(anyString(), anyString(), any(Map.class), any(Map.class))).thenThrow(new ParseException("fail", 0));
+
+        Root r = spy(root);
+        when(r.getQueryEngine()).thenReturn(qe);
+        ExternalGroupPrincipalProvider pp = new ExternalGroupPrincipalProvider(r, getUserConfiguration(), getNamePathMapper(), ImmutableMap.of(idp.getName(), getAutoMembership()));
+
+        Principal gp = pp.getMembershipPrincipals(getUserManager(root).getAuthorizable(USER_ID).getPrincipal()).iterator().next();
+        assertTrue(gp instanceof GroupPrincipal);
+        assertFalse(((GroupPrincipal)gp).members().hasMoreElements());
+    }
 }

Modified: jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/ExternalIdentityRepositoryInitializerTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/ExternalIdentityRepositoryInitializerTest.java?rev=1869208&r1=1869207&r2=1869208&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/ExternalIdentityRepositoryInitializerTest.java (original)
+++ jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/ExternalIdentityRepositoryInitializerTest.java Thu Oct 31 10:00:56 2019
@@ -18,16 +18,26 @@ package org.apache.jackrabbit.oak.spi.se
 
 import com.google.common.collect.Iterables;
 import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.commons.PathUtils;
 import org.apache.jackrabbit.oak.plugins.index.IndexConstants;
 import org.apache.jackrabbit.oak.spi.security.authentication.external.AbstractExternalAuthTest;
 import org.apache.jackrabbit.oak.spi.security.authentication.external.impl.ExternalIdentityConstants;
 import org.apache.jackrabbit.oak.plugins.tree.TreeUtil;
+import org.apache.jackrabbit.oak.spi.state.NodeBuilder;
 import org.junit.Test;
 
+import static org.apache.jackrabbit.oak.plugins.index.IndexConstants.INDEX_DEFINITIONS_NAME;
 import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNull;
 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.never;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
 
 public class ExternalIdentityRepositoryInitializerTest extends AbstractExternalAuthTest {
 
@@ -58,4 +68,30 @@ public class ExternalIdentityRepositoryI
         Iterable<String> declaringNtNames = TreeUtil.getStrings(tree, IndexConstants.DECLARING_NODE_TYPES);
         assertNull(declaringNtNames);
     }
+
+    @Test
+    public void testInitializeNotEnforced() {
+        NodeBuilder builder = mock(NodeBuilder.class);
+        when(builder.hasChildNode(INDEX_DEFINITIONS_NAME)).thenReturn(true);
+        when(builder.child(INDEX_DEFINITIONS_NAME)).thenReturn(builder);
+        when(builder.hasChildNode("externalPrincipalNames")).thenReturn(true);
+
+        ExternalIdentityRepositoryInitializer initializer = new ExternalIdentityRepositoryInitializer(false);
+        initializer.initialize(builder);
+
+        verify(builder, never()).child("externalId");
+        verify(builder, never()).setProperty(anyString(), any());
+    }
+
+    @Test
+    public void testInitializeIndicesExist() {
+        NodeBuilder builder = spy(getTreeProvider().asNodeState(root.getTree(PathUtils.ROOT_PATH)).builder());
+
+        ExternalIdentityRepositoryInitializer initializer = new ExternalIdentityRepositoryInitializer(true);
+        initializer.initialize(builder);
+
+        verify(builder, never()).child("externalId");
+        verify(builder, never()).child("externalPrincipalNames");
+        verify(builder, never()).setProperty(anyString(), any());
+    }
 }
\ No newline at end of file

Modified: jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/ExternalIdentityValidatorTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/ExternalIdentityValidatorTest.java?rev=1869208&r1=1869207&r2=1869208&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/ExternalIdentityValidatorTest.java (original)
+++ jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/ExternalIdentityValidatorTest.java Thu Oct 31 10:00:56 2019
@@ -18,26 +18,36 @@ package org.apache.jackrabbit.oak.spi.se
 
 import javax.jcr.SimpleCredentials;
 
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Lists;
 import org.apache.jackrabbit.api.security.user.Authorizable;
 import org.apache.jackrabbit.api.security.user.User;
 import org.apache.jackrabbit.oak.api.CommitFailedException;
+import org.apache.jackrabbit.oak.api.PropertyState;
 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.spi.commit.Validator;
 import org.apache.jackrabbit.oak.spi.security.authentication.external.ExternalLoginModuleTestBase;
 import org.apache.jackrabbit.oak.spi.security.authentication.external.TestIdentityProvider;
 import org.apache.jackrabbit.oak.spi.security.authentication.external.basic.DefaultSyncConfig;
 import org.apache.jackrabbit.oak.spi.security.authentication.external.impl.ExternalIdentityConstants;
-import org.apache.jackrabbit.oak.util.NodeUtil;
+import org.apache.jackrabbit.oak.spi.security.principal.SystemPrincipal;
+import org.apache.jackrabbit.oak.spi.state.NodeState;
+import org.jetbrains.annotations.NotNull;
 import org.junit.Test;
 
+import static org.apache.jackrabbit.oak.api.CommitFailedException.CONSTRAINT;
+import static org.apache.jackrabbit.oak.spi.security.authentication.external.impl.ExternalIdentityConstants.REP_EXTERNAL_ID;
+import static org.apache.jackrabbit.oak.spi.security.authentication.external.impl.ExternalIdentityConstants.REP_EXTERNAL_PRINCIPAL_NAMES;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
 
 public class ExternalIdentityValidatorTest extends ExternalLoginModuleTestBase {
 
@@ -61,6 +71,7 @@ public class ExternalIdentityValidatorTe
     }
 
     @Override
+    @NotNull
     protected DefaultSyncConfig createSyncConfig() {
         DefaultSyncConfig config = super.createSyncConfig();
         config.user().setDynamicMembership(isDynamic());
@@ -71,33 +82,38 @@ public class ExternalIdentityValidatorTe
         return true;
     }
 
-    @Test
-    public void testAddExternalPrincipalNames() {
+    static void assertException(@NotNull CommitFailedException e, @NotNull String expectedType, int expectedCode) throws CommitFailedException {
+        assertEquals(expectedType, e.getType());
+        assertEquals(expectedCode, e.getCode());
+        throw e;
+    }
+
+    @Test(expected = CommitFailedException.class)
+    public void testAddExternalPrincipalNames() throws Exception {
         Tree userTree = root.getTree(testUserPath);
-        NodeUtil userNode = new NodeUtil(userTree);
         try {
-            userNode.setStrings(ExternalIdentityConstants.REP_EXTERNAL_PRINCIPAL_NAMES, "principalName");
+            userTree.setProperty(ExternalIdentityConstants.REP_EXTERNAL_PRINCIPAL_NAMES, ImmutableList.of("principalName"), Type.STRINGS);
             root.commit();
             fail("Creating rep:externalPrincipalNames must be detected.");
         } catch (CommitFailedException e) {
             // success
-            assertEquals(70, e.getCode());
+            assertException(e, CONSTRAINT, 70);
         } finally {
             root.refresh();
         }
     }
 
-    @Test
+    @Test(expected = CommitFailedException.class)
     public void testAddExternalPrincipalNamesAsSystemMissingExternalId() throws Exception {
         Root systemRoot = getSystemRoot();
         try {
-            NodeUtil n = new NodeUtil(systemRoot.getTree(testUserPath));
-            n.setStrings(ExternalIdentityConstants.REP_EXTERNAL_PRINCIPAL_NAMES, "principalName");
+            Tree userTree = systemRoot.getTree(testUserPath);
+            userTree.setProperty(ExternalIdentityConstants.REP_EXTERNAL_PRINCIPAL_NAMES, ImmutableList.of("principalName"), Type.STRINGS);
             systemRoot.commit();
             fail("Creating rep:externalPrincipalNames without rep:externalId must be detected.");
         } catch (CommitFailedException e) {
             // success
-            assertEquals(72, e.getCode());
+            assertException(e, CONSTRAINT, 72);
         } finally {
             systemRoot.refresh();
         }
@@ -106,13 +122,13 @@ public class ExternalIdentityValidatorTe
     @Test
     public void testAddExternalPrincipalNamesAsSystem() throws Exception {
         Root systemRoot = getSystemRoot();
-        NodeUtil n = new NodeUtil(systemRoot.getTree(testUserPath));
-        n.setString(ExternalIdentityConstants.REP_EXTERNAL_ID, "externalId");
-        n.setStrings(ExternalIdentityConstants.REP_EXTERNAL_PRINCIPAL_NAMES, "principalName");
+        Tree userTree = systemRoot.getTree(testUserPath);
+        userTree.setProperty(REP_EXTERNAL_ID, "externalId");
+        userTree.setProperty(ExternalIdentityConstants.REP_EXTERNAL_PRINCIPAL_NAMES, ImmutableList.of("principalName"), Type.STRINGS);
         systemRoot.commit();
     }
 
-    @Test
+    @Test(expected = CommitFailedException.class)
     public void testRemoveExternalPrincipalNames() throws Exception {
         Tree userTree = root.getTree(externalUserPath);
         try {
@@ -121,7 +137,7 @@ public class ExternalIdentityValidatorTe
             fail("Removing rep:externalPrincipalNames must be detected.");
         } catch (CommitFailedException e) {
             // success
-            assertEquals(70, e.getCode());
+            assertException(e, CONSTRAINT, 70);
         } finally {
             root.refresh();
         }
@@ -130,14 +146,14 @@ public class ExternalIdentityValidatorTe
     @Test
     public void testRemoveExternalPrincipalNamesAsSystem() throws Exception {
         Root systemRoot = getSystemRoot();
-        NodeUtil n = new NodeUtil(systemRoot.getTree(externalUserPath));
+        Tree userTree = systemRoot.getTree(externalUserPath);
 
         // removal with system root must succeed
-        n.removeProperty(ExternalIdentityConstants.REP_EXTERNAL_PRINCIPAL_NAMES);
+        userTree.removeProperty(ExternalIdentityConstants.REP_EXTERNAL_PRINCIPAL_NAMES);
         systemRoot.commit();
     }
 
-    @Test
+    @Test(expected = CommitFailedException.class)
     public void testModifyExternalPrincipalNames() throws Exception {
         Tree userTree = root.getTree(externalUserPath);
         try {
@@ -146,7 +162,7 @@ public class ExternalIdentityValidatorTe
             fail("Changing rep:externalPrincipalNames must be detected.");
         } catch (CommitFailedException e) {
             // success
-            assertEquals(70, e.getCode());
+            assertException(e, CONSTRAINT, 70);
         } finally {
             root.refresh();
         }
@@ -155,14 +171,14 @@ public class ExternalIdentityValidatorTe
     @Test
     public void testModifyExternalPrincipalNamesAsSystem() throws Exception {
         Root systemRoot = getSystemRoot();
-        NodeUtil n = new NodeUtil(systemRoot.getTree(externalUserPath));
+        Tree userTree = systemRoot.getTree(externalUserPath);
 
         // changing with system root must succeed
-        n.setStrings(ExternalIdentityConstants.REP_EXTERNAL_PRINCIPAL_NAMES, "principalNames");
+        userTree.setProperty(ExternalIdentityConstants.REP_EXTERNAL_PRINCIPAL_NAMES, ImmutableList.of("principalNames"), Type.STRINGS);
         systemRoot.commit();
     }
 
-    @Test
+    @Test(expected = CommitFailedException.class)
     public void testExternalPrincipalNamesType() throws Exception {
         Root systemRoot = getSystemRoot();
         Tree userTree = systemRoot.getTree(testUserPath);
@@ -180,46 +196,62 @@ public class ExternalIdentityValidatorTe
                 fail("Creating rep:externalPrincipalNames with type "+t+" must be detected.");
             } catch (CommitFailedException e) {
                 // success
-                assertEquals(71, e.getCode());
+                assertException(e, CONSTRAINT, 71);
             } finally {
                 systemRoot.refresh();
             }
         }
     }
 
-    @Test
+    @Test(expected = CommitFailedException.class)
     public void testExternalPrincipalNamesSingle() throws Exception {
         Root systemRoot = getSystemRoot();
         try {
-            NodeUtil n = new NodeUtil(systemRoot.getTree(testUserPath));
-            n.setString(ExternalIdentityConstants.REP_EXTERNAL_PRINCIPAL_NAMES, "id");
+            Tree userTree = systemRoot.getTree(testUserPath);
+            userTree.setProperty(ExternalIdentityConstants.REP_EXTERNAL_PRINCIPAL_NAMES, "id");
             systemRoot.commit();
             fail("Creating rep:externalPrincipalNames as single STRING property must be detected.");
         } catch (CommitFailedException e) {
             // success
-            assertEquals(71, e.getCode());
+            assertException(e, CONSTRAINT, 71);
         } finally {
             systemRoot.refresh();
         }
     }
 
-    @Test
+    @Test(expected = CommitFailedException.class)
+    public void testExternalPrincipalNamesArrayMismatch() throws Exception {
+        NodeState ns = mock(NodeState.class);
+        PropertyState ps = when(mock(PropertyState.class).getName()).thenReturn(REP_EXTERNAL_PRINCIPAL_NAMES).getMock();
+        Type type = Type.STRINGS;
+        when(ps.getType()).thenReturn(type);
+        when(ps.isArray()).thenReturn(false);
+
+        try {
+            Validator v = new ExternalIdentityValidatorProvider(ImmutableSet.of(SystemPrincipal.INSTANCE), true).getRootValidator(ns, ns, null);
+            v.propertyAdded(ps);
+        } catch (CommitFailedException e) {
+            assertException(e, CONSTRAINT, 71);
+        }
+    }
+
+    @Test(expected = CommitFailedException.class)
     public void testRepExternalIdMultiple() throws Exception {
         Root systemRoot = getSystemRoot();
         try {
-            NodeUtil n = new NodeUtil(systemRoot.getTree(testUserPath));
-            n.setStrings(ExternalIdentityConstants.REP_EXTERNAL_ID, "id", "id2");
+            Tree userTree = systemRoot.getTree(testUserPath);
+            userTree.setProperty(REP_EXTERNAL_ID, ImmutableList.of("id", "id2"), Type.STRINGS);
             systemRoot.commit();
             fail("Creating rep:externalId as multiple STRING property must be detected.");
         } catch (CommitFailedException e) {
             // success
-            assertEquals(75, e.getCode());
+            assertException(e, CONSTRAINT, 75);
         } finally {
             systemRoot.refresh();
         }
     }
 
-    @Test
+    @Test(expected = CommitFailedException.class)
     public void testRepExternalIdType() throws Exception {
         Root systemRoot = getSystemRoot();
         Tree userTree = systemRoot.getTree(testUserPath);
@@ -232,22 +264,38 @@ public class ExternalIdentityValidatorTe
         for (Type t : valMap.keySet()) {
             Object val = valMap.get(t);
             try {
-                userTree.setProperty(ExternalIdentityConstants.REP_EXTERNAL_ID, val, t);
+                userTree.setProperty(REP_EXTERNAL_ID, val, t);
                 systemRoot.commit();
                 fail("Creating rep:externalId with type "+t+" must be detected.");
             } catch (CommitFailedException e) {
                 // success
-                assertEquals(75, e.getCode());
+                assertException(e, CONSTRAINT, 75);
             } finally {
                 systemRoot.refresh();
             }
         }
     }
 
+    @Test(expected = CommitFailedException.class)
+    public void testRepExternalIdTypeArrayMismatch() throws Exception {
+        NodeState ns = mock(NodeState.class);
+        PropertyState ps = when(mock(PropertyState.class).getName()).thenReturn(REP_EXTERNAL_ID).getMock();
+        Type type = Type.STRING;
+        when(ps.getType()).thenReturn(type);
+        when(ps.isArray()).thenReturn(true);
+
+        try {
+            Validator v = new ExternalIdentityValidatorProvider(ImmutableSet.of(SystemPrincipal.INSTANCE), true).getRootValidator(ns, ns, null);
+            v.propertyAdded(ps);
+        } catch (CommitFailedException e) {
+            assertException(e, CONSTRAINT, 75);
+        }
+    }
+
     @Test
     public void testCreateUserWithRepExternalId() throws Exception {
         User u = getUserManager(root).createUser(TestIdentityProvider.ID_SECOND_USER, null);
-        root.getTree(u.getPath()).setProperty(ExternalIdentityConstants.REP_EXTERNAL_ID, TestIdentityProvider.ID_SECOND_USER);
+        root.getTree(u.getPath()).setProperty(REP_EXTERNAL_ID, TestIdentityProvider.ID_SECOND_USER);
         root.commit();
     }
 
@@ -255,20 +303,19 @@ public class ExternalIdentityValidatorTe
     public void testCreateUserWithRepExternalIdAsSystem() throws Exception {
         Root systemRoot = getSystemRoot();
         User u = getUserManager(systemRoot).createUser(TestIdentityProvider.ID_SECOND_USER, null);
-        systemRoot.getTree(u.getPath()).setProperty(ExternalIdentityConstants.REP_EXTERNAL_ID, TestIdentityProvider.ID_SECOND_USER);
+        systemRoot.getTree(u.getPath()).setProperty(REP_EXTERNAL_ID, TestIdentityProvider.ID_SECOND_USER);
         systemRoot.commit();
     }
 
-    @Test
+    @Test(expected = CommitFailedException.class)
     public void testAddRepExternalId() throws Exception {
         try {
-            root.getTree(testUserPath).setProperty(ExternalIdentityConstants.REP_EXTERNAL_ID, "id");
+            root.getTree(testUserPath).setProperty(REP_EXTERNAL_ID, "id");
             root.commit();
             fail("Adding rep:externalId must be detected in the default setup.");
         } catch (CommitFailedException e) {
             // success: verify nature of the exception
-            assertTrue(e.isConstraintViolation());
-            assertEquals(74, e.getCode());
+            assertException(e, CONSTRAINT, 74);
         }
 
     }
@@ -276,88 +323,85 @@ public class ExternalIdentityValidatorTe
     @Test
     public void testAddRepExternalIdAsSystem() throws Exception {
         Root systemRoot = getSystemRoot();
-        systemRoot.getTree(testUserPath).setProperty(ExternalIdentityConstants.REP_EXTERNAL_ID, "id");
+        systemRoot.getTree(testUserPath).setProperty(REP_EXTERNAL_ID, "id");
         systemRoot.commit();
     }
 
-    @Test
+    @Test(expected = CommitFailedException.class)
     public void testModifyRepExternalId() throws Exception {
         try {
-            root.getTree(externalUserPath).setProperty(ExternalIdentityConstants.REP_EXTERNAL_ID, "anotherValue");
+            root.getTree(externalUserPath).setProperty(REP_EXTERNAL_ID, "anotherValue");
             root.commit();
 
             fail("Modification of rep:externalId must be detected in the default setup.");
         } catch (CommitFailedException e) {
             // success: verify nature of the exception
-            assertTrue(e.isConstraintViolation());
-            assertEquals(74, e.getCode());
+            assertException(e, CONSTRAINT, 74);
         }
     }
 
     @Test
     public void testModifyRepExternalIdAsSystem() throws Exception {
         Root systemRoot = getSystemRoot();
-        systemRoot.getTree(externalUserPath).setProperty(ExternalIdentityConstants.REP_EXTERNAL_ID, "anotherValue");
+        systemRoot.getTree(externalUserPath).setProperty(REP_EXTERNAL_ID, "anotherValue");
         systemRoot.commit();
     }
 
-    @Test
-    public void testRemoveRepExternalId() {
+    @Test(expected = CommitFailedException.class)
+    public void testRemoveRepExternalId() throws Exception {
         try {
-            root.getTree(externalUserPath).removeProperty(ExternalIdentityConstants.REP_EXTERNAL_ID);
+            root.getTree(externalUserPath).removeProperty(REP_EXTERNAL_ID);
             root.commit();
 
             fail("Removal of rep:externalId must be detected in the default setup.");
         } catch (CommitFailedException e) {
             // success: verify nature of the exception
-            assertTrue(e.isConstraintViolation());
-            assertEquals(73, e.getCode());
+            assertException(e, CONSTRAINT, 73);
+            throw e;
         }
     }
 
-    @Test
+    @Test(expected = CommitFailedException.class)
     public void testRemoveRepExternalIdAsSystem() throws Exception {
         Root systemRoot = getSystemRoot();
         try {
-            NodeUtil n = new NodeUtil(systemRoot.getTree(externalUserPath));
-
-            n.removeProperty(ExternalIdentityConstants.REP_EXTERNAL_ID);
+            Tree userTree = systemRoot.getTree(externalUserPath);
+            userTree.removeProperty(REP_EXTERNAL_ID);
             systemRoot.commit();
             fail("Removing rep:externalId is not allowed if rep:externalPrincipalNames is present.");
         } catch (CommitFailedException e) {
             // success
-            assertEquals(73, e.getCode());
+            assertException(e, CONSTRAINT, 73);
         } finally {
             systemRoot.refresh();
         }
     }
 
-    @Test
+    @Test(expected = CommitFailedException.class)
     public void testRemoveRepExternalIdWithoutPrincipalNames() throws Exception {
         Root systemRoot = getSystemRoot();
-        systemRoot.getTree(testUserPath).setProperty(ExternalIdentityConstants.REP_EXTERNAL_ID, "id");
+        systemRoot.getTree(testUserPath).setProperty(REP_EXTERNAL_ID, "id");
         systemRoot.commit();
         root.refresh();
 
         try {
-            root.getTree(testUserPath).removeProperty(ExternalIdentityConstants.REP_EXTERNAL_ID);
+            root.getTree(testUserPath).removeProperty(REP_EXTERNAL_ID);
             root.commit();
 
             fail("Removal of rep:externalId must be detected in the default setup.");
         } catch (CommitFailedException e) {
             // success: verify nature of the exception
-            assertTrue(e.isConstraintViolation());
-            assertEquals(74, e.getCode());
+            assertException(e, CONSTRAINT, 74);
         }
     }
 
     @Test
     public void testRemoveRepExternalIdWithoutPrincipalNamesAsSystem() throws Exception {
         Root systemRoot = getSystemRoot();
-        systemRoot.getTree(testUserPath).setProperty(ExternalIdentityConstants.REP_EXTERNAL_ID, "id");
+        systemRoot.getTree(testUserPath).setProperty(REP_EXTERNAL_ID, "id");
         systemRoot.commit();
 
-        systemRoot.getTree(testUserPath).removeProperty(ExternalIdentityConstants.REP_EXTERNAL_ID);
+        systemRoot.getTree(testUserPath).removeProperty(REP_EXTERNAL_ID);
         systemRoot.commit();
     }
 

Modified: jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/ExternalPrincipalConfigurationTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/ExternalPrincipalConfigurationTest.java?rev=1869208&r1=1869207&r2=1869208&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/ExternalPrincipalConfigurationTest.java (original)
+++ jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/ExternalPrincipalConfigurationTest.java Thu Oct 31 10:00:56 2019
@@ -43,9 +43,12 @@ import org.apache.jackrabbit.oak.spi.sec
 import org.apache.jackrabbit.oak.spi.security.authentication.external.impl.DefaultSyncConfigImpl;
 import org.apache.jackrabbit.oak.spi.security.authentication.external.impl.DefaultSyncHandler;
 import org.apache.jackrabbit.oak.spi.security.authentication.external.impl.ExternalIdentityConstants;
+import org.apache.jackrabbit.oak.spi.security.authentication.external.impl.ExternalLoginModuleFactory;
+import org.apache.jackrabbit.oak.spi.security.authentication.external.impl.SyncHandlerMapping;
 import org.apache.jackrabbit.oak.spi.security.principal.PrincipalConfiguration;
 import org.apache.jackrabbit.oak.spi.security.principal.PrincipalProvider;
 import org.apache.jackrabbit.oak.spi.xml.ProtectedItemImporter;
+import org.apache.sling.testing.mock.osgi.MockOsgi;
 import org.jetbrains.annotations.NotNull;
 import org.junit.Ignore;
 import org.junit.Test;
@@ -57,6 +60,7 @@ import static org.junit.Assert.assertFal
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.mock;
 
 public class ExternalPrincipalConfigurationTest extends AbstractExternalAuthTest {
 
@@ -185,6 +189,12 @@ public class ExternalPrincipalConfigurat
     }
 
     @Test
+    public void testDeactivateWithNullTrackers() {
+        ExternalPrincipalConfiguration epc = new ExternalPrincipalConfiguration(getSecurityProvider());
+        MockOsgi.deactivate(epc, context.bundleContext(), Collections.emptyMap());
+    }
+
+    @Test
     public void testAddingSyncHandler() {
         Map<String, Object> enableProps =  ImmutableMap.<String, Object>of(DefaultSyncConfigImpl.PARAM_USER_DYNAMIC_MEMBERSHIP, true);
         Map<String, Object> disableProps =  ImmutableMap.<String, Object>of(DefaultSyncConfigImpl.PARAM_USER_DYNAMIC_MEMBERSHIP, false);
@@ -227,10 +237,10 @@ public class ExternalPrincipalConfigurat
         ServiceRegistration registration = bundleContext.registerService(DefaultSyncHandler.class.getName(), sh, disableProps);
         assertIsEnabled(externalPrincipalConfiguration, false);
 
-        registration.setProperties(enableProps);
+        MockOsgi.modified(sh, bundleContext, enableProps);
         assertIsEnabled(externalPrincipalConfiguration, true);
 
-        registration.setProperties(disableProps);
+        MockOsgi.modified(sh, bundleContext, disableProps);
         assertIsEnabled(externalPrincipalConfiguration, false);
     }
 
@@ -258,6 +268,18 @@ public class ExternalPrincipalConfigurat
         assertIsEnabled(externalPrincipalConfiguration, false);
     }
 
+    @Test
+    public void testAddingIncompleteSyncHandlerMapping() {
+        SyncHandlerMapping mapping = mock(SyncHandlerMapping.class);
+
+        context.registerService(SyncHandlerMapping.class, mapping);
+
+        context.registerService(SyncHandlerMapping.class, mapping, ImmutableMap.of(ExternalLoginModuleFactory.PARAM_IDP_NAME, "idpName"));
+
+        context.registerService(SyncHandlerMapping.class, mapping, ImmutableMap.of(ExternalLoginModuleFactory.PARAM_SYNC_HANDLER_NAME, "syncHandlerName"));
+
+    }
+
     private static final class TestSyncHandler implements SyncHandler {
 
         @NotNull

Modified: jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/PrincipalProviderAutoMembershipTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/PrincipalProviderAutoMembershipTest.java?rev=1869208&r1=1869207&r2=1869208&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/PrincipalProviderAutoMembershipTest.java (original)
+++ jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/PrincipalProviderAutoMembershipTest.java Thu Oct 31 10:00:56 2019
@@ -16,26 +16,35 @@
  */
 package org.apache.jackrabbit.oak.spi.security.authentication.external.impl.principal;
 
-import java.security.Principal;
-import java.util.Iterator;
-import java.util.List;
-import java.util.Set;
-import java.util.UUID;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
 import com.google.common.collect.Iterators;
 import org.apache.jackrabbit.api.security.principal.PrincipalManager;
 import org.apache.jackrabbit.api.security.user.Authorizable;
 import org.apache.jackrabbit.api.security.user.Group;
+import org.apache.jackrabbit.api.security.user.UserManager;
+import org.apache.jackrabbit.oak.namepath.NamePathMapper;
 import org.apache.jackrabbit.oak.spi.security.authentication.external.basic.DefaultSyncConfig;
 import org.apache.jackrabbit.oak.spi.security.principal.PrincipalImpl;
+import org.apache.jackrabbit.oak.spi.security.user.UserConfiguration;
 import org.jetbrains.annotations.NotNull;
 import org.junit.Test;
 
+import java.security.Principal;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Set;
+import java.util.UUID;
+
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.when;
 
 /**
  * Extension of the {@link ExternalGroupPrincipalProviderTest} with 'automembership'
@@ -65,15 +74,17 @@ public class PrincipalProviderAutoMember
     }
 
     @Override
+    @NotNull
     protected DefaultSyncConfig createSyncConfig() {
         DefaultSyncConfig syncConfig = super.createSyncConfig();
-        syncConfig.user().setAutoMembership(USER_AUTO_MEMBERSHIP_GROUP_ID, NON_EXISTING_GROUP_ID);
+        syncConfig.user().setAutoMembership(USER_AUTO_MEMBERSHIP_GROUP_ID, NON_EXISTING_GROUP_ID, USER_ID);
         syncConfig.group().setAutoMembership(GROUP_AUTO_MEMBERSHIP_GROUP_ID, NON_EXISTING_GROUP_ID2);
 
         return syncConfig;
     }
 
     @Override
+    @NotNull
     Set<Principal> getExpectedGroupPrincipals(@NotNull String userId) throws Exception {
         return ImmutableSet.<Principal>builder()
                 .addAll(super.getExpectedGroupPrincipals(userId))
@@ -82,11 +93,10 @@ public class PrincipalProviderAutoMember
     }
 
     @Override
-    @Test
-    public void testFindPrincipalsByTypeAll() throws Exception {
-        Set<? extends Principal> res = ImmutableSet.copyOf(principalProvider.findPrincipals(PrincipalManager.SEARCH_TYPE_ALL));
-        // not automembership principals expected here
-        assertEquals(super.getExpectedGroupPrincipals(USER_ID), res);
+    @NotNull
+    Set<Principal> getExpectedAllSearchResult(@NotNull String userId) throws Exception {
+        // not automembership principals expected when searching for principals => call super method
+        return super.getExpectedGroupPrincipals(userId);
     }
 
     @Test
@@ -109,16 +119,41 @@ public class PrincipalProviderAutoMember
         Set<Principal> result = principalProvider.getMembershipPrincipals(user.getPrincipal());
         assertTrue(result.contains(userAutoMembershipGroup.getPrincipal()));
         assertTrue(result.contains(groupAutoMembershipGroup.getPrincipal()));
+        assertFalse(result.contains(user.getPrincipal()));
         assertEquals(expected, result);
     }
 
     @Test
+    public void testGetGroupPrincipalsTwice() throws Exception {
+        Authorizable user = getUserManager(root).getAuthorizable(USER_ID);
+        Set<Principal> result = principalProvider.getMembershipPrincipals(user.getPrincipal());
+        assertEquals(result, principalProvider.getMembershipPrincipals(user.getPrincipal()));
+    }
+
+    @Test
+    public void testGetGroupPrincipalsIgnoresNonGroupPrincipals() throws Exception {
+        UserConfiguration uc = spy(getUserConfiguration());
+        UserManager um = spy(getUserManager(root));
+
+        Principal p = new PrincipalImpl(USER_AUTO_MEMBERSHIP_GROUP_PRINCIPAL_NAME);
+        Group gr = when(mock(Group.class).getPrincipal()).thenReturn(p).getMock();
+        when(gr.isGroup()).thenReturn(true);
+        when(um.getAuthorizable(USER_AUTO_MEMBERSHIP_GROUP_ID)).thenReturn(gr);
+        when(uc.getUserManager(root, NamePathMapper.DEFAULT)).thenReturn(um);
+
+        ExternalGroupPrincipalProvider pp = new ExternalGroupPrincipalProvider(root, uc, NamePathMapper.DEFAULT, ImmutableMap.of(idp.getName(), getAutoMembership()));
+        Set<Principal> result = pp.getMembershipPrincipals(um.getAuthorizable(USER_ID).getPrincipal());
+        assertFalse(Iterables.contains(Iterables.transform(result, principal -> principal.getName()), USER_AUTO_MEMBERSHIP_GROUP_PRINCIPAL_NAME));
+    }
+
+    @Test
     public void testGetPrincipals() throws Exception {
         Set<Principal> expected = getExpectedGroupPrincipals(USER_ID);
 
         Set<? extends Principal> result = principalProvider.getPrincipals(USER_ID);
         assertTrue(result.contains(userAutoMembershipGroup.getPrincipal()));
         assertTrue(result.contains(groupAutoMembershipGroup.getPrincipal()));
+        assertFalse(result.contains(getUserManager(root).getAuthorizable(USER_ID).getPrincipal()));
         assertEquals(expected, result);
     }
 
@@ -140,13 +175,4 @@ public class PrincipalProviderAutoMember
             assertFalse(Iterators.contains(res, new PrincipalImpl(NON_EXISTING_GROUP_ID)));
         }
     }
-
-    @Test
-    public void testFindPrincipalsByTypeGroup() throws Exception {
-        Iterator<? extends Principal> res = principalProvider.findPrincipals(PrincipalManager.SEARCH_TYPE_GROUP);
-
-        assertFalse(Iterators.contains(res, userAutoMembershipGroup.getPrincipal()));
-        assertFalse(Iterators.contains(res, groupAutoMembershipGroup.getPrincipal()));
-        assertFalse(Iterators.contains(res, new PrincipalImpl(NON_EXISTING_GROUP_ID)));
-    }
 }

Modified: jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/PrincipalProviderDeepNestingTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/PrincipalProviderDeepNestingTest.java?rev=1869208&r1=1869207&r2=1869208&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/PrincipalProviderDeepNestingTest.java (original)
+++ jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/PrincipalProviderDeepNestingTest.java Thu Oct 31 10:00:56 2019
@@ -28,6 +28,7 @@ import org.apache.jackrabbit.oak.spi.sec
 import org.apache.jackrabbit.oak.spi.security.authentication.external.ExternalUser;
 import org.apache.jackrabbit.oak.spi.security.authentication.external.basic.DefaultSyncConfig;
 import org.apache.jackrabbit.oak.spi.security.principal.PrincipalImpl;
+import org.jetbrains.annotations.NotNull;
 import org.junit.Test;
 
 import static org.junit.Assert.assertEquals;
@@ -38,6 +39,7 @@ import static org.junit.Assert.assertTru
 public class PrincipalProviderDeepNestingTest extends ExternalGroupPrincipalProviderTest {
 
     @Override
+    @NotNull
     protected DefaultSyncConfig createSyncConfig() {
         DefaultSyncConfig sc = super.createSyncConfig();
         sc.user().setMembershipNestingDepth(Long.MAX_VALUE);

Modified: jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/ValidatorNoProtectionTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/ValidatorNoProtectionTest.java?rev=1869208&r1=1869207&r2=1869208&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/ValidatorNoProtectionTest.java (original)
+++ jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/ValidatorNoProtectionTest.java Thu Oct 31 10:00:56 2019
@@ -18,13 +18,13 @@ package org.apache.jackrabbit.oak.spi.se
 
 import java.util.Map;
 
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 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.spi.security.ConfigurationParameters;
 import org.apache.jackrabbit.oak.spi.security.authentication.external.impl.ExternalIdentityConstants;
-import org.apache.jackrabbit.oak.util.NodeUtil;
 import org.junit.Test;
 
 public class ValidatorNoProtectionTest extends ExternalIdentityValidatorTest {
@@ -41,8 +41,8 @@ public class ValidatorNoProtectionTest e
     @Test
     public void testRepExternalIdMultiple() throws Exception {
         Root systemRoot = getSystemRoot();
-        NodeUtil n = new NodeUtil(systemRoot.getTree(testUserPath));
-        n.setStrings(ExternalIdentityConstants.REP_EXTERNAL_ID, "id", "id2");
+        Tree userTree = systemRoot.getTree(testUserPath);
+        userTree.setProperty(ExternalIdentityConstants.REP_EXTERNAL_ID, ImmutableList.of("id", "id2"), Type.STRINGS);
         systemRoot.commit();
     }
 
@@ -69,6 +69,7 @@ public class ValidatorNoProtectionTest e
     }
 
     @Override
+    @Test
     public void testAddRepExternalId() throws Exception {
         root.getTree(testUserPath).setProperty(ExternalIdentityConstants.REP_EXTERNAL_ID, "id");
         root.commit();
@@ -82,6 +83,7 @@ public class ValidatorNoProtectionTest e
     }
 
     @Override
+    @Test
     public void testRemoveRepExternalIdWithoutPrincipalNames() throws Exception {
         root.getTree(testUserPath).setProperty(ExternalIdentityConstants.REP_EXTERNAL_ID, "id");
         root.commit();

Modified: jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/ValidatorNotDynamicTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/ValidatorNotDynamicTest.java?rev=1869208&r1=1869207&r2=1869208&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/ValidatorNotDynamicTest.java (original)
+++ jackrabbit/oak/trunk/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/ValidatorNotDynamicTest.java Thu Oct 31 10:00:56 2019
@@ -19,13 +19,12 @@ package org.apache.jackrabbit.oak.spi.se
 import com.google.common.collect.ImmutableList;
 import org.apache.jackrabbit.oak.api.CommitFailedException;
 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.spi.security.authentication.external.impl.ExternalIdentityConstants;
-import org.apache.jackrabbit.oak.util.NodeUtil;
 import org.junit.Test;
 
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
+import static org.apache.jackrabbit.oak.api.CommitFailedException.CONSTRAINT;
 import static org.junit.Assert.fail;
 
 public class ValidatorNotDynamicTest extends ExternalIdentityValidatorTest {
@@ -49,7 +48,7 @@ public class ValidatorNotDynamicTest ext
     }
 
     @Override
-    @Test
+    @Test(expected = CommitFailedException.class)
     public void testRemoveExternalPrincipalNames() throws Exception {
         setExternalPrincipalNames();
 
@@ -65,7 +64,7 @@ public class ValidatorNotDynamicTest ext
     }
 
     @Override
-    @Test
+    @Test(expected = CommitFailedException.class)
     public void testModifyExternalPrincipalNames() throws Exception {
         setExternalPrincipalNames();
 
@@ -82,8 +81,8 @@ public class ValidatorNotDynamicTest ext
 
 
     @Override
-    @Test
-    public void testRemoveRepExternalId() {
+    @Test(expected = CommitFailedException.class)
+    public void testRemoveRepExternalId() throws Exception {
         try {
             root.getTree(externalUserPath).removeProperty(ExternalIdentityConstants.REP_EXTERNAL_ID);
             root.commit();
@@ -91,8 +90,7 @@ public class ValidatorNotDynamicTest ext
             fail("Removal of rep:externalId must be detected in the default setup.");
         } catch (CommitFailedException e) {
             // success: verify nature of the exception
-            assertTrue(e.isConstraintViolation());
-            assertEquals(74, e.getCode());
+            assertException(e, CONSTRAINT, 74);
         }
     }
 
@@ -100,9 +98,9 @@ public class ValidatorNotDynamicTest ext
     @Test
     public void testRemoveRepExternalIdAsSystem() throws Exception {
         Root systemRoot = getSystemRoot();
-        NodeUtil n = new NodeUtil(systemRoot.getTree(externalUserPath));
+        Tree userTree = systemRoot.getTree(externalUserPath);
 
-        n.removeProperty(ExternalIdentityConstants.REP_EXTERNAL_ID);
+        userTree.removeProperty(ExternalIdentityConstants.REP_EXTERNAL_ID);
         systemRoot.commit();
     }
 }
\ No newline at end of file