rework fail-safe - #927
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @abovowebdevelopment, @lennarthendriksma-abovo, @sirolf. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. |
There was a problem hiding this comment.
Pull request overview
This PR refines Two-Factor’s login fail-safe in Two_Factor_Core::get_available_providers_for_user() so the Email fallback is only forced when a user’s stored providers are truly no longer registered, while respecting intentional empty-provider results from the two_factor_enabled_providers_for_user filter (addressing #871’s regression/use case).
Changes:
- Add a normalized “stored provider keys” helper to distinguish raw stored providers from the filtered enabled list.
- Rework the fail-safe to trigger only when none of the stored providers remain registered, and introduce a
two_factor_fallback_provider_for_userfilter to select the fallback provider. - Update PHPUnit coverage for the “filter-cleared list is respected” case and the “missing provider triggers fallback” case; document the new filter in
readme.txt.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
class-two-factor-core.php |
Implements the revised fail-safe logic, adds a normalized stored-provider helper, and introduces the fallback-provider filter. |
tests/class-two-factor-core.php |
Updates an enabled-provider assertion and adds tests covering the filter-cleared list behavior and missing-provider fallback. |
readme.txt |
Documents the new two_factor_fallback_provider_for_user filter hook and its behavior/constraints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
What?
Partially resolves #871 (see #882 (review))
Reworks the
get_available_providers_for_user()fail-safe so it only forces emailed codes on when a user's stored providers are genuinely no longer registered — instead of every time the enabled provider list comes back empty.Why?
The fail-safe exists so that a deprecated or removed provider can't silently drop a user to single-factor auth. It fires when
_two_factor_enabled_providersuser meta is non-empty but the resolved enabled list is empty, and forcesTwo_Factor_Emailon (or returns ano_available_2fa_methodsWP_Errorif Email isn't registered either).The problem is that an empty list has two very different causes, and the old check couldn't tell them apart:
WP_Errortwo_factor_enabled_providers_for_userdeliberately returnedarray()The second case was being overridden. A site using the documented filter to intentionally exclude a user from 2FA got emailed codes forced back on, and had no way to express "no providers" through the filter at all — the filter was effectively inert for that value.
How?
Before forcing the fallback, intersect the user's stored provider keys against the currently registered providers:
If any stored provider is still registered, an empty enabled list can only have come from the filter, so it's honoured. If none are, the fail-safe behaves exactly as before.
Two points worth flagging for review:
"No longer registered" is deliberately cause-agnostic. A provider dropped by plugin deactivation, by the site-wide settings option, or by
two_factor_providers_for_useris treated identically, because the outcome for the user is identical — their configured second factor no longer exists. Note that disabling Email site-wide is already respected without special-casing, via the existingisset( $providers['Two_Factor_Email'] )guard: that path falls through to theWP_Errorrather than forcing a provider the admin turned off.This intentionally widens a fail-open path. Cases that previously got Email forced on now return an empty array, which flows into
is_user_using_two_factor(). That's the intended fix and it's opt-in — a site has to actively clear the list via filter to reach it.Use of AI Tools
AI assistance: Yes
Tool(s): Claude
Model(s): Claude Opus 5
Used for: Code review of the existing implementation, and drafting the manual test matrix below. Implementation authored and manually verified by me.
Testing Instructions
Use a throwaway site and a second admin account for recovery — several cases deliberately lock a user out. Run logins in a private window.
Setup. While all providers are still registered, create and configure three users:
t_email— Email onlyt_totp— Authenticator app onlyt_both— bothAdd
wp-content/mu-plugins/2fa-test.php, uncommenting one block at a time:A. No regressions (no blocks active)
Log in as each of
t_email,t_totp,t_both— each is prompted for their configured method,t_bothcan switch between them.B. Fail-safe still fires
Block 1, unset Totp only → log in as
t_totp→ prompted for an emailed code.Block 1, unset Totp and Email → log in as
t_totp→ refused with the "provider(s) no longer exist" message. Must not reach the dashboard.Block 1, unset Totp only → log in as
t_both→ prompted for an emailed code via the normal path.C. The fix — run each against
mastertoo, to see the differenceBlock 2 only → log in as
t_email→ no 2FA prompt, straight to the dashboard. Onmaster: emailed codes forced on.Blocks 1 (unset Totp) + 2 → log in as
t_both→ no prompt. Onmaster: emailed codes forced on.Blocks 1 (unset both) + 2 → log in as
t_totp→ refused with the error. Unchanged frommaster— this is the boundary and must not flip.D. Settings screen behaves identically to the filter
Settings → Two-Factor, untick Authenticator app → log in as
t_totp→ emailed code. Matches B2.Untick Authenticator app and Email → log in as
t_totp→ refused. Matches B3.Watch
debug.logthroughout — no notices, warnings, or fatals, in particular no "Array to string conversion" orTypeErrorfromarray_intersect().Note on step 9. Failing closed here is intended, but an admin unticking every provider in the settings UI receives no warning that configured users are about to be locked out, and recovery means deactivating the plugin over SFTP. That's a pre-existing footgun this PR doesn't introduce or worsen — this should be discussed.
Screenshots or screencast
No UI changes — behaviour is only observable in the login flow, covered by the steps above.
Changelog Entry