fix: TOTP invalid code and/or 500 error

[kszafran]

Fixes a 500 error if TOTP is removed and then re-added in the same flow.

It also fixes another related bug, that can be reproduced with https://github.com/ory/kratos-selfservice-ui-react-nextjs:

1. go to settings
2. change your name (or email, or whatever)
3. try to set up TOTP
4. "invalid code"

The reason is that because the InternalContext is not reset properly in PostSettingsHook, the flow will have a new QR code and secret key (UI is reset), but the internal context will keep the old TOTP secret.

In the case of the original issue, InternalContextKeyURL is deleted from InternalContext when TOTP is removed, and since it's not set properly in PostSettingsHook, when we try to re-add TOTP, we get 500, because InternalContextKeyURL is missing.

Related issue(s)

#2680

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
  security@ory.sh) 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

WebAuthn is most likely affected by the same bug, but I didn't any tests there. I wasn't able to run WebAuthn tests locally, because of this error:

cy.task('resetCRI') failed with the following error:

The 'task' event has not been registered in the plugins file. You must register it before using cy.task()

[aeneasr]

Great find, thank you!

[kszafran]

@aeneasr I fixed the failing lookup/settings_test.go by changing the assertion from:

assert.Equal(t, "{}", gjson.GetBytes(actualFlow.InternalContext, ...

to

assert.Empty(t, gjson.GetBytes(actualFlow.InternalContext, ...

I haven't looked too deep into it, but my initial guess is that the test was relying on the buggy behavior, where the internal context was not reset properly. There is already an assertion like this (using assert.Empty(t, ...) in another place in the file, so I'm assuming the behavior is correct.

[kszafran]

And now I also fixed webauthn/settings_test.go by changing an assertion. I hope it makes sense, but since I'm not familiar with the WebAuthn flow, I'm not exactly sure.

[codecov]

Codecov Report

Merging #2960 (d9741f0) into master (ca35b45) will decrease coverage by 1.07%.
The diff coverage is 100.00%.

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

@@            Coverage Diff             @@
##           master    #2960      +/-   ##
==========================================
- Coverage   77.59%   76.51%   -1.08%     
==========================================
  Files         310      308       -2     
  Lines       19265    19181      -84     
==========================================
- Hits        14948    14677     -271     
- Misses       3180     3402     +222     
+ Partials     1137     1102      -35     

Impacted Files	Coverage Δ
selfservice/flow/settings/hook.go	59.23% <100.00%> (+0.26%)	⬆️
selfservice/strategy/oidc/provider_netid.go	0.00% <0.00%> (-78.27%)	⬇️
selfservice/strategy/oidc/provider_yandex.go	0.00% <0.00%> (-76.28%)	⬇️
selfservice/strategy/oidc/provider_vk.go	0.00% <0.00%> (-73.92%)	⬇️
selfservice/strategy/oidc/provider_microsoft.go	0.00% <0.00%> (-60.00%)	⬇️
selfservice/strategy/oidc/provider_dingtalk.go	0.00% <0.00%> (-31.53%)	⬇️
selfservice/strategy/oidc/provider_gitlab.go	72.22% <0.00%> (-9.26%)	⬇️
selfservice/strategy/oidc/provider_auth0.go	63.49% <0.00%> (-8.39%)	⬇️
identity/credentials_oidc.go	85.00% <0.00%> (-2.50%)	⬇️
selfservice/strategy/oidc/provider_facebook.go	83.09% <0.00%> (-0.47%)	⬇️
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[kszafran]

And now some snapshot comparisons fail in WebAuthn tests, even though they passed before 😞 .

[aeneasr]

Yeah sorry about that, we had some breaks on master :( Should all be fixed now