Skip to content

Conversation

@lunkwill42
Copy link
Member

@lunkwill42 lunkwill42 commented Oct 23, 2025

Scope and purpose

Fixes #3625. Dependent on #3624

This explicitly sets AUTHENTICATION_BACKENDS in Django settings to include the standard ModelBackend from Django, and the new LdapBackend implemented in #3624.

It changes the existing implementation of NAV's do_login() view to use Django's authentication mechanism, rather than NAV's custom authenticate() function, making the login flow work with the new authentication backends instead.

However, tests are not updated yet. ATM, the failing tests mostly concern themselves with testing the now-removed nav.web.auth.authenticate() function, so these tests will have to be re-thought (likely, they could be removed and rebuilt as tests for the new ldap backend instead?)

Also, these changes to not consider the REMOTE_USER-based login mechanism, so @hmpf may have to look into that before we can merge this.

All in all, the goal is that this should be mergeable before/without completing a transition to implementing django-allauth in NAV.

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
  • Wrote the commit message so that the first line 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/
  • Based this pull request 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 it's not obvious from a linked issue, described how to interact with NAV in order for a reviewer to observe the effects of this change first-hand (commands, URLs, UI interactions)
  • 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

This replaces the authentication mechanism used in the `do_login` view
to instead employ Django's authentication system.
This explicitly sets the `AUTHENTICATION_BACKENDS` setting to include
both the new `LdapBackend` and Django's built-in `ModelBackend` in
the correct order - thereby replicating the legacy LDAP-based login
flow of NAV in Django terms.
The old `nav.web.auth.authenticate()` function has been replaced by
proper Django authentication backends and can therefore be removed.
For some reason, these test modules imported Django models via the
`nav.web.auth` module, which no longer has them.  This just updates
the imports, but many tests still fail, since the
`nav.web.auth.authenticate` function no longer exists.
@lunkwill42 lunkwill42 added 802.1X nonews No news fragment is necessary for this PR (e.g. refactoring, cleanups, workflow/development changes) authentication django labels Oct 23, 2025
@sonarqubecloud
Copy link

@github-actions
Copy link

Test results

    15 files      15 suites   14m 15s ⏱️
 2 625 tests  2 618 ✅ 0 💤  7 ❌
10 338 runs  10 296 ✅ 0 💤 42 ❌

For more details on these failures, see this check.

Results for commit c3bb990.

@codecov
Copy link

codecov bot commented Oct 23, 2025

Codecov Report

❌ Patch coverage is 86.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.41%. Comparing base (27ba495) to head (c3bb990).

Files with missing lines Patch % Lines
python/nav/web/webfront/views.py 84.61% 2 Missing ⚠️
Additional details and impacted files
@@                         Coverage Diff                          @@
##           feature/nav-django-ldap-backend    #3626       +/-   ##
====================================================================
+ Coverage                            18.88%   62.41%   +43.53%     
====================================================================
  Files                                  612      612               
  Lines                                45153    45119       -34     
  Branches                                43       43               
====================================================================
+ Hits                                  8527    28161    +19634     
+ Misses                               36616    16948    -19668     
  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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

802.1X authentication django nonews No news fragment is necessary for this PR (e.g. refactoring, cleanups, workflow/development changes)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace NAV login authentication mechanism with Django authentication backends

3 participants