From c24bdf044ba23f2aa09ea4637a368ea86fd1c128 Mon Sep 17 00:00:00 2001 From: Luke Plant Date: Fri, 28 Jan 2011 14:08:25 +0000 Subject: [PATCH] Fixed #15103 - SuspiciousOperation with limit_choices_to and raw_id_fields Thanks to natrius for the report. This patch also fixes some unicode bugs in affected code. git-svn-id: http://code.djangoproject.com/svn/django/trunk@15347 bcc190cf-cafb-0310-a4f2-bffc1f526a37 --- django/contrib/admin/options.py | 12 +++++-- django/contrib/admin/views/main.py | 10 +++--- django/contrib/admin/widgets.py | 36 ++++++++++++--------- django/db/models/fields/related.py | 2 ++ django/db/models/options.py | 4 +++ tests/regressiontests/admin_views/models.py | 23 +++++++++++++ tests/regressiontests/admin_views/tests.py | 11 +++++++ 7 files changed, 77 insertions(+), 21 deletions(-) diff --git a/django/contrib/admin/options.py b/django/contrib/admin/options.py index 4ad21eb42b..db445247d2 100644 --- a/django/contrib/admin/options.py +++ b/django/contrib/admin/options.py @@ -205,7 +205,16 @@ class BaseModelAdmin(object): qs = qs.order_by(*ordering) return qs - def lookup_allowed(self, lookup): + def lookup_allowed(self, lookup, value): + model = self.model + # Check FKey lookups that are allowed, so that popups produced by + # ForeignKeyRawIdWidget, on the basis of ForeignKey.limit_choices_to, + # are allowed to work. + for l in model._meta.related_fkey_lookups: + for k, v in widgets.url_params_from_lookup_dict(l).items(): + if k == lookup and v == value: + return True + parts = lookup.split(LOOKUP_SEP) # Last term in lookup is a query term (__exact, __startswith etc) @@ -217,7 +226,6 @@ class BaseModelAdmin(object): # if foo has been specificially included in the lookup list; so # drop __id if it is the last part. However, first we need to find # the pk attribute name. - model = self.model pk_attr_name = None for part in parts[:-1]: field, _, _, _ = model._meta.get_field_by_name(part) diff --git a/django/contrib/admin/views/main.py b/django/contrib/admin/views/main.py index 886ccd9be7..00ab9fed21 100644 --- a/django/contrib/admin/views/main.py +++ b/django/contrib/admin/views/main.py @@ -183,16 +183,18 @@ class ChangeList(object): # if key ends with __in, split parameter into separate values if key.endswith('__in'): - lookup_params[key] = value.split(',') + value = value.split(',') + lookup_params[key] = value # if key ends with __isnull, special case '' and false if key.endswith('__isnull'): if value.lower() in ('', 'false'): - lookup_params[key] = False + value = False else: - lookup_params[key] = True + value = True + lookup_params[key] = value - if not self.model_admin.lookup_allowed(key): + if not self.model_admin.lookup_allowed(key, value): raise SuspiciousOperation( "Filtering by %s not allowed" % key ) diff --git a/django/contrib/admin/widgets.py b/django/contrib/admin/widgets.py index f06c5e421c..d861e25e16 100644 --- a/django/contrib/admin/widgets.py +++ b/django/contrib/admin/widgets.py @@ -91,6 +91,22 @@ class AdminFileWidget(forms.ClearableFileInput): template_with_clear = (u'%s' % forms.ClearableFileInput.template_with_clear) +def url_params_from_lookup_dict(lookups): + """ + Converts the type of lookups specified in a ForeignKey limit_choices_to + attribute to a dictionary of query parameters + """ + params = {} + if lookups and hasattr(lookups, 'items'): + items = [] + for k, v in lookups.items(): + if isinstance(v, list): + v = u','.join([str(x) for x in v]) + else: + v = unicode(v) + items.append((k, v)) + params.update(dict(items)) + return params class ForeignKeyRawIdWidget(forms.TextInput): """ @@ -108,33 +124,23 @@ class ForeignKeyRawIdWidget(forms.TextInput): related_url = '../../../%s/%s/' % (self.rel.to._meta.app_label, self.rel.to._meta.object_name.lower()) params = self.url_parameters() if params: - url = '?' + '&'.join(['%s=%s' % (k, v) for k, v in params.items()]) + url = u'?' + u'&'.join([u'%s=%s' % (k, v) for k, v in params.items()]) else: - url = '' + url = u'' if "class" not in attrs: attrs['class'] = 'vForeignKeyRawIdAdminField' # The JavaScript looks for this hook. output = [super(ForeignKeyRawIdWidget, self).render(name, value, attrs)] # TODO: "id_" is hard-coded here. This should instead use the correct # API to determine the ID dynamically. - output.append(' ' % \ + output.append(u' ' % \ (related_url, url, name)) - output.append('%s' % (settings.ADMIN_MEDIA_PREFIX, _('Lookup'))) + output.append(u'%s' % (settings.ADMIN_MEDIA_PREFIX, _('Lookup'))) if value: output.append(self.label_for_value(value)) return mark_safe(u''.join(output)) def base_url_parameters(self): - params = {} - if self.rel.limit_choices_to and hasattr(self.rel.limit_choices_to, 'items'): - items = [] - for k, v in self.rel.limit_choices_to.items(): - if isinstance(v, list): - v = ','.join([str(x) for x in v]) - else: - v = str(v) - items.append((k, v)) - params.update(dict(items)) - return params + return url_params_from_lookup_dict(self.rel.limit_choices_to) def url_parameters(self): from django.contrib.admin.views.main import TO_FIELD_VAR diff --git a/django/db/models/fields/related.py b/django/db/models/fields/related.py index 6e18f6d470..1318706040 100644 --- a/django/db/models/fields/related.py +++ b/django/db/models/fields/related.py @@ -901,6 +901,8 @@ class ForeignKey(RelatedField, Field): # don't get a related descriptor. if not self.rel.is_hidden(): setattr(cls, related.get_accessor_name(), ForeignRelatedObjectsDescriptor(related)) + if self.rel.limit_choices_to: + cls._meta.related_fkey_lookups.append(self.rel.limit_choices_to) if self.rel.field_name is None: self.rel.field_name = cls._meta.pk.name diff --git a/django/db/models/options.py b/django/db/models/options.py index 882d2f5709..2f64c56b00 100644 --- a/django/db/models/options.py +++ b/django/db/models/options.py @@ -55,6 +55,10 @@ class Options(object): self.abstract_managers = [] self.concrete_managers = [] + # List of all lookups defined in ForeignKey 'limit_choices_to' options + # from *other* models. Needed for some admin checks. Internal use only. + self.related_fkey_lookups = [] + def contribute_to_class(self, cls, name): from django.db import connection from django.db.backends.util import truncate_name diff --git a/tests/regressiontests/admin_views/models.py b/tests/regressiontests/admin_views/models.py index e64f713b7e..08a2d36c3f 100644 --- a/tests/regressiontests/admin_views/models.py +++ b/tests/regressiontests/admin_views/models.py @@ -178,6 +178,26 @@ class Thing(models.Model): class ThingAdmin(admin.ModelAdmin): list_filter = ('color__warm', 'color__value') +class Actor(models.Model): + name = models.CharField(max_length=50) + age = models.IntegerField() + def __unicode__(self): + return self.name + +class Inquisition(models.Model): + expected = models.BooleanField() + leader = models.ForeignKey(Actor) + def __unicode__(self): + return self.expected + +class Sketch(models.Model): + title = models.CharField(max_length=100) + inquisition = models.ForeignKey(Inquisition, limit_choices_to={'leader__name': 'Palin', + 'leader__age': 27, + }) + def __unicode__(self): + return self.title + class Fabric(models.Model): NG_CHOICES = ( ('Textured', ( @@ -642,6 +662,9 @@ admin.site.register(Section, save_as=True, inlines=[ArticleInline]) admin.site.register(ModelWithStringPrimaryKey) admin.site.register(Color) admin.site.register(Thing, ThingAdmin) +admin.site.register(Actor) +admin.site.register(Inquisition) +admin.site.register(Sketch) admin.site.register(Person, PersonAdmin) admin.site.register(Persona, PersonaAdmin) admin.site.register(Subscriber, SubscriberAdmin) diff --git a/tests/regressiontests/admin_views/tests.py b/tests/regressiontests/admin_views/tests.py index 661954c7ec..30e8c2f8e0 100644 --- a/tests/regressiontests/admin_views/tests.py +++ b/tests/regressiontests/admin_views/tests.py @@ -393,6 +393,17 @@ class AdminViewBasicTest(TestCase): response = self.client.get("/test_admin/admin/admin_views/workhour/?employee__person_ptr__exact=%d" % e1.pk) self.assertEqual(response.status_code, 200) + def test_allowed_filtering_15103(self): + """ + Regressions test for ticket 15103 - filtering on fields defined in a + ForeignKey 'limit_choices_to' should be allowed, otherwise raw_id_fields + can break. + """ + try: + self.client.get("/test_admin/admin/admin_views/inquisition/?leader__name=Palin&leader__age=27") + except SuspiciousOperation: + self.fail("Filters should be allowed if they are defined on a ForeignKey pointing to this model") + class SaveAsTests(TestCase): fixtures = ['admin-views-users.xml','admin-views-person.xml']