exceptionfactory commented on a change in pull request #4681:
URL: https://github.com/apache/nifi/pull/4681#discussion_r528769423
##########
File path:
nifi-nar-bundles/nifi-ldap-iaa-providers-bundle/nifi-ldap-iaa-providers/src/main/java/org/apache/nifi/ldap/tenants/LdapUserGroupProvider.java
##########
@@ -343,7 +347,14 @@ public void onConfigured(final
AuthorizerConfigurationContext configurationConte
if (StringUtils.isNotBlank(userGroupReferencedGroupAttribute) &&
!performGroupSearch) {
throw new AuthorizerCreationException("'Group Search Base' must be
set when specifying 'User Group Name Attribute - Referenced Group Attribute'.");
}
-
+ //ensure that groupSearchBase is set when userGroupsFilterExpression
is specified.
+ if (StringUtils.isNotBlank(userGroupsFilterExpression) &&
StringUtils.isBlank(groupSearchBase)){
+ throw new AuthorizerCreationException("Group Search Base' must be set
when specifying 'User Groups Filter Expression'.");
Review comment:
The message appears to be missing an opening single quote to call out
'Group Search Base'
##########
File path:
nifi-nar-bundles/nifi-ldap-iaa-providers-bundle/nifi-ldap-iaa-providers/src/main/java/org/apache/nifi/ldap/tenants/LdapUserGroupProvider.java
##########
@@ -512,38 +523,66 @@ protected User doMapFromContext(DirContextOperations ctx)
{
// store the user for group member later
userLookup.put(getReferencedUserValue(ctx), user);
- if
(StringUtils.isNotBlank(userGroupNameAttribute)) {
- final Attribute attributeGroups =
ctx.getAttributes().get(userGroupNameAttribute);
+ // we can compute group values from user in two
ways :
+ // 1 - either directly from user entry using
userGroupNameAttribute
+ // 2 - search for each user's groups using
userGroupsFilterExpression
+ //if either userGroupNameAttribute or
userGroupsFilterExpression is not blank, we try to populate the groupValues
list, otherwise no groups are inferred from users.
+ List<String> groupValues = null;
+ if
(StringUtils.isNotBlank(userGroupNameAttribute)) {
+ Attribute attributeGroups = attributeGroups =
ctx.getAttributes().get(userGroupNameAttribute);
if (attributeGroups == null) {
logger.debug(String.format("User group
name attribute [%s] does not exist for %s. This may be due to "
+ "misconfiguration or the user
may just not belong to any groups. Ignoring group membership.",
userGroupNameAttribute, identity));
} else {
try {
- final NamingEnumeration<String>
groupValues = (NamingEnumeration<String>) attributeGroups.getAll();
- while (groupValues.hasMoreElements()) {
- final String groupValue =
groupValues.next();
-
- // if we are performing a group
search, then we need to normalize the group value so that each
- // user associating with it can be
matched. if we are not performing a group search then these
- // values will be used to actually
build the group itself. case sensitivity is for group
- // membership, not group
identification.
- final String groupValueNormalized;
- if (performGroupSearch) {
- groupValueNormalized =
groupMembershipEnforceCaseSensitivity ? groupValue : groupValue.toLowerCase();
- } else {
- groupValueNormalized =
groupValue;
- }
-
- // store the group -> user
identifier mapping... if case sensitivity is disabled, the group reference
value will
- // be lowercased when adding to
groupToUserIdentifierMappings
-
groupToUserIdentifierMappings.computeIfAbsent(groupValueNormalized, g -> new
HashSet<>()).add(user.getIdentifier());
- }
+ NamingEnumeration<String>
groupValuesFromAttribute = (NamingEnumeration<String>) attributeGroups.getAll();
+ groupValues = new LinkedList<String>();
+ while
(groupValuesFromAttribute.hasMoreElements())
Review comment:
Recommend adding brackets to this while loop block instead of using the
shorthand syntax.
##########
File path:
nifi-nar-bundles/nifi-ldap-iaa-providers-bundle/nifi-ldap-iaa-providers/src/main/java/org/apache/nifi/ldap/tenants/LdapUserGroupProvider.java
##########
@@ -512,38 +523,66 @@ protected User doMapFromContext(DirContextOperations ctx)
{
// store the user for group member later
userLookup.put(getReferencedUserValue(ctx), user);
- if
(StringUtils.isNotBlank(userGroupNameAttribute)) {
- final Attribute attributeGroups =
ctx.getAttributes().get(userGroupNameAttribute);
+ // we can compute group values from user in two
ways :
+ // 1 - either directly from user entry using
userGroupNameAttribute
+ // 2 - search for each user's groups using
userGroupsFilterExpression
+ //if either userGroupNameAttribute or
userGroupsFilterExpression is not blank, we try to populate the groupValues
list, otherwise no groups are inferred from users.
+ List<String> groupValues = null;
+ if
(StringUtils.isNotBlank(userGroupNameAttribute)) {
+ Attribute attributeGroups = attributeGroups =
ctx.getAttributes().get(userGroupNameAttribute);
if (attributeGroups == null) {
logger.debug(String.format("User group
name attribute [%s] does not exist for %s. This may be due to "
+ "misconfiguration or the user
may just not belong to any groups. Ignoring group membership.",
userGroupNameAttribute, identity));
} else {
try {
- final NamingEnumeration<String>
groupValues = (NamingEnumeration<String>) attributeGroups.getAll();
- while (groupValues.hasMoreElements()) {
- final String groupValue =
groupValues.next();
-
- // if we are performing a group
search, then we need to normalize the group value so that each
- // user associating with it can be
matched. if we are not performing a group search then these
- // values will be used to actually
build the group itself. case sensitivity is for group
- // membership, not group
identification.
- final String groupValueNormalized;
- if (performGroupSearch) {
- groupValueNormalized =
groupMembershipEnforceCaseSensitivity ? groupValue : groupValue.toLowerCase();
- } else {
- groupValueNormalized =
groupValue;
- }
-
- // store the group -> user
identifier mapping... if case sensitivity is disabled, the group reference
value will
- // be lowercased when adding to
groupToUserIdentifierMappings
-
groupToUserIdentifierMappings.computeIfAbsent(groupValueNormalized, g -> new
HashSet<>()).add(user.getIdentifier());
- }
+ NamingEnumeration<String>
groupValuesFromAttribute = (NamingEnumeration<String>) attributeGroups.getAll();
+ groupValues = new LinkedList<String>();
+ while
(groupValuesFromAttribute.hasMoreElements())
+
groupValues.add(groupValuesFromAttribute.next());
} catch (NamingException e) {
throw new
AuthorizationAccessException("Error while retrieving user group name attribute
[" + userIdentityAttribute + "].");
}
}
}
+ if
(StringUtils.isNotBlank(userGroupsFilterExpression)) {
+ // for each user, we search for groups
according to userGroupsFilterExpression. Nested Ldap filter such as
LDAP_MATCHING_RULE_IN_CHAIN can be used.
+ AndFilter filter = new AndFilter();
+ if
(StringUtils.isNotBlank(userGroupsFilterExpression)) {
+ String currentUserGroupsFilterExpression =
userGroupsFilterExpression.replaceAll("\\{\\}", ctx.getDn().toString());
Review comment:
Recommend promoting the distinguished name placeholder string to a
static class variable for improved code clarity.
##########
File path:
nifi-nar-bundles/nifi-ldap-iaa-providers-bundle/nifi-ldap-iaa-providers/src/main/java/org/apache/nifi/ldap/tenants/LdapUserGroupProvider.java
##########
@@ -343,7 +347,14 @@ public void onConfigured(final
AuthorizerConfigurationContext configurationConte
if (StringUtils.isNotBlank(userGroupReferencedGroupAttribute) &&
!performGroupSearch) {
throw new AuthorizerCreationException("'Group Search Base' must be
set when specifying 'User Group Name Attribute - Referenced Group Attribute'.");
}
-
+ //ensure that groupSearchBase is set when userGroupsFilterExpression
is specified.
+ if (StringUtils.isNotBlank(userGroupsFilterExpression) &&
StringUtils.isBlank(groupSearchBase)){
+ throw new AuthorizerCreationException("Group Search Base' must be set
when specifying 'User Groups Filter Expression'.");
+ }
+ //ensure we are not simultaneously searching groups inside user entry
through userGroupNameAttribute and using userGroupsFilterExpression.
+ if (StringUtils.isNotBlank(userGroupNameAttribute) &&
StringUtils.isNotBlank(userGroupsFilterExpression)){
+ throw new AuthorizerCreationException("'User Group Name
Attribute' and 'User Groups Filter Expression' are both set. Only one may be
used.");
+ }
Review comment:
Recommend running automatic formatting on these lines to normalize
indentation following standard conventions.
##########
File path:
nifi-nar-bundles/nifi-ldap-iaa-providers-bundle/nifi-ldap-iaa-providers/src/test/java/org/apache/nifi/ldap/tenants/LdapUserGroupProviderTest.java
##########
@@ -745,6 +746,44 @@ public void
testSearchUsersAndGroupsMembershipThroughUsersCaseInsensitive() thro
user -> "user6".equals(user.getIdentity()) ||
"user7".equals(user.getIdentity()) ||
"user8".equals(user.getIdentity())).count());
}
+ @Test
+ public void
testSearchUsersAndGroupsMembershipThroughUserGroupsFilterExpression() throws
Exception {
+ final AuthorizerConfigurationContext configurationContext =
getBaseConfiguration(USER_SEARCH_BASE, GROUP_SEARCH_BASE);
+
when(configurationContext.getProperty(PROP_USER_IDENTITY_ATTRIBUTE)).thenReturn(new
StandardPropertyValue("uid", null, ParameterLookup.EMPTY));
+ // using PROP_USER_GROUPS_FILTER_EXPRESSION to search for each user
groups based on its DN. Allows special filters to retrieve nested groups.
+
when(configurationContext.getProperty(PROP_USER_GROUPS_FILTER_EXPRESSION)).thenReturn(new
StandardPropertyValue("member={}", null, ParameterLookup.EMPTY));
+
when(configurationContext.getProperty(PROP_GROUP_NAME_ATTRIBUTE)).thenReturn(new
StandardPropertyValue("cn", null, ParameterLookup.EMPTY));
+
when(configurationContext.getProperty(PROP_GROUP_MEMBERSHIP_ENFORCE_CASE_SENSITIVITY)).thenReturn(new
StandardPropertyValue("false", null, ParameterLookup.EMPTY));
+ ldapUserGroupProvider.onConfigured(configurationContext);
+
+ assertEquals(8, ldapUserGroupProvider.getUsers().size());
+
+ final Set<Group> groups = ldapUserGroupProvider.getGroups();
+ assertEquals(5, groups.size());
+
+ final Group team1 = groups.stream().filter(group ->
"team1".equals(group.getName())).findFirst().orElse(null);
+ assertNotNull(team1);
+ assertEquals(1, team1.getUsers().size()); // team1 members are
inferred from 'member' attribute. Only user1 is there.
+ assertEquals(1, team1.getUsers().stream().map(
+ userIdentifier ->
ldapUserGroupProvider.getUser(userIdentifier)).filter(
+ user -> "user1".equals(user.getIdentity())).count());
+
+ final Group team4 = groups.stream().filter(group ->
"team4".equals(group.getName())).findFirst().orElse(null);
+ assertNotNull(team4);
+ assertEquals(2, team4.getUsers().size()); // team4 members are
inferred from 'member' attribute. Only user1 and user2 are there.
+ assertEquals(2, team4.getUsers().stream().map(
+ userIdentifier ->
ldapUserGroupProvider.getUser(userIdentifier)).filter(
+ user -> "user1".equals(user.getIdentity()) ||
"user2".equals(user.getIdentity()) ).count());
+ }
+
+ @Test(expected = AuthorizerCreationException.class)
+ public void
tesUserGroupsFilterExpressionAndUserGroupNameAttributeBothSpecified() throws
Exception {
Review comment:
When running the unit test in debug mode, this actually appears to hit
the exception on 352 for an empty Group Search Base as opposed to the check for
having both group filter expression and group attribute specified. Recommend
revisiting this unit test and adding another to exercise the other condition.
##########
File path:
nifi-nar-bundles/nifi-ldap-iaa-providers-bundle/nifi-ldap-iaa-providers/src/main/java/org/apache/nifi/ldap/tenants/LdapUserGroupProvider.java
##########
@@ -512,38 +523,66 @@ protected User doMapFromContext(DirContextOperations ctx)
{
// store the user for group member later
userLookup.put(getReferencedUserValue(ctx), user);
- if
(StringUtils.isNotBlank(userGroupNameAttribute)) {
- final Attribute attributeGroups =
ctx.getAttributes().get(userGroupNameAttribute);
+ // we can compute group values from user in two
ways :
+ // 1 - either directly from user entry using
userGroupNameAttribute
+ // 2 - search for each user's groups using
userGroupsFilterExpression
+ //if either userGroupNameAttribute or
userGroupsFilterExpression is not blank, we try to populate the groupValues
list, otherwise no groups are inferred from users.
+ List<String> groupValues = null;
+ if
(StringUtils.isNotBlank(userGroupNameAttribute)) {
+ Attribute attributeGroups = attributeGroups =
ctx.getAttributes().get(userGroupNameAttribute);
if (attributeGroups == null) {
logger.debug(String.format("User group
name attribute [%s] does not exist for %s. This may be due to "
+ "misconfiguration or the user
may just not belong to any groups. Ignoring group membership.",
userGroupNameAttribute, identity));
} else {
try {
- final NamingEnumeration<String>
groupValues = (NamingEnumeration<String>) attributeGroups.getAll();
- while (groupValues.hasMoreElements()) {
- final String groupValue =
groupValues.next();
-
- // if we are performing a group
search, then we need to normalize the group value so that each
- // user associating with it can be
matched. if we are not performing a group search then these
- // values will be used to actually
build the group itself. case sensitivity is for group
- // membership, not group
identification.
- final String groupValueNormalized;
- if (performGroupSearch) {
- groupValueNormalized =
groupMembershipEnforceCaseSensitivity ? groupValue : groupValue.toLowerCase();
- } else {
- groupValueNormalized =
groupValue;
- }
-
- // store the group -> user
identifier mapping... if case sensitivity is disabled, the group reference
value will
- // be lowercased when adding to
groupToUserIdentifierMappings
-
groupToUserIdentifierMappings.computeIfAbsent(groupValueNormalized, g -> new
HashSet<>()).add(user.getIdentifier());
- }
+ NamingEnumeration<String>
groupValuesFromAttribute = (NamingEnumeration<String>) attributeGroups.getAll();
+ groupValues = new LinkedList<String>();
+ while
(groupValuesFromAttribute.hasMoreElements())
+
groupValues.add(groupValuesFromAttribute.next());
} catch (NamingException e) {
throw new
AuthorizationAccessException("Error while retrieving user group name attribute
[" + userIdentityAttribute + "].");
}
}
}
+ if
(StringUtils.isNotBlank(userGroupsFilterExpression)) {
+ // for each user, we search for groups
according to userGroupsFilterExpression. Nested Ldap filter such as
LDAP_MATCHING_RULE_IN_CHAIN can be used.
+ AndFilter filter = new AndFilter();
+ if
(StringUtils.isNotBlank(userGroupsFilterExpression)) {
Review comment:
Is this check necessary? It seems to be duplicative of line 548.
##########
File path:
nifi-nar-bundles/nifi-ldap-iaa-providers-bundle/nifi-ldap-iaa-providers/src/main/java/org/apache/nifi/ldap/tenants/LdapUserGroupProvider.java
##########
@@ -512,38 +523,66 @@ protected User doMapFromContext(DirContextOperations ctx)
{
// store the user for group member later
userLookup.put(getReferencedUserValue(ctx), user);
- if
(StringUtils.isNotBlank(userGroupNameAttribute)) {
- final Attribute attributeGroups =
ctx.getAttributes().get(userGroupNameAttribute);
+ // we can compute group values from user in two
ways :
+ // 1 - either directly from user entry using
userGroupNameAttribute
+ // 2 - search for each user's groups using
userGroupsFilterExpression
+ //if either userGroupNameAttribute or
userGroupsFilterExpression is not blank, we try to populate the groupValues
list, otherwise no groups are inferred from users.
+ List<String> groupValues = null;
+ if
(StringUtils.isNotBlank(userGroupNameAttribute)) {
+ Attribute attributeGroups = attributeGroups =
ctx.getAttributes().get(userGroupNameAttribute);
if (attributeGroups == null) {
logger.debug(String.format("User group
name attribute [%s] does not exist for %s. This may be due to "
+ "misconfiguration or the user
may just not belong to any groups. Ignoring group membership.",
userGroupNameAttribute, identity));
} else {
try {
- final NamingEnumeration<String>
groupValues = (NamingEnumeration<String>) attributeGroups.getAll();
- while (groupValues.hasMoreElements()) {
- final String groupValue =
groupValues.next();
-
- // if we are performing a group
search, then we need to normalize the group value so that each
- // user associating with it can be
matched. if we are not performing a group search then these
- // values will be used to actually
build the group itself. case sensitivity is for group
- // membership, not group
identification.
- final String groupValueNormalized;
- if (performGroupSearch) {
- groupValueNormalized =
groupMembershipEnforceCaseSensitivity ? groupValue : groupValue.toLowerCase();
- } else {
- groupValueNormalized =
groupValue;
- }
-
- // store the group -> user
identifier mapping... if case sensitivity is disabled, the group reference
value will
- // be lowercased when adding to
groupToUserIdentifierMappings
-
groupToUserIdentifierMappings.computeIfAbsent(groupValueNormalized, g -> new
HashSet<>()).add(user.getIdentifier());
- }
+ NamingEnumeration<String>
groupValuesFromAttribute = (NamingEnumeration<String>) attributeGroups.getAll();
+ groupValues = new LinkedList<String>();
+ while
(groupValuesFromAttribute.hasMoreElements())
+
groupValues.add(groupValuesFromAttribute.next());
} catch (NamingException e) {
throw new
AuthorizationAccessException("Error while retrieving user group name attribute
[" + userIdentityAttribute + "].");
}
}
}
+ if
(StringUtils.isNotBlank(userGroupsFilterExpression)) {
Review comment:
The property check on line 351 ensures that only one of the two group
value determination paths is followed, but the logic here makes it appear that
both could be followed. Recommend changing this an "else if" for clarity.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
