🐛 [#4199] Delete authinfo from session after it's stored on submission

[stevenbal]

Closes #4199 partly
Related PR: open-formulieren/open-forms-sdk#688

Changes

• Delete authinfo from session after it's stored on submission
• Add explicit anonymous option to submission endpoint

TODO:

• add test for anonymous attribute
• check where FORM_AUTH_SESSION_KEY is used
  • set_auth_attribute_on_session
  • set_cosign_data_on_submission (cosign also uses is_authenticated_with_an_allowed_plugin)
  • ResumeFormMixin: is_auth_data_correct / get_redirect_url -> user is always forced to login again (is this an issue?)
    • is_authenticated_with_plugin / meets_plugin_requirements -> run after relogging, so this is fine
    • EHerkenningAuthentication.check_requirements -> used in meets_plugin_requirements which is only used for resume flow
    • VerifyChangeAppointmentLinkView.custom_submission_modifications -> only used in resume flow
• expose function to remove auth info from session in service.py
• call this function at end of add_submission_to_session and other functions that rely on this attr (?)
• update branch in CI of token-exchange lib to see if it passes: [DO NOT MERGE] 📌 Test with session persistance branch open-forms-ext-token-exchange#16

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.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.27%. Comparing base (9b49cd1) to head (49783de).
Report is 729 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #4271   +/-   ##
=======================================
  Coverage   96.26%   96.27%           
=======================================
  Files         731      731           
  Lines       23718    23735   +17     
  Branches     2795     2797    +2     
=======================================
+ Hits        22833    22850   +17     
  Misses        616      616           
  Partials      269      269           

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

[stevenbal]

@sergei-maertens I'm not 100% sure if this needs backport, what is our policy for when to backport bugfixes?

[stevenbal]

@sergei-maertens I checked the usage of the session auth info and I didn't notice any problems if we remove it from the session, the only thing that changes is that when resuming a form, the user will always be forced to login again even if it's within the same session (because the authinfo isn't persisted on the session anymore)

[sergei-maertens]

@sergei-maertens I'm not 100% sure if this needs backport, what is our policy for when to backport bugfixes?

We typically label the ticket as such during refinement, based on how big the impact is and whether a workaround exists or not.

I think that the odds that a end user is filling out multiple forms at the same time where one is authenticated and the other one not are pretty low, so I don't think it warrants a backport. There also doesn't seem to be a stakeholder (except for us) in this, so it doesn't seem to be a big problem.

[sergei-maertens]

@sergei-maertens I checked the usage of the session auth info and I didn't notice any problems if we remove it from the session, the only thing that changes is that when resuming a form, the user will always be forced to login again even if it's within the same session (because the authinfo isn't persisted on the session anymore)

I think that's a good side-effect of this patch, it fits better with the expectations that resuming always forces you to re-authenticate and creates less different authentication flows for the same "mechanism".

[sergei-maertens]

I'd like to see a test with explicit anonymous start in openforms.submissions.tests.test_start_submission.

I don't put too much value on the unit tests of the signal, I'm mostly interested in the integration tests for the POST call that those behave as expected, so please add a test that sets up an authentication state and creates an anonymous submission and make assertions on that created submission.

[sergei-maertens]

Suggested change

request = Request(request)

APIRequestFactory already produces a Request instance, no?

I'd expect your type checker to complain about this too btw

[stevenbal]

I thought so too, that is why I used APIRequestFactory instead of RequestFactory but strangely enough my type checker complains about this if I don't explicitly cast it:

[sergei-maertens]

what happens if you add an assert isinstance(request, Rrequest) after it? That should help the type checker

I am talking nonsense, the factory just generates a plain Django HttpRequest and the Request layer from DRF is something that only happens in views at runtime.

You could use openforms.typing.AnyRequest in your function signature instead, since the Request instance proxies down into the HttpRequest anyway.

[stevenbal]

I'll add AnyRequest 👍

[stevenbal]

So this means my type checker isn't messed up? 😄

[stevenbal]

Is this a known flaky test? @sergei-maertens https://github.com/open-formulieren/open-forms/actions/runs/9208977538/job/25332413671?pr=4271#step:7:3347

[sergei-maertens]

Is this a known flaky test? @sergei-maertens https://github.com/open-formulieren/open-forms/actions/runs/9208977538/job/25332413671?pr=4271#step:7:3347

Yeah, sporadically it crashes. hypothesis tends to reveal those cases...

[sergei-maertens]

I'll leave it up to you to hit the merge button if this is ready to be merged in your opinion :)

[stevenbal]

I'll leave it up to you to hit the merge button if this is ready to be merged in your opinion :)

I pushed the typing changed and will merge it once CI passes