anchela commented on code in PR #862:
URL: https://github.com/apache/jackrabbit-oak/pull/862#discussion_r1182856978
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/ImpersonationImpl.java:
##########
@@ -173,13 +182,49 @@ private void updateImpersonatorNames(@NotNull Tree
userTree, @NotNull Set<String
private boolean isAdmin(@NotNull Principal principal) {
if (principal instanceof AdminPrincipal) {
return true;
- } else if (GroupPrincipals.isGroup(principal)) {
+ }
+ return Utils.isAdmin(principal, user.getUserManager());
+ }
+
+ private boolean isImpersonator(@NotNull Set<String> principalNames) {
+ Set<String> impersonatorGroups =
Set.of(user.getUserManager().getConfig().getConfigValue(
+ PARAM_IMPERSONATOR_GROUPS_ID,
+ new String[]{}));
+
+ if (impersonatorGroups.isEmpty()) {
return false;
- } else {
- return Utils.canImpersonateAllUsers(principal,
user.getUserManager());
}
+ return principalNames.stream()
+ .anyMatch(impersonatorGroups::contains);
}
+ public boolean isImpersonator(@NotNull Authorizable authorizable) {
+ Set<String> impersonatorGroups =
Set.of(user.getUserManager().getConfig().getConfigValue(
+ PARAM_IMPERSONATOR_GROUPS_ID,
+ new String[]{}));
+
+ if (impersonatorGroups.isEmpty()) {
+ return false;
+ }
+
+ try {
+ Iterator<Group> it = authorizable.memberOf();
Review Comment:
thinking about this a bit longer, i also wonder, why you loop over the
membership of the user, which might contain something between multiple to many
groups.
so for the query part of the story, why not doing something along the
following lines:
```
Principal userPrincipal = authorizable.getPrincipal();
for (String principalName : configuredImpersonators) {
Principal principal = principalManager.getPrincipal(principalName);
if (principal != null) {
if (GroupPrincipals.isGroup(principal)) {
if (((GroupPrincipal) principal).isMember(userPrincipal)) {
return true;
}
} else if (principalName.equals(userPrincipal.getName()) {
return true;
}
}
}
return false;
```
the reasons:
- i would expect the configuration to only contain a very limited number of
entries (if we fear that it might become big we can even limit the cardinality
of the configuration0
- instead of looping over all groups (and for most cases not finding a
matching group) we check directly if the user-principal is member of any of the
group principals (lets say there are 1 or 2 entries in the config only)
- for non-group configuration entries we just check for a matching name.
- for configuration entries that link to non-existing prinicipals -> we
ignore it
- for default local group princpals stored in the user manager that might
also be the better way due to the way group membership is stored.
one more thing: this way we are also able to cover the edge case where a
configured principal name that is not stored in the repository as a regular
group but would still end up in the subject of a given user (yes, oak allows to
plug custom sources for principals. i can show it to you if you want).
--
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-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
