From 79fd42e03a8b6391158e69f149b317c665a92634 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Thu, 24 Nov 2022 15:10:25 +0100 Subject: [PATCH] :recycle: [#1068] -- refactor 'update for request' component processing pipeline Instead of using a fixed pipeline, the concerns are now moved to the component level that can opt-in to request-specific component rewriting. This should also lead to slightly more optimized code execution since we're looping over a caching datastructure rather than looping over the entire component tree over and over again. --- src/openforms/formio/components/vanilla.py | 56 +++++++++++++++ .../formio/dynamic_config/__init__.py | 26 ++++++- src/openforms/formio/registry.py | 26 +++++++ src/openforms/formio/service.py | 70 +++++-------------- src/openforms/formio/tests/test_service.py | 6 +- .../forms/api/serializers/form_definition.py | 15 ++-- .../tests/test_get_submission_step.py | 33 +++++++++ 7 files changed, 169 insertions(+), 63 deletions(-) diff --git a/src/openforms/formio/components/vanilla.py b/src/openforms/formio/components/vanilla.py index 6c2094a6a2..b22c3c92d4 100644 --- a/src/openforms/formio/components/vanilla.py +++ b/src/openforms/formio/components/vanilla.py @@ -4,6 +4,15 @@ Custom component types (defined by us or third parties) need to be organized in the adjacent custom.py module. """ +from typing import Literal + +from rest_framework.request import Request +from rest_framework.reverse import reverse + +from csp_post_processor import post_process_html +from openforms.config.models import GlobalConfiguration +from openforms.utils.urls import build_absolute_uri + from ..formatters.formio import ( CheckboxFormatter, CurrencyFormatter, @@ -22,6 +31,7 @@ TimeFormatter, ) from ..registry import BasePlugin, register +from ..typing import Component @register("default") @@ -53,10 +63,28 @@ class PhoneNumber(BasePlugin): formatter = PhoneNumberFormatter +class FileComponent(Component): + storage: Literal["url"] + url: str + useConfigFiletypes: bool + filePattern: str + + @register("file") class File(BasePlugin): formatter = FileFormatter + @staticmethod + def rewrite_for_request(component: FileComponent, request: Request): + # write the upload endpoint information + upload_endpoint = reverse("api:formio:temporary-file-upload") + component["url"] = build_absolute_uri(upload_endpoint, request=request) + + # check if we need to apply "filePattern" modifications + if component.get("useConfigFiletypes", False): + config = GlobalConfiguration.get_solo() + component["filePattern"] = ",".join(config.form_upload_default_file_types) + @register("textarea") class TextArea(BasePlugin): @@ -101,3 +129,31 @@ class Radio(BasePlugin): @register("signature") class Signature(BasePlugin): formatter = SignatureFormatter + + +class ContentComponent(Component): + html: str + + +@register("content") +class Content(BasePlugin): + """ + Formio's WYSIWYG component. + """ + + # not really relevant as content components don't have values + formatter = DefaultFormatter + + @staticmethod + def rewrite_for_request(component: ContentComponent, request: Request): + """ + Ensure that the inline styles are made compatible with Content-Security-Policy. + + .. note:: we apply Bleach and a CSS declaration allowlist as part of the + post-processor because content components are not purely "trusted" content + from form-designers, but can contain malicious user input if the form + designer uses variables inside the HTML. The form submission data is passed + as template context to these HTML blobs, posing a potential injection + security risk. + """ + component["html"] = post_process_html(component["html"], request) diff --git a/src/openforms/formio/dynamic_config/__init__.py b/src/openforms/formio/dynamic_config/__init__.py index 3d98722f86..e405f01f84 100644 --- a/src/openforms/formio/dynamic_config/__init__.py +++ b/src/openforms/formio/dynamic_config/__init__.py @@ -5,22 +5,44 @@ """ from typing import Optional +from rest_framework.request import Request + from openforms.typing import DataMapping from ..datastructures import FormioConfigurationWrapper from ..registry import register -__all__ = ["apply_dynamic_configuration"] +__all__ = ["rewrite_formio_components", "rewrite_formio_components_for_request"] -def apply_dynamic_configuration( +def rewrite_formio_components( configuration_wrapper: FormioConfigurationWrapper, data: Optional[DataMapping] = None, ) -> FormioConfigurationWrapper: """ Loop over the formio configuration and mutate components in place. + + :arg configuration_wrapper: Container object holding the Formio form configuration + to be updated (if applicable). The rewriting loops over all components one-by-one + and applies the changes. + :arg data: key-value mapping of variable name to variable value. If a submission + context is available, the variables of the submission are included here. + :arg request: The HTTP request received by the API view when rewriting is done as + part of an HTTP request-response cycle. This is ``None`` otherwise (e.g. in + background tasks). """ data = data or {} # normalize for component in configuration_wrapper: register.update_config(component, data=data) return configuration_wrapper + + +def rewrite_formio_components_for_request( + configuration_wrapper: FormioConfigurationWrapper, request: Request +) -> FormioConfigurationWrapper: + """ + Loop over the formio configuration and inject request-specific configuration. + """ + for component in configuration_wrapper: + register.update_config_for_request(component, request=request) + return configuration_wrapper diff --git a/src/openforms/formio/registry.py b/src/openforms/formio/registry.py index 9648d7e1b1..b0d8de444c 100644 --- a/src/openforms/formio/registry.py +++ b/src/openforms/formio/registry.py @@ -15,6 +15,8 @@ from django.utils.translation import gettext as _ +from rest_framework.request import Request + from openforms.plugins.plugin import AbstractBasePlugin from openforms.plugins.registry import BaseRegistry from openforms.typing import DataMapping @@ -39,6 +41,11 @@ def __call__(self, component: Component, value: Any) -> Any: ... +class RewriterForRequestProtocol(Protocol): + def __call__(self, component: Component, request: Request) -> None: + ... + + class BasePlugin(AbstractBasePlugin): """ Base class for Formio component plugins. @@ -57,6 +64,10 @@ class BasePlugin(AbstractBasePlugin): """ Specify the normalizer callable to use for value normalization. """ + rewrite_for_request: None | RewriterForRequestProtocol = None + """ + Callback to invoke to rewrite plugin configuration for a given HTTP request. + """ @property def verbose_name(self): @@ -114,6 +125,21 @@ def update_config( plugin = self[component_type] plugin.mutate_config_dynamically(component, data) + def update_config_for_request(self, component: Component, request: Request) -> None: + """ + Mutate the component in place for the given request context. + """ + # if there is no plugin registered for the component, return the input + if (component_type := component["type"]) not in register: + return + + # invoke plugin if exists + rewriter = self[component_type].rewrite_for_request + if rewriter is None: + return + + rewriter(component, request) + def handle_custom_types( self, configuration: FormioConfigurationWrapper, diff --git a/src/openforms/formio/service.py b/src/openforms/formio/service.py index 422566b31d..4a6a10ca29 100644 --- a/src/openforms/formio/service.py +++ b/src/openforms/formio/service.py @@ -1,18 +1,28 @@ -from typing import Any, Optional +""" +Expose the public openforms.formio Python API. + +The service module exposes the functions/utilities that may be used by other Django +apps/packages: -from django.urls import reverse +* Try to keep this module stable and avoid breaking changes - extensions may rely on this! +* Keep it small! The actual implementation should be done in specialized subpackages or + submodules and only their 'public' API should be imported and used. +""" +from typing import Any, Optional import elasticapm from rest_framework.request import Request -from csp_post_processor import post_process_html from openforms.forms.custom_field_types import handle_custom_types from openforms.prefill import inject_prefill from openforms.submissions.models import Submission from openforms.typing import DataMapping from .datastructures import FormioConfigurationWrapper -from .dynamic_config import apply_dynamic_configuration +from .dynamic_config import ( + rewrite_formio_components, + rewrite_formio_components_for_request, +) from .registry import register from .typing import Component from .utils import iter_components @@ -20,15 +30,13 @@ __all__ = [ "get_dynamic_configuration", - "update_configuration_for_request", "normalize_value_for_component", "iter_components", "inject_variables", "format_value", + "rewrite_formio_components_for_request", ] -from ..config.models import GlobalConfiguration - def format_value(component: Component, value: Any, as_html: bool = False): return register.format(component, value, as_html=as_html) @@ -42,8 +50,6 @@ def normalize_value_for_component(component: Component, value: Any) -> Any: return register.normalize(component, value) -# TODO: it might be beneficial to memoize this function if it runs multiple times in -# the context of the same request @elasticapm.capture_span(span_type="app.formio") def get_dynamic_configuration( config_wrapper: FormioConfigurationWrapper, @@ -61,7 +67,8 @@ def get_dynamic_configuration( config_wrapper.configuration, request=request, submission=submission ) - apply_dynamic_configuration(config_wrapper, data=data) + rewrite_formio_components(config_wrapper, data=data) + # prefill is still 'special' even though it uses variables, as we specifically # set the `defaultValue` key to the resulting variable. # This *could* be refactored in the future by assigning a template expression to @@ -70,46 +77,3 @@ def get_dynamic_configuration( # as checkboxes/dropdowns/radios/... inject_prefill(config_wrapper, submission) return config_wrapper - - -def update_configuration_for_request( - config_wrapper: FormioConfigurationWrapper, request: Request -) -> None: - """ - Given a static Formio configuration, apply dynamic changes we always must do, like setting absolute urls. - - The configuration is modified in the context of the provided :arg:`submission`. - """ - - # TODO move this to openforms.formio.dynamic_config - pipeline = ( - update_urls_in_place, - update_default_file_types, - update_content_inline_csp, - ) - for component in config_wrapper: - for function in pipeline: - function(component, request=request) - - -def update_urls_in_place(component: Component, request: Request) -> None: - if component.get("type") == "file": - component["url"] = request.build_absolute_uri( - reverse("api:formio:temporary-file-upload") - ) - - -def update_default_file_types(component: Component, **kwargs) -> None: - if component.get("type") == "file" and component.get("useConfigFiletypes"): - config = GlobalConfiguration.get_solo() - component["filePattern"] = ",".join(config.form_upload_default_file_types) - - -def update_content_inline_csp(component: Component, request: Request) -> None: - if component.get("type") == "content": - """ - NOTE: we apply Bleach and a CSS declaration whitelist because content components are not purely "trusted" content from form-designers, - but can contain malicious user input if the form designer uses variables inside the HTML - (and the form submission data is passed as template context to those HTML blobs) - """ - component["html"] = post_process_html(component["html"], request) diff --git a/src/openforms/formio/tests/test_service.py b/src/openforms/formio/tests/test_service.py index 9b8d7e5008..a4169f3ec8 100644 --- a/src/openforms/formio/tests/test_service.py +++ b/src/openforms/formio/tests/test_service.py @@ -5,13 +5,13 @@ from openforms.formio.service import ( FormioConfigurationWrapper, - update_configuration_for_request, + rewrite_formio_components_for_request, ) class ServiceTestCase(TestCase): @patch("csp_post_processor.processor.get_html_id", return_value="1234") - def test_update_configuration_for_request(self, m): + def test_rewrite_formio_components_for_request(self, m): request = RequestFactory().get("/", HTTP_X_CSP_NONCE="dGVzdA==") configuration = { @@ -31,7 +31,7 @@ def test_update_configuration_for_request(self, m): }, ], } - update_configuration_for_request( + rewrite_formio_components_for_request( FormioConfigurationWrapper(configuration), request ) with self.subTest("temporary file upload url"): diff --git a/src/openforms/forms/api/serializers/form_definition.py b/src/openforms/forms/api/serializers/form_definition.py index 496b8eef72..6b6305ed63 100644 --- a/src/openforms/forms/api/serializers/form_definition.py +++ b/src/openforms/forms/api/serializers/form_definition.py @@ -5,7 +5,7 @@ from drf_spectacular.utils import extend_schema_field from rest_framework import serializers -from openforms.formio.service import update_configuration_for_request +from openforms.formio.service import rewrite_formio_components_for_request from ...models import Form, FormDefinition from ...validators import validate_form_definition_is_reusable @@ -50,10 +50,15 @@ def get_admin_url(self, obj: Form) -> str: class FormDefinitionSerializer(serializers.HyperlinkedModelSerializer): def to_representation(self, instance): representation = super().to_representation(instance=instance) - # set upload urls etc - # TODO: move this to openforms.formio.dynamic_config - update_configuration_for_request( - instance.configuration_wrapper, request=self.context["request"] + # Finalize formio component configuration with dynamic parts that depend on the + # HTTP request. Note that this is invoked through: + # 1. the :class:`openforms.submissions.api.serializers.SubmissionStepSerializer` + # for the dynamic formio configuration in the context of a submission. + # 2. The serializers/API endpoints of :module:`openforms.forms.api` for + # 'standalone' use/introspection. + rewrite_formio_components_for_request( + instance.configuration_wrapper, + request=self.context["request"], ) representation["configuration"] = instance.configuration_wrapper.configuration return representation diff --git a/src/openforms/submissions/tests/test_get_submission_step.py b/src/openforms/submissions/tests/test_get_submission_step.py index 80690320e6..201fb3fa2d 100644 --- a/src/openforms/submissions/tests/test_get_submission_step.py +++ b/src/openforms/submissions/tests/test_get_submission_step.py @@ -10,6 +10,8 @@ """ import uuid +from django.test import tag + from rest_framework import status from rest_framework.reverse import reverse from rest_framework.test import APITestCase @@ -187,6 +189,37 @@ def test_step_data_returned(self): {"someField": "data"}, ) + @tag("gh-1208", "gh-1068") + def test_dynamic_config_applied(self): + submission = SubmissionFactory.from_components( + [ + { + "type": "file", + "key": "file", + "storage": "url", + "url": "", # must be set dynamically + } + ] + ) + self._add_submission_to_session(submission) + step = submission.submissionstep_set.get() + url = reverse( + "api:submission-steps-detail", + kwargs={ + "submission_uuid": submission.uuid, + "step_uuid": step.form_step.uuid, + }, + ) + + response = self.client.get(url) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + data = response.json() + component = data["formStep"]["configuration"]["components"][0] + self.assertEqual(component["key"], "file") + self.assertEqual(component["type"], "file") + self.assertEqual(component["url"], "http://testserver/api/v2/formio/fileupload") + class GetSubmissionStepTests(SubmissionsMixin, APITestCase): @classmethod