open-formulieren / open-forms

Smart and dynamic forms
https://open-forms.readthedocs.io
Other
37 stars 26 forks source link

:sparkles: [#4398] Check object ownership when creating submission #4696

Open stevenbal opened 2 months ago

stevenbal commented 2 months ago

Closes #4398

Changes

Checklist

Check off the items that are completed or not relevant.

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 97.89474% with 2 lines in your changes missing coverage. Please review.

Project coverage is 96.61%. Comparing base (65133e1) to head (2aa507c).

Files with missing lines Patch % Lines
...nforms/registrations/contrib/objects_api/plugin.py 88.88% 1 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #4696 +/- ## ======================================= Coverage 96.60% 96.61% ======================================= Files 751 752 +1 Lines 25692 25785 +93 Branches 3399 3411 +12 ======================================= + Hits 24821 24911 +90 - Misses 609 612 +3 Partials 262 262 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

stevenbal commented 1 month ago

@sergei-maertens it was mentioned in https://github.com/open-formulieren/open-forms/issues/4721 that the mechanism to pass initial_data_reference and update existing objects for Objects API registration (instead of creating new ones) should also function if prefill is not used. What would be the best place to do this permission check without having to implement it both in the registration backend and the prefill plugin?

sergei-maertens commented 1 month ago

@sergei-maertens it was mentioned in #4721 that the mechanism to pass initial_data_reference and update existing objects for Objects API registration (instead of creating new ones) should also function if prefill is not used. What would be the best place to do this permission check without having to implement it both in the registration backend and the prefill plugin?

uh... right. I'll have to think about that :sweat_smile:

stevenbal commented 1 month ago

@sergei-maertens maybe there should be some kind of hook in SubmissionViewSet.perform_create that runs any validation for initial_data_reference for the configured registration backend and prefill plugins? That way it's generic, though I'm not sure what we could do to avoid performing the same validation twice, perhaps we could skip the validation for the prefill plugin if validation with the same PLUGIN_IDENTIFIER (in this case objects_api) has already been performed for a registration backend?

sergei-maertens commented 1 month ago

@stevenbal I still don't see a simple/elegant way to do this, especially because data can be mutated externally and be outdated. E.g., if you verify it at perform_create time and track that it was okay at that time, some other system could make corrections to the data and by the time it's being registered, it could possibly no longer be verified (because a BSN was updated due to a mistake, for example).

I'm kind of leaning towards implementing the permission check itself once in openforms.contrib.objects_api and then re-using that functionality in both prefill and registration. This means that the check is done twice, but it does make it very explicit and I'd rather check too often than not often enough.

It's again a case of two different systems doing very similar things, but that doesn't make them the same. The prefill mapping and registration mapping are technically independent from each other too, we'll just offer convenience UI options to use one as a source for the other, but in the backend, they don't know of each other's existence and that's fine.

The meaning of initial_data_reference only becomes clear when interpreted in the context of the plugin using it anyway - so I don't see an agnostic "generic" mechanism. There's also a risk involved with that though - you may accidentally forget to implement access controls.

Perhaps for registration plugins we define a new hook on the base plugin which raises NotImplementError by default (so it causes a crash if forgotten). Let's call it verify_initial_data_ownership and it only gets called if submission.initial_data_reference is populated, so it won't break existing forms. You can then implement this calling logic in openforms.registrations.tasks.pre_registration so that it's plugin-agnostic. A similar pattern could then probably be applied to prefill?

stevenbal commented 1 month ago

@sergei-maertens that sound like a good idea, I'll see if I can implement this

stevenbal commented 11 hours ago

@sergei-maertens I think I processed all comments except https://github.com/open-formulieren/open-forms/pull/4696#discussion_r1863209778, I'm not sure how to implement that and keep the logging