POC: Use Django authentication system with NAV's Account model

[lunkwill42]

Scope and purpose

This builds a proof-of-concept for #2688, by replacing the internal NAV mechanism used by the do_login view for authenticating users with Django's standard mechanism (the ModelBackend by default).

The POC temporarily disables LDAP authentication to show how this can be achieved. LDAP authentication should probably be re-implemented as a new authentication backend to be added to NAV's list of Django authentication backends.

The django.contrib.auth and django.contrib.contenttypes apps are added to the INSTALLED_APPS setting to enable this. ATM, the POC works, but since NAV uses a custom migration system, the Django development web server will complain that there are unapplied migrations for these apps. The database models provided by these apps aren't touched by NAV, which is why it still works without them - but, as I believe has happened before, the migrations should probably be replicated as pure SQL and squashed into a single NAV migration script (switching to the Django migration system would likely be a huge endeavor, and is out of scope here).

Contributor Checklist

Every pull request should have this checklist filled out, no matter how small it is.
More information about contributing to NAV can be found in the
Hacker's guide to NAV.

• Added a changelog fragment for towncrier
• Added/amended tests for new/changed code
• Added/changed documentation
• Linted/formatted the code with ruff, easiest by using pre-commit
• The first line of the commit message continues the sentence "If applied, this commit will ...", starts with a capital letter, does not end with punctuation and is 50 characters or less long. See https://cbea.ms/git-commit/
• This pull request is based on the correct upstream branch: For a patch/bugfix affecting the latest stable version, it should be based on that version's branch (<major>.<minor>.x). For a new feature or other additions, it should be based on master.
• If applicable: Created new issues if this PR does not fix the issue completely/there is further work to be done
• If this results in changes in the UI: Added screenshots of the before and after
• If this adds a new Python source code file: Added the boilerplate header to that file

[github-actions]

Test results

    9 files      9 suites   7m 59s ⏱️
2 485 tests 2 484 ✅ 0 💤 1 ❌
5 699 runs  5 697 ✅ 0 💤 2 ❌

For more details on these failures, see this check.

Results for commit 6d59681.

♻️ This comment has been updated with latest results.

[codecov]

Codecov Report

❌ Patch coverage is 87.77778% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.76%. Comparing base (4a38fe3) to head (6d59681).
⚠️ Report is 57 commits behind head on master.

Files with missing lines	Patch %	Lines
python/nav/web/auth/utils.py	72.72%	6 Missing ⚠️
python/nav/web/modpython.py	0.00%	2 Missing ⚠️
python/nav/models/profiles.py	88.88%	1 Missing ⚠️
python/nav/web/auth/middleware.py	93.33%	1 Missing ⚠️
python/nav/web/navlets/welcome.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3493      +/-   ##
==========================================
+ Coverage   61.35%   61.76%   +0.41%     
==========================================
  Files         611      611              
  Lines       44782    44860      +78     
  Branches       43       43              
==========================================
+ Hits        27476    27709     +233     
+ Misses      17296    17141     -155     
  Partials       10       10              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[lunkwill42]

the Django development web server will complain that there are unapplied migrations for these apps.

And as evidenced by the test suite, the tests will fail because of this: We test that every model defined in the system seems to work on the SQL level, and naturally those tests will fail for the new models introduced by the new apps in INSTALLED_APPS.

[hmpf]

Missing: django expects "request.user" to be set, we have hardcoded on "request.account". With this, both are supported.

[lunkwill42]

Should probably switch places

[lunkwill42]

It is also highly likely that nav.web.modpython needs to be updated for this. It uses a hard-coded list of middleware to decide if a request should be allowed or redirected to login.

[lunkwill42]

Missing: django expects "request.user" to be set, we have hardcoded on "request.account". With this, both are supported.

And, as discussed, I think this was already fixed in #3332. But the broader scope here would be to stop using request.account altogether, and move everything to use request.user.

[hmpf]

There are two failing tests now:

tests/integration/sql_test.py

The tables and other things that Django expects to be there (auth.(Group|Permission), ContentType) are put in the public namespace (what postgres calls a schema) and this test checks that the public namespace is empty.

tests/integration/web/webfront_test.py::test_shows_password_issue_banner_to_admins_on_other_users_password_issues

The string "There are 1 accounts that have insecure or old passwords." does not show up in the generated page.

[hmpf]

tests/integration/sql_test.py

The tables and other things that Django expects to be there (auth.(Group|Permission), ContentType) are put in the public namespace (what postgres calls a schema) and this test checks that the public namespace is empty.

Manually putting the Django tables into the "profiles" schema fixed this test. If we add more non-NAV tables we should probably reconsider what schemas to use, I would prefer one per app.

[hmpf]

tests/integration/web/webfront_test.py::test_shows_password_issue_banner_to_admins_on_other_users_password_issues

The string "There are 1 accounts that have insecure or old passwords." does not show up in the generated page.

The string is set by the context processor nav.django.context_processors.account_processor and shown in base.html.

Liberally spreading "assert False" inside account_processor doesn't crash the test as it should, maybe the context processor isn't used at all?

[hmpf]

The context processor is used but does not get an admin user.

The context from account_processor does show up in the rendered template, but pytest helpfully strips out the middle of the HTML when you use "assert", making the HTML seem to only contain the first twenty and last twenty lines of the rendered template.

[lunkwill42]

Manually putting the Django tables into the "profiles" schema fixed this test. If we add more non-NAV tables we should probably reconsider what schemas to use, I would prefer one per app.

Might add a historic note here: As you know, NAV web tools were never divided into "apps" in the first place, because much of the web code predates Django. The reason there are multiple namespaces in the PostgreSQL database at all is that there were originally at least four separate PostgreSQL databases involved in NAV - which, of course, made it impossible to utilize proper referential integrity between parts of the data. The namespaces we use today are remnants of this: They are named after the originally separate databases.

[hmpf]

The context processor gets the anonymous user actually (default account).

[hmpf]

The client pytest fixture correctly logs in the admin, and before the redirect to origin (in this test's case: webfront-index) the request has both account and user. The session has the expected keys for a django auth login.

[hmpf]

There are two different ways to check if an account has admin powers:

1. nav.django.utils.is_admin: account.groups.filter(pk=AccountGroup.ADMIN_GROUP).count() > 0
2. account.is_admin_(): via account.has_perm to:

   groups = self.get_groups()  # via self._cached_groups which is a list of group ids
   ...
   if AccountGroup.ADMIN_GROUP in groups:
        return True
   

Both ways could be account.groups.filter(pk=AccountGroup.ADMIN_GROUP).exists() but I digress.

[hmpf]

If you log in with github, an account and it's social auth table is correctly created. But: the user lacks a name and password so it is locked, and the user is sent straight to the login page.

1. Being unlocked seems to need a password. This can be fixed by one of:
   • generating a fake password in a pipeline
   • asking for a new password in a pipeline
   • not allowing unknown users to log in willy-nilly and pre-creating them then connecting a social auth
2. Not having a name can most easily be fixed by adding/replacing the pipeline piece that fills in the user from the social auth data, probably social_core.pipeline.user.user_details.

Setting password and setting other user data, if done in two different steps, could be configurable. So: either ask for a password or set a random password.

I kinda want to set up a test-NAV with this now...

[hmpf]

How data from the external auth is mapped to the local account can be controlled via SOCIAL_AUTH_<backend>_USER_FIELD_MAPPING = {"remote_field_name": "local_field_name"} but it turns out github in particular do not supply anything that will work for the Account.name field. Partial pipeline it is!

[hmpf]

So I have a partial pipeline that shows a form that asks for a new password, but if the pipeline handles the save, the form data will be sent as GET-parameters..

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud