From f51eab796d087439eedcb768cdd312517780940e Mon Sep 17 00:00:00 2001
From: Ramiro Morales <cramm0@gmail.com>
Date: Mon, 24 Sep 2012 22:02:59 -0300
Subject: [PATCH] Fixed #18072 -- Made more admin links use reverse() instead
 of hard-coded 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.
---
 .../admin/templates/admin/change_form.html    |  2 +-
 .../admin/templates/admin/submit_line.html    |  6 +-
 .../admin/templatetags/admin_modify.py        |  6 +-
 django/contrib/admin/util.py                  |  6 +-
 django/contrib/admin/views/main.py            |  7 +-
 .../regressiontests/admin_changelist/tests.py | 10 ++-
 .../admin_custom_urls/fixtures/actions.json   |  7 --
 .../admin_custom_urls/tests.py                | 16 +---
 tests/regressiontests/admin_views/tests.py    | 75 +++++++++++++------
 9 files changed, 79 insertions(+), 56 deletions(-)

diff --git a/django/contrib/admin/templates/admin/change_form.html b/django/contrib/admin/templates/admin/change_form.html
index e27875cdad..4962e732a2 100644
--- a/django/contrib/admin/templates/admin/change_form.html
+++ b/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>
diff --git a/django/contrib/admin/templates/admin/submit_line.html b/django/contrib/admin/templates/admin/submit_line.html
index d6f854a233..8c9d22752d 100644
--- a/django/contrib/admin/templates/admin/submit_line.html
+++ b/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>
diff --git a/django/contrib/admin/templatetags/admin_modify.py b/django/contrib/admin/templatetags/admin_modify.py
index c190533f95..f6ac59635a 100644
--- a/django/contrib/admin/templatetags/admin_modify.py
+++ b/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):
diff --git a/django/contrib/admin/util.py b/django/contrib/admin/util.py
index f95fe53de1..74eef2e733 100644
--- a/django/contrib/admin/util.py
+++ b/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
diff --git a/django/contrib/admin/views/main.py b/django/contrib/admin/views/main.py
index 74ef095b4b..5033ba98bc 100644
--- a/django/contrib/admin/views/main.py
+++ b/django/contrib/admin/views/main.py
@@ -3,6 +3,7 @@ from functools import reduce
 
 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 @@ class ChangeList(object):
             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)
diff --git a/tests/regressiontests/admin_changelist/tests.py b/tests/regressiontests/admin_changelist/tests.py
index be88c9a161..2b1c1a9bcf 100644
--- a/tests/regressiontests/admin_changelist/tests.py
+++ b/tests/regressiontests/admin_changelist/tests.py
@@ -6,6 +6,7 @@ from django.contrib import admin
 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 @@ class ChangeListTests(TestCase):
         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 @@ class ChangeListTests(TestCase):
         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 @@ class ChangeListTests(TestCase):
         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)
diff --git a/tests/regressiontests/admin_custom_urls/fixtures/actions.json b/tests/regressiontests/admin_custom_urls/fixtures/actions.json
index a63cf8135c..7c6341d71d 100644
--- a/tests/regressiontests/admin_custom_urls/fixtures/actions.json
+++ b/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"
-    }
   }
 ]
diff --git a/tests/regressiontests/admin_custom_urls/tests.py b/tests/regressiontests/admin_custom_urls/tests.py
index 64ff9f6692..3e9cf28965 100644
--- a/tests/regressiontests/admin_custom_urls/tests.py
+++ b/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 @@ class AdminCustomUrlsTest(TestCase):
 
         # 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 @@ class AdminCustomUrlsTest(TestCase):
         # 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)
diff --git a/tests/regressiontests/admin_views/tests.py b/tests/regressiontests/admin_views/tests.py
index 36fea59f2e..8151c8c854 100644
--- a/tests/regressiontests/admin_views/tests.py
+++ b/tests/regressiontests/admin_views/tests.py
@@ -260,19 +260,21 @@ class AdminViewBasicTest(TestCase):
         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 @@ class AdminViewBasicTest(TestCase):
         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 @@ class AdminViewBasicTest(TestCase):
         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 @@ class AdminViewBasicTest(TestCase):
         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 @@ class AdminViewBasicTest(TestCase):
         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 @@ class AdminViewStringPrimaryKeyTest(TestCase):
         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 @@ class AdminViewStringPrimaryKeyTest(TestCase):
         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 @@ class AdminViewListEditable(TestCase):
         """
         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',))