Reorganise snippet admin URLs to avoid ambiguous paths (#7208)

* Reorganise snippet admin URLs to avoid ambiguous paths

Snippet admin views allow for arbitrary strings as primary keys, and the current URL patterns don't adequately namespace these from reserved words like 'add' - for example, a snippet with the primary key 'add' would have an edit URL that collides with the add view at `/admin/snippets/foo/bar/add/`.

This is unlikely to come up in practice, but it does mean that our urlconf is more sensitive to ordering than it needs to be. Rearrange so that the verb (add, edit, delete) consistently comes before the pk, and add redirects to handle the legacy URLs.

* Release notes for #7208

Co-authored-by: Storm Heg <storm@stormheg.co>

CHANGELOG.txt

@@ -14,6 +14,7 @@ Changelog
  * Fix: Deleting a page from its listing view no longer results in a 404 error (Tidjani Dia)
  * Fix: The Wagtail admin urls will now respect the `APPEND_SLASH` setting (Tidjani Dia)
  * Fix: Prevent “Forgotten password” link from overlapping with field on mobile devices (Helen Chapman)
+ * Fix: Snippet admin urls are now namespaced to avoid ambiguity with the primary key component of the url (Matt Westcott)
 
 
 2.13 (12.05.2021)

docs/releases/2.14.rst

@@ -26,6 +26,7 @@ Bug fixes
  * Deleting a page from its listing view no longer results in a 404 error (Tidjani Dia)
  * The Wagtail admin urls will now respect the ``APPEND_SLASH`` setting (Tidjani Dia)
  * Prevent “Forgotten password” link from overlapping with field on mobile devices (Helen Chapman)
+ * Snippet admin urls are now namespaced to avoid ambiguity with the primary key component of the url (Matt Westcott)
 
 Upgrade considerations
 ======================

wagtail/contrib/simple_translation/tests/test_views.py

@@ -253,7 +253,7 @@ class TestSubmitSnippetTranslationView(WagtailTestUtils, TestCase):
             "pk": 99,
         }
         self.assertEqual(
-            view.get_success_url(), "/admin/snippets/some_app/some_model/99/"
+            view.get_success_url(), "/admin/snippets/some_app/some_model/edit/99/"
         )
 
     def test_get_success_message(self):

wagtail/snippets/tests.py

@@ -1527,6 +1527,19 @@ class TestSnippetViewWithCustomPrimaryKey(TestCase, WagtailTestUtils):
         self.assertContains(response, 'Used 0 times')
         self.assertContains(response, self.snippet_a.usage_url())
 
+    def test_redirect_to_edit(self):
+        response = self.client.get('/admin/snippets/snippetstests/standardsnippetwithcustomprimarykey/snippet_2F01/')
+        self.assertRedirects(response, '/admin/snippets/snippetstests/standardsnippetwithcustomprimarykey/edit/snippet_2F01/', status_code=301)
+
+    def test_redirect_to_delete(self):
+        response = self.client.get('/admin/snippets/snippetstests/standardsnippetwithcustomprimarykey/snippet_2F01/delete/')
+        self.assertRedirects(response, '/admin/snippets/snippetstests/standardsnippetwithcustomprimarykey/delete/snippet_2F01/', status_code=301)
+
+    @override_settings(WAGTAIL_USAGE_COUNT_ENABLED=True)
+    def test_redirect_to_usage(self):
+        response = self.client.get('/admin/snippets/snippetstests/standardsnippetwithcustomprimarykey/snippet_2F01/usage/')
+        self.assertRedirects(response, '/admin/snippets/snippetstests/standardsnippetwithcustomprimarykey/usage/snippet_2F01/', status_code=301)
+
 
 class TestSnippetChooserBlockWithCustomPrimaryKey(TestCase):
     fixtures = ['test.json']

wagtail/snippets/urls.py

@@ -13,8 +13,14 @@ urlpatterns = [
 
     path('<slug:app_label>/<slug:model_name>/', snippets.list, name='list'),
     path('<slug:app_label>/<slug:model_name>/add/', snippets.create, name='add'),
-    path('<slug:app_label>/<slug:model_name>/<str:pk>/', snippets.edit, name='edit'),
+    path('<slug:app_label>/<slug:model_name>/edit/<str:pk>/', snippets.edit, name='edit'),
     path('<slug:app_label>/<slug:model_name>/multiple/delete/', snippets.delete, name='delete-multiple'),
-    path('<slug:app_label>/<slug:model_name>/<str:pk>/delete/', snippets.delete, name='delete'),
-    path('<slug:app_label>/<slug:model_name>/<str:pk>/usage/', snippets.usage, name='usage'),
+    path('<slug:app_label>/<slug:model_name>/delete/<str:pk>/', snippets.delete, name='delete'),
+    path('<slug:app_label>/<slug:model_name>/usage/<str:pk>/', snippets.usage, name='usage'),
+
+    # legacy URLs that could potentially collide if the pk matches one of the reserved names above
+    # ('add', 'edit' etc) - redirect to the unambiguous version
+    path('<slug:app_label>/<slug:model_name>/<str:pk>/', snippets.redirect_to_edit),
+    path('<slug:app_label>/<slug:model_name>/<str:pk>/delete/', snippets.redirect_to_delete),
+    path('<slug:app_label>/<slug:model_name>/<str:pk>/usage/', snippets.redirect_to_usage),
 ]

wagtail/snippets/views/snippets.py

@@ -405,3 +405,15 @@ def usage(request, app_label, model_name, pk):
         'instance': instance,
         'used_by': used_by
     })
+
+
+def redirect_to_edit(request, app_label, model_name, pk):
+    return redirect('wagtailsnippets:edit', app_label, model_name, pk, permanent=True)
+
+
+def redirect_to_delete(request, app_label, model_name, pk):
+    return redirect('wagtailsnippets:delete', app_label, model_name, pk, permanent=True)
+
+
+def redirect_to_usage(request, app_label, model_name, pk):
+    return redirect('wagtailsnippets:usage', app_label, model_name, pk, permanent=True)