✨ [#4398] Check object ownership when creating submission

[stevenbal]

Closes #4398

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

All modified and coverable lines are covered by tests ✅

Project coverage is 96.61%. Comparing base (0bda3f3) to head (44c89d8).
Report is 42 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4696      +/-   ##
==========================================
- Coverage   96.61%   96.61%   -0.01%     
==========================================
  Files         751      752       +1     
  Lines       25762    25848      +86     
  Branches     3413     3422       +9     
==========================================
+ Hits        24891    24974      +83     
- Misses        609      612       +3     
  Partials      262      262              

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

I'm happy with it being in this state, let's hope CI passes too and then it's finally done! 🎉