fix(totp): delete user TOTP key after disabling the provider#802
fix(totp): delete user TOTP key after disabling the provider#802sjinks wants to merge 1 commit intoWordPress:masterfrom
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
masteradhoc
left a comment
There was a problem hiding this comment.
In the old flow, if disable_provider_for_user() failed for any reason, you had already deleted the key. Looks good to not destroy the totp until the provider disable actually succeeds.
georgestephanis
left a comment
There was a problem hiding this comment.
Sounds good -- I'm wondering if it would be worth seperating the deletions though, so you can temporarily disable totp without purging the key entirely so it could be reenabled with existing authenticators?
What?
The REST handler deletes the TOTP secret key first (
delete_user_totp_key()), then attempts to disable the provider viaTwo_Factor_Core::disable_provider_for_user(). If disabling fails (due to a DB error), the request returns an error, but the secret has already been removed.Why?
Inconsistent user state: the provider may remain enabled even when no secret exists, potentially causing login failures or confusing the UI.
How?
Change operation order: disable the provider first.
Testing Instructions
N/A
Screenshots or screencast
N/A
Changelog Entry