fix: omit irrelevant OIDC providers in forced refresh login flows

[Saancreed]

Whenever an user is asked to reauthenticate (e.g. because they wish to execute settings flow touching their credentials and their session is no longer privileged) they are asked to provide their credentials again. The forced-refresh login flow generated for such cases already excludes some strategies that are enabled in Kratos but cannot be used to authenticate as current identity, and for example the form presented to the user will not have a password field if the identity does not have a password credential.

This, however, does not currently apply to OIDC providers; the user will always see the full set even if some of them can't be used to sign in as current identity. This change causes forced refresh login flows to also omit irrelevant OIDC providers in generated form in order to avoid confunding the user about which strategies/providers are valid and can actually be used to reauthenticate.

Related issue(s)

Not reported separately. To reproduce:

1. Configure Kratos with password credentials and some OIDC providers enabled.
2. Register and login using a password.
3. Wait until session is old enough to no longer be considered privileged (1h by default) and try to update password using settings flow.
4. See that the "Confirm it's you" form offers signing in with OIDC providers that are not linked to current identity and can't actually be used to reauthenticate.

The inverse does not happen: if registering with OIDC, the form does not have a password field because the identity does not have a password credential yet.

Checklist

• I have read the contributing guidelines.
• I have referenced an issue containing the design document if my change
  introduces a new feature.
• I am following the
  contributing code guidelines.
• I have read the security policy.
• I confirm that this pull request does not address a security
  vulnerability. If this pull request addresses a security vulnerability, I
  confirm that I got the approval (please contact
  [email protected]) from the maintainers to push
  the changes.
• I have added tests that prove my fix is effective or that my feature
  works.
• I have added or changed the documentation.

Further Comments

Removing nodes/providers should be safe enough because we are operating in the context of previously authenticated identity. The user already knows which OIDC providers are linked because they can see this info in settings flow itself.

I've tested this by extending Settings flow linking and unlinking when unauthed scenarios but right now the check is a bit fragile (because it extracts login flow ID from URL query params) and probably not as precise as it could be (because I'm only checking that there are no nodes for providers other than our identity's ory and github but ideally I'd do a set-equals there). If you have any ideas on how to improve testing this, let me know and I'll try to apply them.

Hopefully no E2E tests rely on current behavior 🙏

[codecov]

Codecov Report

Merging #3608 (b1d9b46) into master (a639e56) will increase coverage by 0.01%.
The diff coverage is 71.42%.

❗ Current head b1d9b46 differs from pull request most recent head 6dbc940. Consider uploading reports for the commit 6dbc940 to get more accurate results

@@            Coverage Diff             @@
##           master    #3608      +/-   ##
==========================================
+ Coverage   78.19%   78.21%   +0.01%     
==========================================
  Files         341      341              
  Lines       23142    23202      +60     
==========================================
+ Hits        18097    18148      +51     
- Misses       3689     3691       +2     
- Partials     1356     1363       +7     

Files	Coverage Δ
selfservice/strategy/oidc/strategy_login.go	68.37% <100.00%> (ø)
selfservice/strategy/oidc/strategy_registration.go	69.29% <100.00%> (ø)
selfservice/strategy/oidc/strategy.go	64.08% <68.42%> (+0.03%)	⬆️

... and 2 files with indirect coverage changes

[aeneasr]

Very nice! Could you please add an e2e test to show that this works end-to-end as well and doesn't encounter regressions in the future? :)

[Saancreed]

@aeneasr Done 🙂

[jonas-jonas]

Very nice changes! Thank you for adding the e2e test.