Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Fixed #18072 -- Made more admin links use reverse() instead of hard-c…

…oded relative URLs.

Thanks kmike for the report and initial patch for the changelist->edit
object view link URL.

Other affected links include the delete object one and object history
one (in this case the change had been implemented in commit 5a9e127, this
commit adds admin-quoting of the object PK in a way similar to a222d6e.)

Refs #15294.
  • Loading branch information...
commit f51eab796d087439eedcb768cdd312517780940e 1 parent 515fd6a
@ramiro ramiro authored
View
2  django/contrib/admin/templates/admin/change_form.html
@@ -29,7 +29,7 @@
{% if change %}{% if not is_popup %}
<ul class="object-tools">
{% block object-tools-items %}
- <li><a href="{% url opts|admin_urlname:'history' original.pk %}" class="historylink">{% trans "History" %}</a></li>
+ <li><a href="{% url opts|admin_urlname:'history' original.pk|admin_urlquote %}" class="historylink">{% trans "History" %}</a></li>
{% if has_absolute_url %}<li><a href="{% url 'admin:view_on_site' content_type_id original.pk %}" class="viewsitelink">{% trans "View on site" %}</a></li>{% endif%}
{% endblock %}
</ul>
View
6 django/contrib/admin/templates/admin/submit_line.html
@@ -1,8 +1,8 @@
-{% load i18n %}
+{% load i18n admin_urls %}
<div class="submit-row">
{% if show_save %}<input type="submit" value="{% trans 'Save' %}" class="default" name="_save" {{ onclick_attrib }}/>{% endif %}
-{% if show_delete_link %}<p class="deletelink-box"><a href="delete/" class="deletelink">{% trans "Delete" %}</a></p>{% endif %}
+{% if show_delete_link %}<p class="deletelink-box"><a href="{% url opts|admin_urlname:'delete' original.pk|admin_urlquote %}" class="deletelink">{% trans "Delete" %}</a></p>{% endif %}
{% if show_save_as_new %}<input type="submit" value="{% trans 'Save as new' %}" name="_saveasnew" {{ onclick_attrib }}/>{%endif%}
-{% if show_save_and_add_another %}<input type="submit" value="{% trans 'Save and add another' %}" name="_addanother" {{ onclick_attrib }} />{% endif %}
+{% if show_save_and_add_another %}<input type="submit" value="{% trans 'Save and add another' %}" name="_addanother" {{ onclick_attrib }}/>{% endif %}
{% if show_save_and_continue %}<input type="submit" value="{% trans 'Save and continue editing' %}" name="_continue" {{ onclick_attrib }}/>{% endif %}
</div>
View
6 django/contrib/admin/templatetags/admin_modify.py
@@ -28,7 +28,8 @@ def submit_row(context):
change = context['change']
is_popup = context['is_popup']
save_as = context['save_as']
- return {
+ ctx = {
+ 'opts': opts,
'onclick_attrib': (opts.get_ordered_objects() and change
and 'onclick="submitOrderForm();"' or ''),
'show_delete_link': (not is_popup and context['has_delete_permission']
@@ -40,6 +41,9 @@ def submit_row(context):
'is_popup': is_popup,
'show_save': True
}
+ if context.get('original') is not None:
+ ctx['original'] = context['original']
+ return ctx
@register.filter
def cell_count(inline_admin_form):
View
6 django/contrib/admin/util.py
@@ -48,9 +48,9 @@ def prepare_lookup_value(key, value):
def quote(s):
"""
Ensure that primary key values do not confuse the admin URLs by escaping
- any '/', '_' and ':' characters. Similar to urllib.quote, except that the
- quoting is slightly different so that it doesn't get automatically
- unquoted by the Web browser.
+ any '/', '_' and ':' and similarly problematic characters.
+ Similar to urllib.quote, except that the quoting is slightly different so
+ that it doesn't get automatically unquoted by the Web browser.
"""
if not isinstance(s, six.string_types):
return s
View
7 django/contrib/admin/views/main.py
@@ -3,6 +3,7 @@
from django.core.exceptions import SuspiciousOperation, ImproperlyConfigured
from django.core.paginator import InvalidPage
+from django.core.urlresolvers import reverse
from django.db import models
from django.db.models.fields import FieldDoesNotExist
from django.utils.datastructures import SortedDict
@@ -376,4 +377,8 @@ def construct_search(field_name):
return qs
def url_for_result(self, result):
- return "%s/" % quote(getattr(result, self.pk_attname))
+ pk = getattr(result, self.pk_attname)
+ return reverse('admin:%s_%s_change' % (self.opts.app_label,
+ self.opts.module_name),
+ args=(quote(pk),),
+ current_app=self.model_admin.admin_site.name)
View
10 tests/regressiontests/admin_changelist/tests.py
@@ -6,6 +6,7 @@
from django.contrib.admin.options import IncorrectLookupParameters
from django.contrib.admin.views.main import ChangeList, SEARCH_VAR, ALL_VAR
from django.contrib.auth.models import User
+from django.core.urlresolvers import reverse
from django.template import Context, Template
from django.test import TestCase
from django.test.client import RequestFactory
@@ -65,7 +66,8 @@ def test_result_list_empty_changelist_value(self):
template = Template('{% load admin_list %}{% spaceless %}{% result_list cl %}{% endspaceless %}')
context = Context({'cl': cl})
table_output = template.render(context)
- row_html = '<tbody><tr class="row1"><th><a href="%d/">name</a></th><td class="nowrap">(None)</td></tr></tbody>' % new_child.id
+ link = reverse('admin:admin_changelist_child_change', args=(new_child.id,))
+ row_html = '<tbody><tr class="row1"><th><a href="%s">name</a></th><td class="nowrap">(None)</td></tr></tbody>' % link
self.assertFalse(table_output.find(row_html) == -1,
'Failed to find expected row element: %s' % table_output)
@@ -87,7 +89,8 @@ def test_result_list_html(self):
template = Template('{% load admin_list %}{% spaceless %}{% result_list cl %}{% endspaceless %}')
context = Context({'cl': cl})
table_output = template.render(context)
- row_html = '<tbody><tr class="row1"><th><a href="%d/">name</a></th><td class="nowrap">Parent object</td></tr></tbody>' % new_child.id
+ link = reverse('admin:admin_changelist_child_change', args=(new_child.id,))
+ row_html = '<tbody><tr class="row1"><th><a href="%s">name</a></th><td class="nowrap">Parent object</td></tr></tbody>' % link
self.assertFalse(table_output.find(row_html) == -1,
'Failed to find expected row element: %s' % table_output)
@@ -425,7 +428,8 @@ def test_dynamic_list_display_links(self):
request = self._mocked_authenticated_request('/child/', superuser)
response = m.changelist_view(request)
for i in range(1, 10):
- self.assertContains(response, '<a href="%s/">%s</a>' % (i, i))
+ link = reverse('admin:admin_changelist_child_change', args=(i,))
+ self.assertContains(response, '<a href="%s">%s</a>' % (link, i))
list_display = m.get_list_display(request)
list_display_links = m.get_list_display_links(request, list_display)
View
7 tests/regressiontests/admin_custom_urls/fixtures/actions.json
@@ -40,12 +40,5 @@
"fields": {
"description": "An action with a name suspected of being a XSS attempt"
}
- },
- {
- "pk": "The name of an action",
- "model": "admin_custom_urls.action",
- "fields": {
- "description": "A generic action"
- }
}
]
View
16 tests/regressiontests/admin_custom_urls/tests.py
@@ -1,5 +1,6 @@
from __future__ import absolute_import, unicode_literals
+from django.contrib.admin.util import quote
from django.core.urlresolvers import reverse
from django.template.response import TemplateResponse
from django.test import TestCase
@@ -67,7 +68,7 @@ def testAdminUrlsNoClash(self):
# Ditto, but use reverse() to build the URL
url = reverse('admin:%s_action_change' % Action._meta.app_label,
- args=('add',))
+ args=(quote('add'),))
response = self.client.get(url)
self.assertEqual(response.status_code, 200)
self.assertContains(response, 'Change action')
@@ -75,19 +76,8 @@ def testAdminUrlsNoClash(self):
# Should correctly get the change_view for the model instance with the
# funny-looking PK (the one wth a 'path/to/html/document.html' value)
url = reverse('admin:%s_action_change' % Action._meta.app_label,
- args=("path/to/html/document.html",))
+ args=(quote("path/to/html/document.html"),))
response = self.client.get(url)
self.assertEqual(response.status_code, 200)
self.assertContains(response, 'Change action')
self.assertContains(response, 'value="path/to/html/document.html"')
-
- def testChangeViewHistoryButton(self):
- url = reverse('admin:%s_action_change' % Action._meta.app_label,
- args=('The name of an action',))
- response = self.client.get(url)
- self.assertEqual(response.status_code, 200)
- expected_link = reverse('admin:%s_action_history' %
- Action._meta.app_label,
- args=('The name of an action',))
- self.assertContains(response, '<a href="%s" class="historylink"' %
- expected_link)
View
75 tests/regressiontests/admin_views/tests.py
@@ -260,19 +260,21 @@ def testChangeListSortingMultiple(self):
p1 = Person.objects.create(name="Chris", gender=1, alive=True)
p2 = Person.objects.create(name="Chris", gender=2, alive=True)
p3 = Person.objects.create(name="Bob", gender=1, alive=True)
- link = '<a href="%s/'
+ link1 = reverse('admin:admin_views_person_change', args=(p1.pk,))
+ link2 = reverse('admin:admin_views_person_change', args=(p2.pk,))
+ link3 = reverse('admin:admin_views_person_change', args=(p3.pk,))
# Sort by name, gender
# This hard-codes the URL because it'll fail if it runs against the
# 'admin2' custom admin (which doesn't have the Person model).
response = self.client.get('/test_admin/admin/admin_views/person/', {'o': '1.2'})
- self.assertContentBefore(response, link % p3.id, link % p1.id)
- self.assertContentBefore(response, link % p1.id, link % p2.id)
+ self.assertContentBefore(response, link3, link1)
+ self.assertContentBefore(response, link1, link2)
# Sort by gender descending, name
response = self.client.get('/test_admin/admin/admin_views/person/', {'o': '-2.1'})
- self.assertContentBefore(response, link % p2.id, link % p3.id)
- self.assertContentBefore(response, link % p3.id, link % p1.id)
+ self.assertContentBefore(response, link2, link3)
+ self.assertContentBefore(response, link3, link1)
def testChangeListSortingPreserveQuerySetOrdering(self):
"""
@@ -284,37 +286,41 @@ def testChangeListSortingPreserveQuerySetOrdering(self):
p1 = Person.objects.create(name="Amy", gender=1, alive=True, age=80)
p2 = Person.objects.create(name="Bob", gender=1, alive=True, age=70)
p3 = Person.objects.create(name="Chris", gender=2, alive=False, age=60)
- link = '<a href="%s/'
+ link1 = reverse('admin:admin_views_person_change', args=(p1.pk,))
+ link2 = reverse('admin:admin_views_person_change', args=(p2.pk,))
+ link3 = reverse('admin:admin_views_person_change', args=(p3.pk,))
# This hard-codes the URL because it'll fail if it runs against the
# 'admin2' custom admin (which doesn't have the Person model).
response = self.client.get('/test_admin/admin/admin_views/person/', {})
- self.assertContentBefore(response, link % p3.id, link % p2.id)
- self.assertContentBefore(response, link % p2.id, link % p1.id)
+ self.assertContentBefore(response, link3, link2)
+ self.assertContentBefore(response, link2, link1)
def testChangeListSortingModelMeta(self):
# Test ordering on Model Meta is respected
l1 = Language.objects.create(iso='ur', name='Urdu')
l2 = Language.objects.create(iso='ar', name='Arabic')
- link = '<a href="%s/'
+ link1 = reverse('admin:admin_views_language_change', args=(quote(l1.pk),))
+ link2 = reverse('admin:admin_views_language_change', args=(quote(l2.pk),))
response = self.client.get('/test_admin/admin/admin_views/language/', {})
- self.assertContentBefore(response, link % l2.pk, link % l1.pk)
+ self.assertContentBefore(response, link2, link1)
# Test we can override with query string
response = self.client.get('/test_admin/admin/admin_views/language/', {'o':'-1'})
- self.assertContentBefore(response, link % l1.pk, link % l2.pk)
+ self.assertContentBefore(response, link1, link2)
def testChangeListSortingOverrideModelAdmin(self):
# Test ordering on Model Admin is respected, and overrides Model Meta
dt = datetime.datetime.now()
p1 = Podcast.objects.create(name="A", release_date=dt)
p2 = Podcast.objects.create(name="B", release_date=dt - datetime.timedelta(10))
+ link1 = reverse('admin:admin_views_podcast_change', args=(p1.pk,))
+ link2 = reverse('admin:admin_views_podcast_change', args=(p2.pk,))
- link = '<a href="%s/'
response = self.client.get('/test_admin/admin/admin_views/podcast/', {})
- self.assertContentBefore(response, link % p1.pk, link % p2.pk)
+ self.assertContentBefore(response, link1, link2)
def testMultipleSortSameField(self):
# Check that we get the columns we expect if we have two columns
@@ -322,14 +328,16 @@ def testMultipleSortSameField(self):
dt = datetime.datetime.now()
p1 = Podcast.objects.create(name="A", release_date=dt)
p2 = Podcast.objects.create(name="B", release_date=dt - datetime.timedelta(10))
+ link1 = reverse('admin:admin_views_podcast_change', args=(quote(p1.pk),))
+ link2 = reverse('admin:admin_views_podcast_change', args=(quote(p2.pk),))
- link = '<a href="%s/'
response = self.client.get('/test_admin/admin/admin_views/podcast/', {})
- self.assertContentBefore(response, link % p1.pk, link % p2.pk)
+ self.assertContentBefore(response, link1, link2)
p1 = ComplexSortedPerson.objects.create(name="Bob", age=10)
p2 = ComplexSortedPerson.objects.create(name="Amy", age=20)
- link = '<a href="%s/'
+ link1 = reverse('admin:admin_views_complexsortedperson_change', args=(p1.pk,))
+ link2 = reverse('admin:admin_views_complexsortedperson_change', args=(p2.pk,))
response = self.client.get('/test_admin/admin/admin_views/complexsortedperson/', {})
# Should have 5 columns (including action checkbox col)
@@ -342,7 +350,7 @@ def testMultipleSortSameField(self):
self.assertContentBefore(response, 'Name', 'Colored name')
# Check sorting - should be by name
- self.assertContentBefore(response, link % p2.id, link % p1.id)
+ self.assertContentBefore(response, link2, link1)
def testSortIndicatorsAdminOrder(self):
"""
@@ -461,10 +469,12 @@ def testNamedGroupFieldChoicesChangeList(self):
for rows corresponding to instances of a model in which a named group
has been used in the choices option of a field.
"""
+ link1 = reverse('admin:admin_views_fabric_change', args=(1,), current_app=self.urlbit)
+ link2 = reverse('admin:admin_views_fabric_change', args=(2,), current_app=self.urlbit)
response = self.client.get('/test_admin/%s/admin_views/fabric/' % self.urlbit)
fail_msg = "Changelist table isn't showing the right human-readable values set by a model field 'choices' option named group."
- self.assertContains(response, '<a href="1/">Horizontal</a>', msg_prefix=fail_msg, html=True)
- self.assertContains(response, '<a href="2/">Vertical</a>', msg_prefix=fail_msg, html=True)
+ self.assertContains(response, '<a href="%s">Horizontal</a>' % link1, msg_prefix=fail_msg, html=True)
+ self.assertContains(response, '<a href="%s">Vertical</a>' % link2, msg_prefix=fail_msg, html=True)
def testNamedGroupFieldChoicesFilter(self):
"""
@@ -1371,9 +1381,12 @@ def test_get_change_view(self):
self.assertEqual(response.status_code, 200)
def test_changelist_to_changeform_link(self):
- "The link from the changelist referring to the changeform of the object should be quoted"
- response = self.client.get('/test_admin/admin/admin_views/modelwithstringprimarykey/')
- should_contain = """<th><a href="%s/">%s</a></th></tr>""" % (escape(quote(self.pk)), escape(self.pk))
+ "Link to the changeform of the object in changelist should use reverse() and be quoted -- #18072"
+ prefix = '/test_admin/admin/admin_views/modelwithstringprimarykey/'
+ response = self.client.get(prefix)
+ # this URL now comes through reverse(), thus iri_to_uri encoding
+ pk_final_url = escape(iri_to_uri(quote(self.pk)))
+ should_contain = """<th><a href="%s%s/">%s</a></th>""" % (prefix, pk_final_url, escape(self.pk))
self.assertContains(response, should_contain)
def test_recentactions_link(self):
@@ -1441,6 +1454,18 @@ def test_shortcut_view_with_escaping(self):
should_contain = '/%s/" class="viewsitelink">' % model.pk
self.assertContains(response, should_contain)
+ def test_change_view_history_link(self):
+ """Object history button link should work and contain the pk value quoted."""
+ url = reverse('admin:%s_modelwithstringprimarykey_change' %
+ ModelWithStringPrimaryKey._meta.app_label,
+ args=(quote(self.pk),))
+ response = self.client.get(url)
+ self.assertEqual(response.status_code, 200)
+ expected_link = reverse('admin:%s_modelwithstringprimarykey_history' %
+ ModelWithStringPrimaryKey._meta.app_label,
+ args=(quote(self.pk),))
+ self.assertContains(response, '<a href="%s" class="historylink"' % expected_link)
+
@override_settings(PASSWORD_HASHERS=('django.contrib.auth.hashers.SHA1PasswordHasher',))
class SecureViewTests(TestCase):
@@ -2023,12 +2048,14 @@ def test_pk_hidden_fields_with_list_display_links(self):
"""
story1 = OtherStory.objects.create(title='The adventures of Guido', content='Once upon a time in Djangoland...')
story2 = OtherStory.objects.create(title='Crouching Tiger, Hidden Python', content='The Python was sneaking into...')
+ link1 = reverse('admin:admin_views_otherstory_change', args=(story1.pk,))
+ link2 = reverse('admin:admin_views_otherstory_change', args=(story2.pk,))
response = self.client.get('/test_admin/admin/admin_views/otherstory/')
self.assertContains(response, 'id="id_form-0-id"', 1) # Only one hidden field, in a separate place than the table.
self.assertContains(response, 'id="id_form-1-id"', 1)
self.assertContains(response, '<div class="hiddenfields">\n<input type="hidden" name="form-0-id" value="%d" id="id_form-0-id" /><input type="hidden" name="form-1-id" value="%d" id="id_form-1-id" />\n</div>' % (story2.id, story1.id), html=True)
- self.assertContains(response, '<th><a href="%d/">%d</a></th>' % (story1.id, story1.id), 1)
- self.assertContains(response, '<th><a href="%d/">%d</a></th>' % (story2.id, story2.id), 1)
+ self.assertContains(response, '<th><a href="%s">%d</a></th>' % (link1, story1.id), 1)
+ self.assertContains(response, '<th><a href="%s">%d</a></th>' % (link2, story2.id), 1)
@override_settings(PASSWORD_HASHERS=('django.contrib.auth.hashers.SHA1PasswordHasher',))
Please sign in to comment.
Something went wrong with that request. Please try again.