Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Fixed #15103 - SuspiciousOperation with limit_choices_to and raw_id_f…
…ields

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
  • Loading branch information
spookylukey committed Jan 28, 2011
1 parent 22eaf77 commit c24bdf0
Show file tree
Hide file tree
Showing 7 changed files with 77 additions and 21 deletions.
12 changes: 10 additions & 2 deletions django/contrib/admin/options.py
Expand Up @@ -205,7 +205,16 @@ def queryset(self, request):
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)
Expand All @@ -217,7 +226,6 @@ def lookup_allowed(self, lookup):
# 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)
Expand Down
10 changes: 6 additions & 4 deletions django/contrib/admin/views/main.py
Expand Up @@ -183,16 +183,18 @@ def get_query_set(self):

# 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
)
Expand Down
36 changes: 21 additions & 15 deletions django/contrib/admin/widgets.py
Expand Up @@ -91,6 +91,22 @@ class AdminFileWidget(forms.ClearableFileInput):
template_with_clear = (u'<span class="clearable-file-input">%s</span>'
% 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):
"""
Expand All @@ -108,33 +124,23 @@ def render(self, name, value, attrs=None):
related_url = '../../../%s/%s/' % (self.rel.to._meta.app_label, self.rel.to._meta.object_name.lower())
params = self.url_parameters()
if params:
url = '?' + '&amp;'.join(['%s=%s' % (k, v) for k, v in params.items()])
url = u'?' + u'&amp;'.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('<a href="%s%s" class="related-lookup" id="lookup_id_%s" onclick="return showRelatedObjectLookupPopup(this);"> ' % \
output.append(u'<a href="%s%s" class="related-lookup" id="lookup_id_%s" onclick="return showRelatedObjectLookupPopup(this);"> ' % \
(related_url, url, name))
output.append('<img src="%simg/admin/selector-search.gif" width="16" height="16" alt="%s" /></a>' % (settings.ADMIN_MEDIA_PREFIX, _('Lookup')))
output.append(u'<img src="%simg/admin/selector-search.gif" width="16" height="16" alt="%s" /></a>' % (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
Expand Down
2 changes: 2 additions & 0 deletions django/db/models/fields/related.py
Expand Up @@ -901,6 +901,8 @@ def contribute_to_related_class(self, cls, related):
# 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

Expand Down
4 changes: 4 additions & 0 deletions django/db/models/options.py
Expand Up @@ -55,6 +55,10 @@ def __init__(self, meta, app_label=None):
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
Expand Down
23 changes: 23 additions & 0 deletions tests/regressiontests/admin_views/models.py
Expand Up @@ -178,6 +178,26 @@ def __unicode__(self):
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', (
Expand Down Expand Up @@ -642,6 +662,9 @@ def __unicode__(self):
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)
Expand Down
11 changes: 11 additions & 0 deletions tests/regressiontests/admin_views/tests.py
Expand Up @@ -393,6 +393,17 @@ def test_disallowed_filtering(self):
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']

Expand Down

0 comments on commit c24bdf0

Please sign in to comment.