Skip to content
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.

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

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
46 changes: 43 additions & 3 deletions class-two-factor-core.php
Original file line number Diff line number Diff line change
Expand Up @@ -428,7 +428,7 @@ public static function get_enabled_providers_for_user( $user = null ) {
* 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 @@ -439,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(
'no_available_2fa_methods',
__( 'Error: User has Two Factor method(s) enabled, but provider(s) no longer exist,', 'two-factor' ),
georgestephanis marked this conversation as resolved.
Show resolved Hide resolved
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 Expand Up @@ -477,7 +502,7 @@ public static function get_provider_for_user( $user = null, $preferred_provider

if ( is_string( $preferred_provider ) ) {
$providers = self::get_available_providers_for_user( $user );
if ( isset( $providers[ $preferred_provider ] ) ) {
if ( ! is_wp_error( $providers ) && isset( $providers[ $preferred_provider ] ) ) {
return $providers[ $preferred_provider ];
}
}
Expand Down Expand Up @@ -505,6 +530,9 @@ public static function get_primary_provider_for_user( $user = null ) {
// If there's only one available provider, force that to be the primary.
if ( empty( $available_providers ) ) {
return null;
} elseif ( is_wp_error( $available_providers ) ) {
// If it returned an error, the configured methods don't exist, and it couldn't swap in a replacement.
wp_die( $available_providers );
dd32 marked this conversation as resolved.
Show resolved Hide resolved
} elseif ( 1 === count( $available_providers ) ) {
$provider = key( $available_providers );
} else {
Expand Down Expand Up @@ -783,6 +811,11 @@ public static function login_html( $user, $login_nonce, $redirect_to, $error_msg

$rememberme = intval( self::rememberme() );

if ( is_wp_error( $available_providers ) ) {
// If it returned an error, the configured methods don't exist, and it couldn't swap in a replacement.
wp_die( $available_providers );
}

if ( ! function_exists( 'login_header' ) ) {
// We really should migrate login_header() out of `wp-login.php` so it can be called from an includes file.
include_once TWO_FACTOR_DIR . 'includes/function.login-header.php';
Expand Down Expand Up @@ -1696,7 +1729,14 @@ public static function manage_users_custom_column( $output, $column_name, $user_
public static function user_two_factor_options( $user ) {
wp_enqueue_style( 'user-edit-2fa', plugins_url( 'user-edit.css', __FILE__ ), array(), TWO_FACTOR_VERSION );

$enabled_providers = array_keys( self::get_available_providers_for_user( $user ) );
$available_providers = self::get_available_providers_for_user( $user );

if ( is_wp_error( $available_providers ) ) {
// If it returned an error, the configured methods don't exist, and it couldn't swap in a replacement.
wp_die( $available_providers );
}

$enabled_providers = array_keys( $available_providers );
$primary_provider = self::get_primary_provider_for_user( $user->ID );

if ( ! empty( $primary_provider ) && is_object( $primary_provider ) ) {
Expand Down
46 changes: 46 additions & 0 deletions tests/class-two-factor-core.php
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,52 @@ public function test_get_available_providers_for_user_logged_in() {
wp_set_current_user( $old_user_id );
}

/**
* Verify that if a user has a non-existent provider set, that it
* swaps to email instead, rather than treating the user as having
* no methods enabled.
*/
public function test_deprecated_provider_for_user() {
$user = $this->get_dummy_user();

// Set the dummy user with a non-existent provider.
update_user_meta(
$user->ID,
Two_Factor_Core::ENABLED_PROVIDERS_USER_META_KEY,
array(
'Two_Factor_Deprecated',
)
);

// This should fail back to `Two_Factor_Email` then.
$this->assertEquals(
array(
'Two_Factor_Email',
),
array_keys( Two_Factor_Core::get_available_providers_for_user( $user ) )
);

// Set the dummy user with a non-existent provider and a valid one.
update_user_meta(
$user->ID,
Two_Factor_Core::ENABLED_PROVIDERS_USER_META_KEY,
array(
'Two_Factor_Deprecated',
'Two_Factor_Dummy',
)
);

// This time it should just strip out the invalid one, and not inject a new one.
$this->assertEquals(
array(
'Two_Factor_Dummy',
),
array_keys( Two_Factor_Core::get_available_providers_for_user( $user ) )
);

$this->clean_dummy_user();
}

/**
* Verify primary provider for not-logged-in user.
*
Expand Down