feat(controller): Adding LDAP/AD auth support

[phspagiari]

This is the Pull Request for the Proposal that we discussed at #3135 using the PoC in the gist.

I didn't write any test because I imagine that it's not good have specific tests for auth with ldap, so my opinion is that if the actual auth tests pass its good enough.

If sounds good I will make a doc with the README content and create another PR.

[dserodio]

👍 this would be really useful

[lorieri]

👍
after this, production \o/

[bacongobbler]

this shouldn't be here, right? :)

[phspagiari]

I'm not a django expert, some people said that would be better if we put the module here, some people don't. For safe precautions I prefer to have it explicit. I can remove if you think that its better. 👍

[bacongobbler]

We don't use django_fsm any more, so it shouldn't be necessary. The only reason we keep it around are for legacy South migrations. Let's remove it so the module's not taking up precious resources when the server boots. django_auth_ldap should be kept though, of course.

[phspagiari]

Haha, I'm talking about the django_auth_ldap into this list, the fsm probably came in some merge I did, had not noticed, I obviously do not use this module, sorry I will remove. 😄

[bacongobbler]

looks good, but how do you replace the token auth with LDAP? I don't see any changes in /api/urls.py or in the views. As far as I can tell this doesn't actually replace the deis login endpoints.

[gabrtv]

@bacongobbler looks like the PR is modifying the authentication backend, which means token auth and API endpoints should work without any changes -- which is terrific.

@phspagiari nice work, I look forward to seeing this merged! A few questions:

1. It's not clear to me how this functionality is enabled/disabled. What happens for the majority of users who won't have this configured?
2. Without automated tests I'm concerned we could break this functionality over time with, for example, a library update. Thoughts?

[phspagiari]

@bacongobbler as @gabrtv said, token auth and API endpoints works with ldap or with default auth without any changes. If you configure LDAP, the /auth/login will try to login via LDAP, if you dont configure, the django will pass the ldap backend and try to login with other backend (default).

@gabrtv If all the values of LDAP is left blank (not configured) django will pass to the next auth backend and try to login on it. At first I thought that I would need a ugly if to add or remove the ldap auth backend, but latter I saw that django does this automatically.

[bacongobbler]

Sounds good to me! 👍

[bacongobbler]

@gabrtv I guess we'll just have to hope that django_auth_ldap doesn't make any critically breaking changes... Either that or we're gonna have to set up an LDAP server in the future to test updates ;)

[phspagiari]

@bacongobbler @gabrtv A question: I want to make a documentation of how to use LDAP with Deis. What is the best way to do it? Put together with this PR or creating another one after this PR is accepted?

[bacongobbler]

I'd throw it into this PR so others can test LDAP support by building off your branch via http://docs.deis.io/en/latest/contributing/hacking/

[phspagiari]

I added the documentation 😄. I use the "Managing Deis" tree because I saw the "Using a Proxy Server" here, I can move if you guys prefer in another place.

[bacongobbler]

This should be moved over to http://docs.deis.io/en/latest/customizing_deis/controller_settings/

[phspagiari]

I wrote this doc at controller_settings and moved to a separated doc before. I think that since this is optional, should be equal to the proxy settings. But if its better on controller I can move again.

[phspagiari]

Another thing: I said some considerations at docs about the admin user and disabling the default registration, It would be great if you guys give your opinions about it.
First I was imagining that I would have to implement the "disable registration" if the LDAP is enabled, but we can use the default method to this.
I tested and so far so good :)

[azurewraith]

👍 This would be useful...

[deis-bot]

Thanks for the contribution! Please ensure your commits follow our style guide. This code will be tested once a Deis maintainer reviews it.

[lorieri]

just tested it, works like a charm

[lorieri]

good to disable registration

doesn't work

core@coreos1 ~ $ deisctl config controller set registrationEnabled='False'
false

works

core@coreos1 ~ $ etcdctl set /deis/controller/registrationEnabled
False

[phspagiari]

@lorieri In LDAP Docs is referenced the disable_user_registration. The key registrationEnabled is boolean, not string. This changes the value in confd_settings.py L41.

[phspagiari]

@bacongobbler there is a reason for confd_settings.py uses a "syntax" different from the default from confd?
Example: confd_settings.py uses {{ .key_for_something }} instead of {{ getv /key/for/something }} and when I try to test this template with confd into my mac I get error of "bad syntax".

[bacongobbler]

It's based on an older version of confd. 0.4.6, I believe. We're in the process of upgrading to 0.8 in #3361

[phspagiari]

I see.. and #3361 answered my doubt about why the or default works using the older version and not using the new version with getv.
I prefer the or default instead of keep the variables with None value in the settings.py and a if into endpoint key. You agree?

[bacongobbler]

I was thinking more along the lines of it being enabled equivalent to this. In that example, if the user sets /deis/registry/smtpHost then the registry will send emails based on that config.

IOW I agree

[phspagiari]

I went with the if key option... but my idea was to remove the default attributes at settings.py like this. With my idea django will break if the keys doesn't exists. After thinking a little I supose that will be better if the attributes have a default value in settings file for "understanding purposes".

I made a rebase too. :) Lets test it again 👍

[bacongobbler]

Sorry @phspagiari. This'll need another rebase as we just merged #3361 so you'll need to update the confd_settings.py template.

[phspagiari]

@bacongobbler holy cow... this merge is just the confd update =( We are talking about this earlier... Ok I will change all to the new confd template version.

[phspagiari]

I saw the error, the problem is dont know another way to do this

 filterstr="(" + USER_FILTER + "=%(user)s)"

The usually pythonic way to do this "%s=%(user)s" % (USER_FILTER) or even "%s=%s" % (USER_FILTER, user) doesnt work because the user is a placeholder of django-ldap.
String concatenation its ugly but I cant imagine another way to do this.

[bacongobbler]

Set up USER_FILTER as an empty string instead. The format wants a unicode string, not a NoneType.

[bacongobbler]

you can also run make -C docs to check if it works

[phspagiari]

@bacongobbler Yes, I imagine that was a lint error first, I am fixing the two problems right now.

[phspagiari]

Sorry for my mistake, all fixed.

EDIT:
I can't reproduce the error at build #248 with pip.
On my test environment:

$: venv/bin/pip install -q -r requirements.txt -r dev_requirements.txt
  Could not find a tag or branch '7413317', assuming commit.
$:

[mboersma]

Code LGTM. Thanks again @phspagiari!

[bacongobbler]

I'd remove the whitespace between {{else}} and {{end}} so the end result is an empty string if the key's unset.

[mboersma]

Oops, sorry I missed this comment before merging. Should we typo-commit the space change?

[bacongobbler]

nah, it's not a huge issue. Everything will sort itself out once all ldap keys are set :)

[bacongobbler]

small nit, but LGTM otherwise :)

[phspagiari]

Typo :( Thanks guys, I'm very help to be contributing with Deis 😄

[bacongobbler]

Thanks for pushing this one through, @phspagiari 🎈