Merge pull request #3495 from open-formulieren/refactor/3489-api-clie…

…nt-implementations

Refactor API client implementations

CHANGELOG.rst

@@ -2,6 +2,30 @@
 Changelog
 =========
 
+2.4.0 "..." (2023-09-??)
+========================
+
+.. warning:: These release notes are in development!
+
+Upgrade procedure
+-----------------
+
+**KVK configuration**
+
+We've done some extensive code and configuration cleanups, and the KVK configuration may
+be affected by this. The service configuration for the "zoeken" and "basisprofiel" API's
+has been merged into a single service. Normally this configuration update should have
+been performed correctly automatically, but we can't rule out possible mistakes with
+more exotic configurations via API gateways.
+
+Please check and update your configuration if necessary - you can check this via:
+**Admin** > **Configuratie** > **Configuratie overzicht** and look for the KVK entries.
+
+If you run into any errors, then please check your certificate configuration, API key
+and validate that the API root does *not* include the ``v1/`` suffix. An example of a
+correct API root: ``https://api.kvk.nl/api/`` or ``https://api.kvk.nl/test/api/``.
+
+
 2.3.0 "Cruquius" (2023-08-24)
 =============================
 

src/api_client/client.py

@@ -41,7 +41,12 @@ class APIClient(Session):
     _request_kwargs: dict[str, Any]
     _in_context_manager: bool = False
 
-    def __init__(self, base_url: str, request_kwargs: dict[str, Any] | None = None):
+    def __init__(
+        self,
+        base_url: str,
+        request_kwargs: dict[str, Any] | None = None,
+        **kwargs,  # subclasses may require additional configuration
+    ):
         # base class does not take any kwargs
         super().__init__()
         # normalize to dict
@@ -69,10 +74,10 @@ def __exit__(self, *args):
         return super().__exit__(*args)
 
     @classmethod
-    def configure_from(cls, factory: APIClientFactory):
+    def configure_from(cls, factory: APIClientFactory, **kwargs):
         base_url = factory.get_client_base_url()
         session_kwargs = factory.get_client_session_kwargs()
-        return cls(base_url, session_kwargs)
+        return cls(base_url, session_kwargs, **kwargs)
 
     def request(self, method, url, *args, **kwargs):
         for attr, val in self._request_kwargs.items():

src/api_client/tests/test_helpers.py

@@ -0,0 +1,76 @@
+import string
+from urllib.parse import quote_plus, urlsplit
+
+from django.test import SimpleTestCase
+
+from furl import furl
+from hypothesis import assume, example, given, strategies as st
+from hypothesis.provisional import domains, urls
+
+from ..client import is_base_url
+
+printable_text = st.text(string.printable)
+
+
+class IsBaseUrlTests(SimpleTestCase):
+    @given(domains())
+    def test_domain_without_protocol(self, item: str):
+        assume(not item.startswith("http://"))
+        assume(not item.startswith("https://"))
+
+        self.assertFalse(is_base_url(item))
+
+    @given(st.text(string.printable))
+    @example("/some/absolute/path")
+    def test_random_text_without_protocol(self, item: str):
+        assume(not item.startswith("http://"))
+        assume(not item.startswith("https://"))
+
+        try:
+            is_base = is_base_url(item)
+        except ValueError:
+            # furl got something that it can't parse as a URL, and we do want to bubble
+            # this error up to the caller
+            pass
+        else:
+            self.assertFalse(is_base)
+
+    @given(
+        st.sampled_from(["https", "http", "ftp", "file"]),
+        st.lists(printable_text.map(quote_plus)).map("/".join),
+    )
+    def test_protocol_but_no_netloc(self, protocol, path):
+        url = f"{protocol}:///{path}"
+
+        self.assertFalse(is_base_url(url))
+
+    @given(urls())
+    def test_rfc_3986_url(self, url):
+        assert url.startswith("http://") or url.startswith("https://")
+        bits = urlsplit(url)
+        # not allowed for communication between hosts - it's a way to request a dynamically
+        # allocated port number.
+        assume(bits.port != 0)
+
+        self.assertTrue(is_base_url(url))
+
+    @given(
+        st.sampled_from(["ftp", "file", "madeupthing"]),
+        domains(),
+        st.lists(printable_text.map(quote_plus)).map("/".join),
+    )
+    def test_non_http_protocol(self, protocol, domain, path):
+        url = f"{protocol}://{domain}/{path}"
+
+        # we don't really care about the actual protocol, you *could* install a requests
+        # adapter for non-http(s)
+        self.assertTrue(is_base_url(url))
+
+    def test_handle_str_or_furl_instance(self):
+        example = "https://example.com/foo"
+
+        with self.subTest("raw string"):
+            self.assertTrue(is_base_url(example))
+
+        with self.subTest("furl instance string"):
+            self.assertTrue(is_base_url(furl(example)))

src/openforms/appointments/contrib/jcc/client.py

@@ -1,10 +1,12 @@
 from zeep.client import Client
 
+from soap.client import build_client
+
 from .models import JccConfig
 
 
 def get_client() -> Client:
     config = JccConfig.get_solo()
     assert isinstance(config, JccConfig)
     assert config.service is not None
-    return config.service.build_client()
+    return build_client(config.service)

src/openforms/appointments/contrib/jcc/models.py

@@ -3,7 +3,15 @@
 
 from solo.models import SingletonModel
 
-from stuf.managers import ConfigManager
+
+class ConfigManager(models.Manager):
+    def get_queryset(self) -> models.QuerySet:
+        qs = super().get_queryset()
+        return qs.select_related(
+            "service",
+            "service__client_certificate",
+            "service__server_certificate",
+        )
 
 
 class JccConfig(SingletonModel):

src/openforms/appointments/contrib/jcc/tests/test_client.py

@@ -0,0 +1,36 @@
+from unittest.mock import patch
+
+from django.test import TestCase
+
+from privates.test import temp_private_root
+from zeep.client import Client as ZeepClient
+
+from simple_certmanager_ext.tests.factories import CertificateFactory
+from soap.tests.factories import SoapServiceFactory
+
+from ..client import get_client
+from ..models import JccConfig
+from .utils import WSDL
+
+
+@temp_private_root()
+class ClientConfigurationTests(TestCase):
+    @patch("openforms.appointments.contrib.jcc.client.JccConfig.get_solo")
+    def test_client_transport_supports_mtls(self, m_get_solo):
+        # Smoke test to check that the service configuration is honoured
+        client_cert = CertificateFactory.create(with_private_key=True)
+        config = JccConfig(
+            service=SoapServiceFactory.build(
+                url=WSDL,  # cheat by passing a file path
+                client_certificate=client_cert,
+            )
+        )
+        m_get_solo.return_value = config
+
+        client = get_client()
+
+        self.assertIsInstance(client, ZeepClient)
+        self.assertEqual(
+            client.transport.session.cert,
+            (client_cert.public_certificate.path, client_cert.private_key.path),
+        )

src/openforms/appointments/contrib/jcc/tests/utils.py

@@ -19,7 +19,6 @@ def mock_response(filename: str):
     return full_path.read_text()
 
 
-# same behaviour as stuf.models.SoapService.build_client
 ZEEP_CLIENT = Client(WSDL)
 """
 Client singleton to avoid loading the WSDL over and over again, which takes up about
@@ -56,9 +55,8 @@ def setUp(self):
         jcc_config_patcher.start()
         self.addCleanup(jcc_config_patcher.stop)  # type: ignore
 
-        build_client_patcher = patch.object(
-            self.soap_service,
-            "build_client",
+        build_client_patcher = patch(
+            "openforms.appointments.contrib.jcc.client.build_client",
             return_value=ZEEP_CLIENT,
         )
         build_client_patcher.start()

src/openforms/appointments/tests/utils.py

@@ -1,10 +1,7 @@
-import os
-
-from django.conf import settings
-
 from soap.tests.factories import SoapServiceFactory
 
 from ..contrib.jcc.models import JccConfig
+from ..contrib.jcc.tests.utils import WSDL
 from ..models import AppointmentsConfig
 
 
@@ -15,11 +12,5 @@ def setup_jcc() -> None:
     appointments_config.save()
 
     config = JccConfig.get_solo()
-    wsdl = os.path.abspath(
-        os.path.join(
-            settings.DJANGO_PROJECT_DIR,
-            "appointments/contrib/jcc/tests/mock/GenericGuidanceSystem2.wsdl",
-        )
-    )
-    config.service = SoapServiceFactory.create(url=wsdl)
+    config.service = SoapServiceFactory.create(url=str(WSDL))
     config.save()

src/openforms/conf/base.py

@@ -211,7 +211,7 @@
     "openforms.contrib.brp",
     "openforms.contrib.kadaster",
     "openforms.contrib.kvk",
-    "openforms.contrib.bag.apps.BAGConfig",
+    "openforms.contrib.bag.apps.BAGConfig",  # TODO: remove once 2.4.0 is released
     "openforms.contrib.microsoft.apps.MicrosoftApp",
     "openforms.dmn",
     "openforms.dmn.contrib.camunda",
@@ -930,9 +930,6 @@
         BASE_DIR, "src/openforms/registrations/contrib/objects_api/tests/files"
     ),
     os.path.join(BASE_DIR, "src/openforms/prefill/contrib/haalcentraal/tests/files"),
-    os.path.join(BASE_DIR, "src/openforms/contrib/kvk/tests/files"),
-    os.path.join(BASE_DIR, "src/openforms/contrib/bag/tests/files"),
-    os.path.join(BASE_DIR, "src/openforms/contrib/kadaster/tests/files/"),
 ]
 
 #
@@ -1061,13 +1058,6 @@
         }
     )
 
-#
-# Location Client
-#
-OPENFORMS_LOCATION_CLIENT = config(
-    "OPENFORMS_LOCATION_CLIENT", "openforms.contrib.bag.client.BAGClient"
-)
-
 #
 # Mozilla Django OIDC DB settings
 #

src/openforms/config/tests/test_config_check.py

@@ -78,11 +78,7 @@ def test_disabled_plugins_are_skipped(self, mock_get_solo):
         "stuf.stuf_zds.client.parse_soap_error_text", return_value="(parsed error text)"
     )
     def test_check_config_service_failure_correctly_reported(self, m, *mocks):
-        self.service = StufServiceFactory.create()
-        config = StufZDSConfig.get_solo()
-        config.service = self.service
-        config.gemeentecode = "1234"
-        config.save()
+        config = StufZDSConfig(service=StufServiceFactory.create(), gemeentecode="1234")
         m.get(
             "http://zaken/soap/?wsdl",
             status_code=500,
@@ -92,7 +88,11 @@ def test_check_config_service_failure_correctly_reported(self, m, *mocks):
         user = StaffUserFactory(user_permissions=["configuration_overview"])
         self.client.force_login(user)
 
-        response = self.client.get(self.url)
+        with patch(
+            "openforms.registrations.contrib.stuf_zds.plugin.StufZDSConfig.get_solo",
+            return_value=config,
+        ):
+            response = self.client.get(self.url)
 
         self.assertEqual(response.status_code, 200)
         expected_entry = format_html(

src/openforms/config/views.py

@@ -1,16 +1,14 @@
 from typing import Any, Dict, Generator, Optional, Protocol, TypeGuard
 
-from django.conf import settings
 from django.contrib.auth.mixins import PermissionRequiredMixin
 from django.urls import reverse
 from django.utils.encoding import force_str
-from django.utils.module_loading import import_string
-from django.utils.translation import ugettext_lazy as _
+from django.utils.translation import ugettext as _
 from django.views.generic import TemplateView
 
 from openforms.appointments.registry import register as appointments_register
-from openforms.contrib.kadaster.config_check import Check as KadasterConfigCheck
-from openforms.contrib.kvk.checks import check_kvk_remote_validator
+from openforms.contrib.kadaster.config_check import BAGCheck, LocatieServerCheck
+from openforms.contrib.kvk.checks import KVKRemoteValidatorCheck
 from openforms.dmn.registry import register as dmn_register
 from openforms.payments.registry import register as payments_register
 from openforms.plugins.plugin import AbstractBasePlugin
@@ -63,15 +61,21 @@ def get_context_data(self, **kwargs: Any) -> Dict[str, Any]:
             sections += [
                 {
                     "name": _("Address lookup plugins"),
-                    "entries": self.get_geo_entries(),
+                    "entries": [
+                        self.get_plugin_entry(BAGCheck),  # Location client
+                        self.get_plugin_entry(LocatieServerCheck),  # Kadaster search
+                    ],
                 },
             ]
 
         if _subset_match(module, "validators"):
             sections += [
                 {
                     "name": _("Validator plugins"),
-                    "entries": [check_kvk_remote_validator()],
+                    "entries": [
+                        # uses KVK 'zoeken' client
+                        self.get_plugin_entry(KVKRemoteValidatorCheck),
+                    ],
                 },
             ]
 
@@ -122,15 +126,9 @@ def get_plugin_entry(self, plugin: AbstractBasePlugin | ConfigCheckable) -> Entr
             plugin.check_config()
         except Exception as e:
             status, error = False, str(e)
+
         try:
             actions = plugin.get_config_actions()
-        except NotImplementedError:
-            actions: list[Action] = [
-                (
-                    "Not implemented",
-                    "TODO: REMOVE THIS WHEN ALL PLUGINS HAVE THIS FUNCTION.",
-                )
-            ]
         except Exception as e:
             actions = [
                 (
@@ -146,24 +144,9 @@ def get_plugin_entry(self, plugin: AbstractBasePlugin | ConfigCheckable) -> Entr
             error=error,
         )
 
-    def get_geo_entries(self) -> list[Entry]:
-        entries = []
-
-        # Location client
-        try:
-            client = import_string(settings.OPENFORMS_LOCATION_CLIENT)
-        except ImportError as e:
-            entries.append(Entry(name=_("unknown"), status=False, error=str(e)))
-        else:
-            entries.append(self.get_plugin_entry(client))
-
-        # Kadaster search
-        entries.append(self.get_plugin_entry(KadasterConfigCheck))
-
-        return entries
-
     def get_clamav_entry(self):
         config = GlobalConfiguration.get_solo()
+        assert isinstance(config, GlobalConfiguration)
         config_url = reverse(
             "admin:config_globalconfiguration_change", args=(config.pk,)
         )