Conversation
The Django auth system uses the `is_active` property to decide whether a user is allowed to log in. NAV's Account class already has the attribute `locked` for this purpose. In order to edge towards compatibility with Django Auth, this adds `is_active` as the negation of `locked`.
Django's auth system uses the generic concept of "natural keys" to fetch user objects by their names (rather than their primary keys), as entered in a login form. In order for the Django auth system to be able to look up NAV user accounts, the Account model and its manager need to provide the necessary calls.
These are necessary for Django's auth system to work. However, since we use our custom migration system, a custom migration script for these apps needs to be created later.
9cb019b to
62603d2
Compare
Test results 27 files 27 suites 45m 53s ⏱️ Results for commit 55c9d73. ♻️ This comment has been updated with latest results. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3619 +/- ##
=======================================
Coverage 62.50% 62.50%
=======================================
Files 611 611
Lines 45093 45103 +10
Branches 43 43
=======================================
+ Hits 28186 28193 +7
- Misses 16897 16900 +3
Partials 10 10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cfc7686 to
12fddee
Compare
lunkwill42
left a comment
There was a problem hiding this comment.
Low hanging fruit that desn't break anything on the road towards full Django auth. Login/logout should work, sudo should work.
I disagree with this assessment: This PR will break LDAP logins in NAV, since it replaces the NAV-specific authentication mechanism in do_login() with Django's authentication mechanism, before we have a working LDAP backend.
We can either:
- Merge the coming
LdapBackendPR into this once it has been posted and approved, then move on with this,
or
- Rip out the parts of this that implements the actual authentication replacement in
do_login(), and make a new PR for the whole login flow replacement once theLdapBackendis approved and merged (i.e one final PR to sew all the parts together to a working whole)
|
Let's go for option 2. |
12fddee to
720763d
Compare
lunkwill42
left a comment
There was a problem hiding this comment.
Just rename the SQL changescript to avoid conflicts with other in-flight PRs :)
720763d to
55c9d73
Compare
|
|
For the PR that merges the do_login change + new LDAP backend we also need to remove our own remote user support and support the default django auth backend (+ middleware). We really need a way to fix sudo for that one though I'm not sure sudo ever worked for our remote user. |
Scope and purpose
Low hanging fruit that desn't break anything on the road towards full Django auth. Login/logout should work, sudo should work.
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.
<major>.<minor>.x). For a new feature or other additions, it should be based onmaster.[ ] 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