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

georgestephanis · 2023-08-24T14:38:05Z

This also ensures if a user only had U2F enabled, and it's deprecated and removed, that it won't "fail open" for lack of any available methods.

If Email is available, shove it in. If not, return an error.

This also ensures if a user only had U2F enabled, and it's deprecated and removed, that it won't "fail open" for lack of any available methods. If Email is available, shove it in. If not, return an error.

TimothyBJacobs · 2023-08-24T14:44:42Z

class-two-factor-core.php

 			$enabled_providers = array();
 		}
-		$enabled_providers = array_intersect( $enabled_providers, array_keys( $providers ) );
+		$enabled_existing_providers = array_intersect( $enabled_providers, array_keys( $providers ) );


Suggested change

$enabled_existing_providers = array_intersect( $enabled_providers, array_keys( $providers ) );

$enabled_registered_providers = array_intersect( $enabled_providers, array_keys( $providers ) );

Would this be more clear that we are talking about enabled providers that are registered?

georgestephanis · 2023-08-24T15:09:21Z

Possible chance with bad input this changes some test results -- I need to add a test for the use case anyway.

The return value uses the class as keys, and the object as the value.

TOTP being unconfigured stripped it out otherwise.

class-two-factor-core.php

Co-authored-by: Timothy Jacobs <timothy@ironbounddesigns.com>

georgestephanis · 2023-08-24T18:02:43Z

Once this is in, purging u2f from the plugin won't leave users who only had that enabled wide open with no two-factor protection.

iandunn

Didn't test manually, but the code LGTM 👍🏻

class-two-factor-core.php

georgestephanis · 2025-09-17T19:40:13Z

@kasparsd Any objection to getting this merged so we can finally yeet FIDO U2F?

#439

kasparsd

Looks good. Had one suggestion regarding the flow of this and potential edge case with changing the return types.

kasparsd · 2025-09-18T07:02:06Z

class-two-factor-core.php

