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

LDAP group membership #45347

Merged
merged 3 commits into from Jan 31, 2018

Conversation

Projects
None yet
4 participants
@amendlik
Copy link
Contributor

commented Jan 9, 2018

What does this PR do?

Fix a regression in the LDAP auth provider

What issues does this PR fix or reference?

The changes made in #42280 and #42426 impacted the code for determining group membership. This change allows an authenticated LDAP bind when getting group members.

Tests written?

No

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@rallytime rallytime requested a review from DmitryKuzmenko Jan 14, 2018

@DmitryKuzmenko
Copy link
Contributor

left a comment

Looks good at all except the requested minor change. After the change I'm OK as long as all the tests pass.

if _config('binddn', mandatory=False) and _config('bindpw', mandatory=False):
bind = _bind_for_search(anonymous=_config('anonymous', mandatory=False))
else:
bind = _bind(username, kwargs['password'],

This comment has been minimized.

Copy link
@DmitryKuzmenko

DmitryKuzmenko Jan 15, 2018

Contributor

I'd better like to either use kwargs.get here to avoid potential KeyError raising.

This comment has been minimized.

Copy link
@amendlik

amendlik Jan 17, 2018

Author Contributor

I've made the requested change.

@amendlik amendlik force-pushed the amendlik:ldap-groups branch from 157e210 to c297b95 Jan 17, 2018

@DmitryKuzmenko
Copy link
Contributor

left a comment

LGTM

@rallytime rallytime requested review from cachedout and rallytime Jan 18, 2018

@rallytime rallytime merged commit 9b4b945 into saltstack:develop Jan 31, 2018

4 of 10 checks passed

jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #18978 — ABORTED
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #15441 — ABORTED
Details
codeclimate 1 issue to fix
Details
default Build finished.
Details
jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #1430 — FAILURE
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #5957 — FAILURE
Details
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #21412 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #13809 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #18415 — SUCCESS
Details

@amendlik amendlik deleted the amendlik:ldap-groups branch Mar 3, 2018

amendlik added a commit to amendlik/salt that referenced this pull request Dec 31, 2018

Allow unauthenticated bind for listing LDAP groups
This fix was originally made in PR saltstack#45347, then pulled into another
branch under PR saltstack#45811, but was clobbered by c3f1587. This commit
just restores the fix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.