Skip to content
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

Add option search_filter to ldap #396

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

shaardie
Copy link
Contributor

@shaardie shaardie commented Dec 11, 2021

This patch adds the option to override the search_filter in ldap with an own
complex search_filter, because sometimes a single simple argument is not
sufficient.

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?

This patch adds the option to override the search_filter in ldap with an own
complex search_filter, because sometimes a single simple argument is not
sufficient.
@skoranda
Copy link
Contributor

skoranda commented Feb 22, 2022

I tested this PR against v8.0.1 and found no issues. Thank you for this nice commit.
I have two simple requests for changes:

  1. There is a typo, can you please change 'komplex' to 'complex'.
  2. In the example configuration, can you add a simple example, like
      # Override the contructed search_filter with ldap_identifier_attribute
      # with an own filter. This allows more complex queries.
      # {0} will be injected with the ordered_identifier_candidates.
      # For example:
      # search_filter: "(&(uid={0})(isMemberOf=authorized))"

@shaardie
Copy link
Contributor Author

shaardie commented Feb 22, 2022

I updated the example as requested.

@shaardie
Copy link
Contributor Author

Any update here? This PR is approved for quite some time now.

@skoranda
Copy link
Contributor

@shaardie I talked with the group about this PR and also the other PRs you submitted around the LDAP attribute microservice and after new tests have been added to the test harness for the new functionality they will be merged. You and I corresponded before about adding tests and you indicated you were not going to do it. I have begun the work of adding tests using the ldap3 mocking functionality (see https://ldap3.readthedocs.io/en/latest/mocking.html) but it is not a high priority for me and I only have an hour every couple of weeks to work on it.

If you change your mind and want to contribute to developing the tests please let me know and we can try to make faster progress on getting this PR merged. Thanks.

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.

3 participants