LDAP login with username by dtrudg · Pull Request #75 · galaxyproject/galaxy

dtrudg · 2015-04-02T18:30:56Z

This is my first attempt at messing up the galaxy auth to do what I want ;-) Looking for any kind of early comments so that I might eventually end up with a reasonable PR that would be useful to others. Am attempting to implement more generic auth support, particularly for use with LDAP. Per discussion on trello card https://trello.com/c/sxz9SM2R

Things I've messed with so far...

Changed the minimum length for galaxy public usernames to 3 char. We have lots of 3 char names using an initial + surname scheme for usernames in LDAP e.g. 'bli' or 'ydu'. Noticed for toolshed it's already a 3 char limit - so guessing changing the main limit to 3 chars might break something?!
Rename the activedirectory auth plugin to a more generic ldap_ad and add username auth functionality.
Provider authenticate methods now have to return email and username.
Hack of the user controller so that login tries to pull out a user from DB on an email or username match (not just email).
I've tried to change names for vars that might now contain a username or email to 'login' to avoid confusion over a var named 'email' containing a username and vice versa.

Am sure there are better ways to do most of these things, and I'll keep trying to find them and tidy up - but it seems to work for our need to auth against our LDAP using usernames.

Config Example

The option login-use-username in auth_conf.xml for the ldap_ad provider will use usernames instead of email for auth. For auto registration, both email and username sources must now be specified. E.g.

<?xml version="1.0"?>
<auth>
<authenticator>
  <type>ldap_ad</type>
  <options>
    <allow-register>No</allow-register>
    <auto-register>Yes</auto-register>
    <server>ldap://<server-address></server>
    <search-filter>(uid={login})</search-filter>
    <search-base>ou=users,dc=biohpc,dc=swmed,dc=edu</search-base>
    <search-fields>uid,mail</search-fields>
    <bind-user>{dn}</bind-user>
    <bind-password>{password}</bind-password>
    <continue-on-failure>False</continue-on-failure>
    <auto-register-username>{uid}</auto-register-username>
    <auto-register-email>{mail}</auto-register-email>
    <login-use-username>True</login-use-username>  
