joerghoh commented on code in PR #1034:
URL: https://github.com/apache/jackrabbit-oak/pull/1034#discussion_r1270876824
##########
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:
My idea is just to provide more information which groups are involved in
this cycle, but not to break it and/or resolve the underlying root cause. That
needs to be done elsewhere and on a different level, I agree.
I could imagine that something like this could already help:
```
private static boolean hasInheritedMembership(..., String auditTrail) {
[...]
log.error("Cylic group membership detected for group id {} via {}",
memberId, auditTrail);
[...]
for (Group group : groups) {
final String trail = auditTrail + " > " + memberId;
if (hasInheritedMembership(group.declaredMemberOf(), groupId, memberId,
processed, trail)) {
return true;
}
}
}
}
```
The idea is to keep track of the current stack of resolved groups, and in
case of a cycle we know how we ended at this place.
Of course this can be more sophisticated than just appending to a string,
but it illustrates the idea. Also it might not be 100% accurate, as the cycle
might not begin with the first invocation of this method. But still it should
be possible to detect the cycle just be analysing the trail and finding the
group name which appears twice.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
