[#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)

Author: pi-sigma

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 %}

src/open_inwoner/mail/service.py

@@ -2,6 +2,9 @@
 
 
 def send_contact_confirmation_mail(recipient_email: str, form_subject: str):
+    import pdbr
+
+    pdbr.set_trace()
     template = find_template("contactform_confirmation")
     context = {
         "subject": form_subject,

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

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 = "[email protected]"
@@ -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 = "[email protected]"
@@ -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 = "[email protected]"
@@ -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 = "[email protected]"
         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"] = "[email protected]"
         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("[email protected]", 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"] = "[email protected]"
         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("[email protected]", 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"] = "[email protected]"
         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("[email protected]", 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("[email protected]", 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,10 @@ def test_send_email_confirmation_is_configurable(
                 form["email"] = "[email protected]"
                 form["phonenumber"] = "+31612345678"
                 form["question"] = "hey!\n\nwaddup?"
+                form["captcha"] = 42
+
+                # res = form.submit()
+                # import pdbr;pdbr.set_trace()
 
                 response = form.submit().follow()
 

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):

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