Skip to content

fix(totp): delete user TOTP key after disabling the provider#802

Open
sjinks wants to merge 1 commit intoWordPress:masterfrom
sjinks:fix/delete-totp
Open

fix(totp): delete user TOTP key after disabling the provider#802
sjinks wants to merge 1 commit intoWordPress:masterfrom
sjinks:fix/delete-totp

Conversation

@sjinks
Copy link
Contributor

@sjinks sjinks commented Feb 21, 2026

What?

The REST handler deletes the TOTP secret key first (delete_user_totp_key()), then attempts to disable the provider via Two_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

Fixed - delete user TOTP key only after the provider has been successfully disabled.

@sjinks sjinks marked this pull request as ready for review February 21, 2026 13:10
@github-actions
Copy link

github-actions bot commented Feb 21, 2026

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: sjinks <volodymyrkolesnykov@git.wordpress.org>
Co-authored-by: masteradhoc <masteradhoc@git.wordpress.org>
Co-authored-by: georgestephanis <georgestephanis@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link
Collaborator

@masteradhoc masteradhoc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@georgestephanis georgestephanis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@georgestephanis
Copy link
Collaborator

@sjinks any thoughts on ^^ w/r/t temporarily disabling without purging the key? Not sure if it's at all a reasonable use case worth accounting for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

4 participants