Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't confirm SMS setup during sign in #377

Closed
wants to merge 3 commits into from

Conversation

Bertg
Copy link
Contributor

@Bertg Bertg commented Dec 13, 2023

This is a possible simple fix for #376

  • Introduce failing test
  • Fix issue where we accidentally accept a non-confirmed SMS code during sign in

@jeremyevans
Copy link
Owner

Thanks for this. The test is especially helpful. It passes both with the fix here and the fix I posted in #376 (comment). I'm thinking of combining the two, that way we can skip the query in sms_remove_failures if the sms still needs confirmation.

@Bertg
Copy link
Contributor Author

Bertg commented Dec 13, 2023

@jeremyevans Your call on how to proceed.

For me the main thing I wanted to achieve here is writing the test, as that unambiguously proved the issue.
I think the quick return in sms_remove_failures is good, but can be combined with the conditional update from your patch, so that any race condition in the future would also be covered.

I'll apply your changes - as I see them - to this PR.

@Bertg
Copy link
Contributor Author

Bertg commented Dec 13, 2023

@jeremyevans Updated PR is here ☝️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants