Invoke actions on POST only

Wiktor Latanowicz · Wiktor Latanowicz · commit a616d3747531 · 2022-11-18T11:26:25.000+01:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,6 +1,11 @@
 # Changelog
 
 <!--next-version-placeholder-->
+### Feature
+* Drop support for GET method. All action are now invoked with POST method.
+
+### Breaking
+* When dealing with a secondary form in action, you cannot simply check the http method to determine if the form should be rendered or processed. You need to check for specific form inputs in POST payload.
 
 ## v4.1.0 (2022-11-14)
 ### Feature
diff --git a/django_object_actions/templates/django_object_actions/change_form.html b/django_object_actions/templates/django_object_actions/change_form.html
@@ -5,13 +5,16 @@
   {% for tool in objectactions %}
     <li class="objectaction-item" data-tool-name="{{ tool.name }}">
       {% url tools_view_name pk=object_id tool=tool.name as action_url %}
-      <a href="{% add_preserved_filters action_url %}" title="{{ tool.standard_attrs.title }}"
-         {% for k, v in tool.custom_attrs.items %}
-           {{ k }}="{{ v }}"
-         {% endfor %}
-         class="{{ tool.standard_attrs.class }}">
-      {{ tool.label|capfirst }}
-      </a>
+      <form name="{{ tool.name }}__form" method="post" action="{% add_preserved_filters action_url %}">
+        {% csrf_token %}
+        <a href="javascript:window.document.forms['{{ tool.name }}__form'].submit()" title="{{ tool.standard_attrs.title }}"
+          {% for k, v in tool.custom_attrs.items %}
+            {{ k }}="{{ v }}"
+          {% endfor %}
+          class="{{ tool.standard_attrs.class }}">
+          {{ tool.label|capfirst }}
+        </a>
+      </form>
     </li>
   {% endfor %}
   {{ block.super }}
diff --git a/django_object_actions/templates/django_object_actions/change_list.html b/django_object_actions/templates/django_object_actions/change_list.html
@@ -5,13 +5,16 @@
   {% for tool in objectactions %}
     <li class="objectaction-item" data-tool-name="{{ tool.name }}">
       {% url tools_view_name tool=tool.name as action_url %}
-      <a href="{% add_preserved_filters action_url %}" title="{{ tool.standard_attrs.title }}"
-         {% for k, v in tool.custom_attrs.items %}
-           {{ k }}="{{ v }}"
-         {% endfor %}
-         class="{{ tool.standard_attrs.class }}">
-      {{ tool.label|capfirst }}
-      </a>
+      <form name="{{ tool.name }}__form" method="post" action="{% add_preserved_filters action_url %}">
+        {% csrf_token %}
+        <a href="javascript:window.document.forms['{{ tool.name }}__form'].submit()" title="{{ tool.standard_attrs.title }}"
+          {% for k, v in tool.custom_attrs.items %}
+            {{ k }}="{{ v }}"
+          {% endfor %}
+          class="{{ tool.standard_attrs.class }}">
+          {{ tool.label|capfirst }}
+        </a>
+      </form>
     </li>
   {% endfor %}
   {{ block.super }}
diff --git a/django_object_actions/tests/test_admin.py b/django_object_actions/tests/test_admin.py
@@ -21,25 +21,25 @@ def test_action_on_a_model_with_uuid_pk_works(self):
         action_url = "/admin/polls/comment/{0}/actions/hodor/".format(comment.pk)
         # sanity check that url has a uuid
         self.assertIn("-", action_url)
-        response = self.client.get(action_url)
+        response = self.client.post(action_url)
         self.assertRedirects(response, comment_url)
 
-    @patch("django_object_actions.utils.ChangeActionView.get")
+    @patch("django_object_actions.utils.ChangeActionView.post")
     def test_action_on_a_model_with_arbitrary_pk_works(self, mock_view):
         mock_view.return_value = HttpResponse()
         action_url = "/admin/polls/comment/{0}/actions/hodor/".format(" i am a pk ")
 
