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/26 06:52:35 UTC
[jackrabbit-oak] branch OAK-9955 created (now 085dd6692a)
This is an automated email from the ASF dual-hosted git repository.
angela pushed a change to branch OAK-9955
in repository https://gitbox.apache.org/repos/asf/jackrabbit-oak.git
at 085dd6692a OAK-9955 : DynamicSyncContext: avoid duplicate iteration in syncMembership
This branch includes the following new commits:
new 085dd6692a OAK-9955 : DynamicSyncContext: avoid duplicate iteration in syncMembership
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
[jackrabbit-oak] 01/01: OAK-9955 : DynamicSyncContext: avoid duplicate iteration in syncMembership
Posted by an...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
angela pushed a commit to branch OAK-9955
in repository https://gitbox.apache.org/repos/asf/jackrabbit-oak.git
commit 085dd6692a1066d0768ab9bf4f3660116e8cd5b5
Author: angela <an...@adobe.com>
AuthorDate: Mon Sep 26 08:52:19 2022 +0200
OAK-9955 : DynamicSyncContext: avoid duplicate iteration in syncMembership
---
.../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);
- }
}