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

Added new config params for using NTLM based authentication ( ie: windows ldap server) #14

Closed
wants to merge 5 commits into from

Conversation

oneklc
Copy link

@oneklc oneklc commented Aug 17, 2016

Was easy to modify to add NTLM support.
I did not test previous settings to see if they still work.
The changes made are just working for me.
Feel free to reject the pull request.
Thanks for maintaining the ldapauthenticator.

@willingc
Copy link
Contributor

willingc commented Aug 17, 2016

Thanks for the PR @oneklc.

I'm going to defer to @yuvipanda for a formal review and decision to include NTLM since Yuvi has been most involved in this authenticator.

A few general thoughts:

  • NTLM: Perhaps it makes more sense for the default to be set to False for NTLM and have those interested in this authentication to opt in
  • setup.py: I don't believe that these changes are needed

@oneklc
Copy link
Author

oneklc commented Aug 17, 2016

Agreed regarding NTLM defaulting to false and
setup.py changes are totally junkable.

@yuvipanda
Copy link
Collaborator

Hello!

I'm inclined to merge #12 first, since that seems to be required in some ways for proper AD support.

@yuvipanda
Copy link
Collaborator

Hello! I'm going to discard this for right now, since a bunch of other related PRs were merged. Feel free to re-open if they are missing some feature this needs!

Thanks for the patch :)

@yuvipanda yuvipanda closed this Nov 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants