You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-commits@jackrabbit.apache.org by an...@apache.org on 2022/09/29 17:15:06 UTC

[jackrabbit-oak] branch trunk updated: OAK-9954 : Dynamic membership/group should spot conflicts with existing groups (#725)

This is an automated email from the ASF dual-hosted git repository.

angela pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/jackrabbit-oak.git


The following commit(s) were added to refs/heads/trunk by this push:
     new 4c454058b3 OAK-9954 : Dynamic membership/group should spot conflicts with existing groups (#725)
4c454058b3 is described below

commit 4c454058b3629961468c36d058eeffb69973f111
Author: anchela <an...@apache.org>
AuthorDate: Thu Sep 29 19:14:59 2022 +0200

    OAK-9954 : Dynamic membership/group should spot conflicts with existing groups (#725)
    
    * OAK-9954 : Dynamic membership/group should spot conflicts with existing groups (avoid shortcut if dynamic group option is enabled and check for conflicting principal names)
    
    * OAK-9954 : Dynamic membership/group should spot conflicts with existing groups (improve log output)
---
 .../external/impl/DynamicSyncContext.java          |  62 +++++++-----
 .../external/impl/DynamicGroupsTest.java           |   2 +-
 .../external/impl/DynamicSyncContextTest.java      | 108 ++++++++++++++++-----
 .../external/impl/PrincipalResolutionTest.java     |  88 ++++++++++++++++-
 4 files changed, 211 insertions(+), 49 deletions(-)

diff --git a/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/DynamicSyncContext.java b/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/DynamicSyncContext.java
index bd22589b3a..14614ef41e 100644
--- a/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/DynamicSyncContext.java
+++ b/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/DynamicSyncContext.java
@@ -51,6 +51,8 @@ import java.util.Map;
 import java.util.Set;
 import java.util.stream.Collectors;
 
+import static com.google.common.base.Preconditions.checkNotNull;
+
 /**
  * Extension of the {@code DefaultSyncContext} that doesn't synchronize group
  * membership of new external users into the user management of the repository.
@@ -162,7 +164,7 @@ public class DynamicSyncContext extends DefaultSyncContext {
                 // if dynamic-group option is enabled -> sync groups without member-information
                 // in case group-membership has been synched before -> clear it
                 if (hasDynamicGroups() && depth > 0) {
-                    createDynamicGroups(map);
+                    createDynamicGroups(map.values());
                 }
                 
                 // clean up any other membership
@@ -220,7 +222,7 @@ public class DynamicSyncContext extends DefaultSyncContext {
      * @throws ExternalIdentityException If an error occurs while resolving the the external group references.
      */
     private void collectSyncEntries(@NotNull Iterable<ExternalIdentityRef> declaredGroupRefs, long depth, @NotNull Map<ExternalIdentityRef, SyncEntry> map) throws ExternalIdentityException, RepositoryException {
-        boolean shortcut = (depth <= 1 && idp instanceof PrincipalNameResolver);
+        boolean shortcut = shortcut(depth);
         for (ExternalIdentityRef ref : Iterables.filter(declaredGroupRefs, this::isSameIDP)) {
             String principalName = null;
             Authorizable a = null;
@@ -232,8 +234,9 @@ public class DynamicSyncContext extends DefaultSyncContext {
                 // get group from the IDP
                 externalGroup = getExternalGroupFromRef(ref);
                 if (externalGroup != null) {
+                    // only set principal-name if the ref can be resolved to a valid external group
                     principalName = externalGroup.getPrincipalName();
-                    a = userManager.getAuthorizable(new PrincipalImpl(principalName));
+                    a = userManager.getAuthorizable(externalGroup.getId());
 
                     // recursively apply further membership until the configured depth is reached
                     if (depth > 1) {
@@ -247,6 +250,18 @@ public class DynamicSyncContext extends DefaultSyncContext {
             }
         }
     }
+
+    /**
+     * Evaluate if looking up the external group from the IDP can be omitted (i.e. no nesting and IDP implements PrincipalNameResolver.
+     * Finally, the shortcut does not make sense if 'dynamic group' option is enabled, as the external group is needed
+     * for the subsequent group sync.
+     * 
+     * @param depth The configured membership nesting depth.
+     * @return {@code true} if looking up the external group on IDP can be avoided; {@code false} otherwise.
+     */
+    private boolean shortcut(long depth) {
+        return depth <= 1 && idp instanceof PrincipalNameResolver && !hasDynamicGroups();
+    }
     
     /**
      * Tests if the given existing user/group collides with the external group having the same principall name.
@@ -265,38 +280,39 @@ public class DynamicSyncContext extends DefaultSyncContext {
         if (authorizable == null) {
             return false;
         } else if (!authorizable.isGroup()) {
-            log.warn("Existing user '{}' collides with external group.", authorizable.getID());
+            log.warn("Existing user '{}' collides with external group defined by IDP '{}'.", authorizable.getID(), idp.getName());
             return true;
         } else if (!isSameIDP(authorizable)) {
-            // there exists a user or group with that principal name but it doesn't belong to the same IDP
+            // there exists a group with the same id or principal name but it doesn't belong to the same IDP
             // in consistency with DefaultSyncContext don't sync this very membership into the repository
             // and log a warning about the collision instead.
-            log.warn("Existing authorizable with principal name '{}' is not a group from this IDP '{}'.", principalName, idp.getName());
+            log.warn("Existing group with id '{}' and principal name '{}' is not defined by IDP '{}'.", authorizable.getID(), authorizable.getPrincipal().getName(), idp.getName());
             return true;
+        } else if (!principalName.equals(authorizable.getPrincipal().getName())) {
+            // there exists a group with matching ID but principal-mismatch, don't sync this very membership into the 
+            // repository and log a warning about the collision instead.
+            log.warn("Existing group with id '{}' doesn't have matching principal name. found '{}', expected '{}', IDP '{}'.", authorizable.getID(), authorizable.getPrincipal().getName(), principalName, idp.getName());
+            return true;            
         } else {
             // group has been synced before (same IDP, same principal-name)
             return false;
         }
     }
-    
-    private void createDynamicGroups(@NotNull Map<ExternalIdentityRef, SyncEntry> map) throws RepositoryException {
-        for (Map.Entry<ExternalIdentityRef, SyncEntry> entry : map.entrySet()) {
-            ExternalIdentityRef groupRef = entry.getKey();
-            SyncEntry syncEntry = entry.getValue();
+
+    private void createDynamicGroups(@NotNull Iterable<SyncEntry> syncEntries) throws RepositoryException {
+        for (SyncEntry syncEntry : syncEntries) {
+            // since 'shortcut' is omitted if dynamic groups are enabled, there is no need to test if 'external-group' is 
+            // null, nor trying to retrieve external group again. if it could not be resolved during 'collectSyncEntries'
+            // before it didn't got added to the map
+            checkNotNull(syncEntry.externalGroup, "Cannot create dynamic group from null ExternalIdentity.");
             
-            // get external identity from IDP if it has not been resolved before (see 'shortcut' in 'collectSyncEntries').
-            ExternalGroup externalGroup = (syncEntry.externalGroup != null) ? syncEntry.externalGroup : getExternalGroupFromRef(groupRef);
-            if (externalGroup != null) {
-                // lookup of existing group by principal-name has been performed already 
-                // NOTE: if none exists no attempt is made to lookup again by ID as this may lead to inconsistencies 
-                // between rep:externalPrincipalNames and the dynamic group in case there existed a group with the same 
-                // ID but has a different principal name. in this case the sync will fail (conflict with ID).
-                Group gr = syncEntry.group;
-                if (gr == null) {
-                    gr = createGroup(externalGroup);
-                }
-                syncGroup(externalGroup, gr);
+            // lookup of existing group by ID has been performed already including check for conflicting authorizable 
+            // type or principal name
+            Group gr = syncEntry.group;
+            if (gr == null) {
+                gr = createGroup(syncEntry.externalGroup);
             }
+            syncGroup(syncEntry.externalGroup, gr);
         }
     }
     
diff --git a/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/DynamicGroupsTest.java b/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/DynamicGroupsTest.java
index b0e50dae2c..2e260bc2ca 100644
--- a/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/DynamicGroupsTest.java
+++ b/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/DynamicGroupsTest.java
@@ -62,6 +62,7 @@ public class DynamicGroupsTest extends DynamicSyncContextTest {
         return Lists.newArrayList(
                 new Object[] { DefaultSyncConfigImpl.PARAM_USER_MEMBERSHIP_NESTING_DEPTH_DEFAULT, false, "Membership-Nesting-Depth=0" },
                 new Object[] { DefaultSyncConfigImpl.PARAM_USER_MEMBERSHIP_NESTING_DEPTH_DEFAULT+1, false, "Membership-Nesting-Depth=1" },
+                // NOTE: shortcut for PrincipalNameResolver is ignored if dynamic-groups are enabled
                 new Object[] { DefaultSyncConfigImpl.PARAM_USER_MEMBERSHIP_NESTING_DEPTH_DEFAULT+1, true, "Membership-Nesting-Depth=1, IDP implements PrincipalNameResolver" },
                 new Object[] { DefaultSyncConfigImpl.PARAM_USER_MEMBERSHIP_NESTING_DEPTH_DEFAULT+2, false, "Membership-Nesting-Depth=2" });
     }
@@ -84,7 +85,6 @@ public class DynamicGroupsTest extends DynamicSyncContextTest {
         }
     }
 
-
     @Override
     protected @NotNull DefaultSyncConfig createSyncConfig() {
         DefaultSyncConfig config = super.createSyncConfig();
diff --git a/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/DynamicSyncContextTest.java b/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/DynamicSyncContextTest.java
index f39b1ae3b5..9eb2ce1562 100644
--- a/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/DynamicSyncContextTest.java
+++ b/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/DynamicSyncContextTest.java
@@ -46,6 +46,7 @@ import org.apache.jackrabbit.oak.spi.security.authentication.external.basic.Defa
 import org.apache.jackrabbit.oak.spi.security.authentication.external.basic.DefaultSyncContext;
 import org.apache.jackrabbit.oak.spi.security.principal.PrincipalImpl;
 import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
@@ -338,32 +339,71 @@ public class DynamicSyncContextTest extends AbstractExternalAuthTest {
         // create a local group that collides with the external group membership
         // i.e. doesn't have an rep:externalId set
         ExternalIdentity externalGroup = idp.getIdentity(externalUser.getDeclaredGroups().iterator().next());
-        userManager.createGroup(externalGroup.getId(), new PrincipalImpl(externalGroup.getPrincipalName()), null);
-        r.commit();
-        
-        // sync the user with dynamic membership enabled
-        sync(externalUser, SyncResult.Status.ADD);
-
-        // retrieve rep:externalPrincipalNames
-        Tree tree = r.getTree(userManager.getAuthorizable(USER_ID).getPath());
-        PropertyState extPrincipalNames = tree.getProperty(REP_EXTERNAL_PRINCIPAL_NAMES);
-        assertNotNull(extPrincipalNames);
+        assertNotNull(externalGroup);
 
-        // the resulting rep:externalPrincipalNames must NOT contain the name of the colliding principal
-        Set<String> pNames = Sets.newHashSet(extPrincipalNames.getValue(Type.STRINGS));
-        assertFalse(pNames + " must not contain " + externalGroup.getPrincipalName(), pNames.contains(externalGroup.getPrincipalName()));
+        assertIgnored(externalUser, externalGroup, externalGroup.getId(), externalGroup.getPrincipalName(), null);    
     }
 
     @Test
-    public void testSyncExternalUserGroupConflict2() throws Exception {
+    public void testSyncExternalUserGroupConflictDifferentIDP() throws Exception {
         ExternalUser externalUser = idp.getUser(USER_ID);
 
         // create a local group that collides with the external group membership
         // i.e. belongs to a different IDP
         ExternalIdentityRef ref = externalUser.getDeclaredGroups().iterator().next();
         ExternalIdentity externalGroup = idp.getIdentity(ref);
-        Group g = userManager.createGroup(externalGroup.getId(), new PrincipalImpl(externalGroup.getPrincipalName()), null);
-        g.setProperty(REP_EXTERNAL_ID, getValueFactory().createValue(new ExternalIdentityRef(ref.getId(), ref.getProviderName()+"_mod").getString()));
+        assertNotNull(externalGroup);
+
+        assertIgnored(externalUser, externalGroup, externalGroup.getId(), externalGroup.getPrincipalName(), 
+                new ExternalIdentityRef(ref.getId(), ref.getProviderName()+"_mod"));
+    }
+
+    @Test
+    public void testSyncExternalUserGroupConflictPrincipalNameMismatch() throws Exception {
+        ExternalUser externalUser = idp.getUser(USER_ID);
+
+        ExternalIdentityRef ref = externalUser.getDeclaredGroups().iterator().next();
+        ExternalIdentity externalGroup = idp.getIdentity(ref);
+        assertNotNull(externalGroup);
+
+        // create a local group that has the same ID but a mismatching principal name
+        // and verify that the group is ignored;
+        assertIgnored(externalUser, externalGroup, externalGroup.getId(), externalGroup.getPrincipalName()+"mismatch", ref);
+    }
+
+    @Test
+    public void testSyncExternalUserGroupConflictPrincipalNameCaseMismatch() throws Exception {
+        ExternalUser externalUser = idp.getUser(USER_ID);
+
+        ExternalIdentityRef ref = externalUser.getDeclaredGroups().iterator().next();
+        ExternalIdentity externalGroup = idp.getIdentity(ref);
+        assertNotNull(externalGroup);
+
+        // create a local group that has the same ID but a mismatching principal name (only case)
+        // and verify that the group is ignored;
+        assertIgnored(externalUser, externalGroup, externalGroup.getId(), externalGroup.getPrincipalName().toUpperCase(), ref);
+    }
+
+    @Test
+    public void testSyncExternalUserGroupConflictIdCaseMismatch() throws Exception {
+        ExternalUser externalUser = idp.getUser(USER_ID);
+        
+        ExternalIdentityRef ref = externalUser.getDeclaredGroups().iterator().next();
+        ExternalIdentity externalGroup = idp.getIdentity(ref);
+        assertNotNull(externalGroup);
+
+        // create a local group that has the case-mismatch in ID/principal name
+        // and verify that the external group is ignored;
+        assertIgnored(externalUser, externalGroup, externalGroup.getId().toUpperCase(), externalGroup.getPrincipalName(), null);
+    }
+    
+    private void assertIgnored(@NotNull ExternalUser externalUser, @NotNull ExternalIdentity externalGroup, 
+                               @NotNull String existingId, @NotNull String existingPrincipalName, @Nullable ExternalIdentityRef existingGroupRef) throws Exception {
+        
+        Group g = userManager.createGroup(existingId, new PrincipalImpl(existingPrincipalName), null);
+        if (existingGroupRef != null) {
+            g.setProperty(REP_EXTERNAL_ID, getValueFactory().createValue(existingGroupRef.getString()));
+        }
         r.commit();
 
         // sync the user with dynamic membership enabled
@@ -691,6 +731,9 @@ public class DynamicSyncContextTest extends AbstractExternalAuthTest {
         TestIdentityProvider.TestUser testuser = (TestIdentityProvider.TestUser) idp.getUser(ID_TEST_USER);
         Set<ExternalIdentityRef> groupRefs = getExpectedSyncedGroupRefs(syncConfig.user().getMembershipNestingDepth(), idp, testuser);
 
+        // verify that the conflicting user has not been synced before
+        assertNull(userManager.getAuthorizable(ID_SECOND_USER));
+        
         ExternalUser second = idp.getUser(ID_SECOND_USER);
         testuser.withGroups(second.getExternalId());
         assertFalse(Iterables.elementsEqual(groupRefs, testuser.getDeclaredGroups()));
@@ -701,14 +744,31 @@ public class DynamicSyncContextTest extends AbstractExternalAuthTest {
         assertTrue(a.hasProperty(REP_EXTERNAL_PRINCIPAL_NAMES));
         Value[] extPrincipalNames = a.getProperty(REP_EXTERNAL_PRINCIPAL_NAMES);
 
-        if (idp instanceof PrincipalNameResolver) {
-            // with IDP implementing PrincipalNameResolver the extra verification for all member-refs being groups is omitted.
-            assertDynamicMembership(testuser, 1);
-        } else {
-            assertEquals(Iterables.size(groupRefs), extPrincipalNames.length);
-            for (Value v : extPrincipalNames) {
-                assertNotEquals(second.getPrincipalName(), v.getString());
-            }
+        assertEquals(Iterables.size(groupRefs), extPrincipalNames.length);
+        for (Value v : extPrincipalNames) {
+            assertNotEquals(second.getPrincipalName(), v.getString());
+        }
+    }
+
+    @Test
+    public void testSyncMembershipWithUserConflict() throws Exception {
+        TestIdentityProvider.TestUser testuser = (TestIdentityProvider.TestUser) idp.getUser(ID_TEST_USER);
+        Set<ExternalIdentityRef> groupRefs = getExpectedSyncedGroupRefs(syncConfig.user().getMembershipNestingDepth(), idp, testuser);
+
+        // in contrast to 'testSyncMembershipWithUserRef' the conflicting group-ref refers to a user in the repository
+        // and the conflict is spotted as the existing synched identity is not a group.
+        testuser.withGroups(previouslySyncedUser.getExternalId());
+        assertFalse(Iterables.elementsEqual(groupRefs, testuser.getDeclaredGroups()));
+
+        sync(testuser, SyncResult.Status.ADD);
+
+        Authorizable a = userManager.getAuthorizable(ID_TEST_USER);
+        assertTrue(a.hasProperty(REP_EXTERNAL_PRINCIPAL_NAMES));
+        Value[] extPrincipalNames = a.getProperty(REP_EXTERNAL_PRINCIPAL_NAMES);
+
+        assertEquals(Iterables.size(groupRefs), extPrincipalNames.length);
+        for (Value v : extPrincipalNames) {
+            assertNotEquals(previouslySyncedUser.getPrincipalName(), v.getString());
         }
     }
 
diff --git a/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/PrincipalResolutionTest.java b/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/PrincipalResolutionTest.java
index f601d063c1..be66e0a23b 100644
--- a/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/PrincipalResolutionTest.java
+++ b/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/PrincipalResolutionTest.java
@@ -16,8 +16,13 @@
  */
 package org.apache.jackrabbit.oak.spi.security.authentication.external.impl;
 
-import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
+import com.google.common.collect.Sets;
+import org.apache.jackrabbit.api.security.user.Authorizable;
+import org.apache.jackrabbit.api.security.user.Group;
+import org.apache.jackrabbit.oak.api.PropertyState;
+import org.apache.jackrabbit.oak.api.Tree;
+import org.apache.jackrabbit.oak.api.Type;
 import org.apache.jackrabbit.oak.spi.security.authentication.external.ExternalIdentity;
 import org.apache.jackrabbit.oak.spi.security.authentication.external.ExternalIdentityException;
 import org.apache.jackrabbit.oak.spi.security.authentication.external.ExternalIdentityProvider;
@@ -26,14 +31,21 @@ import org.apache.jackrabbit.oak.spi.security.authentication.external.ExternalUs
 import org.apache.jackrabbit.oak.spi.security.authentication.external.PrincipalNameResolver;
 import org.apache.jackrabbit.oak.spi.security.authentication.external.SyncResult;
 import org.apache.jackrabbit.oak.spi.security.authentication.external.TestIdentityProvider;
+import org.apache.jackrabbit.oak.spi.security.principal.PrincipalImpl;
 import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
 import org.junit.Test;
 
 import java.util.Set;
 
 import static org.apache.jackrabbit.oak.spi.security.authentication.external.TestIdentityProvider.ID_SECOND_USER;
 import static org.apache.jackrabbit.oak.spi.security.authentication.external.TestIdentityProvider.ID_TEST_USER;
+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.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
 
 public class PrincipalResolutionTest extends DynamicSyncContextTest {
 
@@ -56,4 +68,78 @@ public class PrincipalResolutionTest extends DynamicSyncContextTest {
             }
         }
     }
+
+    @Test
+    public void testSyncExternalUserGroupConflictPrincipalNameMismatch() throws Exception {
+        ExternalUser externalUser = idp.getUser(USER_ID);
+
+        ExternalIdentityRef ref = externalUser.getDeclaredGroups().iterator().next();
+        ExternalIdentity externalGroup = idp.getIdentity(ref);
+        assertNotNull(externalGroup);
+
+        // create a local group that has the same ID but a mismatching principal name
+        // since the shortcut is in place and dynamic groups are not synched the mismatch in principal-name
+        // cannot be spotted (but also doesn't have too much implication apart from being confusing as principal names
+        // are not case-insensitive and conflicting dynamic groups are not being created)
+        assertSynched(externalUser, externalGroup, externalGroup.getId(), externalGroup.getPrincipalName()+"mismatch", ref);
+    }
+
+    @Test
+    public void testSyncExternalUserGroupConflictPrincipalNameCaseMismatch() throws Exception {
+        ExternalUser externalUser = idp.getUser(USER_ID);
+
+        ExternalIdentityRef ref = externalUser.getDeclaredGroups().iterator().next();
+        ExternalIdentity externalGroup = idp.getIdentity(ref);
+        assertNotNull(externalGroup);
+
+        // create a local group that has the same ID but a mismatching principal name
+        // since the shortcut is in place and dynamic groups are not synched the mismatch in principal-name
+        // cannot be spotted (but also doesn't have too much implication apart from being confusing as principal names
+        // are not case-insensitive and conflicting dynamic groups are not being created)
+        assertSynched(externalUser, externalGroup, externalGroup.getId(), externalGroup.getPrincipalName().toUpperCase(), ref);
+    }
+
+    private void assertSynched(@NotNull ExternalUser externalUser, @NotNull ExternalIdentity externalGroup,
+                               @NotNull String existingId, @NotNull String existingPrincipalName, @Nullable ExternalIdentityRef existingGroupRef) throws Exception {
+
+        Group g = userManager.createGroup(existingId, new PrincipalImpl(existingPrincipalName), null);
+        if (existingGroupRef != null) {
+            g.setProperty(REP_EXTERNAL_ID, getValueFactory().createValue(existingGroupRef.getString()));
+        }
+        r.commit();
+
+        // sync the user with dynamic membership enabled
+        sync(externalUser, SyncResult.Status.ADD);
+
+        // retrieve rep:externalPrincipalNames
+        Tree tree = r.getTree(userManager.getAuthorizable(USER_ID).getPath());
+        PropertyState extPrincipalNames = tree.getProperty(REP_EXTERNAL_PRINCIPAL_NAMES);
+        assertNotNull(extPrincipalNames);
+
+        // the resulting rep:externalPrincipalNames must contain the name of the principal
+        Set<String> pNames = Sets.newHashSet(extPrincipalNames.getValue(Type.STRINGS));
+        assertTrue(pNames + " must contain " + externalGroup.getPrincipalName(), pNames.contains(externalGroup.getPrincipalName()));
+    }
+
+    @Test
+    public void testSyncMembershipWithUserRef() throws Exception {
+        TestIdentityProvider.TestUser testuser = (TestIdentityProvider.TestUser) idp.getUser(ID_TEST_USER);
+        Set<ExternalIdentityRef> groupRefs = getExpectedSyncedGroupRefs(syncConfig.user().getMembershipNestingDepth(), idp, testuser);
+
+        // verify that the conflicting user has not been synced before
+        assertNull(userManager.getAuthorizable(ID_SECOND_USER));
+        
+        ExternalUser second = idp.getUser(ID_SECOND_USER);
+        testuser.withGroups(second.getExternalId());
+        assertFalse(Iterables.elementsEqual(groupRefs, testuser.getDeclaredGroups()));
+
+        sync(testuser, SyncResult.Status.ADD);
+
+        Authorizable a = userManager.getAuthorizable(ID_TEST_USER);
+        assertTrue(a.hasProperty(REP_EXTERNAL_PRINCIPAL_NAMES));
+
+        // with IDP implementing PrincipalNameResolver the extra verification for all member-refs being groups 
+        // is omitted _unless_ dynamic groups are enabled as well, in which case the short-cut is ignored.
+        assertDynamicMembership(testuser, 1);
+    }
 }