Browse Source

Fixed #22569 -- Made ModelAdmin.lookup_allowed() respect get_list_filter().

Thank you Simon Meers for the initial patch.
sarahboyce 1 year ago
parent
commit
594fcc2b74

+ 9 - 2
django/contrib/admin/options.py

@@ -436,7 +436,9 @@ class BaseModelAdmin(metaclass=forms.MediaDefiningClass):
             else self.get_list_display(request)
         )
 
-    def lookup_allowed(self, lookup, value):
+    # RemovedInDjango60Warning: when the deprecation ends, replace with:
+    # def lookup_allowed(self, lookup, value, request):
+    def lookup_allowed(self, lookup, value, request=None):
         from django.contrib.admin.filters import SimpleListFilter
 
         model = self.model
@@ -482,7 +484,12 @@ class BaseModelAdmin(metaclass=forms.MediaDefiningClass):
             # Either a local field filter, or no fields at all.
             return True
         valid_lookups = {self.date_hierarchy}
-        for filter_item in self.list_filter:
+        # RemovedInDjango60Warning: when the deprecation ends, replace with:
+        # for filter_item in self.get_list_filter(request):
+        list_filter = (
+            self.get_list_filter(request) if request is not None else self.list_filter
+        )
+        for filter_item in list_filter:
             if isinstance(filter_item, type) and issubclass(
                 filter_item, SimpleListFilter
             ):

+ 14 - 1
django/contrib/admin/views/main.py

@@ -1,3 +1,4 @@
+import warnings
 from datetime import datetime, timedelta
 
 from django import forms
@@ -31,7 +32,9 @@ from django.core.paginator import InvalidPage
 from django.db.models import Exists, F, Field, ManyToOneRel, OrderBy, OuterRef
 from django.db.models.expressions import Combinable
 from django.urls import reverse
+from django.utils.deprecation import RemovedInDjango60Warning
 from django.utils.http import urlencode
+from django.utils.inspect import func_supports_parameter
 from django.utils.timezone import make_aware
 from django.utils.translation import gettext
 
@@ -174,9 +177,19 @@ class ChangeList:
         may_have_duplicates = False
         has_active_filters = False
 
+        supports_request = func_supports_parameter(
+            self.model_admin.lookup_allowed, "request"
+        )
+        if not supports_request:
+            warnings.warn(
+                f"`request` must be added to the signature of "
+                f"{self.model_admin.__class__.__qualname__}.lookup_allowed().",
+                RemovedInDjango60Warning,
+            )
         for key, value_list in lookup_params.items():
             for value in value_list:
-                if not self.model_admin.lookup_allowed(key, value):
+                params = (key, value, request) if supports_request else (key, value)
+                if not self.model_admin.lookup_allowed(*params):
                     raise DisallowedModelAdminLookup(f"Filtering by {key} not allowed")
 
         filter_specs = []

+ 4 - 2
django/contrib/auth/admin.py

@@ -106,10 +106,12 @@ class UserAdmin(admin.ModelAdmin):
             ),
         ] + super().get_urls()
 
-    def lookup_allowed(self, lookup, value):
+    # RemovedInDjango60Warning: when the deprecation ends, replace with:
+    # def lookup_allowed(self, lookup, value, request):
+    def lookup_allowed(self, lookup, value, request=None):
         # Don't allow lookups involving passwords.
         return not lookup.startswith("password") and super().lookup_allowed(
-            lookup, value
+            lookup, value, request
         )
 
     @sensitive_post_parameters_m

+ 3 - 0
docs/internals/deprecation.txt

@@ -21,6 +21,9 @@ details on these changes.
 * Support for passing positional arguments to ``BaseConstraint`` will be
   removed.
 
+* ``request`` will be required in the signature of
+  ``ModelAdmin.lookup_allowed()`` subclasses.
+
 .. _deprecation-removed-in-5.1:
 
 5.1

+ 10 - 5
docs/ref/contrib/admin/index.txt

@@ -1845,7 +1845,7 @@ templates used by the :class:`ModelAdmin` views:
                 kwargs["formset"] = MyAdminFormSet
                 return super().get_changelist_formset(request, **kwargs)
 
-.. method:: ModelAdmin.lookup_allowed(lookup, value)
+.. method:: ModelAdmin.lookup_allowed(lookup, value, request)
 
     The objects in the changelist page can be filtered with lookups from the
     URL's query string. This is how :attr:`list_filter` works, for example. The
@@ -1855,10 +1855,11 @@ templates used by the :class:`ModelAdmin` views:
     unauthorized data exposure.
 
     The ``lookup_allowed()`` method is given a lookup path from the query string
-    (e.g. ``'user__email'``) and the corresponding value
-    (e.g. ``'user@example.com'``), and returns a boolean indicating whether
-    filtering the changelist's ``QuerySet`` using the parameters is permitted.
-    If ``lookup_allowed()`` returns ``False``, ``DisallowedModelAdminLookup``
+    (e.g. ``'user__email'``), the corresponding value
+    (e.g. ``'user@example.com'``), and the request, and returns a boolean
+    indicating whether filtering the changelist's ``QuerySet`` using the
+    parameters is permitted. If ``lookup_allowed()`` returns ``False``,
+    ``DisallowedModelAdminLookup``
     (subclass of :exc:`~django.core.exceptions.SuspiciousOperation`) is raised.
 
     By default, ``lookup_allowed()`` allows access to a model's local fields,
@@ -1870,6 +1871,10 @@ templates used by the :class:`ModelAdmin` views:
     Override this method to customize the lookups permitted for your
     :class:`~django.contrib.admin.ModelAdmin` subclass.
 
