✨ [#4398] Check object ownership when creating submission

[stevenbal]

Closes #4398

TODO:

• add (required) auth_attribute_path to PrefillConfigurationForm (relies on 👽 [#4693] Integrate Objects API prefill modal with backend #4799)
• Ensure auth_attribute_path is copied when copying registration -> prefill config

Changes

• Check object ownership when creating submission

Checklist

Check off the items that are completed or not relevant.

• Impact on features

  • Checked copying a form
  • Checked import/export of a form
  • Config checks in the configuration overview admin page
  • Problem detection in the admin email digest is handled

• Release management

  • I have labelled the PR as "needs-backport" accordingly

• I have updated the translations assets (you do NOT need to provide translations)

  • Ran ./bin/makemessages_js.sh
  • Ran ./bin/compilemessages_js.sh

• Commit hygiene

  • Commit messages refer to the relevant Github issue
  • Commit messages explain the "why" of change, not the how

[codecov]

Codecov Report

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

Project coverage is 96.58%. Comparing base (4b73a1d) to head (15203ce).

Files with missing lines	Patch %	Lines
...nforms/registrations/contrib/objects_api/plugin.py	88.23%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4696      +/-   ##
==========================================
+ Coverage   96.57%   96.58%   +0.01%     
==========================================
  Files         747      748       +1     
  Lines       25400    25487      +87     
  Branches     3355     3366      +11     
==========================================
+ Hits        24529    24616      +87     
  Misses        608      608              
  Partials      263      263              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sergei-maertens]

I think this is happening at the wrong place with the wrong assumptions 😬

For starters, session[FORM_AUTH_SESSION_KEY] should not be relied on - ideally once the submission is created/started, this should be cleaned from the session data entirely since we have a hook/signal that records it on the submission itself.

Second - we should always pass a submission instance when performing this kind of validation, exactly because different credentials could apply to a submission vs. what's in the session (the session data is a data race, since you can have different authentication details there compared to a submission that is also still in your session, for example when you are filling out two forms at the same time with different login options).

I think the prefill plugin itself should perform this access control, since:

1. in submissions.api.viewsets.SubmissionViewSet.perform_create we dispatch the submission start signal, that ensures we have the auth details stored
2. after that, we call prefill_variables, which receives the submission instance and passes it to the plugin
3. inside the plugin, you should be able to raise PermissionDenied if anything is fishy
4. if that exception is raised, it should bubble up to the API endpoint, will result in an HTTP 403 and the DB transaction should be rolled back so that the submission record is never persisted in the database

I'm afraid that makes this PR/ticket depend on #4620 then 🤔

[stevenbal]

@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?

[sergei-maertens]

@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 😅

[stevenbal]

@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]

@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]

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

[sergei-maertens]

yes

[stevenbal]

What is the right place to configure this, should it be on both registration backend and prefill options? I don't know if there is a way around that, since I think it could be possible to use one without the other

[sergei-maertens]

Yeah indeed, and the UI copy button is then the convenience so that users can setup prefill from their registration configuration

[stevenbal]

I've added a TODO in the PR to add the field to the prefill config form as well (since this is probably reliant on #4799)

[stevenbal]

I'm not sure if this is a general issue with the objects API config modal, but it seems that the error returned by the backend is not shown on the field:

[sergei-maertens]

you should be able to set up the necesary frontend state & reproduce this problem in storybook! might be that we don't have validation error display hooked up into this arrayfield (?) yet