Skip to content

Commit

Permalink
Fixed #22569 -- Made ModelAdmin.lookup_allowed() respect get_list_fil…
Browse files Browse the repository at this point in the history
…ter().

Thank you Simon Meers for the initial patch.
  • Loading branch information
sarahboyce authored and felixxm committed Apr 17, 2023
1 parent 57f2b93 commit 594fcc2
Show file tree
Hide file tree
Showing 7 changed files with 133 additions and 17 deletions.
11 changes: 9 additions & 2 deletions django/contrib/admin/options.py
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,9 @@ def get_sortable_by(self, request):
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
Expand Down Expand Up @@ -482,7 +484,12 @@ def lookup_allowed(self, lookup, value):
# 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
):
Expand Down
15 changes: 14 additions & 1 deletion django/contrib/admin/views/main.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import warnings
from datetime import datetime, timedelta

from django import forms
Expand Down Expand Up @@ -31,7 +32,9 @@
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

Expand Down Expand Up @@ -174,9 +177,19 @@ def get_filters(self, request):
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 = []
Expand Down
6 changes: 4 additions & 2 deletions django/contrib/auth/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -106,10 +106,12 @@ def get_urls(self):
),
] + 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
Expand Down
3 changes: 3 additions & 0 deletions docs/internals/deprecation.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
15 changes: 10 additions & 5 deletions docs/ref/contrib/admin/index.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
Expand All @@ -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.
Expand Down
4 changes: 4 additions & 0 deletions docs/releases/5.0.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
=======================

Expand Down
96 changes: 89 additions & 7 deletions tests/modeladmin/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,9 @@
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

Expand Down Expand Up @@ -121,7 +122,10 @@ class BandAdmin(ModelAdmin):
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):
Expand All @@ -147,11 +151,15 @@ class EmployeeProfileAdmin(ModelAdmin):
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")
Expand All @@ -175,13 +183,87 @@ class WaiterAdmin(ModelAdmin):
]

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
Expand Down

5 comments on commit 594fcc2

@psyonara
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have any idea when this will make it into a Django release?
Thank you.

@shangxiao
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have any idea when this will make it into a Django release? Thank you.

In the next feature release: 5.0 See: https://www.djangoproject.com/download/#future-versions

@psyonara
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thank you. Is it technically possible to back-port this to 4.2.x? The bug with lookup_allowed and get_list_filter is quite significant, and if you only use LTS versions, it will be a long wait. :-P

@shangxiao
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thank you. Is it technically possible to back-port this to 4.2.x? The bug with lookup_allowed and get_list_filter is quite significant, and if you only use LTS versions, it will be a long wait. :-P

Sorry only issues classified as release blockers (those that would've prevented the release if found during testing) get backported.

@psyonara
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, understood. :) Thanks for explaining.

Please sign in to comment.