You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jackrabbit.apache.org by "antoniu98 (via GitHub)" <gi...@apache.org> on 2023/02/28 11:28:46 UTC
[GitHub] [jackrabbit-oak] antoniu98 opened a new pull request, #862: Give admin impersonation rights to members of administrator groups
antoniu98 opened a new pull request, #862:
URL: https://github.com/apache/jackrabbit-oak/pull/862
Allow members of administrator groups to have admin permission when it comes to impersonation.
--
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
[GitHub] [jackrabbit-oak] anchela commented on a diff in pull request #862: Give admin impersonation rights to members of administrator groups
Posted by "anchela (via GitHub)" <gi...@apache.org>.
anchela commented on code in PR #862:
URL: https://github.com/apache/jackrabbit-oak/pull/862#discussion_r1125479846
##########
oak-security-spi/src/main/java/org/apache/jackrabbit/oak/spi/security/user/UserConstants.java:
##########
@@ -78,6 +78,16 @@ public interface UserConstants {
*/
String PARAM_ADMIN_ID = "adminId";
+ /**
+ * Configuration option defining the ID of the administratorGroups field.
+ */
+ String ADMINISTRATOR_GROUPS_CONFIG_ID = "administratorGroups";
Review Comment:
- this constant should follow the naming convention of the other configuration options and start with PARAM_ (instead of trailing CONFIG_ID)
- also it should make the intention clear, which is that this an option used for the impersonation feature
- i would also not limit this to groups.... but rather allow for any principal name.
##########
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 = "Administrators groups",
Review Comment:
see below. the name is misleading and imho not describing what this option is being used for.
##########
oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/UserImplTest.java:
##########
@@ -87,6 +89,19 @@ public void testAdministratorIsAdmin() throws Exception {
assertTrue(getAdminUser().isAdmin());
}
+ @Test
+ public void testUserInAdministratorGroupsIsAdmin() throws RepositoryException, CommitFailedException {
Review Comment:
see comment above.... i am not really happy with the change and this test should not be needed in the first place.
##########
oak-security-spi/src/main/java/org/apache/jackrabbit/oak/spi/security/user/util/package-info.java:
##########
@@ -14,7 +14,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-@Version("1.2.1")
+@Version("1.3.0")
Review Comment:
see above. is IMHO not needed.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserImpl.java:
##########
@@ -129,9 +135,17 @@ public void changePassword(@Nullable String password, @NotNull String oldPasswor
changePassword(password);
}
+ /**
+ * Disables the user.
+ * <p>
+ * The user with the configured param {@link UserConstants#PARAM_ADMIN_ID} cannot be disabled.
+ *
+ * @throws RepositoryException if the user is the default admin one (configuration param
+ * {@link UserConstants#PARAM_ADMIN_ID})
+ */
@Override
public void disable(@Nullable String reason) throws RepositoryException {
- if (isAdmin) {
+ if (UserUtil.isAdmin(getUserManager().getConfig(), getID())) {
Review Comment:
please revert (see comment above)
##########
oak-security-spi/src/main/java/org/apache/jackrabbit/oak/spi/security/user/UserConstants.java:
##########
@@ -78,6 +78,16 @@ public interface UserConstants {
*/
String PARAM_ADMIN_ID = "adminId";
+ /**
+ * Configuration option defining the ID of the administratorGroups field.
+ */
+ String ADMINISTRATOR_GROUPS_CONFIG_ID = "administratorGroups";
+
+ /**
+ * Default value for the administrator group {@link #ADMINISTRATOR_GROUPS_CONFIG_ID}.
+ */
+ String DEFAULT_ADMINISTRATORS_GROUP = "administrators";
Review Comment:
this is an AEM implementation detail.
there exists no 'administrators' group out of the box in Oak and this constant doesn't make sense.
please remove and leave the default value in the UserConfigurationImpl empty.
##########
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 = "Administrators groups",
+ description = "List of groups whose members have admin rights.",
+ type = AttributeType.STRING)
+ String[] administratorGroups() default {UserConstants.DEFAULT_ADMINISTRATORS_GROUP};
Review Comment:
- for backwards compatibility the default should be an empty list
- the default administrators group name does not make sense in the context of oak as it is an implementation detail of Adobe AEM which does not exist in Oak out of the box
- the name of the option is a bit misleading: this feature is not about administrative rights but about who is able to always impersonate any other user.... and it should say so
- finally i would not limit this to 'groups' but allow for any kind of principal names
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserImpl.java:
##########
@@ -16,41 +16,38 @@
*/
package org.apache.jackrabbit.oak.security.user;
+import static org.apache.jackrabbit.oak.api.Type.STRING;
+
import java.security.Principal;
import javax.jcr.Credentials;
import javax.jcr.RepositoryException;
-
import org.apache.jackrabbit.api.security.user.Impersonation;
import org.apache.jackrabbit.api.security.user.User;
import org.apache.jackrabbit.oak.api.PropertyState;
import org.apache.jackrabbit.oak.api.Tree;
import org.apache.jackrabbit.oak.namepath.NamePathMapper;
+import org.apache.jackrabbit.oak.plugins.tree.TreeUtil;
import org.apache.jackrabbit.oak.spi.security.user.AuthorizableType;
import org.apache.jackrabbit.oak.spi.security.user.UserConstants;
import org.apache.jackrabbit.oak.spi.security.user.UserIdCredentials;
import org.apache.jackrabbit.oak.spi.security.user.util.PasswordUtil;
import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil;
-import org.apache.jackrabbit.oak.plugins.tree.TreeUtil;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
-import static org.apache.jackrabbit.oak.api.Type.STRING;
-
/**
* UserImpl...
*/
class UserImpl extends AuthorizableImpl implements User {
- private final boolean isAdmin;
private final PasswordHistory pwHistory;
UserImpl(String id, Tree tree, UserManagerImpl userManager) throws RepositoryException {
super(id, tree, userManager);
-
- isAdmin = UserUtil.isAdmin(userManager.getConfig(), id);
pwHistory = new PasswordHistory(userManager.getConfig());
}
+
Review Comment:
additional whitespace unrelated to the proposed improvement. please remove
##########
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 = "Administrators groups",
+ description = "List of groups whose members have admin rights.",
Review Comment:
nope.... has nothing to do with 'admin rights'. this is about impersonation and the description should state so.
also i would not limit this to groups... see below.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserImpl.java:
##########
@@ -79,9 +76,18 @@ public Principal getPrincipal() throws RepositoryException {
}
//---------------------------------------------------------------< User >---
+
+ /**
+ * The user is considered admin if it is the user with the id {@link UserConstants#DEFAULT_ADMIN_ID} or a member of
+ * a group configured as an administrators group using the config id
+ * {@link UserConstants#ADMINISTRATOR_GROUPS_CONFIG_ID}.
+ *
+ * @return true if the user has the id "admin" or a member of a configured administrators group.
+ */
@Override
public boolean isAdmin() {
- return isAdmin;
+ return UserUtil.isAdmin(getUserManager().getConfig(), getID())
+ || UserUtil.isMemberOfAnAdministratorGroup(this, getUserManager().getConfig());
Review Comment:
this is a change in API contract of the User.isAdmin definition which I don't like as it may have undesired consequences outside of the task at hand, which is to allow for configured principals to be able to impersonate any other user.
this method is public API and used in many places and i don't think changing it in the context of "Give admin impersonation rights to members of administrator groups" is justified.
also: your UserUtil.isMemberofAnAdministratorsGroup is not truely evaluating group membership. this is quite misleading....
##########
oak-core/src/test/java/org/apache/jackrabbit/oak/security/authentication/token/TokenInfoTest.java:
##########
@@ -204,6 +210,12 @@ public void testRemoveTokenTreeRemovalFails() {
Root r = mock(Root.class);
when(r.getTree(path)).thenReturn(tokenTree);
when(r.getTree(userPath)).thenReturn(root.getTree(userPath));
+ when(r.getTree(NODE_TYPES_PATH)).thenReturn(root.getTree(NODE_TYPES_PATH));
Review Comment:
i don't understand why the additional lines 213-218 are needed.
this seems unrelated to the task at hand, no?
if it is unrelated -> remove
if it is relate this indicates imho that the patch is changing too many things for the task at hand (all-impersonation allowed for certain principals) -> in this case i would the patch to be refactored such that it is not needed.
##########
oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/UserManagerImplTest.java:
##########
@@ -91,7 +91,7 @@ public void after() throws Exception {
}
private UserManagerImpl createUserManager(@NotNull Root root, @NotNull PartialValueFactory pvf) {
- return new UserManagerImpl(root, pvf, getSecurityProvider(), UserMonitor.NOOP, mock(DynamicMembershipService.class));
+ return new UserManagerImpl(root, pvf, getSecurityProvider(), UserMonitor.NOOP, new DynamicMembershipTracker());
Review Comment:
how is that related to the task at hand? not sure i get it.... IMHO this should not be needed.
##########
oak-security-spi/src/main/java/org/apache/jackrabbit/oak/spi/security/user/util/UserUtil.java:
##########
@@ -39,13 +48,43 @@
*/
public final class UserUtil implements UserConstants {
+ private static final Logger log = LoggerFactory.getLogger(UserUtil.class);
Review Comment:
see comments below -> will no longer be needed
##########
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:
imho this is not needed
##########
oak-security-spi/src/main/java/org/apache/jackrabbit/oak/spi/security/user/util/UserUtil.java:
##########
@@ -39,13 +48,43 @@
*/
public final class UserUtil implements UserConstants {
+ private static final Logger log = LoggerFactory.getLogger(UserUtil.class);
+
private UserUtil() {
}
public static boolean isAdmin(@NotNull ConfigurationParameters parameters, @NotNull String userId) {
return getAdminId(parameters).equals(userId);
}
+ public static boolean isMemberOfAnAdministratorGroup(@NotNull Authorizable authorizable, @NotNull ConfigurationParameters parameters) {
Review Comment:
i am not convinced that this method should be part of the public API because it is only evaluating an configuration option related to impersonation, which can be kept internally.... the only place where the configuration option needs to be evaluated is in "ImpersonationImpl" -> move it there or in the non-exported user-utility class in org.apache.jackrabbit.oak.security.user (in oak-core)
so, no need to change the public API and the package version.
also, and this is the most important piece:
the reason why ImpersonationImpl today does NOT support impersonation being granted for groups today is the fact that I don't want group-membership to be evaluated in the 'Impersonation.allows' method. that was the reason for the comment you removed :). because that neither scales nor performs.
So, instead this method should not be limited to any kind of group operation but instead, I would like to have a simple principal-name comparison as follows:
while looping over the set of principal in the subject passed to 'Impersonation.allows', simply test if any of the principal-name matches any of the values present in the configuration. done.
this means:
- not limiting it to group principals
- not making any attempt to evaluate group membership
as long as the principal is present in the subject we are good an no costy evaluation is need.
if you believe limiting this to group principals would be sensible we can have a simple check for matching name + principal being an instanceof GroupPrincipal.
long story short:
move it to oak-core and simplify
##########
oak-security-spi/src/main/java/org/apache/jackrabbit/oak/spi/security/user/util/UserUtil.java:
##########
@@ -39,13 +48,43 @@
*/
public final class UserUtil implements UserConstants {
+ private static final Logger log = LoggerFactory.getLogger(UserUtil.class);
+
private UserUtil() {
}
public static boolean isAdmin(@NotNull ConfigurationParameters parameters, @NotNull String userId) {
return getAdminId(parameters).equals(userId);
}
+ public static boolean isMemberOfAnAdministratorGroup(@NotNull Authorizable authorizable, @NotNull ConfigurationParameters parameters) {
+ String[] configuredAdministratorGroups = parameters.getConfigValue(ADMINISTRATOR_GROUPS_CONFIG_ID, new String[]{
+ UserConstants.DEFAULT_ADMINISTRATORS_GROUP
+ });
+ @NotNull Set<String> groupIds = getGroupIds(authorizable);
+ return Arrays.stream(configuredAdministratorGroups).anyMatch(groupIds::contains);
+ }
+
+ /**
+ * Retrieves the group ids of the groups this user is a member of.
+ *
+ * @return a set of group ids
+ */
+ private static @NotNull Set<String> getGroupIds(@NotNull Authorizable authorizable) {
+ Set<String> groupIds = new HashSet<>();
+ try {
+ @NotNull Iterator<Group> groups = authorizable.declaredMemberOf();
Review Comment:
if you wanted to truely evaluate group membership (which is not needed imho), you would need to call 'memberOf()' which also includes inherited group membership.
just evaluating declared membership is most likely not what you intended..... but as i said that method should not be needed to start with.
##########
oak-security-spi/src/test/java/org/apache/jackrabbit/oak/spi/security/user/util/UserUtilTest.java:
##########
@@ -246,7 +275,7 @@ public void testGetAuthorizableRootPathNullType() {
}
- @Test(expected = NullPointerException.class)
+ @Test(expected = IllegalArgumentException.class)
Review Comment:
why is this change needed?
##########
oak-security-spi/src/main/java/org/apache/jackrabbit/oak/spi/security/user/util/UserUtil.java:
##########
@@ -39,13 +48,43 @@
*/
public final class UserUtil implements UserConstants {
+ private static final Logger log = LoggerFactory.getLogger(UserUtil.class);
+
private UserUtil() {
}
public static boolean isAdmin(@NotNull ConfigurationParameters parameters, @NotNull String userId) {
return getAdminId(parameters).equals(userId);
}
+ public static boolean isMemberOfAnAdministratorGroup(@NotNull Authorizable authorizable, @NotNull ConfigurationParameters parameters) {
+ String[] configuredAdministratorGroups = parameters.getConfigValue(ADMINISTRATOR_GROUPS_CONFIG_ID, new String[]{
+ UserConstants.DEFAULT_ADMINISTRATORS_GROUP
+ });
+ @NotNull Set<String> groupIds = getGroupIds(authorizable);
+ return Arrays.stream(configuredAdministratorGroups).anyMatch(groupIds::contains);
+ }
+
+ /**
+ * Retrieves the group ids of the groups this user is a member of.
+ *
+ * @return a set of group ids
+ */
+ private static @NotNull Set<String> getGroupIds(@NotNull Authorizable authorizable) {
Review Comment:
see above. this is potentially an expensive operation. see above on how i would envision this impersonation-for-administrators to work. this method is not needed imho
##########
oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/UserAuthenticationTest.java:
##########
@@ -187,6 +189,29 @@ public void testAuthenticateImpersonationCredentials2() throws Exception {
assertTrue(authentication.authenticate(new ImpersonationCredentials(sc, mockAuthInfo(userId))));
}
+ @Test
+ public void testImpersonationByAdministrator() throws Exception {
Review Comment:
once you have refactored the patch, please add a bit of testing to ImpersonationImplTest that essentially verifies that group membership is not being evaluated during thee 'impersonation.allows' call :)
something like:
- get the Impersonation object from user
- call allows(Subject) with a subject that you have created
> test1: configured-can-impersonate-all principal name not contained in the principal-set => not allowed
> test2: configured-can-impersonate-all principal name is contained in the principal set of the subject => must be allowed.
> more tests depending on th impl: either verify that type of principal does not matter.... or if you prefer that this option is limited to GroupPrincipals -> add a test that verifies that the config option is being ignored if no group-principal with the configured name is present
that's how i believe it should work :)
--
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
[GitHub] [jackrabbit-oak] antoniu98 commented on a diff in pull request #862: Give admin impersonation rights to members of administrator groups
Posted by "antoniu98 (via GitHub)" <gi...@apache.org>.
antoniu98 commented on code in PR #862:
URL: https://github.com/apache/jackrabbit-oak/pull/862#discussion_r1170242265
##########
oak-security-spi/src/test/java/org/apache/jackrabbit/oak/spi/security/user/util/UserUtilTest.java:
##########
@@ -246,7 +275,7 @@ public void testGetAuthorizableRootPathNullType() {
}
- @Test(expected = NullPointerException.class)
+ @Test(expected = IllegalArgumentException.class)
Review Comment:
the method was updated in 2018 and got a @NotNull param annotation added
--
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
[GitHub] [jackrabbit-oak] antoniu98 commented on a diff in pull request #862: Give admin impersonation rights to members of administrator groups
Posted by "antoniu98 (via GitHub)" <gi...@apache.org>.
antoniu98 commented on code in PR #862:
URL: https://github.com/apache/jackrabbit-oak/pull/862#discussion_r1171194858
##########
oak-security-spi/src/main/java/org/apache/jackrabbit/oak/spi/security/user/util/package-info.java:
##########
@@ -14,7 +14,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-@Version("1.2.1")
+@Version("1.3.0")
Review Comment:
reverted
--
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
[GitHub] [jackrabbit-oak] anchela commented on pull request #862: Give admin impersonation rights to members of administrator groups
Posted by "anchela (via GitHub)" <gi...@apache.org>.
anchela commented on PR #862:
URL: https://github.com/apache/jackrabbit-oak/pull/862#issuecomment-1454759466
hi @antoniu98, we need a JIRA ticket in https://issues.apache.org/jira/projects/OAK/issues for this PR. can you please create one (and request access to JIRA in case you don't have that yet)?
all changes made need to be backed with a ticket in order to be able to produce the release notes. thanks.
in the mean time i am going to start with the review....
--
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
[GitHub] [jackrabbit-oak] antoniu98 commented on a diff in pull request #862: OAK-10173 : Allow members of configured groups to impersonate any user
Posted by "antoniu98 (via GitHub)" <gi...@apache.org>.
antoniu98 commented on code in PR #862:
URL: https://github.com/apache/jackrabbit-oak/pull/862#discussion_r1183440243
##########
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:
I like the edge case that your proposal is also covering, might be something wanted but nobody thought of.
I believe the number of groups somebody is member of is also small most of the times, so it comes up to what is more expensive:
a)"memberOf" + looping through list of groups a principal is member of
b) getting groups from the config and comparing them with the principal
I believe b) is faster.
_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.
--
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
[GitHub] [jackrabbit-oak] antoniu98 commented on a diff in pull request #862: OAK-10173 : Allow members of configured groups to impersonate any user
Posted by "antoniu98 (via GitHub)" <gi...@apache.org>.
antoniu98 commented on code in PR #862:
URL: https://github.com/apache/jackrabbit-oak/pull/862#discussion_r1183404798
##########
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:
The "isAdmin" check excludes group principals -> I was thinking of not including those group principals in the "isImpersonator" evaluation either
--
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
[GitHub] [jackrabbit-oak] antoniu98 commented on a diff in pull request #862: Give admin impersonation rights to members of administrator groups
Posted by "antoniu98 (via GitHub)" <gi...@apache.org>.
antoniu98 commented on code in PR #862:
URL: https://github.com/apache/jackrabbit-oak/pull/862#discussion_r1170242265
##########
oak-security-spi/src/test/java/org/apache/jackrabbit/oak/spi/security/user/util/UserUtilTest.java:
##########
@@ -246,7 +275,7 @@ public void testGetAuthorizableRootPathNullType() {
}
- @Test(expected = NullPointerException.class)
+ @Test(expected = IllegalArgumentException.class)
Review Comment:
the method was updated in 2018 and got a @NotNull param annotation added
--
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
[GitHub] [jackrabbit-oak] antoniu98 commented on a diff in pull request #862: OAK-10173 : Allow members of configured groups to impersonate any user
Posted by "antoniu98 (via GitHub)" <gi...@apache.org>.
antoniu98 commented on code in PR #862:
URL: https://github.com/apache/jackrabbit-oak/pull/862#discussion_r1183445304
##########
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:
Initially I wanted to allow only group names to be configured, but I didn't think about this benefit by allowing any principal name 👍 will change
--
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
[GitHub] [jackrabbit-oak] anchela commented on a diff in pull request #862: OAK-10173 : Allow members of configured groups to impersonate any user
Posted by "anchela (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] anchela merged pull request #862: OAK-10173 : Allow members of configured groups to impersonate any user
Posted by "anchela (via GitHub)" <gi...@apache.org>.
anchela merged PR #862:
URL: https://github.com/apache/jackrabbit-oak/pull/862
--
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
[GitHub] [jackrabbit-oak] antoniu98 commented on a diff in pull request #862: Give admin impersonation rights to members of administrator groups
Posted by "antoniu98 (via GitHub)" <gi...@apache.org>.
antoniu98 commented on code in PR #862:
URL: https://github.com/apache/jackrabbit-oak/pull/862#discussion_r1171194509
##########
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:
the additions to UserConstants required a version increase to `org.apache.jackrabbit.oak.spi.security.user` package
--
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
[GitHub] [jackrabbit-oak] anchela commented on a diff in pull request #862: OAK-10173 : Allow members of configured groups to impersonate any user
Posted by "anchela (via GitHub)" <gi...@apache.org>.
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] anchela commented on a diff in pull request #862: OAK-10173 : Allow members of configured groups to impersonate any user
Posted by "anchela (via GitHub)" <gi...@apache.org>.
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] antoniu98 commented on a diff in pull request #862: OAK-10173 : Allow members of configured groups to impersonate any user
Posted by "antoniu98 (via GitHub)" <gi...@apache.org>.
antoniu98 commented on code in PR #862:
URL: https://github.com/apache/jackrabbit-oak/pull/862#discussion_r1189900554
##########
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:
reverted
--
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