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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
