LDAP Authentication #73

Merged
merged 40 commits into from Mar 10, 2013

Conversation

Projects
None yet
5 participants
Contributor

rmartinjak commented Jun 20, 2012

"Update" to Trac #11501

Optional user authentication via LDAP (only visible in the notebook settings if python-ldap is installed)

(in order to not list all users found in LDAP, the "share worksheet" page now has a search box instead of the list. [Bonus: search results are added to the textbox if you click them])

kini commented on 1b93d9b Jun 20, 2012

Note: this will conflict with #63, where I moved install_requires outside the setup() call. A manual merge resolution would be required, but it shouldn't be too difficult.

Also it would be nice if this were an optional dependency, don't you think? Very few sagenb users will actually need ldap, I feel... Is it possible to wrap the ldap import with a try/except and skip defining the ldap stuff if python-ldap wasn't found? Or does that just cause problems with the configuration templating etc.?

Owner

rmartinjak replied Jun 20, 2012

I absolutely agree, optional dep makes much more sense.
Imports from ldap are only done when an actual query is performed. Maybe an try/except to automatically disable LDAP (or better: also don't display it in the settings menu at all) would make sense.I'll take a look what I can do

kini replied Jun 20, 2012

Great! If you can make a pull request now I think our comments here will be more visible to others. Don't worry, I won't merge it until all issues are hashed out :)

Contributor

rmartinjak commented Jun 20, 2012

f40903b does not include automatic deactivation of LDAP if an import fails. Is this needed?
I doubt anybody would

  1. install python-ldap
  2. activate LDAP auth in the notebook
  3. remove python-ldap

which is the only case where an import x from ldap would raise an ImportError

Owner

kini commented Jun 20, 2012

For thoroughness I think it certainly can't hurt...

@kini kini referenced this pull request Jun 20, 2012

Closed

ldap auth #46

Contributor

rmartinjak commented Jun 28, 2012

I set up sagenb/ldap at https://rmartinjak.de:8080/ in case anybody wants to play around a bit.
It only has two LDAP users so far, just comment here if you want some more.

User / Password:
admin / sagemath
mjwatson / ilovespidey
loislane / dailyplanet

Owner

jasongrout commented Oct 30, 2012

Judging from the many merges, I take it that you have done a great job of keeping up with master. Is this ready to go? Did anyone review it?

Owner

jasongrout commented Nov 2, 2012

It looks like you might still be working on this? Is that right?

Contributor

rmartinjak commented Nov 2, 2012

I just modified some small parts because I found the code a bit ugly (especially the exception handling should be much cleaner now)

rmartinjak added some commits Nov 2, 2012

Merge branch 'master' into ldap
Conflicts:
	sagenb/data/sage/html/settings/user_management.html
	sagenb/notebook/user.py
Owner

jasongrout commented Nov 2, 2012

Okay, so we just need someone to review it.

Contributor

rmartinjak commented Nov 20, 2012

How is review done? I couldn't find much in http://www.sagemath.org/doc/developer/sagenb/index.html except "Ask for code review!"

Owner

kini commented Nov 20, 2012

AFAIK it's similar to the review process for Sage itself (?), i.e. someone who knows what they're talking about should come along and say it looks good, and then we can merge it... problem is I don't know who knows enough about LDAP to review this (maybe @ijstokes, if he's still doing Sage development?). It's really a shame how long this pull request has been sitting around and how many merge commits you've made...

+ # so we can turn this auth method on/off
+ self._auth_methods = {
+ 'auth_ldap': LdapAuth(self._conf),
+ }
@rmartinjak

rmartinjak Nov 23, 2012

Contributor

I'm not 100% happy with this solution. Probably a "constant" in server_conf would be better than a string. Or maybe somebody has a better idea.

Member

ppurka commented Mar 5, 2013

Can people who are using this code just chime in and say "it works"? If there are at least two such groups, then we should simply merge this set of patches. It is an often asked for functionality and so it should go in.

I know that it used to work with sage-4.7, and still works with that installation (the servers are still running happily). But quite a bit has changed in the meanwhile (4.7 was almost two years ago).

Contributor

rmartinjak commented Mar 6, 2013

We've been using it with 5.5 for some weeks and haven't encountered any problems so far.

laevar commented Mar 6, 2013

Well, we initially wanted this functionality and asked rmartinjak to extend the code, which he did. We are using this over a year now with sage 4.7 and recently with sage 5.5 for university-courses. We can surely say: it works :)

sagenb/notebook/notebook.py
@@ -1369,7 +1369,7 @@ def html_specific_revision(self, username, ws, rev):
username = username, rev = rev, prev_rev = prev_rev,
next_rev = next_rev, time_ago = time_ago)
- def html_share(self, worksheet, username):
+ def html_share(self, worksheet, username, lookup=None):
@jasongrout

jasongrout Mar 9, 2013

Owner

The documentation to html_share should be updated to explain this lookup parameter

Owner

jasongrout commented Mar 9, 2013

The changes to the share page probably shouldn't be added, at least until we have a better system for managing shared worksheets. On the one hand, it's nice to have a search, but on the other hand, it exposes the exact same sort of problems that we had before---it is trivial for someone to spam a huge number of people with a shared worksheet.

Member

ppurka commented Mar 9, 2013

@jasongrout Maybe the lookup code can be kept around, and only the lookup parameter removed from the function definition so that it can enabled in the future.

Owner

jasongrout commented Mar 9, 2013

With the search users feature removed, I don't see anything else jump out at me as a cause for concern. I haven't tested this, though.

Owner

jasongrout commented Mar 9, 2013

And thanks for working on this! I will likely use it this fall.

Member

ppurka commented Mar 10, 2013

Thanks a lot for the updates! I checked that the notebook is still working (local and openid) with the new updates. Merging this now.

ppurka added a commit that referenced this pull request Mar 10, 2013

Merge pull request #73 from rmartinjak/ldap
Introduce LDAP Authentication into the notebook.

@ppurka ppurka merged commit f3e6e20 into sagemath:master Mar 10, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment