Page MenuHomeSoftware Heritage
Differential D5219

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

Authored by ardumont on Mar 8 2021, 6:24 PM.

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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
swh-public-ci added a comment.Mar 8 2021, 6:25 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!
ardumont updated this revision to Diff 18693.Mar 8 2021, 6:29 PM

Remove type: ignore

swh-public-ci added a comment.Mar 8 2021, 6:30 PM

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)
ardumont updated this revision to Diff 18694.Mar 9 2021, 9:02 AM
  • 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).
swh-public-ci added a comment.Mar 9 2021, 9:03 AM

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
ardumont planned changes to this revision.Mar 9 2021, 9:03 AM

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

ardumont updated this revision to Diff 18695.Mar 9 2021, 10:15 AM

Add coverage test on the OIDCUser model class

swh-public-ci added a comment.Mar 9 2021, 10:16 AM

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 ↗(On Diff #18695)

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 ↗(On Diff #18695)

You can remove this generated comment and replace with license header.

swh/auth/tests/conftest.py
9–18 ↗(On Diff #18695)

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
10

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 ↗(On Diff #18695)

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
10

thx, did not realize there was a better way.

ardumont added a comment.Mar 9 2021, 11:09 AM

Thanks, i'll adapt accordingly.

swh/auth/django.py
1 ↗(On Diff #18695)

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.

anlambert added a comment.Mar 9 2021, 11:14 AM

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

You mean in that diff I guess ?

ardumont updated this revision to Diff 18698.Mar 9 2021, 11:21 AM

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.
swh-public-ci added a comment.Mar 9 2021, 11:21 AM

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
ardumont marked 2 inline comments as done.EditedMar 9 2021, 11:37 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.

anlambert added a comment.EditedMar 9 2021, 11:51 AM

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.

ardumont added a comment.Mar 9 2021, 12:08 PM

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.

ardumont updated this revision to Diff 18699.Mar 9 2021, 12:22 PM

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.

swh-public-ci added a comment.Mar 9 2021, 12:22 PM

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
ardumont updated this revision to Diff 18700.Mar 9 2021, 12:27 PM

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

swh-public-ci added a comment.Mar 9 2021, 12:27 PM

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
anlambert accepted this revision.Mar 9 2021, 1:29 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.

Revision Contents
Changeset List

PathSize
3 lines
1 line
1 line
1 line
5 lines
swh/
auth/
django/
85 lines
tests/
app/
apptest/
10 lines
19 lines
7 lines
6 lines
23 lines
71 lines
1 line

Diff 18701

View Options

mypy.ini

Loading...
View Options

pytest.ini

Loading...
View Options

requirements-django.txt

Loading...
View Options

requirements-test.txt

Loading...
View Options

setup.py

Loading...
View Options

swh/auth/django/__init__.py

Loading...
View Options

swh/auth/django/models.py

Loading...
View Options

swh/auth/tests/app/__init__.py

Loading...
View Options

swh/auth/tests/app/apptest/__init__.py

Loading...
View Options

swh/auth/tests/app/apptest/apps.py

Loading...
View Options

swh/auth/tests/app/apptest/models.py

Loading...
View Options

swh/auth/tests/app/apptest/settings.py

Loading...
View Options

swh/auth/tests/app/apptest/urls.py

Loading...
View Options

swh/auth/tests/app/manage.py

Loading...
View Options

swh/auth/tests/test_models.py

Loading...
View Options

tox.ini

Loading...
About · Developer information · Legal