Fix: is_user_using_two_factor(): respect intentional bypass via new filter — currently unreachable due to leftover early return - #882
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new hook to let integrators intentionally bypass the “email fallback” behavior when two_factor_enabled_providers_for_user clears a user’s enabled provider list, while preserving the existing fail-safe when providers genuinely disappear.
Changes:
- Updates
Two_Factor_Core::get_available_providers_for_user()to distinguish “providers missing” vs “providers intentionally cleared” and introduces thetwo_factor_email_fallback_enabledfilter. - Documents the new filter in
readme.txt.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
class-two-factor-core.php |
Adds logic + a new filter to optionally disable the email fallback when enabled providers are intentionally cleared. |
readme.txt |
Documents the new two_factor_email_fallback_enabled filter and its purpose. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $unfiltered = array_intersect( (array) $user_providers_raw, array_keys( $providers ) ); | ||
|
|
||
| /** | ||
| * Filters whether the email provider fallback is applied when a user's | ||
| * enabled provider list resolves to empty but they have providers configured | ||
| * in user meta. Return false to disable the fallback and allow an empty | ||
| * provider list to pass through — for example, to bypass two-factor for | ||
| * trusted IP addresses. | ||
| * | ||
| * This filter only runs when the configured providers still exist. If | ||
| * providers are genuinely missing or removed, the fail-safe always applies | ||
| * regardless of this filter. | ||
| * | ||
| * @since 0.17.0 | ||
| * | ||
| * @param bool $apply_fallback Whether to apply the email fallback. Default true. | ||
| * @param int $user_id The user ID. | ||
| */ | ||
| $apply_fallback = empty( $unfiltered ) || apply_filters( 'two_factor_email_fallback_enabled', true, $user->ID ); | ||
|
|
||
| if ( $apply_fallback ) { | ||
| if ( isset( $providers['Two_Factor_Email'] ) ) { | ||
| // Force Emailed codes to 'on'. | ||
| $enabled_providers[] = 'Two_Factor_Email'; |
There was a problem hiding this comment.
The new two_factor_email_fallback_enabled behavior changes the outcome of get_available_providers_for_user() when a valid provider list is intentionally cleared via two_factor_enabled_providers_for_user, but there’s no unit test coverage validating (1) fallback remains enabled by default and (2) the fallback can be disabled via the new filter while still preserving the existing fail-safe when providers are genuinely missing. Please add/extend tests (likely in tests/class-two-factor-core.php, near test_deprecated_provider_for_user) to cover these cases so regressions are caught.
There was a problem hiding this comment.
@nimesh-xecurify would you be able to support on unit test extension here?
|
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. |
georgestephanis
left a comment
There was a problem hiding this comment.
If someone wants to intentionally disable 2fa for a given user, surely we have a better way than zeroing out the providers?
It feels like the PR is working around the wrong thing.
Here's the call chain that matters:
filter_authenticate()andwp_login()both callis_user_using_two_factor()is_user_using_two_factor()callsget_primary_provider_for_user()get_primary_provider_for_user()exposes atwo_factor_primary_provider_for_userfilter
So there's actually already an indirect bypass: return null from two_factor_primary_provider_for_user and is_user_using_two_factor() returns false, skipping the whole 2FA flow. But that's a hack — it's not what that filter is for and the semantics are wrong.
What's missing is a clean, direct filter on is_user_using_two_factor() itself. Something like:
return (bool) apply_filters( 'two_factor_is_required_for_user', ! empty( $provider ), $user );
Then the trusted-IP bypass use case that #882 is trying to solve becomes:
add_filter( 'two_factor_is_required_for_user', function( $required, $user ) {
return is_trusted_ip() ? false : $required;
}, 10, 2 );
That's one filter, clear intent, no interaction with the provider list at all, and it doesn't require understanding the email fallback mechanics.
The PR's approach requires two filters working in concert (two_factor_enabled_providers_for_user + the new two_factor_email_fallback_enabled), plus understanding the subtle distinction between "providers missing" and "providers intentionally cleared." That's a lot of cognitive load for what should be a simple bypass.
My take is that we should instead open a focused PR that adds a two_factor_is_required_for_user filter to is_user_using_two_factor(). It's a smaller change, more composable, and covers the use case cleanly without touching the email fallback logic at all. The underlying issue (#871) would still be worth fixing separately as a bug — the fail-safe shouldn't fight against two_factor_enabled_providers_for_user when providers genuinely exist in meta — but that's orthogonal to the bypass use case.
|
Thanks for the review @georgestephanis! I've updated the PR and will split the second part in a new PR - as you've suggested. One small addition to your sketch: the method now resolves the user via For everyone following from #871 — the IP-based bypass becomes one filter instead of the two-filter combo proposed here: add_filter(
'two_factor_is_required_for_user',
function ( $is_required, $user ) {
$trusted_ips = array( '203.0.113.10' ); // Replace with your trusted IP(s).
if ( in_array( $_SERVER['REMOTE_ADDR'] ?? '', $trusted_ips, true ) ) {
return false; // Skip 2FA entirely for this request.
}
return $is_required;
},
10,
2
);
|
Updates the authentication bypass test to include a control case and more accurately verify that the plugin's cookie-blocking filter is correctly installed or omitted based on the bypass filter state.
Improve test coverage for 2FA bypass filter
georgestephanis
left a comment
There was a problem hiding this comment.
Good to ship, but may want to rename the pr as its on a new filter.
What?
Partially Resolves #871
This PR was split off from the original #882 approach following George's review, which pointed out that routing a bypass through two_factor_enabled_providers_for_user plus a new email-fallback filter was solving the wrong problem — and proposed instead a direct, composable two_factor_is_required_for_user filter on is_user_using_two_factor() itself, so a site can express e.g. "skip 2FA for trusted IPs" without touching the provider-list or fail-safe mechanics at all. The original fail-safe bug from #871 is now being addressed separately in #927; this PR is scoped only to adding the filter George proposed.
Related: #927 (companion PR reworking the underlying fail-safe logic)
Use of AI Tools
AI assistance: Yes
Tool(s): Claude (claude.ai)
Model(s): Claude Sonnet 4.6
Used for: Identifying the root cause, drafting the implementation.
Testing Instructions
Two_Factor_Emailenabled.functions.php:return).Log in as a user with no provider configured — login should now be blocked as 2FA-required, rather than succeeding with a single factor.
Screenshots or screencast
N/A — no UI changes.
Changelog Entry