Skip to content

Ensure that doesn't 'fail open' if existing providers poof. #586

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
georgestephanis merged 12 commits into from
Feb 18, 2026
Merged

Ensure that doesn't 'fail open' if existing providers poof. #586

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
c79bbcc
Ensure that doesn't 'fail open' if existing providers poof.
georgestephanis Aug 24, 2023
bd676f9
Do the fallback slightly downstream.
georgestephanis Aug 24, 2023
9ad6dd6
Handle the WP_Error fail cases
georgestephanis Aug 24, 2023
c9aa7ac
Add a unit test for this.
georgestephanis Aug 24, 2023
a745c3c
Ensure we're comparing the keys.
georgestephanis Aug 24, 2023
f8c780d
Use a class that won't get stripped out.
georgestephanis Aug 24, 2023
2e616ea
Update the error messages for understandability.
georgestephanis Aug 24, 2023
947afdd
Merge branch 'master' into add/fallback-email
dd32 Sep 19, 2024
d12c89a
Merge branch 'master' into add/fallback-email
georgestephanis Sep 17, 2025
1f0035b
Whoops, reintroduced in merge.
georgestephanis Sep 17, 2025
66b43ae
Tidy up and run it further downstream.
georgestephanis Sep 18, 2025
953e5c2
Revert "Tidy up and run it further downstream."
georgestephanis Feb 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Do the fallback slightly downstream.
  • Loading branch information
@georgestephanis
georgestephanis committed Aug 24, 2023
commit bd676f9516768a8a9ac22b643268e8f84071140c
53 changes: 28 additions & 25 deletions class-two-factor-core.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,44 +413,22 @@ public static function get_enabled_providers_for_user( $user = null ) {
if ( empty( $enabled_providers ) ) {
$enabled_providers = array();
}
$enabled_existing_providers = array_intersect( $enabled_providers, array_keys( $providers ) );

/**
* If the user had enabled providers, but none of them exist currently,
* if emailed codes is available force it to be on, so that deprecated
* or removed providers don't result in the two-factor requirement being
* removed and 'failing open'.
*/
if ( $enabled_providers && empty( $enabled_existing_providers ) ) {
if ( isset( $providers['Two_Factor_Email'] ) ) {
// Force Emailed codes to 'on'.
$enabled_existing_providers[] = 'Two_Factor_Email';
} else {
return new WP_Error(
'no_available_2fa_methods',
__( 'Error: User has Two Factor method(s) enabled, but provider(s) no longer exist,', 'two-factor' ),
array(
'enabled_providers' => $enabled_providers,
'available_providers' => array_keys( $providers ),
)
);
}
}
$enabled_providers = array_intersect( $enabled_providers, array_keys( $providers ) );

/**
* Filter the enabled two-factor authentication providers for this user.
*
* @param array $enabled_providers The enabled providers.
* @param int $user_id The user ID.
*/
return apply_filters( 'two_factor_enabled_providers_for_user', $enabled_existing_providers, $user->ID );
return apply_filters( 'two_factor_enabled_providers_for_user', $enabled_providers, $user->ID );
}

/**
* Get all Two-Factor Auth providers that are both enabled and configured for the specified|current user.
*
* @param int|WP_User $user Optonal. User ID, or WP_User object of the the user. Defaults to current user.
* @return array
* @return array|WP_Error
*/
public static function get_available_providers_for_user( $user = null ) {
$user = self::fetch_user( $user );
Expand All @@ -461,6 +439,31 @@ public static function get_available_providers_for_user( $user = null ) {
$providers = self::get_providers();
$enabled_providers = self::get_enabled_providers_for_user( $user );
$configured_providers = array();
$user_providers_raw = get_user_meta( $user->ID, self::ENABLED_PROVIDERS_USER_META_KEY, true );

/**
* If the user had enabled providers, but none of them exist currently,
* if emailed codes is available force it to be on, so that deprecated
* or removed providers don't result in the two-factor requirement being
* removed and 'failing open'.
*
* Possible enhancement: add a filter to change the fallback method?
*/
if ( empty( $enabled_providers ) && $user_providers_raw ) {
if ( isset( $providers['Two_Factor_Email'] ) ) {
// Force Emailed codes to 'on'.
$enabled_providers[] = 'Two_Factor_Email';
} else {
return new WP_Error(

@kasparsd kasparsd Sep 18, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is the only part that I'm concerned about since we're changing the return type of a public helper method that previously only returned an array. Some clients might not account for that.

Maybe we could split this into two parts -- one that adds back the email factor and another one used during login flow to return the error if no fallback options are available (but were historically configured)? This would keep the function signatures while still preventing login.

@georgestephanis georgestephanis Sep 18, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I mean, if returning the error breaks something not accounting for it, I honestly kinda see that as a win, as we don't want it to silently allow stuff to go through if they had ... FIDO U2F as their only provider and it goes away, for example.

@kasparsd kasparsd Sep 18, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I honestly kinda see that as a win

I agree! I'm only suggesting that we do that during the login attempts instead of whenever somebody calls the get_available_providers_for_user(). My suggestion would be to silently add Two_Factor_Email in this method and use another helper during login to return error if they previously had a method configured but none of them are supported anymore. This approach would keep the method signature intact while still preventing login.

@TimothyBJacobs TimothyBJacobs Sep 18, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Right now we are adding the Email method is available. Are you suggesting we always add the Email method? Or that we would return an empty array in that case instead of a WP_Error?

I do lean towards returning a WP_Error and accepting this as an intentional BC break.

@georgestephanis georgestephanis Sep 18, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I rewrote this in 66b43ae but would be fine reverting that -- so either way.

We're only adding the Email method if the user previously had some method configured (by checking their usermeta key) but now no methods are available (for example, because a configured method has gone away).

The goal is to ensure only that if an existing method is removed, the account won't lose all two-factor protections without the user even realizing.

@TimothyBJacobs TimothyBJacobs Sep 18, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It does worry me that this function is now failing open, and a developer would have to know to implement the same logic as the 2FA plugin is doing to be safe.

Inspired a bit by various Core function signatures, what if we introduce a $wp_error parameter. It'd be false by default, but when true would do the necessary validation and construct a proper WP_Error object.

We could then fire a _doing_it_wrong notice if the function is called without $wp_error. That way developers would know they are doing something dangerous.

Inspired by GB's BC promise, we could then potentially switch the default to true or just make that the default value after a few versions.

@georgestephanis georgestephanis Sep 18, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'd be fine with that from what I understand of it, but I'm not positive I follow all of it. My brain may be too much in the weeds of a different problem right now. @TimothyBJacobs , do you think you may be able to do a pr to the pr to revert 66b43ae and show what you're thinking? Or just take over this branch -- I'm 100% fine with that too.

@TimothyBJacobs TimothyBJacobs Sep 18, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yep, I can push a commit this evening.

@obenland obenland Mar 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This change is causing fatal errors on wordpress.org now.

E_ERROR: Uncaught Error: Cannot use object of type WP_Error as array in wp-content/plugins/wporg-two-factor/wporg-two-factor.php:426Uncaught Error:   
Cannot use object of type WP_Error as array in wp-content/plugins/wporg-two-factor/wporg-two-factor.php:426

From https://github.com/WordPress/wporg-two-factor/blob/trunk/wporg-two-factor.php#L423-L426

There seem to other instances in this plugin where it assumes the return value to always be an array:

'no_available_2fa_methods',
__( 'Error: User has Two Factor method(s) enabled, but provider(s) no longer exist,', 'two-factor' ),
Comment thread
georgestephanis marked this conversation as resolved.
Outdated
array(
'user_providers_raw' => $user_providers_raw,
'available_providers' => array_keys( $providers ),
)
);
}
}

foreach ( $providers as $provider_key => $provider ) {
if ( in_array( $provider_key, $enabled_providers, true ) && $provider->is_available_for_user( $user ) ) {
Expand Down