Repository: pennersr/django-allauth
Pull request #3818 (2024-05-15 19:10:59)

Add secure_admin_login() decorator
Alternative to #3809 

coveralls: 
[![Coverage Status](https://coveralls.io/builds/67512785/badge)](https://coveralls.io/builds/67512785)

coverage: 95.526% (-0.08%) from 95.607%
when pulling **03809d69926be79f42820cf097396e24450dcc39 on feat-secure-admin-login**
into **ed376425c313e90159c839cd3e1b37a2e0bb986a on main**.


Patch:
diff --git a/ChangeLog.rst b/ChangeLog.rst
index 3f9899b1..9522d7bd 100644
--- a/ChangeLog.rst
+++ b/ChangeLog.rst
@@ -1,12 +1,21 @@
 0.63.1 (unreleased)
 *******************
 
+Note worthy changes
+-------------------
+
 - Subpackages from the ``tests`` package were packaged, fixed.
 
 - When ``SOCIALACCOUNT_EMAIL_AUTHENTICATION`` was turned on, and a user would
   connect a third-party account for which email authentication would kick in,
   the connect was implicitly skipped. Fixed.
 
+- The recommendation from the documentation to protect the Django admin login
+  could cause an infinite redirect loop in case of
+  ``AUTHENTICATED_LOGIN_REDIRECTS``. A decorator ``secure_admin_login()`` is now
+  offered out of the box ensure that the Django admin is properly secured by
+  allauth (e.g. rate limits, 2FA).
+
 
 0.63.0 (2024-05-14)
 *******************
diff --git a/allauth/account/decorators.py b/allauth/account/decorators.py
index dd89070f..77c9b004 100644
--- a/allauth/account/decorators.py
+++ b/allauth/account/decorators.py
@@ -1,13 +1,20 @@
 from functools import wraps
 
+from django.conf import settings
 from django.contrib.auth import REDIRECT_FIELD_NAME
 from django.contrib.auth.decorators import login_required
-from django.shortcuts import render
+from django.core.exceptions import PermissionDenied
+from django.http import HttpResponseRedirect
+from django.shortcuts import render, resolve_url
 
 from allauth.account import reauthentication
 from allauth.account.models import EmailAddress
-from allauth.account.utils import send_email_confirmation
+from allauth.account.utils import (
+    get_next_redirect_url,
+    send_email_confirmation,
+)
 from allauth.core.exceptions import ReauthenticationRequired
+from allauth.core.internal import httpkit
 
 
 def verified_email_required(
@@ -68,3 +75,28 @@ def reauthentication_required(
     if function:
         return decorator(function)
     return decorator
+
+
+def secure_admin_login(function=None):
+    def decorator(view_func):
+        @wraps(view_func)
+        def _wrapper_view(request, *args, **kwargs):
+            if request.user.is_authenticated:
+                if not request.user.is_staff or not request.user.is_active:
+                    raise PermissionDenied()
+                return view_func(request, *args, **kwargs)
+            else:
+                next_url = get_next_redirect_url(request)
+                if not next_url:
+                    next_url = request.get_full_path()
+                login_url = resolve_url(settings.LOGIN_URL)
+                login_url = httpkit.add_query_params(
+                    login_url, {REDIRECT_FIELD_NAME: next_url}
+                )
+                return HttpResponseRedirect(login_url)
+
+        return _wrapper_view
+
+    if function:
+        return decorator(function)
+    return decorator
diff --git a/docs/common/admin.rst b/docs/common/admin.rst
index 11b8e883..10f6e8a1 100644
--- a/docs/common/admin.rst
+++ b/docs/common/admin.rst
@@ -10,8 +10,8 @@ the normal Django allauth workflow.
     This limitation means that Django allauth features are not applied to the
     Django admin site:
 
-    * ``ACCOUNT_LOGIN_ATTEMPTS_LIMIT`` and ``ACCOUNT_LOGIN_ATTEMPTS_TIMEOUT`` do not
-      protect Django’s admin login from being brute forced.
+    * The admin login is not protected from being brute forced (``ACCOUNT_RATE_LIMITS``).
+    * Two-factor authentication is not enforced.
     * Any other custom workflow that overrides the Django allauth adapter's
       login method will not be applied.
 
@@ -21,10 +21,8 @@ every instance of ``AdminSite``):
 
 .. code-block:: python
 
-    from django.conf import settings
     from django.contrib import admin
-    from django.contrib.admin.views.decorators import staff_member_required
+    from allauth.account.decorators import secure_admin_login
 
-    admin.site.login = staff_member_required(
-        admin.site.login, login_url=settings.LOGIN_URL
-    )
+    admin.autodiscover()
+    admin.site.login = secure_admin_login(admin.site.login)
diff --git a/examples/regular-django/example/urls.py b/examples/regular-django/example/urls.py
index ffdf8ec2..45620c73 100644
--- a/examples/regular-django/example/urls.py
+++ b/examples/regular-django/example/urls.py
@@ -2,8 +2,11 @@ from django.contrib import admin
 from django.urls import include, path
 from django.views.generic.base import TemplateView
 
+from allauth.account.decorators import secure_admin_login
+
 
 admin.autodiscover()
+admin.site.login = secure_admin_login(admin.site.login)
 
 urlpatterns = [
     path("", TemplateView.as_view(template_name="index.html")),


Test patch:
diff --git a/allauth/account/tests/test_decorators.py b/allauth/account/tests/test_decorators.py
index 00a5a8ae..925e0441 100644
--- a/allauth/account/tests/test_decorators.py
+++ b/allauth/account/tests/test_decorators.py
@@ -1,3 +1,5 @@
+from django.urls import reverse
+
 from pytest_django.asserts import assertTemplateUsed
 
 from allauth.account.decorators import verified_email_required
@@ -14,3 +16,24 @@ def test_verified_email_required(user_factory, request_factory):
     request.user = user
     view(request)
     assertTemplateUsed("account/verified_email_required.html")
+
+
+def test_secure_admin_login_skips_admin_login_next(client):
+    """
+    Test that we're not using 'next=/admin/login%2Fnext=/foo'
+    """
+    resp = client.get(reverse("admin:login") + "?next=/foo")
+    assert resp["location"] == "/login/?next=%2Ffoo"
+
+
+def test_secure_admin_login_denies_regular_users(auth_client):
+    resp = auth_client.get(reverse("admin:login"))
+    assert resp.status_code == 403
+
+
+def test_secure_admin_login_passes_staff(auth_client, user):
+    user.is_staff = True
+    user.is_superuser = True
+    user.save(update_fields=["is_staff", "is_superuser"])
+    resp = auth_client.get(reverse("admin:auth_user_changelist"))
+    assert resp.status_code == 200
diff --git a/allauth/socialaccount/tests/test_connect.py b/allauth/socialaccount/tests/test_connect.py
index c7a0bff1..ca6eaa6c 100644
--- a/allauth/socialaccount/tests/test_connect.py
+++ b/allauth/socialaccount/tests/test_connect.py
@@ -94,7 +94,5 @@ def test_connect_vs_email_authentication(
         "allauth.account.adapter.DefaultAccountAdapter.add_message"
     ) as add_message:
         flows.login.complete_login(request, sociallogin)
-        assert (
-            add_message.call_args.kwargs["message_context"]["action"] == expected_action
-        )
+        assert add_message.call_args[1]["message_context"]["action"] == expected_action
     assert SocialAccount.objects.filter(user=user, uid=sociallogin.account.uid).exists()
diff --git a/tests/regular/urls.py b/tests/regular/urls.py
index 262bc5ff..d2afbbe4 100644
--- a/tests/regular/urls.py
+++ b/tests/regular/urls.py
@@ -1,7 +1,15 @@
+from django.contrib import admin
 from django.urls import include, path
 
+from allauth.account.decorators import secure_admin_login
+
+
+admin.autodiscover()
+admin.site.login = secure_admin_login(admin.site.login)
+
 
 urlpatterns = [
+    path("admin/", admin.site.urls),
     path("", include("allauth.urls")),
     path("_allauth/", include("allauth.headless.urls")),
 ]