Skip to content

Enhance the LDAP attribute store micro-service #252

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

Merged

Conversation

skoranda
Copy link
Contributor

@skoranda skoranda commented Jul 11, 2019

A number of commits to enhance the LDAP attribute store. See the commit message for each commit for details.

All Submissions:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you added an explanation of what problem you are trying to solve with this PR?
  • Have you added information on what your changes do and why you chose this as your solution?
  • Have you written new tests for your changes?
  • Does your submission pass tests?
  • This project follows PEP8 style guide. Have you run your code against the 'flake8' linter?

@peppelinux
Copy link
Member

As reference to RESTARTABLE/REUSABLE client strategy

cannatag/ldap3#221

@skoranda
Copy link
Contributor Author

Hi @peppelinux. I have no problem with making the RESTARTABLE client strategy possible (your PR was already accepted), and I am also fine with making it the default for a later release, but since it would substantially change the behavior for an existing deployment it needs to be a fully documented change. In my opinion it should be considered a breaking change.

In my testing the REUSABLE client strategy is substantially faster and necessary for SATOSA deployments under significant load (tens of SAML flows per second or more).

@peppelinux
Copy link
Member

peppelinux commented Jul 23, 2019

Really thanks Scott but I do not need It, I can use both now and the must important thing Is to have parameters instead of constants. Really appreciate Indeed.

I use this New Born client to do massive tests with differents configurations on one or many LDAP servers:
https://github.com/peppelinux/pyMultiLDAP/blob/master/README.md

I Just went deeper in ldap3 API and I found a very good sharing time with you in this thread. I can't ask more!

@skoranda skoranda changed the title Applied flake8 to ldap_attribute_store.py LDAP attribute store Sep 12, 2019
@c00kiemon5ter c00kiemon5ter force-pushed the ldap_attribute_store_enhancement_01 branch from 37ae861 to 7da1e8d Compare September 13, 2019 22:00
Copy link
Member

@peppelinux peppelinux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo fix

@skoranda
Copy link
Contributor Author

All tests pass locally with Python 3.7. Something transient happened with Python 3.6 and Travis.

@c00kiemon5ter c00kiemon5ter dismissed their stale review September 17, 2019 22:14

The mapping to internal attributes may include the ldap-options.

@c00kiemon5ter c00kiemon5ter changed the title LDAP attribute store Enhance the LDAP attribute store micro-service Sep 17, 2019
skoranda and others added 16 commits September 18, 2019 01:20
Applied flake8 to ldap_attribute_store.py since it was written before
the project adopted flake8. No functional changes in this commit.
…utes

The config option search_return_attributes for the LDAP attribute store
conflated what attribute values to return from the LDAP query with how
those values should be mapped to internal attributes. This commit
separates the functionality by introducing two new config options,
query_return_attributes and ldap_to_internal_map. The
search_return_attributes option is still supported for backwards
compatibility.
Added logic so that the LDAP attribute store will add the found record
to the context so that microservices that are called later can use it
if so desired.
Revert to using ldap3.REUSABLE as the default client strategy and fix
configuration to allow setting the client strategy.
A Python False is not an acceptable value for the auto_bind argument to
the ldap3.Connection object. This commit sets the default value to a module
defined constant that makes the most sense when trying to preserve
the REUSABLE strategy as the default (for now), and allows full
configuration by defining a mapping between configuration string values
and the ldap3 module constants, as is done for the client_strategy.
Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>
Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>
Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>
Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>
Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>
Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>
Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>
Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>
Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>
Better consumption of the attributes from the returned record to take
into account handling of attributes from LDAP that include attribute
options. Also included new option to determine whether attributes
resolved from LDAP should overwrite existing internal attributes, the
default, or be merged.
Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>
@c00kiemon5ter c00kiemon5ter force-pushed the ldap_attribute_store_enhancement_01 branch from 0a2f4d7 to 83a3230 Compare September 17, 2019 22:21
@c00kiemon5ter c00kiemon5ter merged commit 2000fc5 into IdentityPython:master Sep 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants