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

Revert #199 to fix group membership lookups #234

Open
wants to merge 1 commit into
base: master
from

Conversation

@davidjb
Copy link
Contributor

davidjb commented Mar 23, 2020

This reverts commit bf64cf2, reversing changes made to f022103.

The PR in #199 contained changes that weren't right - breaking the behaviour of LDAP setups when group_attribute_is_dn on is enabled (eg what this line of code bf64cf2#diff-c05c0daefb48996cbf510b81002b49bcR2230 is conditionally targeting). The PR in effect changes the subsequent LDAP query (eg user_val) from correctly looking up the user's DN as a group attribute in LDAP to querying for groups with a group_attribute equalling the group's DN (eg ctx->dn is the group's dn at this point, see here). This isn't right and won't work, hence the breaking change.

This PR reverts the previous change to make this work correctly again.

Fwiw, the originally-referenced issue #180 in PR #199 seems to be a completely different issue, relating to escaping and parentheses so I'm not sure what/how the original author was trying to fix.

This reverts commit bf64cf2, reversing
changes made to f022103.

This change isn't right -- it an LDAP setup when `group_attribute_is_dn
on` is enabled, which is what this section of code
(bf64cf2#diff-c05c0daefb48996cbf510b81002b49bcR2230)
is conditionally targeting.  This original PR #199 changed the underlying
LDAP query (eg `user_val`) from looking up the user's DN as a group
attribute in LDAP (eg set via the `group_attribute` directive in nginx)
to looking up the _group's_ DN, which isn't right and won't work.

This PR reverts the previous change to make this work correctly again.

Fwiw, the originally-referenced issue #180 seems to be a completely
different issue, relating to escaping and parentheses.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

1 participant
You can’t perform that action at this time.