anchela commented on code in PR #862:
URL: https://github.com/apache/jackrabbit-oak/pull/862#discussion_r1183470803
##########
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:
hi @antoniu98 ,
> I believe the number of groups somebody is member of is also small most of
the times
uh.... in the setup i am having in mind it usually is somewhere between >2
and many. but most users are not member of a privileged group like some
administrators group. so the case we want to cover with this improvement is
only for a small number of users, right? for all the others looping over all
the groups will result no matching entry for the config.
> so it comes up to what is more expensive:
if you really want to know for sure lets create a benchmark.
regarding the question:
> I have a question: shouldn't we loop through the "principals of the
authorizable" when evaluating membership? By this I mean getting the subject
for the current authorizable.getPrincipal() and then get all the principals
associated to that subject ? If YES, I didn't know a way of getting the
subject, this is why I chose looping through the groups of a principal.
there is no reliable way to obtain the subject if you just have the
principal name of the potential impersonator at hand like in
`XPathConditionVisitor.visit(Condition.Impersonation condition) ` and don't get
passed a complete subject like in `Impersonation.allows(Subject)`. the API is
not aligned in the Visitor :(.
That's why I would turn the evaluation around for the XPathConditionVisitor:
- retrieve configured impersonator-principal-names from the config
- lookup the principals using PrincipalManager.getPrincipal
- if the configured principal is a group principal -> check if the principal
passed to the visitor is a member of that group
- if the configured principal is not a group (and we don't mandate group
principals) -> check if the name match. if we mandate that the config only
contains groups we ignore this case here (and log a message on warn if we want
to be nice).... but i wonder why we should limit it to group principals.
--
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]
