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 Auth Support #99

Closed
wants to merge 1 commit into from
Closed

LDAP Auth Support #99

wants to merge 1 commit into from

Conversation

jasonrm
Copy link

@jasonrm jasonrm commented Aug 7, 2014

So I wrote this a couple of months back, but didn't do a pull request because it felt like a bit of an ugly hack.

Well, it's still an ugly hack, but since I saw another request for LDAP support in #84, I figured I might as well make it a little more public. I wish I could spend more time cleaning this up, but unfortunately I really am lacking the time, so no hard feelings if you don't want to merge/support this. It works well enough for our needs right now so it's hard to justify spending any more work time on this.

Part of the changes were to the authentication and storage auth flow. I moved authentication/authorization related things out of lib/config and into lib/auth, lib/auth-local, and lib/auth-ldap. Most of the authorization functions are now in lib/access-control-list, which is used by lib/authenticated-user.

The recent addition of htpasswd support does conflict a little as it will create a user even if it fails to login to the LDAP server if both ldap and local backends are enabled and users_file is set. I moved the htpasswd related functions into auth-local, although now that I think about it, auth-htpasswd might have been better.

@rlidwka
Copy link
Owner

rlidwka commented Aug 7, 2014

Ohh... I so much don't want to maintain this...

Is it possible to add some kind of a plugin interface instead?

@jasonrm
Copy link
Author

jasonrm commented Aug 7, 2014

Well, the LDAP specific bits are minor, but the changes to make auth more modular is the much larger set of changes. However, with a few more changes, I think it would be possible to take the actual LDAP bits out and make just that a separate module.

@kfatehi
Copy link
Contributor

kfatehi commented Sep 1, 2014

+1 for a plugin interface. +2 for a plugin interface and auth plugin type template w/ LDAP as functional first example :)

@jasonrm are you looking into doing such a thing?

@rlidwka
Copy link
Owner

rlidwka commented Sep 3, 2014

I can't merge this directly, causes too many tests to fail. Mostly because of how unauthorized users are supported currently.

But I'm working to either merge some of those changes, or rewrite existing code along those lines, so keeping it open for the reference.

@rlidwka
Copy link
Owner

rlidwka commented Sep 3, 2014

@keyvanfatehi ,

+2 for a plugin interface and auth plugin type template w/ LDAP as functional first example :)

I think functional first example will be this one:
https://github.com/rlidwka/sinopia-htpasswd

Code that understands it is in auth branch here. Not quite ready yet.

If anyone is willing to support ldap plugin, I could help with that (the code is right here, needs a tiny bit of changing and a bunch of documentation probably).

I'd welcome suggestions about the api. Usergroups added, but maybe there are other things I missed.

@rlidwka
Copy link
Owner

rlidwka commented Sep 17, 2014

I created a ldap plugin based on this PR:
https://github.com/rlidwka/sinopia-ldap

I have no idea whether it works or not, and I don't use ldap, so I can't really test it.

Basically, if you are interested in having ldap authentication, you can take over its development (or just write your own from scratch). Let me know if I can help with this.

Closing this PR, since it has too much tests failed. I might return to it if we decide to refactor config system (it's pretty terrible right now).

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