scragg0x / realms-wiki

Git based wiki inspired by Gollum
http://realms.io
GNU General Public License v2.0
833 stars 90 forks source link

LDAP support #79

Closed larsimmisch closed 8 years ago

larsimmisch commented 8 years ago

As promised, LDAP support.

Feel free to comment the pull request if you want things done differently.

One notable difference between the LDAP backend and the DB backend is that the LDAP backend uses the full user name for git committer information (because it is there already). It might be worth to consider adding this to the DB backend too.

larsimmisch commented 8 years ago

I think I know how to fix the Travis tests. I'll commit a fix when I have one (but I need to get the nosetests working first)

larsimmisch commented 8 years ago

Travis passes again, comments?

scragg0x commented 8 years ago

Thanks for the PR. I have been thinking about it a bit and also @alvinchow86 PR on github auth. I am brainstorming a bit on how to architect this to easily allow an admin to activate mulitple forms of authentication. For example. I may want github and ldap.

Using the search module as an example. I enable the user to choose any kind of search from a single module. Auth is a bit different because I want 1 or more options and there are some frontend implications to consider. I'm basically asking myself if alternative authentication methods could live in the same module and extend some base logic, without getting messy. Local auth would not be the base but probably a default enabled type.

larsimmisch commented 8 years ago

My very first attempt looked a bit like what you're thinking about: I put the LDAP auth code alongside the DB auth code. I ditched that attempt, but mostly because I also wanted to drop the SQLAlchemy dependency when DB auth was not used, and the easiest way was to do it as a separate module. I think it could be done without being messy (a layer of auth plugins?)

In any case, I also like #82 from @alvinchow86. Github auth would be neat. Or Google auth for small shops that run on Google Apps for Work (or whatever it's called).

alvinchow86 commented 8 years ago

Agreed that we should find a clean way to architect this. We should establish what should be common between the different auth backends. One thing we could decide is if we want a user database for the alternative backends - I'm guessing probably not because it's redundant with Github/LDAP/etc.

Fortunately because Flask-login is pretty flexible it was not too hard to integrate.

larsimmisch commented 8 years ago

I've thought about multiple mix and match auth backends some more.

The initial auth modules might be:

The auth backends would differ in capabilities, like:

(anon and github can do neither, local can do both, LDAP can change passwords)

If backends can be mixed and matched, we have to deal with auth backends with differing capabilities and this has to be reflected in the UI.

To start, the login screen would need a dropdown for the backend. Then, if one auth backend allows new user registration, this should be visible in the UI, but it also needs to be clear where the new user is registered. It's a bit easier for change password, that would only be visible at all if the current user's backend allows it.

This is certainly doable, but quite a bit of work.

And then, what are the usecases? And what are their implications? Where does LDAP+github make sense?

I can think of a a smallish shop, where employees have LDAP accounts and they want to whitelist some github users (interns or customers) for collaboration. Local+github would be similar.

But that usecase would also cry for finer grained access rights, at the minimum something like: users from auth backend X can write, and users from auth backend Y can read. Plus whitelisting/blacklisting.

I think this is too little gain for too much work.

Personally, I have picked realms over gollum because gollum has no authentication, so offering authentication was a unique selling point for me. But I don't need anything fancy - a single auth backend will do the trick.

scragg0x commented 8 years ago

Just to give you an update, I have been working on this issue. If I have something that I feel is satisfactory, I will present it in a branch for you guys to see.

larsimmisch commented 8 years ago

Cool, thanks.

scragg0x commented 8 years ago

@larsimmisch I got an auth branch up and ldap to work with a local Zentyal server. I personally used bind by search method. I used this library https://github.com/ContinuumIO/flask-ldap-login which made it quite easy. There are a couple examples here with direct bind and search. https://github.com/ContinuumIO/flask-ldap-login/tree/master/examples You can put an LDAP config var in realms config and it will pass it directly.

I am still sorting out organizing the forms and oauth but it's almost there.

larsimmisch commented 8 years ago

@scragg0x Excellent, thanks.

I like the use of https://github.com/ContinuumIO/flask-ldap-login. That is a better fit than the ad-hoc code I wrote.

LDAP authentication itself seems to be working in my setup (against OpenLDAP), but it looks as if the user isn't stored in the session. The username and/or avatar is never displayed and with "PRIVATE_WIKI": true, I just go into a loop.

I am not terribly keen on the current UI. There is one login form (with email address) for the name, and the LDAP Login button opens a popup. I'd rather have one form (username/email and password) and multiple buttons. But maybe this is something for later.

Also, when I enter an invalid password, I get two errors. One Invalid LDAP credentials (which I expect) and one Invalid form which seems to be an error.

In any case, thanks for your work. I'll have a look if I can fix the problem that the user isn't saved in the session.

scragg0x commented 8 years ago

@larsimmisch Thanks for testing it out. The UI still needs some work and promise to make it better taking your suggestions into consideration.

Those error messages should be an easy fix, I think I was spitting everything out trying to get it to work at first. That and the loop bug should be resolved before a merge into master.

syky27 commented 8 years ago

I would love to use LDAP support, will this be merges any time soon? I would like to install realms over pip...

Thanks guys you are doing great job!

phutchins commented 8 years ago

I too would love to use LDAP support... Would be great to see this get pushed through.

scragg0x commented 8 years ago

Added in PR #85