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

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>

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?

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