Fixed #31405 -- Added LoginRequiredAuthenticationMiddleware force all views to require authentication by default. #17792

Hisham-Pak · 2024-01-29T14:45:52Z

Ticket: https://code.djangoproject.com/ticket/31405
Mailing list: https://groups.google.com/forum/#!topic/django-developers/PUQQUHIxEXQ
Follows: #12632

Any suggestions are welcome.

django/contrib/auth/middleware.py

Hisham-Pak · 2024-02-01T04:42:09Z

why does make black expects (at all code blocks):

class MyView(TestMixin1, TestMixin2, View): ...

instead of accepting:

class MyView(TestMixin1, TestMixin2, View):
    ...

nessita · 2024-02-01T11:08:54Z

why does make black expects (at all code blocks):
class MyView(TestMixin1, TestMixin2, View): ...
instead of accepting:
class MyView(TestMixin1, TestMixin2, View):
    ...

I believe the cause of this is that the formatting tool Black has been recently updated to its 2024 style. The Django code has been updated as well, so if you rebase your branch onto main (git rebase main with no uncommitted changes) you will get the necessary fixes.

Hisham-Pak · 2024-02-01T12:53:19Z

My bad, thanks for the fix!

mdisec · 2024-02-01T19:29:23Z

Amazing work @Hisham-Pak !! Couple of weeks ago friend of mine remind me this.. I was literally thinking about coming back and getting the work done on this PR.

Hisham-Pak · 2024-02-02T04:42:36Z

Thank you for the kind words. Appreciate the effort you put in!

sarahboyce

Thank you for this @Hisham-Pak 👍
This is looking good, I added a few comments.

