[GitHub] [jackrabbit-oak] anchela commented on a diff in pull request #1034: OAK-10318 : Improve AutoMembershipPrincipals#isInheritedMember

["anchela (via GitHub)" <gi...@apache.org>]

anchela commented on code in PR #1034:
URL: https://github.com/apache/jackrabbit-oak/pull/1034#discussion_r1270787291


##########
oak-auth-external/src/main/java/org/apache/jackrabbit/oak/spi/security/authentication/external/impl/principal/AutoMembershipPrincipals.java:
##########
@@ -127,23 +128,55 @@ boolean isMember(@NotNull String idpName, @NotNull String groupId, @NotNull Auth
     }
 
     boolean isInheritedMember(@NotNull String idpName, @NotNull Group group, @NotNull Authorizable authorizable) throws RepositoryException {
-        return isInheritedMember(idpName, group, authorizable, new HashSet<>());
-    }
-
-    boolean isInheritedMember(@NotNull String idpName, @NotNull Group group, @NotNull Authorizable authorizable, @NotNull Set<String> processedIds) throws RepositoryException {
         String groupId = group.getID();
-        if (!processedIds.add(groupId)) {
-            log.error("Cyclic group membership detected for group id {}", groupId);
-            return false;
-        }
         if (isMember(idpName, groupId, authorizable)) {
+            // groupId is listed in auto-membership configuration
             return true;
         }
 
-        Iterator<Authorizable> declaredGroupMembers = Iterators.filter(group.getDeclaredMembers(), Authorizable::isGroup);
-        while (declaredGroupMembers.hasNext()) {
-            Group grMember = (Group) declaredGroupMembers.next();
-            if (isInheritedMember(idpName, grMember, authorizable, processedIds)) {
+        // to test for inherited membership collect automembership-ids and loop auto-membership groups
+        Set<String> automembershipIds = new HashSet<>(Arrays.asList(autoMembershipMapping.get(idpName)));
+        AutoMembershipConfig config = autoMembershipConfigMap.get(idpName);
+        if (config != null) {
+            automembershipIds.addAll(config.getAutoMembership(authorizable));
+        }
+
+        // keep track of processed ids over all 'automembership' ids to avoid repeated evaluation 
+        Set<String> processed = new HashSet<>();
+        for (String automembershipId : automembershipIds) {
+            Authorizable gr = userManager.getAuthorizable(automembershipId);
+            if (gr == null || !gr.isGroup()) {
+                log.warn("Configured automembership id '{}' is not a valid group", automembershipId);
+            } else if (hasInheritedMembership(gr.declaredMemberOf(), groupId, automembershipId, processed)) {
+                return true;
+            }
+        }
+        return false;
+    }
+
+    private static boolean hasInheritedMembership(@NotNull Iterator<Group> declared, @NotNull String groupId, 
+                                                  @NotNull String memberId, @NotNull Set<String> processed) throws RepositoryException {
+        List<Group> groups = new ArrayList<>();
+        while (declared.hasNext()) {
+            Group gr = declared.next();
+            String grId = gr.getID();
+            if (memberId.equals(grId)) {
+                log.error("Cyclic group membership detected for group id {}", memberId);

Review Comment:
   hi @joerghoh i am not sure this is possible. the best way to investigate the cycle is using Apache Sling. breaklng it involves regular JCR user management API, but that seems obvious to me.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org