Page MenuHomeSoftware Heritage

swh.auth.django: Expose OIDCUser model object
ClosedPublic

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

Details

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

Event Timeline

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!

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.

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.

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.

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
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.

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.

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

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.

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