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/27 07:40:19 UTC
[jackrabbit-oak] branch trunk updated: OAK-9955 : DynamicSyncContext: avoid duplicate iteration in syncMembership (#720)
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 ab3d2e79e1 OAK-9955 : DynamicSyncContext: avoid duplicate iteration in syncMembership (#720)
ab3d2e79e1 is described below
commit ab3d2e79e15920733fa12284fe704da0fc1e18dc
Author: anchela <an...@apache.org>
AuthorDate: Tue Sep 27 09:40:12 2022 +0200
OAK-9955 : DynamicSyncContext: avoid duplicate iteration in syncMembership (#720)
---
.../external/impl/DynamicSyncContext.java | 143 ++++++++++++++-------
.../external/impl/DynamicGroupsTest.java | 27 +++-
.../external/impl/DynamicSyncContextTest.java | 35 ++++-
.../external/impl/PrincipalResolutionTest.java | 20 +--
4 files changed, 149 insertions(+), 76 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 1e4ce26ebc..bd22589b3a 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
@@ -35,6 +35,7 @@ import org.apache.jackrabbit.oak.spi.security.authentication.external.basic.Defa
import org.apache.jackrabbit.oak.spi.security.authentication.external.basic.DefaultSyncedIdentity;
import org.apache.jackrabbit.oak.spi.security.principal.PrincipalImpl;
import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -42,9 +43,13 @@ import javax.jcr.RepositoryException;
import javax.jcr.Value;
import javax.jcr.ValueFactory;
import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
+import java.util.Map;
import java.util.Set;
+import java.util.stream.Collectors;
/**
* Extension of the {@code DefaultSyncContext} that doesn't synchronize group
@@ -148,13 +153,16 @@ public class DynamicSyncContext extends DefaultSyncContext {
} else {
try {
Iterable<ExternalIdentityRef> declaredGroupRefs = external.getDeclaredGroups();
+ // resolve group-refs respecting depth to avoid iterating twice
+ Map<ExternalIdentityRef, SyncEntry> map = collectSyncEntries(declaredGroupRefs, depth);
+
// store dynamic membership with the user
- setExternalPrincipalNames(auth, declaredGroupRefs, depth);
+ setExternalPrincipalNames(auth, map.values());
// 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(declaredGroupRefs, depth);
+ createDynamicGroups(map);
}
// clean up any other membership
@@ -177,95 +185,117 @@ public class DynamicSyncContext extends DefaultSyncContext {
* rep:externalPrincipalNames property with the accurate collection of principal names.
*
* @param authorizable The target synced user
- * @param declareGroupRefs The declared group references for the external user
- * @param depth The configured depth to resolve nested groups.
- * @throws ExternalIdentityException If group principal names cannot be calculated
+ * @param syncEntries The set of sync entries collected before.
* @throws RepositoryException If another error occurs
*/
- private void setExternalPrincipalNames(@NotNull Authorizable authorizable, Iterable<ExternalIdentityRef> declareGroupRefs, long depth) throws ExternalIdentityException, RepositoryException {
+ private void setExternalPrincipalNames(@NotNull Authorizable authorizable, @NotNull Collection<SyncEntry> syncEntries) throws RepositoryException {
Value[] vs;
- if (depth <= 0) {
+ if (syncEntries.isEmpty()) {
vs = new Value[0];
} else {
- Set<String> principalsNames = new HashSet<>();
- collectPrincipalNames(principalsNames, declareGroupRefs, depth);
+ Set<String> principalsNames = syncEntries.stream().map(syncEntry -> syncEntry.principalName).collect(Collectors.toSet());
vs = createValues(principalsNames);
}
authorizable.setProperty(ExternalIdentityConstants.REP_EXTERNAL_PRINCIPAL_NAMES, vs);
}
+ @NotNull
+ private Map<ExternalIdentityRef, SyncEntry> collectSyncEntries(@NotNull Iterable<ExternalIdentityRef> declaredGroupRefs, long depth) throws RepositoryException, ExternalIdentityException {
+ if (depth <= 0) {
+ return Collections.emptyMap();
+ }
+ Map<ExternalIdentityRef, SyncEntry> map = new HashMap<>();
+ collectSyncEntries(declaredGroupRefs, depth, map);
+ return map;
+ }
+
/**
- * Recursively collect the principal names of the given declared group
- * references up to the given depth.
+ * Recursively collect the sync entries of the given declared group references up to the given depth.
*
* Note, that this method will filter out references that don't belong to the same IDP (see OAK-8665).
*
- * @param principalNames The set used to collect the names of the group principals.
- * @param declaredGroupIdRefs The declared group references for a user or a group.
+ * @param declaredGroupRefs The declared group references for a user or a group.
* @param depth Configured membership nesting; the recursion will be stopped once depths is < 1.
+ * @param map The map to be filled with all group refs and the corresponding sync entries.
* @throws ExternalIdentityException If an error occurs while resolving the the external group references.
*/
- private void collectPrincipalNames(@NotNull Set<String> principalNames, @NotNull Iterable<ExternalIdentityRef> declaredGroupIdRefs, long depth) throws ExternalIdentityException, RepositoryException {
+ private void collectSyncEntries(@NotNull Iterable<ExternalIdentityRef> declaredGroupRefs, long depth, @NotNull Map<ExternalIdentityRef, SyncEntry> map) throws ExternalIdentityException, RepositoryException {
boolean shortcut = (depth <= 1 && idp instanceof PrincipalNameResolver);
- for (ExternalIdentityRef ref : Iterables.filter(declaredGroupIdRefs, this::isSameIDP)) {
+ for (ExternalIdentityRef ref : Iterables.filter(declaredGroupRefs, this::isSameIDP)) {
+ String principalName = null;
+ Authorizable a = null;
+ ExternalGroup externalGroup = null;
if (shortcut) {
- addPrincipalName(((PrincipalNameResolver) idp).fromExternalIdentityRef(ref), principalNames);
+ principalName = ((PrincipalNameResolver) idp).fromExternalIdentityRef(ref);
+ a = userManager.getAuthorizable(new PrincipalImpl(principalName));
} else {
// get group from the IDP
- ExternalIdentity extId = getExternalGroupFromRef(ref);
- if (extId != null) {
- addPrincipalName(extId.getPrincipalName(), principalNames);
+ externalGroup = getExternalGroupFromRef(ref);
+ if (externalGroup != null) {
+ principalName = externalGroup.getPrincipalName();
+ a = userManager.getAuthorizable(new PrincipalImpl(principalName));
+
// recursively apply further membership until the configured depth is reached
if (depth > 1) {
- collectPrincipalNames(principalNames, extId.getDeclaredGroups(), depth - 1);
+ collectSyncEntries(externalGroup.getDeclaredGroups(), depth - 1, map);
}
}
}
+
+ if (principalName != null && !isConflictingGroup(a, principalName)) {
+ map.put(ref, new SyncEntry(principalName, externalGroup, (Group) a));
+ }
}
}
- private void addPrincipalName(@NotNull String principalName, @NotNull Set<String> principalNames) throws RepositoryException {
- if (existsConflicting(principalName)) {
- // there exists a user or group with that 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());
- return;
- }
- // no conflict detected
- principalNames.add(principalName);
- }
-
/**
- * Tests if there exists an user/group in the repository that has the same principal name but doesn't belong to the same IDP.
+ * Tests if the given existing user/group collides with the external group having the same principall name.
+ * It is considered a conflict if the existing authorizable is a user (and not a group) or if it doesn't belong to the
+ * same IDP (i.e. a local group or one defined for a different IDP).
*
- * @param externalGroupPrincipalName A principal name
- * @return {@code true} if there exists an user/group in the repository with the given principal name that doesn't
- * belong to this IDP; {@code false} otherwise.
+ * NOTE: this method does not verify if the 'rep:authorizableId' or the identifier part of 'rep:externalId' match,
+ * Instead it assumes that external identities and synced authorizables that are associated with the same IDP can be
+ * trusted to be consistent as long as they have the same principal name.
+ *
+ * @param authorizable An authorizable with the given principal name or {@code null}.
+ * @return {@code true} if the given user/group collides with the external group with the given principal name; {@code false} otherwise.
* @throws RepositoryException If an error occurs
*/
- private boolean existsConflicting(@NotNull String externalGroupPrincipalName) throws RepositoryException {
- Authorizable a = userManager.getAuthorizable(new PrincipalImpl(externalGroupPrincipalName));
- if (a == null) {
+ private boolean isConflictingGroup(@Nullable Authorizable authorizable, @NotNull String principalName) throws RepositoryException {
+ if (authorizable == null) {
return false;
+ } else if (!authorizable.isGroup()) {
+ log.warn("Existing user '{}' collides with external group.", authorizable.getID());
+ 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
+ // 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());
+ return true;
} else {
- return !isSameIDP(a);
+ // group has been synced before (same IDP, same principal-name)
+ return false;
}
}
- private void createDynamicGroups(@NotNull Iterable<ExternalIdentityRef> declaredGroupIdRefs,
- long depth) throws RepositoryException, ExternalIdentityException {
- for (ExternalIdentityRef groupRef : declaredGroupIdRefs) {
- ExternalGroup externalGroup = getExternalGroupFromRef(groupRef);
+ 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();
+
+ // 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) {
- Group gr = userManager.getAuthorizable(externalGroup.getId(), Group.class);
+ // 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);
- if (depth > 1) {
- createDynamicGroups(externalGroup.getDeclaredGroups(),depth-1);
- }
}
}
}
@@ -330,4 +360,21 @@ public class DynamicSyncContext extends DefaultSyncContext {
private static boolean groupsSyncedBefore(@NotNull Authorizable authorizable) throws RepositoryException {
return authorizable.hasProperty(REP_LAST_SYNCED) && !authorizable.hasProperty(ExternalIdentityConstants.REP_EXTERNAL_PRINCIPAL_NAMES);
}
+
+ /**
+ * Helper object to avoid repeated lookup of principalName, {@link ExternalGroup} and synchronized {@link Group} for
+ * a given {@link ExternalIdentityRef} during {@link #syncMembership(ExternalIdentity, Authorizable, long)}.
+ */
+ private static class SyncEntry {
+
+ private final String principalName;
+ private final ExternalGroup externalGroup;
+ private final Group group;
+
+ private SyncEntry(@NotNull String principalName, @Nullable ExternalGroup externalGroup, @Nullable Group group) {
+ this.principalName = principalName;
+ this.externalGroup = externalGroup;
+ this.group = group;
+ }
+ }
}
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 9e9c7ae68e..b0e50dae2c 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
@@ -26,6 +26,7 @@ import org.apache.jackrabbit.api.security.user.User;
import org.apache.jackrabbit.oak.api.Tree;
import org.apache.jackrabbit.oak.spi.security.authentication.external.ExternalGroup;
import org.apache.jackrabbit.oak.spi.security.authentication.external.ExternalIdentity;
+import org.apache.jackrabbit.oak.spi.security.authentication.external.ExternalIdentityProvider;
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.authentication.external.SyncResult;
@@ -56,20 +57,34 @@ import static org.junit.Assert.assertTrue;
@RunWith(Parameterized.class)
public class DynamicGroupsTest extends DynamicSyncContextTest {
- @Parameterized.Parameters(name = "name={1}")
+ @Parameterized.Parameters(name = "name={2}")
public static Collection<Object[]> parameters() {
return Lists.newArrayList(
- new Object[] { DefaultSyncConfigImpl.PARAM_USER_MEMBERSHIP_NESTING_DEPTH_DEFAULT, "Membership-Nesting-Depth=0" },
- new Object[] { DefaultSyncConfigImpl.PARAM_USER_MEMBERSHIP_NESTING_DEPTH_DEFAULT+1, "Membership-Nesting-Depth=1" },
- new Object[] { DefaultSyncConfigImpl.PARAM_USER_MEMBERSHIP_NESTING_DEPTH_DEFAULT+2, "Membership-Nesting-Depth=2" });
+ 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" },
+ 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" });
}
private final long membershipNestingDepth;
+ private final boolean isPrincipalNameResolver;
- public DynamicGroupsTest(long membershipNestingDepth, @NotNull String name) {
+ public DynamicGroupsTest(long membershipNestingDepth, boolean isPrincipalNameResolver, @NotNull String name) {
this.membershipNestingDepth = membershipNestingDepth;
+ this.isPrincipalNameResolver = isPrincipalNameResolver;
}
-
+
+ @Override
+ @NotNull
+ protected ExternalIdentityProvider createIDP() {
+ if (isPrincipalNameResolver) {
+ return new PrincipalResolutionTest.PrincipalResolvingIDP();
+ } else {
+ return super.createIDP();
+ }
+ }
+
+
@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 0f2552e830..f39b1ae3b5 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
@@ -37,6 +37,7 @@ import org.apache.jackrabbit.oak.spi.security.authentication.external.ExternalId
import org.apache.jackrabbit.oak.spi.security.authentication.external.ExternalIdentityProvider;
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.authentication.external.PrincipalNameResolver;
import org.apache.jackrabbit.oak.spi.security.authentication.external.SyncException;
import org.apache.jackrabbit.oak.spi.security.authentication.external.SyncResult;
import org.apache.jackrabbit.oak.spi.security.authentication.external.SyncedIdentity;
@@ -378,6 +379,29 @@ public class DynamicSyncContextTest extends AbstractExternalAuthTest {
assertFalse(pNames + " must not contain " + externalGroup.getPrincipalName(), pNames.contains(externalGroup.getPrincipalName()));
}
+ @Test
+ public void testSyncExternalUserGroupConflictWithUser() throws Exception {
+ ExternalUser externalUser = idp.getUser(USER_ID);
+
+ // create a local user that collides with the first external group ref
+ ExternalIdentityRef ref = externalUser.getDeclaredGroups().iterator().next();
+ ExternalIdentity externalGroup = idp.getIdentity(ref);
+ User collision = userManager.createUser(externalGroup.getId(), null, 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);
+
+ // 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()));
+ }
+
@Test
public void testSyncExternalUserExistingGroups() throws Exception {
// verify group membership of the previously synced user
@@ -677,9 +701,14 @@ public class DynamicSyncContextTest extends AbstractExternalAuthTest {
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(second.getPrincipalName(), v.getString());
+ 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());
+ }
}
}
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 60b1ea3540..f601d063c1 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
@@ -43,7 +43,7 @@ public class PrincipalResolutionTest extends DynamicSyncContextTest {
return new PrincipalResolvingIDP();
}
- private static class PrincipalResolvingIDP extends TestIdentityProvider implements PrincipalNameResolver {
+ static class PrincipalResolvingIDP extends TestIdentityProvider implements PrincipalNameResolver {
@NotNull
@Override
@@ -56,22 +56,4 @@ public class PrincipalResolutionTest extends DynamicSyncContextTest {
}
}
}
-
- /**
- * With {@code PrincipalNameResolver} the extra verification for all member-refs being groups is omitted.
- * @throws Exception
- */
- @Test
- public void testSyncMembershipWithUserRef() throws Exception {
- TestIdentityProvider.TestUser testuser = (TestIdentityProvider.TestUser) idp.getUser(ID_TEST_USER);
- Set<ExternalIdentityRef> groupRefs = ImmutableSet.copyOf(testuser.getDeclaredGroups());
-
- ExternalUser second = idp.getUser(ID_SECOND_USER);
- testuser.withGroups(second.getExternalId());
- assertFalse(Iterables.elementsEqual(groupRefs, testuser.getDeclaredGroups()));
-
- sync(testuser, SyncResult.Status.ADD);
-
- assertDynamicMembership(testuser, 1);
- }
}