As a side note, we should keep an eye on the async work that @bigfootjon is doing (PR: #18036 discussion: https://forum.djangoproject.com/t/asyncifying-django-contrib-auth-and-signals-and-maybe-sessions/18770/26). This might require some updates here 👍

django/contrib/auth/middleware.py

django/contrib/auth/mixins.py

tests/auth_tests/test_middleware.py

bigfootjon

@sarahboyce thanks for tagging me! I added a few comments inline. The test cases are essential, but the Middleware reworking is nice-to-have.

bigfootjon · 2024-04-03T15:00:32Z

tests/auth_tests/test_middleware.py

+    class EmptyResponseBaseView(View):
+        def get(self, request, *args, **kwargs):
+            return HttpResponse()


I think it would be worthwhile to have a few async test cases as well

bigfootjon · 2024-04-03T15:04:06Z

django/contrib/auth/middleware.py

@@ -34,6 +38,49 @@ def process_request(self, request):
        request.auser = partial(auser, request)


+class LoginRequiredMiddleware(MiddlewareMixin):


Using MiddlewareMixin requires a context-switch in async code. Considering that contrib.auth has an async interface (as applied here: you can use await request.auser() to get the current user, instead of request.user). can this middleware be rewritten to not require a context switch in async code?

Ref: 4296102

I've updated the above commit reference after I added testing and cleaned up the implementation.

bigfootjon

(just a friendly reminder, when your PR is ready for review uncheck the "patch needs improvement" checkbox on the ticket)

bigfootjon · 2024-04-13T18:35:29Z

django/contrib/auth/middleware.py

+    def process_view(self, request, view_func, view_args, view_kwargs):
+        if self.user_is_authenticated:
+            return None
+
+        view_class = getattr(view_func, "view_class", None)
+        if view_class and not getattr(view_class, "login_required", True):
+            return None
+
+        if not getattr(view_func, "login_required", True):
+            return None
+
+        try:
+            if request.path.startswith(reverse("admin:index")):
+                return redirect_to_login(
+                    request.get_full_path(), reverse("admin:login")
+                )
+        except NoReverseMatch:
+            pass
+
+        return redirect_to_login(request.get_full_path())


Defining process_view will still result in context switches :(

Ref this code:

https://github.com/django/django/blob/main/django/core/handlers/base.py#L77-L81

I think you need to rename this function to something else (so it doesn't get caught by that auto-rewrite code) and then call it from __call__ / __acall__

Yes, I thought nothing in process_view was io bound so it didn't really matter much if it got wrapped in sync_to_async?

The way I see to achieve no context switch here is to have our local resolve_request. But I don't know if its worth it? What do you think?

Yes, I thought nothing in process_view was io bound so it didn't really matter much if it got wrapped in sync_to_async?

Wrapping in sync_to_async still causes a context switch / performance hit :(

Yes, I thought nothing in process_view was io bound so it didn't really matter much if it got wrapped in sync_to_async?

The way I see to achieve no context switch here is to have our local resolve_request. But I don't know if its worth it? What do you think?

I don't think that's necessary, I think you can just change the name to not hit any of the names that will be rewritten and then call this function from __call__ / __acall__.

Yes, but the thing is __call__/__acall__ is not provided a view_func/callback it is only passed to process_view here only after resolving the request.

So the way forward I see is:

Using a private resolve_request (this gets called twice here and in handler)

Changing how handler works to support native async (not using same process_view name for both sync and async)

Finding a way to not use view_func/callback at all (I didn't look much into it yet)

Am I missing something? Thank you for your patience.

Changing how handler works to support native async (not using same process_view name for both sync and async)

I'm kinda in favor of this idea, but I can understand your hesitancy towards doing it. I think for now we can leave this as you have implemented in this PR and I'll think about how we can solve it better in the general case.

+1 for changing the handler, but in a separate ticket and PR.

bigfootjon

Async code LGTM, and thanks for adding the test cases. I leave the rest of the review to @sarahboyce et al

bigfootjon · 2024-04-14T00:02:33Z

django/contrib/auth/middleware.py

+    def process_view(self, request, view_func, view_args, view_kwargs):
+        if self.user_is_authenticated:
+            return None
+
+        view_class = getattr(view_func, "view_class", None)
+        if view_class and not getattr(view_class, "login_required", True):
+            return None
+
+        if not getattr(view_func, "login_required", True):
+            return None
+
+        try:
+            if request.path.startswith(reverse("admin:index")):
+                return redirect_to_login(
+                    request.get_full_path(), reverse("admin:login")
+                )
+        except NoReverseMatch:
+            pass
+
+        return redirect_to_login(request.get_full_path())


Changing how handler works to support native async (not using same process_view name for both sync and async)

I'm kinda in favor of this idea, but I can understand your hesitancy towards doing it. I think for now we can leave this as you have implemented in this PR and I'll think about how we can solve it better in the general case.

tests/auth_tests/urls.py

sarahboyce

Thank you for the updates @Hisham-Pak - we're making progress 🔥

please see this comment
we are missing .. versionadded:: 5.1 in a few places

Other note, if you rebase with the latest changes from main the Python 3.11 failures will go away 👍

adamchainz

Thanks for taking this on. This is a small feature I’ve wanted to see for a while.

The current approach is insecure for some server configurations - please see my comment on __call__.

django/contrib/auth/mixins.py

adamchainz · 2024-04-26T19:12:18Z

django/contrib/auth/middleware.py

+    def process_view(self, request, view_func, view_args, view_kwargs):
+        if self.user_is_authenticated:
+            return None
+
+        view_class = getattr(view_func, "view_class", None)
+        if view_class and not getattr(view_class, "login_required", True):
+            return None
+
+        if not getattr(view_func, "login_required", True):
+            return None
+
+        try:
+            if request.path.startswith(reverse("admin:index")):
+                return redirect_to_login(
+                    request.get_full_path(), reverse("admin:login")
+                )
+        except NoReverseMatch:
+            pass
+
+        return redirect_to_login(request.get_full_path())


+1 for changing the handler, but in a separate ticket and PR.

adamchainz · 2024-04-26T19:15:27Z

django/contrib/auth/middleware.py

+        self.user_is_authenticated = request.user.is_authenticated
+        return self.get_response(request)
+
+    async def __acall__(self, request):
+        self.user_is_authenticated = (await request.auser()).is_authenticated


The current approach is insecure. Storing values on self allows those values to be shared across threads and async tasks. This leads to the possibility that two concurrent requests to a threaded/async server will mix up the “is authenticated” values and incorrectly show a private page.

Please change the middleware to inherit from MiddlewareMixin, drop the custom __init__, __call__ and __acall__, and only implement process_view.

Thanks for confirming! Reverted async stuff in 3dd63ed

django/contrib/auth/middleware.py

angusholder · 2024-04-27T20:43:03Z

django/contrib/auth/mixins.py

@@ -73,6 +74,15 @@ def dispatch(self, request, *args, **kwargs):
        return super().dispatch(request, *args, **kwargs)


+class LoginNotRequiredMixin:


I might be missing something, but what's the reason for also having a mixin class? It seems like the @login_not_required decorator will work perfectly well for class-based views too no?

I believe it largely comes down to personal preferences.

sarahboyce

Thank you for the updates!
I have a few more comments, can you also rebase and squash the commits? ⭐

django/contrib/admin/sites.py

docs/ref/middleware.txt

docs/releases/5.1.txt

docs/topics/auth/default.txt

django/contrib/auth/middleware.py

docs/topics/auth/default.txt

django/contrib/admin/sites.py

django/contrib/auth/checks.py

django/contrib/auth/decorators.py

django/contrib/auth/middleware.py

adamchainz · 2024-05-14T21:59:51Z

django/contrib/auth/middleware.py

+        """
+        Override this method to override the login_url attribute.
+        """
+        login_url = self.login_url or getattr(view_func, "login_url", None)


Isn’t this precedence order incorrect? I would expect we prefer values from view_func over self.login_url. If we keep the current implementation, a customized middleware will, for example, make admin views redirect to the middleware’s login page rather than the admin one.

Good catch, updated 👍

adamchainz · 2024-05-14T22:10:48Z

I suggest we do not support class-based attributes on views, as currently defined in LoginNotRequiredMixin, and as @angusholder previously queried.

Supporting the class-level attributes adds significant complexity to the middleware implementation, as it has to look in two places on views for all of the attributes (login_required, login_url, redirect_field_name`). This makes it harder to understand the middleware, as well as any class-based views that might combine both options.

Instead, I think we should only support view-function-level attributes. Class-based views can use @method_decorator, as covered in the “Decorating the class” docs.

This would introduce the oddity that if you subclass a View with @method_decorator(login_not_required, name='dispatch') applied, and override dispatch(), that new dispatch() method will not have the login_required = False attribute. But we consider this okay as it “fails closed”, the view simply isn’t public until the decorator is reapplied in the subclass.

Alternatively, we could make the mixin still work with an implementation like:

class LoginNotRequiredMixin(View):
    def __init_subclass__(cls):
        method_decorator(login_not_required, name='dispatch')(cls)

But IMO better to go with only @method_decorator here, to keep the API minimal for this critical security feature. We can always add a mixin later if there is demand.

django/contrib/auth/mixins.py

adamchainz

Thanks Sarah, that has really tidied up the implementation. I spotted a couple places where view_class is still being looked at. Can you check the test coverage of the middleware?

I also agree on removing the raise_exception feature.

adamchainz · 2024-05-16T06:35:09Z

django/contrib/auth/middleware.py

+        login_url = getattr(view_func, "login_url", None)
+        if hasattr(view_func, "view_class"):
+            login_url = getattr(view_func.view_class, "login_url", login_url)
+        login_url = login_url or self.login_url or settings.LOGIN_URL


The view_class block can be removed not that we’re no longer using class-level attributes.

Suggested change

login_url = getattr(view_func, "login_url", None)

if hasattr(view_func, "view_class"):

login_url = getattr(view_func.view_class, "login_url", login_url)

login_url = login_url or self.login_url or settings.LOGIN_URL

login_url = (

getattr(view_func, "login_url", None)

or self.login_url

or settings.LOGIN_URL

)

If you were using LoginRequiredMixin before, set the login_url this would now be ignored. I don't think we should ignore it or we will have to state that the LoginRequiredMixin is incompatible?

I don't intuitive that we respect the login_required decorator attributes but not the LoginRequiredMixin attributes

Updated, I also removed the login_url attribute on the middleware as couldn't find the extra value compared to setting the LOGIN_URL setting

Oh, right. Yeah, it’s not intuitive that LoginRequiredMixin wouldn’t be respected. I guess we can add back class-level support for login_url only..?

redirect_field_name as well otherwise redirect could be broken after login if it is not the default "next"?

I feel we should either support all the ways login_url and redirect_field_name can be updated through LoginRequiredMixin or not support it.
If we think we need to support it, I can revert the recent changes.
If we think we can keep this as it is but it is not clear in the docs currently I can add a note.

Added to the release notes that LoginRequiredMixin is not respected. If users which to use rather than the login_required decorator, this can be added in a feature release 👍

django/contrib/auth/middleware.py

nessita

Hey @sarahboyce, as agreed I went over the code and the docs, I think this is in a very good shape and we can land before feature freeze. I think most of my comments can be addressed, if necessary, in a follow up PR as bug fixes for the feature. I would like to have the missing test (or exception removed from the except block), a definitive check error msg before landing though... would that be doable? Thank you!!! 🌟

django/contrib/auth/checks.py

django/contrib/auth/decorators.py

django/contrib/auth/middleware.py

docs/releases/5.1.txt

docs/topics/auth/default.txt

Co-authored-by: Adam Johnson <me@adamj.eu> Co-authored-by: Mehmet İnce <mehmet@mehmetince.net> Co-authored-by: Sarah Boyce <42296566+sarahboyce@users.noreply.github.com>

Hisham-Pak commented Jan 29, 2024

View reviewed changes

django/contrib/auth/middleware.py Outdated Show resolved Hide resolved

Hisham-Pak force-pushed the ticket_31405 branch 2 times, most recently from 90e31fe to 1f5f02a Compare January 31, 2024 14:29

Hisham-Pak marked this pull request as ready for review January 31, 2024 14:49

Hisham-Pak force-pushed the ticket_31405 branch 2 times, most recently from 7a22f7b to 75e3ce9 Compare February 1, 2024 04:26

Hisham-Pak force-pushed the ticket_31405 branch from 75e3ce9 to 18a849e Compare February 1, 2024 12:51

sarahboyce reviewed Apr 3, 2024

View reviewed changes

django/contrib/auth/middleware.py Outdated Show resolved Hide resolved

django/contrib/auth/middleware.py Outdated Show resolved Hide resolved

django/contrib/auth/mixins.py Outdated Show resolved Hide resolved

tests/auth_tests/test_middleware.py Show resolved Hide resolved

bigfootjon suggested changes Apr 3, 2024

View reviewed changes

Hisham-Pak mentioned this pull request Apr 13, 2024

Refs #31405 -- Moved redirect_to_login to utils.py. #18072

Closed

6 tasks

Hisham-Pak requested a review from bigfootjon April 13, 2024 12:18

bigfootjon suggested changes Apr 13, 2024

View reviewed changes

bigfootjon approved these changes Apr 14, 2024

View reviewed changes

sarahboyce reviewed Apr 15, 2024

View reviewed changes

tests/auth_tests/urls.py Show resolved Hide resolved

sarahboyce reviewed Apr 15, 2024

View reviewed changes

adamchainz suggested changes Apr 26, 2024

View reviewed changes

angusholder reviewed Apr 27, 2024

View reviewed changes

sarahboyce reviewed Apr 29, 2024

View reviewed changes

django/contrib/admin/sites.py Outdated Show resolved Hide resolved

docs/ref/middleware.txt Outdated Show resolved Hide resolved

docs/releases/5.1.txt Outdated Show resolved Hide resolved

docs/topics/auth/default.txt Outdated Show resolved Hide resolved

Hisham-Pak force-pushed the ticket_31405 branch 3 times, most recently from dbfe048 to a369114 Compare May 5, 2024 06:21

Hisham-Pak commented May 5, 2024

View reviewed changes

django/contrib/auth/middleware.py Outdated Show resolved Hide resolved

Hisham-Pak force-pushed the ticket_31405 branch from a369114 to c5c7b20 Compare May 12, 2024 10:37

sarahboyce force-pushed the ticket_31405 branch 2 times, most recently from 91e23ab to c1d04b9 Compare May 13, 2024 09:00

nessita reviewed May 14, 2024

View reviewed changes

docs/topics/auth/default.txt Outdated Show resolved Hide resolved

sarahboyce force-pushed the ticket_31405 branch from 064ea8a to 3a1221e Compare May 14, 2024 16:25

sarahboyce changed the title ~~Refs #31405 -- Added LoginRequiredAuthenticationMiddleware force all views to require authentication by default~~ Fixed #31405 -- Added LoginRequiredAuthenticationMiddleware force all views to require authentication by default. May 14, 2024

sarahboyce force-pushed the ticket_31405 branch from 3a1221e to dd90c43 Compare May 14, 2024 16:33

sarahboyce requested a review from nessita May 14, 2024 16:37

adamchainz reviewed May 14, 2024

View reviewed changes

sarahboyce force-pushed the ticket_31405 branch 3 times, most recently from 0f42613 to e98c613 Compare May 15, 2024 08:00

sarahboyce requested a review from adamchainz May 15, 2024 10:36

Hisham-Pak commented May 15, 2024

View reviewed changes

django/contrib/auth/mixins.py Outdated Show resolved Hide resolved

sarahboyce force-pushed the ticket_31405 branch from e98c613 to 579260f Compare May 15, 2024 12:01

adamchainz suggested changes May 16, 2024

View reviewed changes

sarahboyce force-pushed the ticket_31405 branch 6 times, most recently from a6e5420 to f40a7e5 Compare May 17, 2024 12:35

sarahboyce requested a review from adamchainz May 17, 2024 14:05

knyghty reviewed May 20, 2024

View reviewed changes

django/contrib/auth/middleware.py Show resolved Hide resolved

sarahboyce force-pushed the ticket_31405 branch 2 times, most recently from d65bd04 to e208f8a Compare May 21, 2024 14:38

nessita approved these changes May 21, 2024

View reviewed changes

sarahboyce force-pushed the ticket_31405 branch 2 times, most recently from 79a97df to 55a37e5 Compare May 21, 2024 22:17

Fixed #31405 -- Added LoginRequiredMiddleware.

c86e727

Co-authored-by: Adam Johnson <me@adamj.eu> Co-authored-by: Mehmet İnce <mehmet@mehmetince.net> Co-authored-by: Sarah Boyce <42296566+sarahboyce@users.noreply.github.com>

sarahboyce force-pushed the ticket_31405 branch from 55a37e5 to c86e727 Compare May 22, 2024 05:47

sarahboyce merged commit c7fc9f2 into django:main May 22, 2024
35 checks passed

		@@ -34,6 +38,49 @@ def process_request(self, request):
		request.auser = partial(auser, request)


		class LoginRequiredMiddleware(MiddlewareMixin):

		@@ -73,6 +74,15 @@ def dispatch(self, request, args, *kwargs):
		return super().dispatch(request, args, *kwargs)


		class LoginNotRequiredMixin:

Fixed #31405 -- Added LoginRequiredAuthenticationMiddleware force all views to require authentication by default. #17792

Fixed #31405 -- Added LoginRequiredAuthenticationMiddleware force all views to require authentication by default. #17792

Conversation

Hisham-Pak commented Jan 29, 2024 • edited

Hisham-Pak commented Feb 1, 2024 • edited

nessita commented Feb 1, 2024

Hisham-Pak commented Feb 1, 2024

mdisec commented Feb 1, 2024

Hisham-Pak commented Feb 2, 2024

sarahboyce left a comment

Choose a reason for hiding this comment

bigfootjon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bigfootjon Apr 3, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bigfootjon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hisham-Pak Apr 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bigfootjon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarahboyce left a comment

Choose a reason for hiding this comment

adamchainz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarahboyce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamchainz commented May 14, 2024

adamchainz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarahboyce May 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nessita left a comment

Choose a reason for hiding this comment

Hisham-Pak commented Jan 29, 2024 •

edited

Hisham-Pak commented Feb 1, 2024 •

edited

bigfootjon Apr 3, 2024 •

edited

Hisham-Pak Apr 13, 2024 •

edited

sarahboyce May 20, 2024 •

edited