Page MenuHomeSoftware Heritage

auth: Improve login management and configuration
ClosedPublic

Authored by anlambert on Sep 8 2022, 1:59 PM.

Details

Summary

Declare login and logout URLs in django settings:

  • basic authentication is used in development mode and when running cypress tests
  • OIDC authentication is used in production mode and when running Python tests

Do not expose basic authentication login URL in production webapp
for obvious security reasons.

Align query parameter name for redirection after login in basic
authentication backend with the OIDC one, it is now named next_path.

Simplify some code in django templates.

Remove documentation about API authentication when using the basic
django backend as no bearer token can be generated in that case.

Diff Detail

Repository
rDWAPPS Web applications
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Build is green

Patch application report for D8420 (id=30368)

Rebasing onto 33212c44d3...

Current branch diff-target is up to date.
Changes applied before test
commit 47f67bfc900838cb44496053c6648376801f6ce1
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Thu Sep 8 13:51:09 2022 +0200

    auth: Improve login management and configuration
    
    Declare login and logout URLs in django settings:
    
      - basic authentication is used in development mode and when running
        cypress tests
    
      - OIDC authentication is used in production mode and when running
        Python tests
    
    Do not expose basic authentication login URL in production webapp
    for obvious security reasons.
    
    Align query parameter name for redirection after login in basic
    authentication backend with the OIDC one, it is now named next_path.
    
    Simplify some code in django templates.
    
    Remove documentation about API authentication when using the basic
    django backend as no bearer token can be generated in that case.

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

Build is green

Patch application report for D8420 (id=30416)

Rebasing onto c52158a8d7...

Current branch diff-target is up to date.
Changes applied before test
commit 0d1c5d0d7a8112151a7b06a7086713cd9e8a0500
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Thu Sep 8 13:51:09 2022 +0200

    auth: Improve login management and configuration
    
    Declare login and logout URLs in django settings:
    
      - basic authentication is used in development mode and when running
        cypress tests
    
      - OIDC authentication is used in production mode and when running
        Python tests
    
    Do not expose basic authentication login URL in production webapp
    for obvious security reasons.
    
    Align query parameter name for redirection after login in basic
    authentication backend with the OIDC one, it is now named next_path.
    
    Simplify some code in django templates.
    
    Remove documentation about API authentication when using the basic
    django backend as no bearer token can be generated in that case.

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

While you're refactoring this, is there a reason for not using the default REDIRECT_FIELD_NAME from django.contrib.auth ("next") for the login/logout views, instead of "next_path"? This would save us from having to override it on every user_passes_test call.

In D8420#219363, @olasd wrote:

While you're refactoring this, is there a reason for not using the default REDIRECT_FIELD_NAME from django.contrib.auth ("next") for the login/logout views, instead of "next_path"? This would save us from having to override it on every user_passes_test call.

Oh I missed that django setting, let me check.

Prefer to override django.contrib.auth.REDIRECT_FIELD_NAME instead of
using redirect_field_name parameter in auth related decorators.

In D8420#219363, @olasd wrote:

While you're refactoring this, is there a reason for not using the default REDIRECT_FIELD_NAME from django.contrib.auth ("next") for the login/logout views, instead of "next_path"? This would save us from having to override it on every user_passes_test call.

Oh I missed that django setting, let me check.

That's much better indeed, thanks ! Diff updated.

In D8420#219363, @olasd wrote:

While you're refactoring this, is there a reason for not using the default REDIRECT_FIELD_NAME from django.contrib.auth ("next") for the login/logout views, instead of "next_path"? This would save us from having to override it on every user_passes_test call.

Oh I missed that django setting, let me check.

That's much better indeed, thanks ! Diff updated.

Hmm, that's not a setting, your change is overriding a module-level variable (which may be fine? but may not be? Really depends on import ordering and whether code in settings.py is run before other modules import django.contrib.auth).

What I was suggesting was to change the query parameter of our own oidc login views to use django's default name of next instead of next_path, removing the need for any override.

Build has FAILED

Patch application report for D8420 (id=30459)

Rebasing onto c52158a8d7...

Current branch diff-target is up to date.
Changes applied before test
commit b87de3cc9cdbd8832fc3fae58a4fb850774e1072
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Thu Sep 8 13:51:09 2022 +0200

    auth: Improve login management and configuration
    
    Declare login and logout URLs in django settings:
    
      - basic authentication is used in development mode and when running
        cypress tests
    
      - OIDC authentication is used in production mode and when running
        Python tests (in terms of login/logout URLs)
    
    Do not expose basic authentication login URL in production webapp
    for obvious security reasons.
    
    Align query parameter name for redirection after login in basic
    authentication backend with the OIDC one, it is now named next_path.
    
    Simplify some code in django templates.
    
    Remove documentation about API authentication when using the basic
    django backend as no bearer token can be generated in that case.

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

Hmm, that's not a setting, your change is overriding a module-level variable (which may be fine? but may not be? Really depends on import ordering and whether code in settings.py is run before other modules import django.contrib.auth).

Indeed that's why there is a cypress test failing.

What I was suggesting was to change the query parameter of our own oidc login views to use django's default name of next instead of next_path, removing the need for any override.

The thing is swh-auth views for OIDC login use the next_path query parameter while django login views use next by default.
I would prefer to align the basic login query parameter name to the one used in production (as basic login is only used for development).

So I think the best way would be to implement custom decorators wrapping the django ones and setting redirect field name.

Is there anything preventing swh-auth from using next= instead of next_path=?

In D8420#219455, @olasd wrote:

Is there anything preventing swh-auth from using next= instead of next_path=?

I do not think we shared such URLs in the wild as they are used internally during login process
so we could rename the query parameter to next in swh-auth yes, that would be simpler for sure.

In D8420#219455, @olasd wrote:

Is there anything preventing swh-auth from using next= instead of next_path=?

I do not think we shared such URLs in the wild as they are used internally during login process
so we could rename the query parameter to next in swh-auth yes, that would be simpler for sure.

Only swh-deposit doc references a login URL with the next_path query parameter:

16:34 $ mr grep next_path
...
mr grep: /home/anlambert/swh/swh-environment/swh-deposit
docs/api/register-account.rst:<https://archive.softwareheritage.org/oidc/login/?next_path=https://archive.softwareheritage.org/>`_
docs/api/register-account.rst:<https://webapp.staging.swh.network/oidc/login/?next_path=https://webapp.staging.swh.network/>`_

However, that parameter can be omitted in that case so we can safely rename it and I will update that doc.

Use next as redirect field name (default in django.contrib.auth).

However, that parameter can be omitted in that case so we can safely rename it and I will update that doc.

D8451 and D8452

Build is green

Patch application report for D8420 (id=30471)

Rebasing onto c52158a8d7...

Current branch diff-target is up to date.
Changes applied before test
commit 96965c8a1a725a2768f91d7c8492002923e3e50e
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Thu Sep 8 13:51:09 2022 +0200

    auth: Improve login management and configuration
    
    Declare login and logout URLs in django settings:
    
      - basic authentication is used in development mode and when running
        cypress tests
    
      - OIDC authentication is used in production mode and when running
        Python tests (in terms of login/logout URLs)
    
    Do not expose basic authentication login URL in production webapp
    for obvious security reasons.
    
    Align query parameter name for redirection after login in basic
    authentication backend with the OIDC one, it is now named next_path.
    
    Simplify some code in django templates.
    
    Remove documentation about API authentication when using the basic
    django backend as no bearer token can be generated in that case.

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

Build is green

Patch application report for D8420 (id=30478)

Rebasing onto c52158a8d7...

Current branch diff-target is up to date.
Changes applied before test
commit 4362a103295dd3d9a48775e735040d5c79dd0bee
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Thu Sep 8 13:51:09 2022 +0200

    auth: Improve login management and configuration
    
    Declare login and logout URLs in django settings:
    
      - basic authentication is used in development mode and when running
        cypress tests
    
      - OIDC authentication is used in production mode and when running
        Python tests (in terms of login/logout URLs)
    
    Do not expose basic authentication login URL in production webapp
    for obvious security reasons.
    
    Align query parameter name for redirection after login in basic
    authentication backend with the OIDC one, it is now named next_path.
    
    Simplify some code in django templates.
    
    Remove documentation about API authentication when using the basic
    django backend as no bearer token can be generated in that case.

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

This revision is now accepted and ready to land.Sep 13 2022, 12:00 PM

Build is green

Patch application report for D8420 (id=30486)

Rebasing onto 13158c81d2...

Current branch diff-target is up to date.
Changes applied before test
commit 104f3b79bf3a39aca5c95646430dfcd055774db4
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Thu Sep 8 13:51:09 2022 +0200

    auth: Improve login management and configuration
    
    Declare login and logout URLs in django settings:
    
      - basic authentication is used in development mode and when running
        cypress tests
    
      - OIDC authentication is used in production mode and when running
        Python tests (in terms of login/logout URLs)
    
    Do not expose basic authentication login URL in production webapp
    for obvious security reasons.
    
    Align query parameter name for redirection after login in basic
    authentication backend with the OIDC one, it is now named next_path.
    
    Simplify some code in django templates.
    
    Remove documentation about API authentication when using the basic
    django backend as no bearer token can be generated in that case.

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