+    .. versionchanged:: 5.0
+
+        The ``request`` argument was added.
+
 .. method:: ModelAdmin.has_view_permission(request, obj=None)
 
     Should return ``True`` if viewing ``obj`` is permitted, ``False`` otherwise.

+ 4 - 0
docs/releases/5.0.txt

@@ -387,6 +387,10 @@ Miscellaneous
   :class:`~django.db.models.BaseConstraint` is deprecated in favor of
   keyword-only arguments.
 
+* ``request`` is added to the signature of :meth:`.ModelAdmin.lookup_allowed`.
+  Support for ``ModelAdmin`` subclasses that do not accept this argument is
+  deprecated.
+
 Features removed in 5.0
 =======================
 

+ 89 - 7
tests/modeladmin/tests.py

@@ -19,8 +19,9 @@ from django.contrib.admin.widgets import (
 from django.contrib.auth.models import User
 from django.db import models
 from django.forms.widgets import Select
-from django.test import SimpleTestCase, TestCase
+from django.test import RequestFactory, SimpleTestCase, TestCase
 from django.test.utils import isolate_apps
+from django.utils.deprecation import RemovedInDjango60Warning
 
 from .models import Band, Concert, Song
 
@@ -121,7 +122,10 @@ class ModelAdminTests(TestCase):
             fields = ["name"]
 
         ma = BandAdmin(Band, self.site)
-        self.assertTrue(ma.lookup_allowed("name__nonexistent", "test_value"))
+        self.assertIs(
+            ma.lookup_allowed("name__nonexistent", "test_value", request),
+            True,
+        )
 
     @isolate_apps("modeladmin")
     def test_lookup_allowed_onetoone(self):
@@ -147,11 +151,15 @@ class ModelAdminTests(TestCase):
         ma = EmployeeProfileAdmin(EmployeeProfile, self.site)
         # Reverse OneToOneField
         self.assertIs(
-            ma.lookup_allowed("employee__employeeinfo__description", "test_value"), True
+            ma.lookup_allowed(
+                "employee__employeeinfo__description", "test_value", request
+            ),
+            True,
         )
         # OneToOneField and ForeignKey
         self.assertIs(
-            ma.lookup_allowed("employee__department__code", "test_value"), True
+            ma.lookup_allowed("employee__department__code", "test_value", request),
+            True,
         )
 
     @isolate_apps("modeladmin")
@@ -175,13 +183,87 @@ class ModelAdminTests(TestCase):
             ]
 
         ma = WaiterAdmin(Waiter, self.site)
-        self.assertIs(ma.lookup_allowed("restaurant__place__country", "1"), True)
         self.assertIs(
-            ma.lookup_allowed("restaurant__place__country__id__exact", "1"), True
+            ma.lookup_allowed("restaurant__place__country", "1", request),
+            True,
+        )
+        self.assertIs(
+            ma.lookup_allowed("restaurant__place__country__id__exact", "1", request),
+            True,
+        )
+        self.assertIs(
+            ma.lookup_allowed(
+                "restaurant__place__country__name", "test_value", request
+            ),
+            True,
+        )
+
+    def test_lookup_allowed_considers_dynamic_list_filter(self):
+        class ConcertAdmin(ModelAdmin):
+            list_filter = ["main_band__sign_date"]
+
+            def get_list_filter(self, request):
+                if getattr(request, "user", None):
+                    return self.list_filter + ["main_band__name"]
+                return self.list_filter
+
+        model_admin = ConcertAdmin(Concert, self.site)
+        request_band_name_filter = RequestFactory().get(
+            "/", {"main_band__name": "test"}
+        )
+        self.assertIs(
+            model_admin.lookup_allowed(
+                "main_band__sign_date", "?", request_band_name_filter
+            ),
+            True,
         )
         self.assertIs(
-            ma.lookup_allowed("restaurant__place__country__name", "test_value"), True
+            model_admin.lookup_allowed(
+                "main_band__name", "?", request_band_name_filter
+            ),
+            False,
+        )
+        request_with_superuser = request
+        self.assertIs(
+            model_admin.lookup_allowed(
+                "main_band__sign_date", "?", request_with_superuser
+            ),
+            True,
+        )
+        self.assertIs(
+            model_admin.lookup_allowed("main_band__name", "?", request_with_superuser),
+            True,
+        )
+
+    def test_lookup_allowed_without_request_deprecation(self):
+        class ConcertAdmin(ModelAdmin):
+            list_filter = ["main_band__sign_date"]
+
+            def get_list_filter(self, request):
+                return self.list_filter + ["main_band__name"]
+
+            def lookup_allowed(self, lookup, value):
+                return True
+
+        model_admin = ConcertAdmin(Concert, self.site)
+        msg = (
+            "`request` must be added to the signature of ModelAdminTests."
+            "test_lookup_allowed_without_request_deprecation.<locals>."
+            "ConcertAdmin.lookup_allowed()."
+        )
+        request_band_name_filter = RequestFactory().get(
+            "/", {"main_band__name": "test"}
+        )
+        request_band_name_filter.user = User.objects.create_superuser(
+            username="bob", email="bob@test.com", password="test"
         )
+        with self.assertWarnsMessage(RemovedInDjango60Warning, msg):
+            changelist = model_admin.get_changelist_instance(request_band_name_filter)
+            filterspec = changelist.get_filters(request_band_name_filter)[0][0]
+            self.assertEqual(filterspec.title, "sign date")
+            filterspec = changelist.get_filters(request_band_name_filter)[0][1]
+            self.assertEqual(filterspec.title, "name")
+            self.assertSequenceEqual(filterspec.lookup_choices, [self.band.name])
 
     def test_field_arguments(self):
         # If fields is specified, fieldsets_add and fieldsets_change should