pennersr / django-allauth

Integrated set of Django applications addressing authentication, registration, account management as well as 3rd party (social) account authentication.
https://allauth.org
MIT License
9.52k stars 3.03k forks source link

Django-allauth views not compatible with Django 5.1 LoginRequiredMiddleware #4001

Closed danjac closed 1 month ago

danjac commented 2 months ago

As of Django 5.1, the LoginRequiredMiddleware will be part of the Django core. This will be included by default with new Django projects in the MIDDLEWARE setting. All Django views will redirect to LOGIN_URL by default if the user is not authenticated.

There is a decorator login_not_required which should be added to any views you do not wish to apply the redirect.

Currently django-allauth login, signup and other views do not use this decorator, so will cause an infinite loop when trying to access them.

As Django 5.1 is due for production release in a few weeks, is there a branch for handling 5.1 compatibility?

pennersr commented 2 months ago

This will be included by default with new Django projects in the MIDDLEWARE setting.

Where did you read that? It's not documented here:

https://docs.djangoproject.com/en/dev/topics/http/middleware/#activating-middleware

AFAICT, LoginRequiredMiddleware is not enabled by default, meaning, out of the box, django-allauth is compatible with Django 5.1

danjac commented 2 months ago

That is perhaps my misunderstanding. Nevertheless, it is likely this middleware will be commonly used in projects that want to provide a default authentication check for all views, so is there a plan for adding the login_not_required middleware to all-auth views?

pennersr commented 2 months ago

it is likely this middleware will be commonly used

Let's see how the eco system around this change evolves first, switching to LoginRequiredMiddleware would break a lot of upstream projects, so it will definitely take some time for that to become the default, if we ever reach that point.

Having said that, if anybody wants to propose a solution for gracefully handling this within django-allauth, feel free to go ahead.

mcastle commented 2 months ago

There may be limited if any need for allauth to switch to using the new LoginRequiredMiddleware, as the middleware can just be subclassed by a custom middleware and adapted for a given project's needs. For example, the below middleware uses the new LoginRequiredMiddleware to require authentication for all urls except for the login url and any other urls in OPEN_URLS:

class LoginRequiredMiddleware(LoginRequiredMiddleware):
    def __init__(self, get_response):
        self.login_url = reverse(settings.LOGIN_URL)
        self.open_urls = [self.login_url, *getattr(settings, "OPEN_URLS", [])]
        super().__init__(get_response)

    def process_view(self, request, view_func, view_args, view_kwargs):
        if request.path_info in self.open_urls:
            return None
        return super().process_view(request, view_func, view_args, view_kwargs)
danjac commented 2 months ago

There may be limited if any need for allauth to switch to using the new LoginRequiredMiddleware, as the middleware can just be subclassed by a custom middleware and adapted for a given project's needs. For example, the below middleware uses the new LoginRequiredMiddleware to require authentication for all urls except for the login url and any other urls in OPEN_URLS:

class LoginRequiredMiddleware(LoginRequiredMiddleware):
    def __init__(self, get_response):
        self.login_url = reverse(settings.LOGIN_URL)
        self.open_urls = [self.login_url, *getattr(settings, "OPEN_URLS", [])]
        super().__init__(get_response)

    def process_view(self, request, view_func, view_args, view_kwargs):
        if request.path_info in self.open_urls:
            return None
        return super().process_view(request, view_func, view_args, view_kwargs)

Given that there is a decorator login_not_required it would probably be easier to wrap all the allauth URLs (or at least the ones you are using) e.g.

from allauth.accounts import views
from django.contrib.auth.decorators import login_not_required

login = login_not_required(views.login)
mcastle commented 2 months ago

There may be limited if any need for allauth to switch to using the new LoginRequiredMiddleware, as the middleware can just be subclassed by a custom middleware and adapted for a given project's needs. For example, the below middleware uses the new LoginRequiredMiddleware to require authentication for all urls except for the login url and any other urls in OPEN_URLS:

class LoginRequiredMiddleware(LoginRequiredMiddleware):
    def __init__(self, get_response):
        self.login_url = reverse(settings.LOGIN_URL)
        self.open_urls = [self.login_url, *getattr(settings, "OPEN_URLS", [])]
        super().__init__(get_response)

    def process_view(self, request, view_func, view_args, view_kwargs):
        if request.path_info in self.open_urls:
            return None
        return super().process_view(request, view_func, view_args, view_kwargs)

Given that there is a decorator login_not_required it would probably be easier to wrap all the allauth URLs (or at least the ones you are using) e.g.

from allauth.accounts import views
from django.contrib.auth.decorators import login_not_required

login = login_not_required(views.login)

That's also a good approach. It would need to be done at the url level:

from allauth.account import views
from django.contrib.auth.decorators import login_not_required

path("accounts/login/", login_not_required(views.login), name="account_login"),
Flimm commented 2 months ago

For django-allauth to support projects that use LoginRequiredMiddleware, I'm pretty sure it only needs to apply the login_not_required decorator to views that accept unauthenticated users. That decorator is very simple, here is in its entire implementation:

def login_not_required(view_func):
    """
    Decorator for views that allows access to unauthenticated requests.
    """
    view_func.login_required = False
    return view_func
pennersr commented 1 month ago

Moved to https://codeberg.org/allauth/django-allauth/issues/4001

pennersr commented 1 month ago

FYI: https://codeberg.org/allauth/django-allauth/pulls/4104

pennersr commented 1 month ago

Supported via d096a0b6