</options>
</authenticator>
</auth>
```xml

hexylena · 2015-04-02T18:44:24Z

+1 from me,

thanks, username/email is better than just email since we support bare usernames since we don't handle multiple domains
The remote user login already submitted 3 character usernames (mine is esr and it was forced into the DB) so it shouldn't cause issues (or at least I didn't see any)
I'm a bit worried about hacking on the user controller, someone else should comment on that.

again, untested, but LGTM.

martenson · 2015-04-02T20:46:57Z

lib/galaxy/security/validate_user_input.py

Why is this changing? Also the condition does not correspond with the return message.
Note that TS has 3 and Galaxy 4 char username min length limit.

see comments in PR, our LDAP allows 3 character usernames (as do many others), and if my memory is correct (we've since changed a few things) REMOTE_USER forced esrinto the database despite Galaxy not explicity permitting it. I don't remember my public-name ending with a -.

Makes sense. In that case the message should also state 3 though.

If 3 is ok then doesn't need separate logic for tool shed and I'll tidy that. Was in a rush to push a PR to get some initial comments on direction as have had to switch to a different project for a bit. Will do some tidying at home. Thanks.

Good point. I would not rush into lowering the requirements of 4 char min as others might chime in with different insights though.

martenson · 2015-04-03T00:10:47Z

Overall this looks like a great start and awesome contribution! Thank you.
I need to test it more before supporting it but I like it so far. Few notes:

Would be great if TravisCI tests pass (some of your changes fail on PEP8).
I think form for creation of the users needs to be adjusted to reflect changes in username requirement (I think there is client-side validation too).

I will happily help you with these and others in the start of the next week.
Thanks again.

martenson · 2015-04-03T00:18:51Z

lib/galaxy/webapps/galaxy/controllers/user.py

This first() is troublesome. I suspect it is a remnant of inconsistent DB on Main back when we did not enforce email to be unique nor valid. Username uniqueness is DB-enforced so it should not change anything but we might want to think about how to do this right. I like using .one() and catching exceptions from sqlalchemy (as in https://github.com/galaxyproject/galaxy/blob/dev/lib/galaxy/managers/libraries.py#L39)
Other ideas?

I never really think .one() should be used if it's okay a record doesn't exist, and that leads onto valid code. Not a fan of using exceptions for flow-control logic like that.

In this particular case it's perfectly okay for a user record not to exist - as we then go into the auto registration code. Not really an exception, just flow control logic.

What about doing a .count() and throwing an exception if >1 records match, doing the auto reg if 0 etc.. ?

count() is fine with me

Regarding one/first, http://alextechrants.blogspot.com/2013/11/10-common-stumbling-blocks-for.html has a decent description of the correct usage. When I asked for a little more info in the sqlalchemy IRC channel, the gist was: one() only when the record should exist barring some error; first() is to be used when the record may or may not exist and either one of those outcomes is ok. Verifying that there aren't multiple user emails (or whatever other field) should be enforced within the DB by unique constraints on the email column, and should not require a separate query each time we want to look something up.

I guess you can do .all() and check len() in Python to prevent second query.

Assuming 3 character username is okay, update the info and registration forms. NB - username regex in register.mako already allowed 3 char. In info.mako was 4 char.

dtrudg · 2015-04-04T03:07:34Z

@martenson Assuming 3 character username is okay, updated the info and registration
mako templates - JS and text. NB - username_re regex in register.mako already allowed 3 char. In
info.mako was 4 char.

dannon · 2015-04-06T16:11:54Z

Changing the minimum to 3 characters seems totally fine to me. Nice work here, @dctrud!

blankenberg · 2015-04-06T16:27:27Z

We have some 3 character usernames in usegalaxy.org, so it shouldn't cause an issue.

martenson · 2015-04-06T17:31:21Z

I have tested login/registration/update basics of this PR and did not find a problem. I tried try to test this with some fake-ldap-provider tool but nothing worked well for me (not fail of this code).

Also I do not think you have to solve our .first() issue so do not worry about it in this PR.

This looks ready to me.
👍

jmchilton · 2015-04-06T17:45:41Z

lib/galaxy/auth/providers/ldap_ad.py

LDAPAD is fine for a class/plugin name but I would prefer if it were just LDAP. Can we just make this ldap and the class LDAP?

Then toward the bottom of the file define

class ActiveDirectory(LDAP): """ Effectively just an alias for LDAP auth, but may contain active directory specific logic in the future. """ plugin_type = 'activedirectory' __all__ = ['LDAP', 'ActiveDirectory']

Sounds reasonable to me. Keep the module as ldap_ad.py to avoid clash with the python-ldap ldap module?

Sure - sounds like a good plan.

dannon · 2015-04-06T19:20:53Z

+1

@jmchilton

Suggested and supplied by @jmchilton

dtrudg · 2015-04-06T19:40:50Z

Removing WIP in hope that this can be merged now if useful. Won't be able to work on other things I mentioned on the trello card for a while.

dannon · 2015-04-06T19:42:03Z

Thanks for the update, looks good!

jmchilton · 2015-04-06T19:42:11Z

I'll add my 👍 to the chorus - thanks @dctrud - this is awesome!

LDAP login with username

dtrudg · 2015-04-06T19:43:17Z

Guess follow up is to ensure Wiki is updated, and reflects these additional changes? Do I need to contribute on that?

martenson · 2015-04-06T20:04:01Z

I added a checklist for documentation to the trello card. We need to streamline the scattered information into one authentication wiki page/hub. Your contributions are welcomed but not required.

nsoranzo · 2015-04-07T14:01:01Z

lib/galaxy/auth/__init__.py

Presently the type in this example should be ldap.

Nice catch!

nsoranzo · 2015-05-01T18:18:18Z

I just noticed that this broke the <filter> tag of config/auth_conf.xml . In fact, active_authenticators() method in lib/galaxy/auth/__init__.py is now receiving a galaxy.model.User when called from check_password() and check_change_password(), an email address when called by check_registration_allowed(), and an email/username when called by check_auto_registration().

I'll try to work on a fix next week.

… revert changes in auth_conf.xml syntax. After galaxyproject#75 , the <filter> tag of config/auth_conf.xml is broken, see galaxyproject#75 (comment) . I don't think that there is a meaningful way to filter on usernames, so I changed it to filter on email instead of login. Also: - fix AuthManager.check_registration_allowed to use both email and username since which is used for login depends on the AuthProvider used; - move ldap <authenticator> example to config/auth_conf.xml.sample .

Preparation for new Tool class in BioBlend.objects

…ello_sbKLjewQ Ugly hack of track vs same track bug

Fixes for askomics IT

Askomics GxIT: fix crash when no input selected

Initial attempt at LDAP login with username - WIP

660cfcb

martenson reviewed Apr 2, 2015
View reviewed changes

martenson reviewed Apr 3, 2015
View reviewed changes

dtrudg added 2 commits April 3, 2015 21:55

Fix some PEP8 complaints and comments

204314a

3 char for username in user info and registration

0cb2e75

Assuming 3 character username is okay, update the info and registration forms. NB - username regex in register.mako already allowed 3 char. In info.mako was 4 char.

jmchilton reviewed Apr 6, 2015
View reviewed changes

David Trudgian added 2 commits April 6, 2015 14:31

Split LDAP from AD auth provider (in name only at present)

359dcea

Suggested and supplied by @jmchilton

Missed PEP8 indent rule again - sorry

42eb483

dtrudg changed the title ~~Initial attempt at LDAP login with username - WIP~~ Initial attempt at LDAP login with username Apr 6, 2015

dtrudg changed the title ~~Initial attempt at LDAP login with username~~ LDAP login with username Apr 6, 2015

dannon added a commit that referenced this pull request Apr 6, 2015

Merge pull request #75 from dctrud/ldapsupport2

ffe58de

LDAP login with username

dannon merged commit ffe58de into galaxyproject:dev Apr 6, 2015

nsoranzo reviewed Apr 7, 2015
View reviewed changes

nsoranzo mentioned this pull request May 5, 2015

[15.05] Fix AuthManager.active_authenticators(). Partially re... #216

Merged

jmchilton referenced this pull request in jmchilton/galaxy May 11, 2015

Fix a variety of tool shed tests broken with PR #75.

593d38f

mvdbeek pushed a commit to mvdbeek/galaxy that referenced this pull request Jan 24, 2017

Merge pull request galaxyproject#75 from nsoranzo/master

e6465c2

Preparation for new Tool class in BioBlend.objects

sveinugu added a commit to elixir-oslo/proto that referenced this pull request Oct 31, 2018

Merge pull request galaxyproject#75 from elixir-no-nels/bug_master/tr…

22fd581

…ello_sbKLjewQ Ugly hack of track vs same track bug

blankenberg pushed a commit to blankenberg/galaxy that referenced this pull request Aug 14, 2019

Merge pull request galaxyproject#75 from abretaud/it_askoko

01c23ce

Fixes for askomics IT

simonbray pushed a commit to simonbray/galaxy that referenced this pull request Sep 22, 2020

Merge pull request galaxyproject#75 from abretaud/patch-5

59563af

Askomics GxIT: fix crash when no input selected

Conversation

dtrudg commented Apr 2, 2015

Uh oh!

hexylena commented Apr 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martenson commented Apr 3, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dtrudg commented Apr 4, 2015

Uh oh!

dannon commented Apr 6, 2015

Uh oh!

blankenberg commented Apr 6, 2015

Uh oh!

martenson commented Apr 6, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dannon commented Apr 6, 2015

Uh oh!

dtrudg commented Apr 6, 2015

Uh oh!

dannon commented Apr 6, 2015

Uh oh!

jmchilton commented Apr 6, 2015

Uh oh!

dtrudg commented Apr 6, 2015

Uh oh!

martenson commented Apr 6, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nsoranzo commented May 1, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants