Details

Reviewers

anlambert

Group Reviewers

Reviewers

Maniphest Tasks

T3079: Boostrap swh-auth module for authentication

Commits

rDAUTHd6204f0f526b: swh.auth.django: Expose OIDCUser model object

Summary

That class is a custom User proxy model for remote users storing OpenID Connect related
data (profile containing authentication tokens, ...).

The model is also not saved to database as all users are already stored in the Keycloak
one.

That class will be used for example by both the webapp and the deposit.

No test yet, just the plain minimal django test configuration for the tests to not break...

Related to T3079

Diff Detail

Repository

rDAUTH Common authentication libraries

Branch

master

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 19788
Build 30723: Phabricator diff pipeline on jenkins	Jenkins console · Jenkins
Build 30722: arc lint + arc unit

Event Timeline

ardumont created this revision.Mar 8 2021, 6:24 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptMar 8 2021, 6:24 PM

Build has FAILED

Patch application report for D5219 (id=18692)

Rebasing onto fa523d66b6...

Current branch diff-target is up to date.

Changes applied before test

commit e2efdbf1eacf4260e1661463b93c52cf140fec0e
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Mar 8 18:22:18 2021 +0100

    swh.auth.django: Expose OIDCuser model object
    
    Related to T3079

Link to build: https://jenkins.softwareheritage.org/job/DAUTH/job/tests-on-diff/18/
See console output for more information: https://jenkins.softwareheritage.org/job/DAUTH/job/tests-on-diff/18/console

Harbormaster returned this revision to the author for changes because remote builds failed.Mar 8 2021, 6:25 PM

Harbormaster failed remote builds in B19786: Diff 18692!

Remove type: ignore

Build is green

Patch application report for D5219 (id=18693)

Rebasing onto fa523d66b6...

Current branch diff-target is up to date.

Changes applied before test

commit e29b7f565e8738b2e761942b13d31052eda514ee
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Mar 8 18:22:18 2021 +0100

    swh.auth.django: Expose OIDCuser model object
    
    Related to T3079

See https://jenkins.softwareheritage.org/job/DAUTH/job/tests-on-diff/19/ for more details.

Harbormaster completed remote builds in B19787: Diff 18693.Mar 8 2021, 6:30 PM

ardumont requested review of this revision.Mar 8 2021, 6:30 PM

ardumont retitled this revision from swh.auth.django: Expose OIDCuser model object to swh.auth.django: Expose OIDCUser model object.Mar 9 2021, 8:58 AM

ardumont edited the summary of this revision. (Show Details)

Rework commit message to explicit the reasons (same for diff description)
Move swh.auth.django arborescence tree into one plain swh.auth.django module (file).

Build is green

Patch application report for D5219 (id=18694)

Rebasing onto fa523d66b6...

Current branch diff-target is up to date.

Changes applied before test

commit d0fd393445ef5bfd56bcb7fd4651ac0a8c31c6ae
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Mar 8 18:22:18 2021 +0100

    swh.auth.django: Expose OIDCUser model object
    
    That class is a custom User proxy model for remote users storing OpenID Connect related
    data (profile containing authentication tokens, ...).
    
    The model is also not saved to database as all users are already stored in the Keycloak
    one.
    
    That class will be used for example by both the webapp and the deposit.
    
    Related to T3079

See https://jenkins.softwareheritage.org/job/DAUTH/job/tests-on-diff/20/ for more details.

Harbormaster completed remote builds in B19788: Diff 18694.Mar 9 2021, 9:03 AM

Plan changes as i'm trying to determine how to actually add test on this.

Add coverage test on the OIDCUser model class

Build is green

Patch application report for D5219 (id=18695)

Rebasing onto fa523d66b6...

Current branch diff-target is up to date.

Changes applied before test

commit 13a0ea5809d3850f790548801f2cc21e679aca80
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Mar 8 18:22:18 2021 +0100

    swh.auth.django: Expose OIDCUser model object
    
    That class is a custom User proxy model for remote users storing OpenID Connect related
    data (profile containing authentication tokens, ...).
    
    The model is also not saved to database as all users are already stored in the Keycloak
    one.
    
    That class will be used for example by both the webapp and the deposit.
    
    Related to T3079

See https://jenkins.softwareheritage.org/job/DAUTH/job/tests-on-diff/21/ for more details.

Harbormaster completed remote builds in B19789: Diff 18695.Mar 9 2021, 10:16 AM

anlambert requested changes to this revision.Mar 9 2021, 11:03 AM

anlambert added a subscriber: anlambert.

anlambert added inline comments.

swh/auth/django.py
1	I think it will be better to create a `swh.auth.django.models` module and put that model in it. In a similar manner, authentication backends should be put in a `swh.auth.django.backends` (in an other diff of course).
swh/auth/tests/apptest/urls.py
1–15	You can remove this generated comment and replace with license header.
swh/auth/tests/conftest.py
9–18	You should rather create a `swh.auth.tests.apptest.settings` module and put default settings in it. Currently, tests can only be executed with `tox`, running with `pytest` only raises the following exception: `django.core.exceptions.AppRegistryNotReady: Apps aren't loaded yet`. Once you put the settings in a dedicated module, you should be able to run the tests with `pytest` without `tox` by putting this configuration in the `pytest.ini` file: [pytest] DJANGO_SETTINGS_MODULE = swh.auth.tests.apptest.settings
swh/auth/tests/test_django.py
19–28 ↗	(On Diff #18695)	Why the copy operation here ? Function scope fixtures (default) are resetted between each test.
tox.ini
9	It should rather be: [testenv] extras = testing django

This revision now requires changes to proceed.Mar 9 2021, 11:03 AM

ardumont added inline comments.Mar 9 2021, 11:06 AM

swh/auth/tests/conftest.py
9–18	ah! I did not run it with pytest... I wanted a minimal settings to not write all over the places. Fine, i'll change that.
swh/auth/tests/test_django.py
19–28 ↗	(On Diff #18695)	right, no need.
tox.ini
9	thx, did not realize there was a better way.

Thanks, i'll adapt accordingly.

swh/auth/django.py
1	I think it will be better to create a swh.auth.django.models module and put that model in it. ok In a similar manner, authentication backends should be put in a swh.auth.django.backends (in an other diff of course). Yes, although note i was not planning on adding the backends.

Yes, although note i was not planning on adding the backends.

You mean in that diff I guess ?

Adapt according to suggestions

Remains to satisfy pytest which is not yet happy [1] (tox is already though)

[1]

ImportError: No module named 'swh.auth.tests.apptest'

pytest-django could not find a Django project (no manage.py file could be found). You must explicitly add your Django project to the Python path to have it picked up.

Build is green

Patch application report for D5219 (id=18698)

Rebasing onto fa523d66b6...

Current branch diff-target is up to date.

Changes applied before test

commit a3b8cbe1d4d9ef1f0e3955bc58086cd83949dc9e
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Mar 8 18:22:18 2021 +0100

    swh.auth.django: Expose OIDCUser model object
    
    That class is a custom User proxy model for remote users storing OpenID Connect related
    data (profile containing authentication tokens, ...).
    
    The model is also not saved to database as all users are already stored in the Keycloak
    one.
    
    That class will be used for example by both the webapp and the deposit.
    
    Related to T3079

See https://jenkins.softwareheritage.org/job/DAUTH/job/tests-on-diff/22/ for more details.

Harbormaster completed remote builds in B19792: Diff 18698.Mar 9 2021, 11:21 AM

You mean in that diff I guess ?

no, no, i meant in general (i saw you mentioned earlier "in another diff" :).
I may be wrong.

The one from the webapp looks generic enough so they could come here.

But the one i started to craft in the deposit is specific (and it derives from the djangorestframework's
basic http one) so in my mind, i would stop the refactoring at the OIDCUser model.

This morning, i realized the parsing utilities from token to OIDCUser are interesting to go here as well, so that's coming up.
But that's it ;)

Also i'd like to finalize the deposit evolution to actually use swh-auth and show it (update my still wip diff, my updates are still local, need to add tests there :)

In any case, if you are right and the backends can go here, there is nothing urgent yet and that can be done indeed later.

But the one i started to craft in the deposit is specific (and it derives from the djangorestframework's
basic http one) so in my mind, i would stop the refactoring at the OIDCUser model.

I do not see any reason why this OIDC direct grant flow backend should be kept in swh-deposit.
I mean this is generic django drf code that could be used in other use cases in the future.

I do not see any reason why this OIDC direct grant flow backend should be kept in swh-deposit.
I mean this is generic django drf code that could be used in other use cases in the future.

well, the only reason i want it in the deposit first (where it's needed) is i'd like to see it through first.
And also ensures it works.

Adapt so pytest is happy.

That makes tox sad though (i expect a build failure as i currenly have locally) Still
updating the diff as I don't get the error yet.

Build has FAILED

Patch application report for D5219 (id=18699)

Rebasing onto fa523d66b6...

Current branch diff-target is up to date.

Changes applied before test

commit 1a72bf00aaf217e9f4ed6d30fcb083b719f8646c
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Mar 8 18:22:18 2021 +0100

    swh.auth.django: Expose OIDCUser model object
    
    That class is a custom User proxy model for remote users storing OpenID Connect related
    data (profile containing authentication tokens, ...).
    
    The model is also not saved to database as all users are already stored in the Keycloak
    one.
    
    That class will be used for example by both the webapp and the deposit.
    
    Related to T3079

Link to build: https://jenkins.softwareheritage.org/job/DAUTH/job/tests-on-diff/23/
See console output for more information: https://jenkins.softwareheritage.org/job/DAUTH/job/tests-on-diff/23/console

Harbormaster failed remote builds in B19793: Diff 18699!Mar 9 2021, 12:22 PM

Add missing __init__.py which made the build fail within tox

Build is green

Patch application report for D5219 (id=18700)

Rebasing onto fa523d66b6...

Current branch diff-target is up to date.

Changes applied before test

commit d6204f0f526b1c0f54b18c629d08f318765b840d
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Mar 8 18:22:18 2021 +0100

    swh.auth.django: Expose OIDCUser model object
    
    That class is a custom User proxy model for remote users storing OpenID Connect related
    data (profile containing authentication tokens, ...).
    
    The model is also not saved to database as all users are already stored in the Keycloak
    one.
    
    That class will be used for example by both the webapp and the deposit.
    
    Related to T3079

See https://jenkins.softwareheritage.org/job/DAUTH/job/tests-on-diff/24/ for more details.

Harbormaster completed remote builds in B19794: Diff 18700.Mar 9 2021, 12:27 PM

Looks good to me.

This revision is now accepted and ready to land.Mar 9 2021, 1:29 PM

Closed by commit rDAUTHd6204f0f526b: swh.auth.django: Expose OIDCUser model object (authored by ardumont). · Explain WhyMar 9 2021, 1:47 PM

This revision was automatically updated to reflect the committed changes.

ardumont added a commit: rDAUTHd6204f0f526b: swh.auth.django: Expose OIDCUser model object.

swh.auth.django: Expose OIDCUser model object
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Patch application report for D5219 (id=18692)

Changes applied before test

Patch application report for D5219 (id=18693)

Changes applied before test

Patch application report for D5219 (id=18694)

Changes applied before test

Patch application report for D5219 (id=18695)

Changes applied before test

Patch application report for D5219 (id=18698)

Changes applied before test

Patch application report for D5219 (id=18699)

Changes applied before test

Patch application report for D5219 (id=18700)

Changes applied before test

Revision Contents
Changeset List

Diff 18694

mypy.ini

requirements-django.txt

requirements-test.txt

setup.py

swh/auth/django.py

swh/auth/tests/apptest/init.py

swh/auth/tests/apptest/apps.py

swh/auth/tests/apptest/models.py

swh/auth/tests/apptest/urls.py

swh/auth/tests/conftest.py

swh/auth/tests/test_models.py

tox.ini

swh.auth.django: Expose OIDCUser model objectClosedPublicActions

Details

Diff Detail

Event Timeline

Patch application report for D5219 (id=18692)

Changes applied before test

Patch application report for D5219 (id=18693)

Changes applied before test

Patch application report for D5219 (id=18694)

Changes applied before test

Patch application report for D5219 (id=18695)

Changes applied before test

Patch application report for D5219 (id=18698)

Changes applied before test

Patch application report for D5219 (id=18699)

Changes applied before test

Patch application report for D5219 (id=18700)

Changes applied before test

Revision ContentsChangeset List

Diff 18694

mypy.ini

requirements-django.txt

requirements-test.txt

setup.py

swh/auth/django.py

swh/auth/tests/apptest/__init__.py

swh/auth/tests/apptest/apps.py

swh/auth/tests/apptest/models.py

swh/auth/tests/apptest/urls.py

swh/auth/tests/conftest.py

swh/auth/tests/test_models.py

tox.ini

swh.auth.django: Expose OIDCUser model object
ClosedPublic
Actions

Revision Contents
Changeset List

swh/auth/tests/apptest/init.py