+				// Force Emailed codes to 'on'.
+				$enabled_providers[] = 'Two_Factor_Email';
+			} else {
+				return new WP_Error(


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.

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.

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.

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.

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.

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.

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.

Yep, I can push a commit this evening.

georgestephanis · 2025-09-18T13:02:58Z

Also, just confirming that I removed this bit in the merge --

9ad6dd6#diff-ee04f1d323104504c6bfa38dd11ef43e78d1dbac2883293af0b5c310d16e9519L1724-R1739

In retrospect, it didn't seem as important; if someone is updating their providers, they don't need to have a hard fail because an old one isn't there anymore.

Props Kaspars for the suggestion to relocate to keep the function signatures. Co-Authored-By: Kaspars Dambis <hi@kaspars.net>

georgestephanis · 2025-09-18T15:35:58Z

Also the latest rewrite in 66b43ae shouldn't impact the unit tests for this -- as those are only testing to make sure email is added, and not any circumstance where email isn't available as a provider (which is the only case that rewrite should impact)

kasparsd

This looks great to me!

class-two-factor-core.php

TimothyBJacobs · 2025-09-19T16:44:03Z

This is what I was thinking.

Patch

diff --git a/class-two-factor-core.php b/class-two-factor-core.php
index 99bb69d..d4897d4 100644
--- a/class-two-factor-core.php
+++ b/class-two-factor-core.php
@@ -543,13 +543,22 @@ class Two_Factor_Core {
 	 * Get all two-factor providers that are both enabled and configured
 	 * for the specified (or current) user.
 	 *
-	 * @see Two_Factor_Core::get_supported_providers_for_user()
+	 * @since 0.15 Added $wp_error parameter.
+	 *
+	 * @param int|WP_User $user     Optional. User ID, or WP_User object of the user. Defaults to the current user.
+	 * @param bool        $wp_error Set to true to return a WP_Error object when there are issues determining
+	 *                              the available providers.
+	 *
+	 * @return array|WP_Error List of provider instances.
 	 * @see Two_Factor_Core::get_enabled_providers_for_user()
 	 *
-	 * @param int|WP_User $user Optional. User ID, or WP_User object of the the user. Defaults to current user.
-	 * @return array List of provider instances.
+	 * @see Two_Factor_Core::get_supported_providers_for_user()
 	 */
-	public static function get_available_providers_for_user( $user = null ) {
+	public static function get_available_providers_for_user( $user = null, bool $wp_error = false ) {
+		if ( true !== $wp_error ) {
+			_doing_it_wrong( __METHOD__, __( 'The $wp_error should be passed as true to evaluate errors correctly.', 'LION' ), '0.15' );
+		}
+
 		$user = self::fetch_user( $user );
 		if ( ! $user ) {
 			return array();
@@ -565,18 +574,37 @@ class Two_Factor_Core {
 		 * 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 {
-				// Email isn't available?  Okay, then you get nothing.  Downstream should verify
-				// there aren't any that have gone away before allowing authentication when trying
-				// to login.
-				return $configured_providers;
+				$error = new WP_Error(
+					'no_available_2fa_methods',
+					__( 'Error: You have Two Factor method(s) enabled, but the provider(s) no longer exist. Please contact a site administrator for assistance.', 'two-factor' ),
+					array(
+						'user_providers_raw' => $user_providers_raw,
+					)
+				);
+
+				/**
+				 * Filters the error condition when a user has enabled 2FA methods, but the providers no longer exist.
+				 *
+				 * @param WP_Error|array $error   A WP_Error object or a list of provider keys that should be offered instead.
+				 * @param int            $user_id The user ID.
+				 */
+				$error = apply_filters( 'two_factor_error_no_available_methods', $error, $user->ID );
+
+				if ( is_wp_error( $error ) ) {
+					if ( $wp_error ) {
+						return $error;
+					} else {
+						return array();
+					}
+				} elseif ( is_array( $error ) ) {
+					$enabled_providers = $error;
+				}
 			}
 		}
 
@@ -950,36 +978,14 @@ class Two_Factor_Core {
 		}
 
 		$provider_key        = $provider->get_key();
-		$available_providers = self::get_available_providers_for_user( $user );
+		$available_providers = self::get_available_providers_for_user( $user, true );
 		$backup_providers    = array_diff_key( $available_providers, array( $provider_key => null ) );
 		$interim_login       = isset( $_REQUEST['interim-login'] ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended
 
 		$rememberme = intval( self::rememberme() );
 
-		if ( empty( $available_providers ) ) {
-			$user_providers_raw = get_user_meta( $user->ID, self::ENABLED_PROVIDERS_USER_META_KEY, true );
-			if ( $user_providers_raw ) {
-				// The user has a provider configured according to usermeta, but none are currently available.
-				$error = new WP_Error(
-					'no_available_2fa_methods',
-					__( 'Error: You have Two Factor method(s) enabled, but the provider(s) no longer exist. Please contact a site administrator for assistance.', 'two-factor' ),
-					array(
-						'user_providers_raw'  => $user_providers_raw,
-						'available_providers' => array_keys( $available_providers ),
-					)
-				);
-
-				/**
-				 * Provide an opportunity for other code to change this fail-closed behavior, or listen for its events.
-				 *
-				 * @param WP_Error|mixed $error The WP_Error indicating there are no available 2FA Methods. To allow anyways, just return anything else.
-				 */
-				$error = apply_filters( 'two_factor_error_no_available_methods', $error );
-
-				if ( is_wp_error( $error ) ) {
-					wp_die( $error );
-				}
-			}
+		if ( is_wp_error( $available_providers ) ) {
+			wp_die( $available_providers );
 		}
 
 		if ( ! function_exists( 'login_header' ) ) {

Thinking about it more, though, I really think we should just go with the BC break. It's not just this function that would need to be udpated. Basically everything downstream of it needs to be updated as well. I think always returning an empty array is a non-starter, because it makes the usages of the APIs unsafe or even incorrect in some spots. We could thread a $wp_error parameter through everything, though it starts to become messy.

For instance get_primary_provider_key_selected_for_user should that return a WP_Error or nothing? Or the key of the key of the now no longer available provider?

The answer for each may well be different. is_user_using_two_factor should probably always return true.

I think we probably need to look at each of the API methods and determine how they should behave in this circumstance.

georgestephanis · 2025-09-22T17:03:54Z

@kasparsd Do you have a strong opinion here at this point? I could go either way tbh.

kasparsd · 2025-09-22T17:34:53Z

I really think we should just go with the BC break.

Considering the amount of additional flags/args we would need to pass to capture this special user state, I agree that failing quickly and early is the best way to go.

There are probably only a few user that (1) are currently using only FIDO keys (2) and don't have email allowed.

So having it return an error on get_available_providers_for_user() in that case works fine for me.

TimothyBJacobs · 2025-09-23T18:12:56Z

👍 Cool. I can expand on my patch this weekend.

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

c79bbcc

This also ensures if a user only had U2F enabled, and it's deprecated and removed, that it won't "fail open" for lack of any available methods. If Email is available, shove it in. If not, return an error.

georgestephanis requested a review from TimothyBJacobs August 24, 2023 14:38

TimothyBJacobs reviewed Aug 24, 2023

View reviewed changes

georgestephanis added 2 commits August 24, 2023 11:06

Do the fallback slightly downstream.

bd676f9

Handle the WP_Error fail cases

9ad6dd6

georgestephanis added 3 commits August 24, 2023 11:19

Add a unit test for this.

c9aa7ac

Ensure we're comparing the keys.

a745c3c

The return value uses the class as keys, and the object as the value.

Use a class that won't get stripped out.

f8c780d

TOTP being unconfigured stripped it out otherwise.

TimothyBJacobs reviewed Aug 24, 2023

View reviewed changes

class-two-factor-core.php Outdated Show resolved Hide resolved

Update the error messages for understandability.

2e616ea

Co-authored-by: Timothy Jacobs <timothy@ironbounddesigns.com>

jeffpaul added this to the 0.9.0 milestone Aug 24, 2023

This was referenced Aug 28, 2023

Handle WP_Error result from get_available_providers_for_user WordPress/wporg-two-factor#231

Open

Notify user/admin when sensitive events occur #476

Open

iandunn approved these changes Aug 28, 2023

View reviewed changes

class-two-factor-core.php Outdated Show resolved Hide resolved

jeffpaul modified the milestones: 0.9.0, 0.10.0 May 8, 2024

Merge branch 'master' into add/fallback-email

947afdd

jeffpaul modified the milestones: 0.14.0, 0.15.0 Jul 3, 2025

georgestephanis added 2 commits September 17, 2025 15:35

Merge branch 'master' into add/fallback-email

d12c89a

Whoops, reintroduced in merge.

1f0035b

kasparsd approved these changes Sep 18, 2025

View reviewed changes

Tidy up and run it further downstream.

66b43ae

Props Kaspars for the suggestion to relocate to keep the function signatures. Co-Authored-By: Kaspars Dambis <hi@kaspars.net>

kasparsd approved these changes Sep 18, 2025

View reviewed changes

class-two-factor-core.php Show resolved Hide resolved

jeffpaul modified the milestones: 0.14.2, 0.15.0 Dec 11, 2025

	$enabled_existing_providers = array_intersect( $enabled_providers, array_keys( $providers ) );
	$enabled_registered_providers = array_intersect( $enabled_providers, array_keys( $providers ) );

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

georgestephanis commented Aug 24, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

georgestephanis commented Aug 24, 2023

Uh oh!

Uh oh!

georgestephanis commented Aug 24, 2023

Uh oh!

iandunn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

georgestephanis commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kasparsd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

georgestephanis commented Sep 18, 2025

Uh oh!

georgestephanis commented Sep 18, 2025

Uh oh!

kasparsd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TimothyBJacobs commented Sep 19, 2025

Uh oh!

georgestephanis commented Sep 22, 2025

Uh oh!

kasparsd commented Sep 22, 2025

Uh oh!

TimothyBJacobs commented Sep 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

georgestephanis commented Sep 17, 2025 •

edited

Loading