anchela commented on a change in pull request #471:
URL: https://github.com/apache/jackrabbit-oak/pull/471#discussion_r794561124
##########
File path:
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/AuthorizablePropertiesImpl.java
##########
@@ -234,9 +236,34 @@ private PropertyState getAuthorizableProperty(@NotNull
Tree authorizableTree, @N
}
ReadOnlyNodeTypeManager nodeTypeManager =
authorizable.getUserManager().getNodeTypeManager();
PropertyDefinition def = nodeTypeManager.getDefinition(parent,
property, true);
- if (def.isProtected() || (authorizablePath.equals(parent.getPath())
- &&
!def.getDeclaringNodeType().isNodeType(UserConstants.NT_REP_AUTHORIZABLE))) {
+ if (def.isProtected()) {
+ // exclude all protected properties
return null;
+ } else if (authorizablePath.equals(parent.getPath())) {
+ // non-protected properties on the root must be defined by the
expected
+ // primary type or one of the configured mixin types
+ Boolean allowed = null;
Review comment:
i would change that to
`boolean allowed = false;`
and then set it to `allowed = true` as i don't see any benefit of a 'null'
value.
what was the reason for this?
##########
File path:
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/AuthorizablePropertiesImpl.java
##########
@@ -234,9 +236,34 @@ private PropertyState getAuthorizableProperty(@NotNull
Tree authorizableTree, @N
}
ReadOnlyNodeTypeManager nodeTypeManager =
authorizable.getUserManager().getNodeTypeManager();
PropertyDefinition def = nodeTypeManager.getDefinition(parent,
property, true);
- if (def.isProtected() || (authorizablePath.equals(parent.getPath())
- &&
!def.getDeclaringNodeType().isNodeType(UserConstants.NT_REP_AUTHORIZABLE))) {
+ if (def.isProtected()) {
+ // exclude all protected properties
return null;
+ } else if (authorizablePath.equals(parent.getPath())) {
+ // non-protected properties on the root must be defined by the
expected
+ // primary type or one of the configured mixin types
+ Boolean allowed = null;
+ NodeType declaringNodeType = def.getDeclaringNodeType();
+ if
(declaringNodeType.isNodeType(UserConstants.NT_REP_AUTHORIZABLE)) {
+ // defined by the expected primary type so allowed
+ allowed = Boolean.TRUE;
Review comment:
my understanding of the original improvement request is that allowing
all types of properties is too permissive the the intention is to limit what
properties can be stored below a given user/group node.
see the following sentence from the original report:
> This is in support of a use case where we want a stricter constraint on
what is allowed to be stored in an authorizable property.
given that `rep:Authorizables `allows for any residiual property of any
type, single and multi-valued, this seems to defeat the purpose. my
understanding of the request is that _only_ properties defined by the
configured mixin types should be allow instead of allow any residual property
as it would still be the case here.
as far as i can see this requirement isn't covered by the tests either.
##########
File path:
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/AuthorizablePropertiesImpl.java
##########
@@ -234,9 +236,34 @@ private PropertyState getAuthorizableProperty(@NotNull
Tree authorizableTree, @N
}
ReadOnlyNodeTypeManager nodeTypeManager =
authorizable.getUserManager().getNodeTypeManager();
PropertyDefinition def = nodeTypeManager.getDefinition(parent,
property, true);
- if (def.isProtected() || (authorizablePath.equals(parent.getPath())
- &&
!def.getDeclaringNodeType().isNodeType(UserConstants.NT_REP_AUTHORIZABLE))) {
+ if (def.isProtected()) {
+ // exclude all protected properties
return null;
+ } else if (authorizablePath.equals(parent.getPath())) {
+ // non-protected properties on the root must be defined by the
expected
+ // primary type or one of the configured mixin types
+ Boolean allowed = null;
+ NodeType declaringNodeType = def.getDeclaringNodeType();
+ if
(declaringNodeType.isNodeType(UserConstants.NT_REP_AUTHORIZABLE)) {
+ // defined by the expected primary type so allowed
+ allowed = Boolean.TRUE;
+ } else {
+ // OAK-9675 - not declared by the primary type, so let's check
for a mixin
+ if (declaringNodeType.isMixin()) {
+ // check if the property is defined by any of the
configured mixin type
+ String[] allowedMixins =
authorizable.getUserManager().getConfig().getConfigValue(UserConstants.PARAM_AUTHORIZABLE_PROPERTIES_MIXINS,
null, String[].class);
+ if (allowedMixins != null) {
+ // check if the actual mixin name is one of the
configured items
+ String actualMixinName = declaringNodeType.getName();
+ allowed =
Stream.of(allowedMixins).anyMatch(actualMixinName::equals);
+ }
+ }
+ }
+
+ if (!Boolean.TRUE.equals(allowed)) {
Review comment:
this should just be
`if (!allowed)`
##########
File path:
oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserConfigurationImpl.java
##########
@@ -176,6 +176,13 @@
name = "RFC7613 Username Comparison Profile",
description = "Enable the UsercaseMappedProfile defined in
RFC7613 for username comparison.")
boolean enableRFC7613UsercaseMappedProfile() default false;
+
+ // for OAK-9675
+ @AttributeDefinition(
+ name = "Authorizable Properties Mixin Types",
+ description = "Optional configuration defining the names of
additional " +
Review comment:
the 'additional' strikes me here..... my reading of the improvement
request (as pointed out before) would be to limit authorizable properties to
those defined by the configured mixin types and not allow them 'in addition'.
as by this comment:
`The proposed improvement here is to add an optional configuration property
that would define the names of mixin types that are allowed to define
authorizable properties. Any property definition defined by a mixin type in
this set would be included, and anything else would be excluded as before`.
--
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]
