anchela commented on code in PR #862:
URL: https://github.com/apache/jackrabbit-oak/pull/862#discussion_r1182673682
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserConfigurationImpl.java:
##########
@@ -83,6 +84,12 @@ public class UserConfigurationImpl extends ConfigurationBase
implements UserConf
description = "Path underneath which user nodes are being
created.")
String usersPath() default UserConstants.DEFAULT_USER_PATH;
+ @AttributeDefinition(
+ name = "Impersonator groups",
+ description = "List of groups whose members can impersonate
any user.",
Review Comment:
i would not limit that to groups but allow for any principal name. this
would allow us to also configure principal names of service users that need to
have the ability to impersonate every single user.
also the description should make clear that the configured values must be
principal names and not IDs (see above)
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/ImpersonationImpl.java:
##########
@@ -126,15 +128,22 @@ public boolean allows(@NotNull Subject subject) {
return false;
}
+ Set<Principal> principals = subject.getPrincipals();
Set<String> principalNames = new HashSet<>();
- for (Principal principal : subject.getPrincipals()) {
- principalNames.add(principal.getName());
+ for (Principal principal : principals) {
+ if (!GroupPrincipals.isGroup(principal)) {
Review Comment:
i don't get why you would filter out group-principals here.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/Utils.java:
##########
@@ -93,7 +93,7 @@ static boolean isEveryone(@NotNull Authorizable authorizable)
{
/**
* Return {@code true} if the given principal can impersonate all users.
* The implementation tests if the given principal refers to an existing
{@code User} for which {@link User#isAdmin()}
- * returns {@code true}.
+ * returns {@code true} OR if the user is an impersonator (member of an
impersonator group).
Review Comment:
that would also be slightly adjusted to to something like: the subject
contains a configured 'impersonator' principal that can impersonate all users.
##########
oak-security-spi/src/main/java/org/apache/jackrabbit/oak/spi/security/user/UserConstants.java:
##########
@@ -78,6 +78,11 @@ public interface UserConstants {
*/
String PARAM_ADMIN_ID = "adminId";
+ /**
+ * Configuration option defining the ID of the impersonatorGroups field.
+ */
+ String PARAM_IMPERSONATOR_GROUPS_ID = "impersonatorGroups";
Review Comment:
the parameter should be based on principal names and not on IDs.
see discussion above about the 2 options:
- we define that it must contain names of group principals => only change
the ID part
- we define that i may contain any principal name => drop group and replace
ID>
##########
oak-security-spi/src/main/java/org/apache/jackrabbit/oak/spi/security/user/package-info.java:
##########
@@ -14,7 +14,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-@Version("2.5.0")
+@Version("2.6.0")
Review Comment:
got it!
##########
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:
as i commented before in the last review round there should not no need
resolve all group membership for the given authorizable during
Impersonation.allows(Subject). you have to full list of principals that are
present in the subject and can just check if any of them matches the configured
group-principals.
the matching has at least 1 mandatory step:
- the prinicipal name must be equal
there is an optional second step:
- if the configuration option is limited to group-principals -> verify that
the matching principal in the subject is also of type group-principal
- if the configuration option does not mandate that the configured names
belong to group principals, then any matching name would be ok. i have a slight
preference for the latter as it might come handy for service-users that need to
be able to impersonate every single user.
one more note:
resolving membership is an expensive operation and i don't want this to be
perform during Session.impersonate. in particular since it is simply not needed.
one reason why you might have ended up adding is the the fact that the
configuration option must contain principal names and not IDs. those can differ
and it's important that the configuration is really about principal names (see
below)
the second reason is the query-piece: there you don't have a subject at
hand. but i would really want the Impersonation.allow call to become slow. so
the membership resolution should be limited to the query parts (if the name of
the passed principal does not match any of the configured values in case of
option 2 above).
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/Utils.java:
##########
@@ -103,12 +103,43 @@ static boolean isEveryone(@NotNull Authorizable
authorizable) {
public static boolean canImpersonateAllUsers(@NotNull Principal principal,
@NotNull UserManager userManager) {
try {
Authorizable authorizable = userManager.getAuthorizable(principal);
- return authorizable != null && !authorizable.isGroup() && ((User)
authorizable).isAdmin();
+ if (authorizable == null || authorizable.isGroup()) {
+ return false;
+ }
+
+ User user = (User)authorizable;
+ ImpersonationImpl impersonation =
(ImpersonationImpl)user.getImpersonation();
+ return user.isAdmin() ||
impersonation.isImpersonator(authorizable);
Review Comment:
i see now where the membership resolution in the
`ImpersonationImpl.isImpersonator(authorizable)` is coming from.... the
problem is that you have the Subject at hand in `Impersonation.allows` but not
in `XPathConditionVisitor.visit(Condition.Impersonation condition)`
i would refactor the patch such that there is no need to resolve membership
in `Impersonation.allows` but resolve optionally resolve it for the query
piece.... e.g. refactoring the utility method or adding a variant that resolves
membership.
note: it's also not super pretty that you have to casts the impersonation
object to the impl. so i would rather move the membership resolution where it
is really neeed (in impersonationimpl it's not needed for the reasons
explained).
now: you can argue that the query part would in this case not work 100% the
same as Impersonation.allow. but that's anyway the case.... as the query piece
doesn't not allow with certainty to reflect the subject of the editing session
(which most likely was the intention of the original author). it's
unfortunately just a best effort approximation.... if we want it to be 100%
equivalent we would have to pass in a subject or the session object (which is
based on a subject).
finally:
there is one more thing to consider regarding the config option. in case we
define that it is not limited to group-principals then also the name of the
passed principal should be checked for matching the config and only if that
short-cut does not work resolve group membership.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserConfigurationImpl.java:
##########
@@ -83,6 +84,12 @@ public class UserConfigurationImpl extends ConfigurationBase
implements UserConf
description = "Path underneath which user nodes are being
created.")
String usersPath() default UserConstants.DEFAULT_USER_PATH;
+ @AttributeDefinition(
+ name = "Impersonator groups",
+ description = "List of groups whose members can impersonate
any user.",
+ type = AttributeType.STRING)
+ String[] impersonatorGroups() default {};
Review Comment:
see above. i would rename to 'impersonatorPrincipals' or something along
that line.
--
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