-        self.client.get(action_url)
+        self.client.post(action_url)
 
         self.assertTrue(mock_view.called)
         self.assertEqual(mock_view.call_args[1]["pk"], " i am a pk ")
 
-    @patch("django_object_actions.utils.ChangeActionView.get")
+    @patch("django_object_actions.utils.ChangeActionView.post")
     def test_action_on_a_model_with_slash_in_pk_works(self, mock_view):
         mock_view.return_value = HttpResponse()
         action_url = "/admin/polls/comment/{0}/actions/hodor/".format("pk/slash")
 
-        self.client.get(action_url)
+        self.client.post(action_url)
 
         self.assertTrue(mock_view.called)
         self.assertEqual(mock_view.call_args[1]["pk"], "pk/slash")
@@ -55,7 +55,7 @@ def test_action_on_a_model_with_complex_id(self):
             quote(related_data.pk)
         )
 
-        response = self.client.get(action_url)
+        response = self.client.post(action_url)
         self.assertNotEqual(response.status_code, 404)
         self.assertRedirects(response, related_data_url)
 
@@ -76,12 +76,12 @@ def test_changelist_template_context(self):
 
     def test_changelist_action_view(self):
         url = "/admin/polls/choice/actions/delete_all/"
-        response = self.client.get(url)
+        response = self.client.post(url)
         self.assertRedirects(response, "/admin/polls/choice/")
 
     def test_changelist_nonexistent_action(self):
         url = "/admin/polls/choice/actions/xyzzy/"
-        response = self.client.get(url)
+        response = self.client.post(url)
         self.assertEqual(response.status_code, 404)
 
     def test_get_changelist_can_remove_action(self):
@@ -94,13 +94,23 @@ def test_get_changelist_can_remove_action(self):
         response = self.client.get(admin_change_url)
         self.assertIn(action_url, response.rendered_content)
 
-        response = self.client.get(action_url)  # Click on the button
+        response = self.client.post(action_url)  # Click on the button
         self.assertRedirects(response, admin_change_url)
 
         # button is not in the admin anymore
         response = self.client.get(admin_change_url)
         self.assertNotIn(action_url, response.rendered_content)
 
+    def test_changelist_get_method_action_view(self):
+        url = "/admin/polls/choice/actions/delete_all/"
+        response = self.client.get(url)
+        self.assertEqual(response.status_code, 405)
+
+    def test_changelist_get_method_nonexistent_action(self):
+        url = "/admin/polls/choice/actions/xyzzy/"
+        response = self.client.get(url)
+        self.assertEqual(response.status_code, 405)
+
 
 class ChangeListTests(LoggedInTestCase):
     def test_changelist_template_context(self):
@@ -122,5 +132,5 @@ def test_redirect_back_from_secondary_admin(self):
         action_url = "/support/polls/poll/1/actions/question_mark/"
         self.assertTrue(admin_change_url.startswith("/support/"))
 
-        response = self.client.get(action_url)
+        response = self.client.post(action_url)
         self.assertRedirects(response, admin_change_url)
diff --git a/django_object_actions/tests/test_urls.py b/django_object_actions/tests/test_urls.py
@@ -11,5 +11,5 @@ def test_changelist_action_is_rendered(self):
         response = self.client.get(reverse("admin:polls_choice_changelist"))
         self.assertEqual(response.status_code, 200)
         self.assertIn(
-            b'href="/admin/polls/choice/actions/delete_all/"', response.content
+            b'action="/admin/polls/choice/actions/delete_all/"', response.content
         )
diff --git a/django_object_actions/tests/tests.py b/django_object_actions/tests/tests.py
@@ -22,7 +22,7 @@ class AppTests(LoggedInTestCase):
     def test_tool_func_gets_executed(self):
         c = Choice.objects.get(pk=1)
         votes = c.votes
-        response = self.client.get(
+        response = self.client.post(
             reverse("admin:polls_choice_actions", args=(1, "increment_vote"))
         )
         self.assertEqual(response.status_code, 302)
@@ -34,7 +34,7 @@ def test_tool_func_gets_executed(self):
     def test_tool_can_return_httpresponse(self):
         # we know this url works because of fixtures
         url = reverse("admin:polls_choice_actions", args=(2, "edit_poll"))
-        response = self.client.get(url)
+        response = self.client.post(url)
         # we expect a redirect
         self.assertEqual(response.status_code, 302)
         self.assertTrue(
@@ -45,35 +45,35 @@ def test_can_return_template(self):
         # This is more of a test of render_to_response than the app, but I think
         # it's good to document that this is something we can do.
         url = reverse("admin:polls_poll_actions", args=(1, "delete_all_choices"))
-        response = self.client.get(url)
+        response = self.client.post(url)
         self.assertTemplateUsed(response, "clear_choices.html")
 
     def test_message_user_sends_message(self):
         url = reverse("admin:polls_poll_actions", args=(1, "delete_all_choices"))
         self.assertNotIn("messages", self.client.cookies)
-        self.client.get(url)
+        self.client.post(url)
         self.assertIn("messages", self.client.cookies)
 
     def test_intermediate_page_with_post_works(self):
         self.assertTrue(Choice.objects.filter(poll=1).count())
         url = reverse("admin:polls_poll_actions", args=(1, "delete_all_choices"))
-        response = self.client.post(url)
+        response = self.client.post(url, data={"sure": "Sure"})
         self.assertEqual(response.status_code, 302)
         self.assertEqual(Choice.objects.filter(poll=1).count(), 0)
 
     def test_undefined_tool_404s(self):
-        response = self.client.get(
+        response = self.client.post(
             reverse("admin:polls_poll_actions", args=(1, "weeeewoooooo"))
         )
         self.assertEqual(response.status_code, 404)
 
     def test_key_error_tool_500s(self):
         self.assertRaises(
             KeyError,
-            self.client.get,
+            self.client.post,
             reverse("admin:polls_choice_actions", args=(1, "raise_key_error")),
         )
 
     def test_render_button(self):
-        response = self.client.get(reverse("admin:polls_choice_change", args=(1,)))
+        response = self.client.post(reverse("admin:polls_choice_change", args=(1,)))
         self.assertEqual(response.status_code, 200)
diff --git a/django_object_actions/utils.py b/django_object_actions/utils.py
@@ -238,7 +238,7 @@ def back_url(self):
         """
         raise NotImplementedError
 
-    def get(self, request, tool, **kwargs):
+    def post(self, request, tool, **kwargs):
         # Fix for case if there are special symbols in object pk
         for k, v in self.kwargs.items():
             self.kwargs[k] = unquote(v)
@@ -254,9 +254,6 @@ def get(self, request, tool, **kwargs):
 
         return HttpResponseRedirect(self.back_url)
 
-    # HACK to allow POST requests too
-    post = get
-
     def message_user(self, request, message):
         """
         Mimic Django admin actions's `message_user`.
diff --git a/example_project/polls/admin.py b/example_project/polls/admin.py
@@ -108,7 +108,7 @@ def change_view(self, request, object_id, form_url="", extra_context=None):
     def delete_all_choices(self, request, obj):
         from django.shortcuts import render
 
-        if request.method == "POST":
+        if "sure" in request.POST:
             obj.choice_set.all().delete()
             return
 
diff --git a/example_project/polls/templates/clear_choices.html b/example_project/polls/templates/clear_choices.html
@@ -1,5 +1,5 @@
 <form action="." method="POST">
   {% csrf_token %}
   Delete All Choices?
-  <input type=submit value="Sure">
+  <input type=submit value="Sure" name="sure">
 </form>

Original file line number	Diff line number	Diff line change
`@@ -11,5 +11,5 @@ def test_changelist_action_is_rendered(self):`
`11`	`11`	`response = self.client.get(reverse("admin:polls_choice_changelist"))`
`12`	`12`	`self.assertEqual(response.status_code, 200)`
`13`	`13`	`self.assertIn(`
`14`		`- b'href="/admin/polls/choice/actions/delete_all/"', response.content`
	`14`	`+ b'action="/admin/polls/choice/actions/delete_all/"', response.content`
`15`	`15`	`)`