Skip to content
This repository has been archived by the owner on May 6, 2020. It is now read-only.

feat(api): LDAP authentication #1156

Merged
merged 2 commits into from
Dec 19, 2016
Merged

feat(api): LDAP authentication #1156

merged 2 commits into from
Dec 19, 2016

Conversation

Kooper
Copy link

@Kooper Kooper commented Dec 6, 2016

Bring back LDAP authentication as discussed in #1140

Basically it is a rollback of 2883c0c on the up2date codebase, with updated versions of involved components.

Changes have been tested with LDAP server, which currently works for Deis v1

@deis-admin
Copy link

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

@deis-bot
Copy link

deis-bot commented Dec 6, 2016

@helgi, @mboersma and @bacongobbler are potential reviewers of this pull request based on my analysis of git blame information. Thanks @Kooper!

@kmala kmala added this to the v2.10 milestone Dec 6, 2016
@Kooper
Copy link
Author

Kooper commented Dec 7, 2016

Can not reproduce the failure which TravisCI reports :(
In my environment database creation and unit tests run without any problems:

# sudo -u postgres make test-unit
cd rootfs \
		&& coverage run manage.py test --settings=api.settings.testing --noinput registry api scheduler.tests \
		&& coverage report -m
Creating test database for alias 'default'...
............................................................................................................................................................................................................................................................................................................................
----------------------------------------------------------------------
Ran 316 tests in 665.885s

OK
Destroying test database for alias 'default'...
Name                                                       Stmts   Miss Branch BrPart  Cover   Missing
------------------------------------------------------------------------------------------------------
api/authentication.py                                         12      3      0      0    75%   12, 23-24
...

@Kooper
Copy link
Author

Kooper commented Dec 9, 2016

I managed to reproduce the problem. Will investigate and fix it...

@bacongobbler
Copy link
Member

Apologies for the lack of response. Glad you sorted it out. Let us know if you need help, we're also available on slack.deis.io

@Kooper
Copy link
Author

Kooper commented Dec 9, 2016

Well, the problem is caused by missing migrations for model of django_auth_ldap module.
The module uses model entirely for testing purposes, defining TestUser/TestProfile classes, which references django.auth. As the module does not declare migrations - there is a dependency problem upon database schema creation, when models are imported before test.

Now I in doubt of proper resolution: I don't want to import TestUser/TestProfile as an ordinary migrations as it makes no sense in production configuration (albeit in Deis v1 it was so - just checked our production installation of v1). But I also didn't find how to prohibit Django loading model of django_auto_ldap.

Here how it looks in details - 'Synchronizing apps without migrations' stage is running prior to defined migrations, and 'Running deferred SQL' tries to reference auth_user which doesn't exist yet.

Operations to perform:
  Synchronize unmigrated apps: corsheaders, django_auth_ldap, gunicorn, humanize, jsonfield, messages, rest_framework
  Apply all migrations: api, auth, authtoken, contenttypes, guardian, sessions
Running pre-migrate handlers for application auth
Running pre-migrate handlers for application contenttypes
Running pre-migrate handlers for application sessions
Running pre-migrate handlers for application corsheaders
Running pre-migrate handlers for application guardian
Running pre-migrate handlers for application jsonfield
Running pre-migrate handlers for application rest_framework
Running pre-migrate handlers for application authtoken
Running pre-migrate handlers for application api
Running pre-migrate handlers for application django_auth_ldap
Synchronizing apps without migrations:
  Creating tables...
    Creating table corsheaders_corsmodel
    Creating table django_auth_ldap_testuser
    Creating table django_auth_ldap_testprofile
    Running deferred SQL...
CREATE INDEX "django_auth_ldap_testuser_identifier_8359accd_like" ON "django_auth_ldap_testuser" ("identifier" varchar_pattern_ops)
ALTER TABLE "django_auth_ldap_testprofile" ADD CONSTRAINT "django_auth_ldap_testprofile_user_id_9f1e9774_fk_auth_user_id" FOREIGN KEY ("user_id") REFERENCES "auth_user" ("id") DEFERRABLE INITIALLY DEFERRED
Traceback (most recent call last):
  File "/usr/local/lib/python3.5/dist-packages/django/db/backends/utils.py", line 62, in execute
    return self.cursor.execute(sql)
psycopg2.ProgrammingError: relation "auth_user" does not exist

@kmala
Copy link
Contributor

kmala commented Dec 9, 2016

Jenkins, Add to whitelist.

@codecov-io
Copy link

codecov-io commented Dec 12, 2016

Current coverage is 87.56% (diff: 100%)

Merging #1156 into master will decrease coverage by 0.01%

@@             master      #1156   diff @@
==========================================
  Files            43         43          
  Lines          3799       3795     -4   
  Methods           0          0          
  Messages          0          0          
  Branches        664        661     -3   
==========================================
- Hits           3327       3323     -4   
  Misses          308        308          
  Partials        164        164          

Powered by Codecov. Last update 51cab9d...38275ea

@Kooper
Copy link
Author

Kooper commented Dec 12, 2016

I finally figured out that INSTALLED_APPS contains list of model for import. With django-auth-ldap there is no model except for testing module itself, so it shouldn't be included into this list.

@Kooper Kooper changed the title Restore LDAP authentication as it was in v1 feat(api): LDAP authentication Dec 13, 2016
@rvadim
Copy link
Contributor

rvadim commented Dec 14, 2016

We need this PR, when it will be merged?

@bacongobbler
Copy link
Member

bacongobbler commented Dec 14, 2016

@kmala marked it for v2.10 for review. It's jut a matter of finding the time to review it :)

@bacongobbler
Copy link
Member

I think the last thing this PR needs is documentation at https://github.com/deis/workflow before it's ready to be merged.

@kmala
Copy link
Contributor

kmala commented Dec 14, 2016

@bacongobbler this is the docs PR deis/workflow#649

@bacongobbler
Copy link
Member

Nice, how did I miss that?!

Anyways, this looks good to me. e2e tests are passing so this will be disabled by default.

Restore user authentication in LDAP as it was in v1
Basically it is a rollback of 2883c0c on the up2date codebase,
with updated versions of involved components.
@bacongobbler
Copy link
Member

closes #1140

@rvadim
Copy link
Contributor

rvadim commented Dec 19, 2016

I tested this PR in our infrastructure, already. Working great.

@kmala kmala added LGTM2 and removed needs docs labels Dec 19, 2016
@kmala
Copy link
Contributor

kmala commented Dec 19, 2016

corresponding docs PR deis/workflow#649

@kmala kmala merged commit 184b54c into deis:master Dec 19, 2016
@kmala
Copy link
Contributor

kmala commented Dec 19, 2016

@rvadim Thanks for the contribution.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants