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

Require an "ordinary" provider to use Backup Codes #164

Merged
merged 2 commits into from
May 26, 2023
Merged

Conversation

iandunn
Copy link
Member

@iandunn iandunn commented May 16, 2023

Fixes #158

This preserves the code from #75, but goes further to disable the provider for the user until they have an "ordinary" provider enabled.

"Ordinary" means that the provider is intended for normal use (like TOTP), as opposed to a fallback provider (like Backup Codes) that is only used when the user can't access their ordinary device.

Testing

  1. Enable TOTP and Backup Codes
  2. Disable TOTP. You should see the elevated privileges warning on the front- and back-ends. The Backup Codes provider will also show as disabled on the front- and back-ends.
Screenshot 2023-05-22 at 2 45 41 PM

Notes

  • If WebAuthn is enabled and TOTP is disabled, the custom UI will still show "Two-Factor auth" (TOTP) as disabled. That will be fixed by Add WebAuthn UI stub #87 and subsequent PRs.

  • This doesn't automatically update the Two_Factor_Core::ENABLED_PROVIDERS_USER_META_KEY database value, so, there are some normal situations where the two values are out of sync.

    For example, when a new user sets up TOTP and Backup Codes through the custom UI. In that scenario, the db value will only have TOTP, but in memory it'll have Backup Codes added. If there's a subsequent save to the value, then the database will be updated to reflect the value in memory.

    I don't think that'll cause any tangible issues in practice, but it's worth mentioning. If we want, we could add extra code to update the DB, but the time vs benefit tradeoff doesn't seem worth it to me at this point.

@iandunn iandunn self-assigned this May 16, 2023
@iandunn iandunn modified the milestones: MVP, Iteration 1 May 16, 2023
@iandunn iandunn force-pushed the disable-backup-158 branch 2 times, most recently from 0fbac03 to 1e649dd Compare May 22, 2023 21:50
@iandunn iandunn requested review from adamwoodnz and dd32 May 22, 2023 21:50
@iandunn iandunn marked this pull request as ready for review May 22, 2023 21:50
@iandunn iandunn marked this pull request as draft May 22, 2023 21:52
@iandunn
Copy link
Member Author

iandunn commented May 22, 2023

Nevermind @dd32 , @adamwoodnz . I'm switching this back to draft for now, to fix a test and also because it'll be hard to see the changes in the diff until #161 is merged.

@iandunn iandunn force-pushed the disable-backup-158 branch 3 times, most recently from 262dc41 to 6417c65 Compare May 23, 2023 19:22
@iandunn iandunn force-pushed the disable-backup-158 branch from 6417c65 to ecc3e27 Compare May 23, 2023 19:28
@iandunn iandunn marked this pull request as ready for review May 23, 2023 19:35
@iandunn
Copy link
Member Author

iandunn commented May 23, 2023

@adamwoodnz , @dd32 ok, this is ready for review now.

@iandunn iandunn merged commit 7d9251d into trunk May 26, 2023
@iandunn iandunn deleted the disable-backup-158 branch May 26, 2023 16:36
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.

Rest API shows backup codes enabled when TOTP disabled
1 participant