From a3c0487151d52843a33bd6b427611e53551f6700 Mon Sep 17 00:00:00 2001
From: Paul Schilling <mail@paulschilling.de>
Date: Mon, 21 Oct 2024 16:49:03 +0200
Subject: [PATCH] [#2813] Refactor math captcha on contactform

    - replace dedicated MathCaptcha field with simple IntegerField
      and perform actual captcha logic in the form view (the field
      was not thread-safe, the values - question + answer - had to
      be stored in the session, so there is no reason to keep the
      field)
---
 .../components/Contact/ContactForm.html       |  2 +-
 src/open_inwoner/openklant/forms.py           | 20 ++++++-
 .../openklant/tests/test_cms_plugins.py       |  2 +-
 .../openklant/tests/test_contactform.py       | 55 +++++++++++++------
 .../openklant/views/contactform.py            | 12 ++++
 src/open_inwoner/openklant/views/utils.py     | 28 ++++++++++
 src/open_inwoner/utils/forms.py               | 54 ------------------
 .../utils/tests/test_form_fields.py           | 52 ------------------
 8 files changed, 97 insertions(+), 128 deletions(-)
 create mode 100644 src/open_inwoner/openklant/views/utils.py
 delete mode 100644 src/open_inwoner/utils/tests/test_form_fields.py

diff --git a/src/open_inwoner/components/templates/components/Contact/ContactForm.html b/src/open_inwoner/components/templates/components/Contact/ContactForm.html
index 63a6eec19e..55dfbccc61 100644
--- a/src/open_inwoner/components/templates/components/Contact/ContactForm.html
+++ b/src/open_inwoner/components/templates/components/Contact/ContactForm.html
@@ -31,7 +31,7 @@
                             </p>
                             <label class="label captcha__label">
                                 <span class="captcha__check">
-                                    {{ form_object.captcha.field.question }}
+                                    {{ form_object.request_session.captcha_question }}
                                     <span class="captcha__input">{% field_as_widget form_object.captcha "input" form_id %}</span>
                                 </span>
                                 {% if form_object.captcha.errors %}
diff --git a/src/open_inwoner/openklant/forms.py b/src/open_inwoner/openklant/forms.py
index 9541e432a3..f716b96c70 100644
--- a/src/open_inwoner/openklant/forms.py
+++ b/src/open_inwoner/openklant/forms.py
@@ -3,7 +3,7 @@
 
 from open_inwoner.accounts.models import User
 from open_inwoner.openklant.models import ContactFormSubject, OpenKlantConfig
-from open_inwoner.utils.forms import MathCaptchaField
+from open_inwoner.openklant.views.utils import generate_question_answer_pair
 from open_inwoner.utils.validators import DutchPhoneNumberValidator
 
 
@@ -45,16 +45,17 @@ class ContactForm(forms.Form):
         widget=forms.Textarea(attrs={"rows": "5"}),
         required=True,
     )
-    captcha = MathCaptchaField(
+    captcha = forms.IntegerField(
         label=_("Beantwoord deze rekensom"),
         required=True,
     )
 
     user: User
 
-    def __init__(self, user, *args, **kwargs):
+    def __init__(self, user, request_session, *args, **kwargs):
         super().__init__(*args, **kwargs)
         self.user = user
+        self.request_session = request_session
 
         config = OpenKlantConfig.get_solo()
         self.fields["subject"].queryset = config.contactformsubject_set.all()
@@ -91,5 +92,18 @@ def clean(self, *args, **kwargs):
             cleaned_data["first_name"] = self.user.first_name
             cleaned_data["infix"] = self.user.infix
             cleaned_data["last_name"] = self.user.last_name
+        else:
+            # captcha for anon users
+            captcha_answer = cleaned_data.get("captcha")
+            if (
+                captcha_answer
+                and not captcha_answer == self.request_session["captcha_answer"]
+            ):
+                self.add_error("captcha", _("Fout antwoord, probeer het opnieuw."))
+
+            captcha_question, captcha_answer = generate_question_answer_pair()
+
+            self.request_session["captcha_question"] = captcha_question
+            self.request_session["captcha_answer"] = captcha_answer
 
         return cleaned_data
diff --git a/src/open_inwoner/openklant/tests/test_cms_plugins.py b/src/open_inwoner/openklant/tests/test_cms_plugins.py
index 466322128d..e3dff7c20b 100644
--- a/src/open_inwoner/openklant/tests/test_cms_plugins.py
+++ b/src/open_inwoner/openklant/tests/test_cms_plugins.py
@@ -13,7 +13,7 @@ def test_contact_form_plugin_render(self):
         description = "A description for the contactform"
 
         user = UserFactory()
-        form = ContactForm(user=user)
+        form = ContactForm(user=user, request_session={})
 
         html, context = cms_tools.render_plugin(
             ContactFormPlugin,
diff --git a/src/open_inwoner/openklant/tests/test_contactform.py b/src/open_inwoner/openklant/tests/test_contactform.py
index 04d134a5dc..da424c20a0 100644
--- a/src/open_inwoner/openklant/tests/test_contactform.py
+++ b/src/open_inwoner/openklant/tests/test_contactform.py
@@ -17,7 +17,6 @@
 from open_inwoner.openklant.tests.factories import ContactFormSubjectFactory
 from open_inwoner.openklant.views.contactform import ContactFormView
 from open_inwoner.openzaak.tests.factories import ServiceFactory
-from open_inwoner.utils.forms import MathCaptchaField
 from open_inwoner.utils.test import ClearCachesMixin, DisableRequestLogMixin
 from open_inwoner.utils.tests.helpers import AssertFormMixin, AssertTimelineLogMixin
 
@@ -26,11 +25,15 @@
 @modify_settings(
     MIDDLEWARE={"remove": ["open_inwoner.kvk.middleware.KvKLoginMiddleware"]}
 )
-@patch.object(MathCaptchaField, "clean", autospec=True)
 @patch(
     "open_inwoner.openklant.views.contactform.send_contact_confirmation_mail",
     autospec=True,
 )
+@patch(
+    "open_inwoner.openklant.views.contactform.generate_question_answer_pair",
+    autospec=True,
+    return_value=("", 42),
+)
 class ContactFormIntegrationTest(
     ClearCachesMixin,
     AssertTimelineLogMixin,
@@ -64,7 +67,7 @@ def setUp(self):
         ContactFormView.template_name = "pages/contactform/form.html"
 
     def test_singleton_has_configuration_method(
-        self, m, mock_send_confirm, mock_captcha
+        self, m, mock_captcha, mock_send_confirm
     ):
         # use cleared (from setUp()
         config = OpenKlantConfig.get_solo()
@@ -92,7 +95,7 @@ def test_singleton_has_configuration_method(
         mock_send_confirm.assert_not_called()
 
     def test_no_form_shown_if_not_has_configuration(
-        self, m, mock_send_confirm, mock_captcha
+        self, m, mock_captcha, mock_send_confirm
     ):
         # set nothing
         config = OpenKlantConfig.get_solo()
@@ -103,7 +106,7 @@ def test_no_form_shown_if_not_has_configuration(
         self.assertEqual(0, len(response.pyquery("#contactmoment-form")))
 
     def test_anon_form_requires_either_email_or_phonenumber(
-        self, m, mock_send_confirm, mock_captcha
+        self, m, mock_captcha, mock_send_confirm
     ):
         config = OpenKlantConfig.get_solo()
         config.register_email = "example@example.com"
@@ -132,6 +135,7 @@ def test_anon_form_requires_either_email_or_phonenumber(
         form["email"] = ""
         form["phonenumber"] = ""
         form["question"] = "hey!\n\nwaddup?"
+        form["captcha"] = 42
 
         response = form.submit(status=200)
         self.assertEqual(
@@ -140,7 +144,7 @@ def test_anon_form_requires_either_email_or_phonenumber(
         mock_send_confirm.assert_not_called()
 
     def test_regular_auth_form_fills_email_and_phonenumber(
-        self, m, mock_send_confirm, mock_captcha
+        self, m, mock_captcha, mock_send_confirm
     ):
         config = OpenKlantConfig.get_solo()
         config.register_email = "example@example.com"
@@ -165,7 +169,7 @@ def test_regular_auth_form_fills_email_and_phonenumber(
         mock_send_confirm.assert_called_once_with(user.email, subject.subject)
 
     def test_expected_ordered_subjects_are_shown(
-        self, m, mock_send_confirm, mock_captcha
+        self, m, mock_captcha, mock_send_confirm
     ):
         config = OpenKlantConfig.get_solo()
         config.register_email = "example@example.com"
@@ -203,7 +207,7 @@ def test_expected_ordered_subjects_are_shown(
         )
         mock_send_confirm.assert_not_called()
 
-    def test_submit_and_register_via_email(self, m, mock_send_confirm, mock_captcha):
+    def test_submit_and_register_via_email(self, m, mock_captcha, mock_send_confirm):
         config = OpenKlantConfig.get_solo()
         config.register_email = "example@example.com"
         config.has_form_configuration = True
@@ -219,6 +223,7 @@ def test_submit_and_register_via_email(self, m, mock_send_confirm, mock_captcha)
         form["email"] = "foo@example.com"
         form["phonenumber"] = "+31612345678"
         form["question"] = "hey!\n\nwaddup?"
+        form["captcha"] = 42
 
         response = form.submit().follow()
 
@@ -244,7 +249,7 @@ def test_submit_and_register_via_email(self, m, mock_send_confirm, mock_captcha)
         mock_send_confirm.assert_called_once_with("foo@example.com", subject.subject)
 
     def test_submit_and_register_anon_via_api_with_klant(
-        self, m, mock_send_confirm, mock_captcha
+        self, m, mock_captcha, mock_send_confirm
     ):
         MockAPICreateData.setUpServices()
 
@@ -273,6 +278,7 @@ def test_submit_and_register_anon_via_api_with_klant(
         form["email"] = "foo@example.com"
         form["phonenumber"] = "+31612345678"
         form["question"] = "hey!\n\nwaddup?"
+        form["captcha"] = 42
 
         response = form.submit().follow()
 
@@ -329,7 +335,7 @@ def test_submit_and_register_anon_via_api_with_klant(
         mock_send_confirm.assert_called_once_with("foo@example.com", subject.subject)
 
     def test_submit_and_register_anon_via_api_without_klant(
-        self, m, mock_send_confirm, mock_captcha
+        self, m, mock_captcha, mock_send_confirm
     ):
         MockAPICreateData.setUpServices()
 
@@ -359,6 +365,7 @@ def test_submit_and_register_anon_via_api_without_klant(
         form["email"] = "foo@example.com"
         form["phonenumber"] = "+31612345678"
         form["question"] = "hey!\n\nwaddup?"
+        form["captcha"] = 42
 
         response = form.submit().follow()
 
@@ -405,9 +412,13 @@ def test_submit_and_register_anon_via_api_without_klant(
         self.assertTimelineLog("registered contactmoment by API")
         mock_send_confirm.assert_called_once_with("foo@example.com", subject.subject)
 
+    @patch("open_inwoner.openklant.forms.generate_question_answer_pair")
     def test_submit_and_register_anon_via_api_without_klant_does_not_send_empty_email_or_telephone(
-        self, m, mock_send_confirm, mock_captcha
+        self, m, mock_captcha2, mock_captcha, mock_send_confirm
     ):
+        # we need to patch the captcha Q&A twice because they are re-generated by the form
+        mock_captcha2.return_value = ("", 42)
+
         config = OpenKlantConfig.get_solo()
         config.register_contact_moment = True
         config.register_bronorganisatie_rsin = "123456789"
@@ -441,6 +452,7 @@ def test_submit_and_register_anon_via_api_without_klant_does_not_send_empty_emai
                 form["question"] = "foobar"
                 form["phonenumber"] = contact_details["phonenumber"]
                 form["email"] = contact_details["email"]
+                form["captcha"] = 42
 
                 response = form.submit().follow()
 
@@ -463,7 +475,7 @@ def test_submit_and_register_anon_via_api_without_klant_does_not_send_empty_emai
                     self.assertNotIn("telefoonnummer", contactgegevens.keys())
 
     def test_register_bsn_user_via_api_without_id(
-        self, m, mock_send_confirm, mock_captcha
+        self, m, mock_captcha, mock_send_confirm
     ):
         MockAPICreateData.setUpServices()
 
@@ -516,7 +528,7 @@ def test_register_bsn_user_via_api_without_id(
         )
 
     def test_submit_and_register_bsn_user_via_api(
-        self, m, mock_send_confirm, mock_captcha
+        self, m, mock_captcha, mock_send_confirm
     ):
         MockAPICreateData.setUpServices()
 
@@ -592,7 +604,7 @@ def test_submit_and_register_bsn_user_via_api(
         mock_send_confirm.assert_called_once_with("foo@example.com", subject.subject)
 
     def test_submit_and_register_kvk_or_rsin_user_via_api(
-        self, _m, mock_send_confirm, mock_captcha
+        self, _m, mock_captcha, mock_send_confirm
     ):
         MockAPICreateData.setUpServices()
 
@@ -697,7 +709,7 @@ def test_submit_and_register_kvk_or_rsin_user_via_api(
                     mock_send_confirm.reset_mock()
 
     def test_submit_and_register_bsn_user_via_api_and_update_klant(
-        self, m, mock_send_confirm, mock_captcha
+        self, m, mock_captcha, mock_send_confirm
     ):
         MockAPICreateData.setUpServices()
 
@@ -780,12 +792,16 @@ def test_submit_and_register_bsn_user_via_api_and_update_klant(
         mock_send_confirm.assert_called_once_with(data.user.email, subject.subject)
         mock_send_confirm.reset_mock()
 
+    @patch("open_inwoner.openklant.forms.generate_question_answer_pair")
     def test_submit_and_register_kvk_or_rsin_user_via_api_and_update_klant(
-        self, m, mock_send_confirm, mock_captcha
+        self, m, mock_captcha2, mock_captcha, mock_send_confirm
     ):
         self.maxDiff = None
         MockAPICreateData.setUpServices()
 
+        # we need to patch the captcha Q&A twice because they are re-generated by the form
+        mock_captcha2.return_value = ("", 42)
+
         config = OpenKlantConfig.get_solo()
         config.register_contact_moment = True
         config.register_bronorganisatie_rsin = "123456789"
@@ -884,11 +900,15 @@ def test_submit_and_register_kvk_or_rsin_user_via_api_and_update_klant(
                     )
                     mock_send_confirm.reset_mock()
 
+    @patch("open_inwoner.openklant.forms.generate_question_answer_pair")
     def test_send_email_confirmation_is_configurable(
-        self, m, mock_send_confirm, mock_captcha
+        self, m, mock_captcha2, mock_captcha, mock_send_confirm
     ):
         MockAPICreateData.setUpServices()
 
+        # we need to patch the captcha Q&A twice because they are re-generated by the form
+        mock_captcha2.return_value = ("", 42)
+
         config = OpenKlantConfig.get_solo()
         config.register_contact_moment = True
         config.register_bronorganisatie_rsin = "123456789"
@@ -918,6 +938,7 @@ def test_send_email_confirmation_is_configurable(
                 form["email"] = "foo@example.com"
                 form["phonenumber"] = "+31612345678"
                 form["question"] = "hey!\n\nwaddup?"
+                form["captcha"] = 42
 
                 response = form.submit().follow()
 
diff --git a/src/open_inwoner/openklant/views/contactform.py b/src/open_inwoner/openklant/views/contactform.py
index 108b9a2a6b..b72a7cf088 100644
--- a/src/open_inwoner/openklant/views/contactform.py
+++ b/src/open_inwoner/openklant/views/contactform.py
@@ -17,6 +17,7 @@
 )
 from open_inwoner.openklant.forms import ContactForm
 from open_inwoner.openklant.models import OpenKlantConfig
+from open_inwoner.openklant.views.utils import generate_question_answer_pair
 from open_inwoner.openklant.wrap import get_fetch_parameters
 from open_inwoner.utils.views import CommonPageMixin, LogMixin
 
@@ -44,7 +45,18 @@ def get_success_url(self):
 
     def get_form_kwargs(self):
         kwargs = super().get_form_kwargs()
+
+        captcha_question = self.request.session.get("captcha_question")
+        captcha_answer = self.request.session.get("captcha_answer")
+
+        if not (captcha_question and captcha_answer):
+            captcha_question, captcha_answer = generate_question_answer_pair()
+
+        self.request.session["captcha_question"] = captcha_question
+        self.request.session["captcha_answer"] = captcha_answer
+
         kwargs["user"] = self.request.user
+        kwargs["request_session"] = self.request.session
         return kwargs
 
     def get_initial(self):
diff --git a/src/open_inwoner/openklant/views/utils.py b/src/open_inwoner/openklant/views/utils.py
new file mode 100644
index 0000000000..baccf614b8
--- /dev/null
+++ b/src/open_inwoner/openklant/views/utils.py
@@ -0,0 +1,28 @@
+import secrets
+
+from django.utils.translation import gettext as _
+
+
+def generate_question_answer_pair(
+    range_: tuple[int, int] = (1, 10),
+    operators: list[str] = ["+", "-"],
+) -> tuple[str, int]:
+    lower, upper = range_
+    num1 = secrets.choice(range(lower, upper))
+    num2 = secrets.choice(range(lower, upper))
+    operator = secrets.choice(operators)
+
+    # exclude negative results
+    num1, num2 = max(num1, num2), min(num1, num2)
+
+    question = _("What is {num1} {operator} {num2}?").format(
+        num1=num1, operator=operator, num2=num2
+    )
+
+    match operator:
+        case "+":
+            answer = num1 + num2
+        case "-":
+            answer = num1 - num2
+
+    return question, answer
diff --git a/src/open_inwoner/utils/forms.py b/src/open_inwoner/utils/forms.py
index eddb9bac1f..ddb58266ef 100644
--- a/src/open_inwoner/utils/forms.py
+++ b/src/open_inwoner/utils/forms.py
@@ -1,5 +1,4 @@
 import mimetypes
-import secrets
 
 from django import forms
 from django.conf import settings
@@ -164,56 +163,3 @@ def clean(self, *args, **kwargs):
                 )
 
         return f
-
-
-class MathCaptchaField(forms.Field):
-    def __init__(
-        self,
-        range_: tuple = (1, 10),
-        operators: list[str] | None = None,
-        *args,
-        **kwargs,
-    ):
-        super().__init__(*args, **kwargs)
-        self.widget = forms.TextInput()
-        self.range_ = range_
-        self.operators = operators or ["+", "-"]
-        self.question, self.answer = self.generate_question_answer_pair(
-            self.range_, self.operators
-        )
-
-    @staticmethod
-    def generate_question_answer_pair(
-        range_: tuple[int, int],
-        operators: list[str],
-    ) -> tuple[str, int]:
-        lower, upper = range_
-        num1 = secrets.choice(range(lower, upper))
-        num2 = secrets.choice(range(lower, upper))
-        operator = secrets.choice(operators)
-
-        # exclude negative results
-        num1, num2 = max(num1, num2), min(num1, num2)
-
-        question = _("What is {num1} {operator} {num2}?").format(
-            num1=num1, operator=operator, num2=num2
-        )
-
-        match operator:
-            case "+":
-                answer = num1 + num2
-            case "-":
-                answer = num1 - num2
-
-        return question, answer
-
-    def clean(self, value: str) -> str:
-        if not value:
-            raise forms.ValidationError(_("Dit veld is vereist."))
-        if not isinstance(value, str):
-            raise forms.ValidationError(_("Voer een geheel getal in."))
-        if value.isspace():
-            raise forms.ValidationError(_("Voer een geheel getal in."))
-        if int(value) != self.answer:
-            raise forms.ValidationError(_("Fout antwoord, probeer het opnieuw."))
-        return value
diff --git a/src/open_inwoner/utils/tests/test_form_fields.py b/src/open_inwoner/utils/tests/test_form_fields.py
deleted file mode 100644
index d2cac53534..0000000000
--- a/src/open_inwoner/utils/tests/test_form_fields.py
+++ /dev/null
@@ -1,52 +0,0 @@
-from django import forms
-from django.test import TestCase
-from django.utils.translation import gettext as _
-
-from ..forms import MathCaptchaField
-
-
-class MockForm(forms.Form):
-    captcha = MathCaptchaField(range_=(4, 5), operators=["+"])
-    captcha_2 = MathCaptchaField(range_=(4, 5), operators=["-"])
-
-
-class MathCaptchaFieldUnitTest(TestCase):
-    def test_captcha_invalid(self):
-        test_cases = [
-            {
-                "captcha": "",
-                "message": _("Dit veld is vereist."),
-                "reason": "field required",
-            },
-            {
-                "captcha": " ",
-                "message": _("Voer een geheel getal in."),
-                "reason": "wrong input type",
-            },
-            {
-                "captcha": 42,
-                "message": _("Voer een geheel getal in."),
-                "reason": "wrong input type",
-            },
-            {
-                "captcha": "42",  # captcha only computes 2 numbers between 1 and 10
-                "message": _("Fout antwoord, probeer het opnieuw."),
-                "reason": "wrong answer",
-            },
-        ]
-        for test_case in test_cases:
-            with self.subTest(reason=test_case["reason"]):
-                form = MockForm(
-                    data={
-                        "captcha": test_case["captcha"],
-                        "captcha_2": test_case["captcha"],
-                    },
-                )
-                self.assertFalse(form.is_valid())
-                self.assertEqual(form.errors["captcha"], [test_case["message"]])
-
-    def test_captcha_valid(self):
-        form = MockForm(
-            data={"captcha": "8", "captcha_2": "0"},
-        )
-        self.assertTrue(form.is_valid())