You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2020/11/23 15:38:15 UTC
[GitHub] [nifi] exceptionfactory commented on a change in pull request #4681: added userGroupsFilterExpression to allow per user LDAP filters
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:
users@infra.apache.org