Support login_required in AuthorizationMiddleware#3676
Conversation
|
Test results 13 files 13 suites 24m 56s ⏱️ Results for commit ae07bfa. ♻️ This comment has been updated with latest results. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3676 +/- ##
==========================================
+ Coverage 63.74% 63.82% +0.07%
==========================================
Files 624 624
Lines 46154 46175 +21
Branches 43 43
==========================================
+ Hits 29421 29470 +49
+ Misses 16723 16695 -28
Partials 10 10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
fd4c392 to
6fd3bd4
Compare
ff6fcd3 to
3977a3f
Compare
3977a3f to
f3aa882
Compare
lunkwill42
left a comment
There was a problem hiding this comment.
The change in flow of logic seems reasonable here, but I am still not sure how this will actually affect mod-python...
We should clean up the modpython stuff anyway (out of scope). The code is as it is to make it work as before, without any deep thinking. Maybe we should have a "modpython" privilege in addition to "web_access"? |
Simrayz
left a comment
There was a problem hiding this comment.
I think this is good to go after re-organizing the utils 🚀
changelog.d/3676.added.md
Outdated
| @@ -0,0 +1,4 @@ | |||
| Made NAV's authorization system compatible with the LoginRequiredMiddleware | |||
| that was added in Django 5.2. A view can now be explicitly marked as not | |||
There was a problem hiding this comment.
Nit: The middleware was added in 5.1 right? Does the changelog refer to the correct version?
lunkwill42
left a comment
There was a problem hiding this comment.
Alright, as long as we follow up on this by refactoring to the mentioned utils module (which is a nice idea), I'm good with this as-is :-)
6f7a35c to
1356714
Compare
Make AuthorizationMiddleware compatible with Django's LoginRequiredMiddlware. With the latter, if a view has the attribute "login_required" set to False, the view is public and everyone is authorized to access it. The way we reuse Django's middlewares for ModPython is not compatible with middlewares that process the view, so we need a ModPython-only middleware for authorization.
Co-authored-by: Morten Brekkevold <morten.brekkevold@sikt.no>
Remove when we are on Django 5.2!
1356714 to
ae07bfa
Compare
|
Simrayz
left a comment
There was a problem hiding this comment.
Nice! That looks a lot better organization wise.
Make AuthorizationMiddleware compatible with Django's LoginRequiredMiddleware. With the latter, if a view has the attribute "login_required" set to False, the view is public and everyone is authorized to access it.
This is necessary for supporting django-allauth's "social" login methods (oidc, saml).
This change should not be visible to end users or affect existing views, it will only take effect if we include 3rd party views that are adapted to LoginRequiredMiddleware.
Since ModPython cannot use middlewares that process views, we refactor out what modpython needs into functions used in both middlewares. (Easier to test, too.)
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 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