-
Notifications
You must be signed in to change notification settings - Fork 17
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
Adds an option to enable sAMAccountname logins when upndomain is set #146
Adds an option to enable sAMAccountname logins when upndomain is set #146
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps some unit tests?
A unit test is a great idea! I've attempted adding a test based on success-with-anon-bind-upn-domain by adding the |
I've added a few unit tests that sets Ideally I would have wanted to test the ldap filter by passing |
I did some digging and I agree that gldap needs some changes. I'm willing to accept this as is for now and then write a proper unit test in a future PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment: we need better unit tests in a future PR
Active Directory allows LDAP binds as
userprincipalname@upndomain
as well assamaccountname@updomain
.With the current LDAP filter
samaccountname
logins fail when theupndomain
configuration parameter is set, since the filter only only checks foruserprincipalname=username@updomain
.This PR provides an
enable_samaccountname_login
option that can be set in the LDAP Authentication method. This will cause the LDAP user search filter to match eitheruserprincipalname
orsamaccountname
attributes instead of just theuserprincipalname
when theupndomain
configuration parameter is set.