Skip to content

Compare AD DNs case-insensitively when checking group membership #82

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
merged 1 commit into from
May 14, 2015

Conversation

jatoben
Copy link
Contributor

@jatoben jatoben commented May 8, 2015

Active Directory is case-insensitive but case-preserving for most attributes, including DNs.

When binding, the DN will be returned in the entry with the same case as you originally provided, regardless of how the DN is actually stored. But because the Active Directory group membership validator uses a case-sensitive comparison, it excludes the user from groups they should belong to:

domain.user?('user1').dn
=> "CN=User 1,CN=Users,DC=example,DC=com"

entry = strategy.domain('cn=user 1,cn=users,dc=example,dc=com').bind
entry.dn
=> "cn=user 1,cn=users,dc=example,dc=com"

groups = domain.groups(['all-users'])
groups.first.member
=> ["CN=User 1,CN=Users,DC=example,DC=com"]

validator = strategy.membership_validator.new(strategy, groups)
validator.perform(entry)
=> false

This PR makes the AD membership validator case-insensitive when comparing DNs.

/cc @github/enterprise-support @github/ldap @mtodd @shayfrendt

@mtodd
Copy link
Member

mtodd commented May 12, 2015

👍 diff looks good. Will get this merged in this week.

@shayfrendt
Copy link

Yup, looks good to me as well. 🚀

mtodd added a commit that referenced this pull request May 14, 2015
Compare AD DNs case-insensitively when checking group membership
@mtodd mtodd merged commit 157fcfa into master May 14, 2015
@mtodd mtodd deleted the case-insensitive-dn-compare branch May 14, 2015 18:31
@mtodd mtodd mentioned this pull request May 14, 2015
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