New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NIFI-4059: Introduce LdapUserGroupProvider #1923

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@mcgilman
Contributor

mcgilman commented Jun 16, 2017

NIFI-4059:

  • Introducing the LdapUserGroupProvider.
  • Updating documentation accordingly.
  • Moving the IdentityMapping utilities so they were accessible.
NIFI-4059:
- Introducing the LdapUserGroupProvider.
- Updating documentation accordingly.
- Moving the IdentityMapping utilities so they were accessible.
@pvillard31

This comment has been minimized.

Show comment
Hide comment
@pvillard31

pvillard31 Jun 17, 2017

Contributor

Reviewing - first remark:

[WARNING] src/main/java/org/apache/nifi/ldap/tenants/LdapUserGroupProvider.java[586] (sizes) LineLength: Line is longer than 200 characters (found 202).

;)

Contributor

pvillard31 commented Jun 17, 2017

Reviewing - first remark:

[WARNING] src/main/java/org/apache/nifi/ldap/tenants/LdapUserGroupProvider.java[586] (sizes) LineLength: Line is longer than 200 characters (found 202).

;)

@pvillard31

This comment has been minimized.

Show comment
Hide comment
@pvillard31

pvillard31 Jun 17, 2017

Contributor

Hey @mcgilman, I played a bit with it and that's really great. It's going to be super useful. It also gave me the occasion to have a look at the Managed Authorizer stuff - it's neat!

Tested using Apache Directory Studio and tried multiple scenarios: users first, groups first, both users and groups. All is working as expected. I've just one remark: when using users only search, but also setting the group name attribute, the full DN of the group is used. Would be nice to also take into account the group name attribute in that case (Note: the other way is working - if searching for groups and defining the user name attribute, we don't have the full DN for users).

Overall LGTM.

Contributor

pvillard31 commented Jun 17, 2017

Hey @mcgilman, I played a bit with it and that's really great. It's going to be super useful. It also gave me the occasion to have a look at the Managed Authorizer stuff - it's neat!

Tested using Apache Directory Studio and tried multiple scenarios: users first, groups first, both users and groups. All is working as expected. I've just one remark: when using users only search, but also setting the group name attribute, the full DN of the group is used. Would be nice to also take into account the group name attribute in that case (Note: the other way is working - if searching for groups and defining the user name attribute, we don't have the full DN for users).

Overall LGTM.

NIFI-4059:
- Fixing contrib check issues.
- Fixing typo in admin guide.
@mcgilman

This comment has been minimized.

Show comment
Hide comment
@mcgilman

mcgilman Jun 19, 2017

Contributor

Thanks @pvillard31 for having a look at this PR! I've addressed the two issues above and I think resolving the group name when searching users only and detecting group membership is supported. Check out this unit test here [1]. Please let me know if I misunderstood. Thanks again!

[1] https://github.com/mcgilman/nifi/blob/4dd7aaae8de2ea2e2000510e5501f6e6b71d7f4b/nifi-nar-bundles/nifi-ldap-iaa-providers-bundle/nifi-ldap-iaa-providers/src/test/java/org/apache/nifi/ldap/tenants/LdapUserGroupProviderTest.java#L213

Contributor

mcgilman commented Jun 19, 2017

Thanks @pvillard31 for having a look at this PR! I've addressed the two issues above and I think resolving the group name when searching users only and detecting group membership is supported. Check out this unit test here [1]. Please let me know if I misunderstood. Thanks again!

[1] https://github.com/mcgilman/nifi/blob/4dd7aaae8de2ea2e2000510e5501f6e6b71d7f4b/nifi-nar-bundles/nifi-ldap-iaa-providers-bundle/nifi-ldap-iaa-providers/src/test/java/org/apache/nifi/ldap/tenants/LdapUserGroupProviderTest.java#L213

@pvillard31

This comment has been minimized.

Show comment
Hide comment
@pvillard31

pvillard31 Jun 19, 2017

Contributor

Hey @mcgilman, just tried again and it's working... I guess I made a typo when I tried the first time. Thanks for the corrections. Will wait for travis build and will merge by eod. Thanks!

Contributor

pvillard31 commented Jun 19, 2017

Hey @mcgilman, just tried again and it's working... I guess I made a typo when I tried the first time. Thanks for the corrections. Will wait for travis build and will merge by eod. Thanks!

@asfgit asfgit closed this in 6bc6f95 Jun 19, 2017

@pvillard31

This comment has been minimized.

Show comment
Hide comment
@pvillard31

pvillard31 Jun 19, 2017

Contributor

+1, squashed and merged, thanks!

Contributor

pvillard31 commented Jun 19, 2017

+1, squashed and merged, thanks!

peter-gergely-horvath added a commit to peter-gergely-horvath/nifi that referenced this pull request Jun 28, 2017

NIFI-4059:
- Introducing the LdapUserGroupProvider.
- Updating documentation accordingly.
- Moving the IdentityMapping utilities so they were accessible.

Signed-off-by: Pierre Villard <pierre.villard.fr@gmail.com>

This closes apache#1923.

peter-gergely-horvath added a commit to peter-gergely-horvath/nifi that referenced this pull request Jun 28, 2017

NIFI-4059:
- Introducing the LdapUserGroupProvider.
- Updating documentation accordingly.
- Moving the IdentityMapping utilities so they were accessible.

Signed-off-by: Pierre Villard <pierre.villard.fr@gmail.com>

This closes apache#1923.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment