OAK-10486 : Resolution of inherited groups may terminate pre-maturely for external users
diff --git a/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/InheritedMembershipIterator.java b/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/InheritedMembershipIterator.java
index 8b19f07..bc142cd 100644
--- a/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/InheritedMembershipIterator.java
+++ b/oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/InheritedMembershipIterator.java
@@ -54,6 +54,7 @@
try {
// call 'memberof' to cover nested inheritance
Iterator<Group> it = gr.memberOf();
+ // verify that the group-iterator has any elements before remembering it for further processing
if (it.hasNext()) {
inherited.add(it);
}
@@ -63,18 +64,11 @@
return gr;
}
- if (inheritedIterator == null) {
- inheritedIterator = getNextInheritedIterator();
- }
-
- while (inheritedIterator.hasNext()) {
+ while (inheritedHasNext()) {
Group gr = inheritedIterator.next();
if (notProcessedBefore(gr)) {
return gr;
}
- if (!inheritedIterator.hasNext()) {
- inheritedIterator = getNextInheritedIterator();
- }
}
// all inherited groups have been processed
@@ -88,6 +82,21 @@
return true;
}
}
+
+ private boolean inheritedHasNext() {
+ if (inheritedIterator == null) {
+ // initialize the inherited iterator (i.e. get the first one after having processed all dynamic groups)
+ inheritedIterator = getNextInheritedIterator();
+ }
+ if (inheritedIterator.hasNext()) {
+ return true;
+ } else {
+ // no more elements in the current 'inheritedIterator'. move on to the next inherited iterator
+ // (or an empty one if there are no more iterators left to process)
+ inheritedIterator = getNextInheritedIterator();
+ return inheritedIterator.hasNext();
+ }
+ }
@NotNull
private Iterator<Group> getNextInheritedIterator() {
diff --git a/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/DynamicSyncTest.java b/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/DynamicSyncTest.java
index c653741..1ac7309 100644
--- a/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/DynamicSyncTest.java
+++ b/oak-auth-external/src/test/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/DynamicSyncTest.java
@@ -50,6 +50,8 @@
private static final String BASE_ID = "base";
private static final String BASE2_ID = "base2";
+ private static final String BASE3_ID = "base3";
+ private static final String BASE4_ID = "base4";
private static final String AUTO_GROUPS = "autoForGroups";
private static final String AUTO_USERS = "autoForUsers";
@@ -57,6 +59,8 @@
private Group autoForUsers;
private Group base;
private Group base2;
+ private Group base3;
+ private Group base4;
@Override
public void before() throws Exception {
@@ -76,6 +80,9 @@
base2 = userManager.createGroup(BASE2_ID);
base2.addMember(autoForUsers);
+ base3 = userManager.createGroup(BASE3_ID);
+ base4 = userManager.createGroup(BASE4_ID);
+ base4.addMembers(BASE3_ID);
r.commit();
}
@@ -131,6 +138,65 @@
}
@Test
+ public void testSyncedUserDifferentBaseGroups() throws Exception {
+ // 'b' is member of local group 'base3' (not of 'base')
+ // 'a' is member of local group 'base'
+ assertTrue(base3.addMembers("b").isEmpty());
+ assertTrue(base.removeMembers("b").isEmpty());
+ root.commit();
+
+ ExternalUser externalUser = idp.getUser(USER_ID);
+ sync(externalUser, SyncResult.Status.ADD);
+
+ Authorizable user = userManager.getAuthorizable(USER_ID);
+ assertNotNull(user);
+
+ // assert membership
+ Set<String> expDeclaredGroupIds = ImmutableSet.of("a", "b", "c", "aa", "aaa", AUTO_GROUPS, AUTO_USERS, EveryonePrincipal.NAME);
+ assertExpectedIds(expDeclaredGroupIds, user.declaredMemberOf());
+
+ Set<String> expGroupIds = ImmutableSet.of(BASE_ID, BASE2_ID, BASE3_ID, BASE4_ID, "a", "b", "c", "aa", "aaa", AUTO_GROUPS, AUTO_USERS, EveryonePrincipal.NAME);
+ assertExpectedIds(expGroupIds, user.memberOf());
+
+ // assert groups
+ user.declaredMemberOf().forEachRemaining(group -> assertIsMember(group, true, user));
+ user.memberOf().forEachRemaining(group -> assertIsMember(group, false, user));
+
+ // assert principals
+ List<String> principalNames = getPrincipalNames(getPrincipalManager(r).getGroupMembership(user.getPrincipal()));
+ assertEquals(12, principalNames.size());
+ }
+
+ @Test
+ public void testSyncedUserDifferentBaseGroupsWithDuplication() throws Exception {
+ // 'b' is member of local group 'base3' and 'base'
+ // 'a' is member of local group 'base'
+ assertTrue(base3.addMembers("b").isEmpty());
+ root.commit();
+
+ ExternalUser externalUser = idp.getUser(USER_ID);
+ sync(externalUser, SyncResult.Status.ADD);
+
+ Authorizable user = userManager.getAuthorizable(USER_ID);
+ assertNotNull(user);
+
+ // assert membership
+ Set<String> expDeclaredGroupIds = ImmutableSet.of("a", "b", "c", "aa", "aaa", AUTO_GROUPS, AUTO_USERS, EveryonePrincipal.NAME);
+ assertExpectedIds(expDeclaredGroupIds, user.declaredMemberOf());
+
+ Set<String> expGroupIds = ImmutableSet.of(BASE_ID, BASE2_ID, BASE3_ID, BASE4_ID, "a", "b", "c", "aa", "aaa", AUTO_GROUPS, AUTO_USERS, EveryonePrincipal.NAME);
+ assertExpectedIds(expGroupIds, user.memberOf());
+
+ // assert groups
+ user.declaredMemberOf().forEachRemaining(group -> assertIsMember(group, true, user));
+ user.memberOf().forEachRemaining(group -> assertIsMember(group, false, user));
+
+ // assert principals
+ List<String> principalNames = getPrincipalNames(getPrincipalManager(r).getGroupMembership(user.getPrincipal()));
+ assertEquals(12, principalNames.size());
+ }
+
+ @Test
public void testSyncedGroup() throws Exception {
ExternalUser externalUser = idp.getUser(USER_ID);
sync(externalUser, SyncResult.Status.ADD);
