Skip to content

Redefine nav.models.profiles.Account as a Django AbstractBaseUser #3332

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

lunkwill42
Copy link
Member

@lunkwill42 lunkwill42 commented Mar 20, 2025

This attempts to achieve some of the initial things laid out by @hmpf in #2688. It's up for discussion and refinement, but seems to pass the test suite and simple manual tests.

Fixed #3339

@lunkwill42 lunkwill42 self-assigned this Mar 20, 2025
@lunkwill42 lunkwill42 requested a review from hmpf March 20, 2025 09:17
Copy link

github-actions bot commented Mar 20, 2025

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Warnings Elapsed time
✅ PYTHON ruff 3 0 0 0.28s

See detailed report in MegaLinter reports
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by OX Security

Copy link

github-actions bot commented Mar 20, 2025

Test results

   12 files     12 suites   12m 48s ⏱️
2 187 tests 2 187 ✅ 0 💤 0 ❌
6 036 runs  6 036 ✅ 0 💤 0 ❌

Results for commit a1a40cd.

♻️ This comment has been updated with latest results.

Copy link

codecov bot commented Mar 20, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.76%. Comparing base (0a43109) to head (3396595).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3332      +/-   ##
==========================================
- Coverage   60.79%   60.76%   -0.03%     
==========================================
  Files         606      606              
  Lines       43794    43805      +11     
  Branches       48       48              
==========================================
- Hits        26623    26618       -5     
- Misses      17159    17175      +16     
  Partials       12       12              

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

This changes NAV's Account model to inherit from Django's
AbstractBaseUser in order to move towards more compatibility
with Django libraries for authentication and authorization.
This makes NAV's authentication middleware more or less replace
Django's authentication middleware (for now).

`request.account` was initially chosen by NAV to stay out of Django's
hair - but most Django stuff expects `request.user` to represent the
authenticated user.
@lunkwill42 lunkwill42 force-pushed the feature/add-django-user-model branch from d34d15c to 3396595 Compare March 24, 2025 11:55
@lunkwill42
Copy link
Member Author

This likely has some relevance to @hmpf's research in #2865 . I suspect NAV went with custom session middleware due to issues with how django.contrib.sessions interacted with Django's auth system, vs. how NAV had a custom auth system.

Switching to a Django-compatible auth model may potentially make it possible/easier to ditch our own custom session code as well.

The biggest difference currently seems to be how Django's auth middleware uses a pseudo-user class for anonymous users, while NAV uses an actual anonymous user representation in the database (which is why this PR sticks with the existing custom middleware)

Copy link
Contributor

@hmpf hmpf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine but it's only the first step, many of the cool 3rd party apps that use django.contrib.auth assumes the middleware as well and the fall-through auth-system.

You need to check that the masquerading (user switching) works.

lunkwill42 added 4 commits May 6, 2025 14:11
As per suggestions from code review, it might be a good idea to
implement these to ensure some semblance of compatibility with other
Django libraries that may expect user objects to come from
django.contrib.auth
As suggested in code review, many libraries expect the Django user
account to have an email field.  The AbstractUser implementation even
provides a method to get the name of the field that contains the user's
email address.

NAV currently has no need for emails attached directly to user objects,
so this field is just being added for the sake of compatibility.  It
may slip into usage at a later stage.
rename migration script due to naming conflict
@lunkwill42 lunkwill42 force-pushed the feature/add-django-user-model branch from f645214 to 289d319 Compare May 6, 2025 12:36
@lunkwill42 lunkwill42 requested a review from hmpf May 6, 2025 12:40
Copy link

sonarqubecloud bot commented May 6, 2025

@lunkwill42 lunkwill42 marked this pull request as ready for review May 7, 2025 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert NAV to Django's user model
2 participants