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.

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

Enable TOTP method when method is configured #643

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
+44 −43
Open

Enable TOTP method when method is configured #643

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
253a823
Introduce a matching disable provider for user method
kasparsd Sep 19, 2024
7f05a5d
Account for primary being disabled
kasparsd Sep 19, 2024
4031d4d
Enable by default
kasparsd Sep 19, 2024
edf24d2
Disable on deactivate
kasparsd Sep 19, 2024
d86f234
Describe tests to quickly identify failing tests
kasparsd Sep 19, 2024
64c0fce
Maintain the previous logic
kasparsd Sep 19, 2024
faf80cb
Don’t deal with setting the primary
kasparsd Sep 19, 2024
a8a6c4a
Toggle the checkbox outside of the control panel
kasparsd Sep 19, 2024
a5b2214
get_primary_provider_for_user() already ensures that primary is alway…
kasparsd Sep 19, 2024
ba5750d
Remove debug logic
kasparsd Sep 19, 2024
c95bc1d
Merge branch 'master' into 556-enable-totp-on-submit
kasparsd Dec 2, 2024
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
48 changes: 23 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 @@ -1793,9 +1793,13 @@ public static function user_two_factor_options( $user ) {
$enabled_providers = array_keys( self::get_available_providers_for_user( $user ) );
$primary_provider = self::get_primary_provider_for_user( $user->ID );

if ( ! empty( $primary_provider ) && is_object( $primary_provider ) ) {
$primary_provider_key = null;
if ( is_object( $primary_provider ) ) {
$primary_provider_key = $primary_provider->get_key();
} else {
}

// Reset the primary if it isn't set to one of the enabled providers.
if ( ! in_array( $primary_provider_key, $enabled_providers ) ) {
$primary_provider_key = null;
}

Expand Down Expand Up @@ -1889,30 +1893,20 @@ public static function user_two_factor_options( $user ) {
* @return bool True if the provider was enabled, false otherwise.
*/
public static function enable_provider_for_user( $user_id, $new_provider ) {
$available_providers = self::get_providers();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This method is very heavy as it will instantiate new set of provider classes on every call. That's why we introduced get_provider_classes() earlier which doesn't do any of that.


if ( ! array_key_exists( $new_provider, $available_providers ) ) {
if ( ! in_array( $new_provider, self::get_providers_classes(), true ) ) {
return false;
}

$user = get_userdata( $user_id );
$enabled_providers = self::get_enabled_providers_for_user( $user );
$enabled_providers = self::get_enabled_providers_for_user( $user_id );

// Check if this is enabled already.
if ( in_array( $new_provider, $enabled_providers ) ) {
return true;
}

$enabled_providers[] = $new_provider;
$enabled = update_user_meta( $user_id, self::ENABLED_PROVIDERS_USER_META_KEY, $enabled_providers );

// Primary provider must be enabled.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The primary provider is determined during get_primary_provider_for_user() so there is no need update it here. If no primary is configured, we always default to the first enabled.

$has_primary = is_object( self::get_primary_provider_for_user( $user_id ) );

if ( ! $has_primary ) {
$has_primary = update_user_meta( $user_id, self::PROVIDER_USER_META_KEY, $new_provider );
}

return $enabled && $has_primary;
return (bool) update_user_meta( $user_id, self::ENABLED_PROVIDERS_USER_META_KEY, $enabled_providers );
}

/**
Expand All @@ -1929,23 +1923,27 @@ public static function enable_provider_for_user( $user_id, $new_provider ) {
* @return bool True if the provider was disabled, false otherwise.
*/
public static function disable_provider_for_user( $user_id, $provider_to_delete ) {
$is_registered = array_key_exists( $provider_to_delete, self::get_providers() );

if ( ! $is_registered ) {
// Check if the provider is even enabled.
if ( ! in_array( $provider_to_delete, self::get_providers_classes(), true ) ) {
return false;
}

$old_enabled_providers = self::get_enabled_providers_for_user( $user_id );
$is_enabled = in_array( $provider_to_delete, $old_enabled_providers );
$enabled_providers = self::get_enabled_providers_for_user( $user_id );

if ( ! $is_enabled ) {
// Check if this is disabled already.
if ( ! in_array( $provider_to_delete, $enabled_providers ) ) {
return true;
}

$new_enabled_providers = array_diff( $old_enabled_providers, array( $provider_to_delete ) );
$was_disabled = update_user_meta( $user_id, self::ENABLED_PROVIDERS_USER_META_KEY, $new_enabled_providers );
$enabled_providers = array_diff( $enabled_providers, array( $provider_to_delete ) );

// Remove this from being a primary provider, if set.
$primary_provider = self::get_primary_provider_for_user( $user_id );
if ( $primary_provider && $primary_provider->get_key() === $provider_to_delete ) {
delete_user_meta( $user_id, self::PROVIDER_USER_META_KEY );
}

return (bool) $was_disabled;
return (bool) update_user_meta( $user_id, self::ENABLED_PROVIDERS_USER_META_KEY, $enabled_providers );
}

/**
Expand Down
8 changes: 8 additions & 0 deletions providers/class-two-factor-totp.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,10 @@ public function rest_delete_totp( $request ) {

$this->delete_user_totp_key( $user_id );

if ( ! Two_Factor_Core::disable_provider_for_user( $user_id, 'Two_Factor_Totp' ) ) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Deleting the TOTP key now also disables the method because it is unusable without the key.

return new WP_Error( 'db_error', __( 'Unable to disable TOTP provider for this user.', 'two-factor' ), array( 'status' => 500 ) );
}

ob_start();
$this->user_two_factor_options( $user );
$html = ob_get_clean();
Expand Down Expand Up @@ -368,6 +372,7 @@ public function user_two_factor_options( $user ) {
user_id: <?php echo wp_json_encode( $user->ID ); ?>,
key: key,
code: code,
enable_provider: true,
}
} ).fail( function( response, status ) {
var errorMessage = response.responseJSON.message || status,
Expand All @@ -379,8 +384,10 @@ public function user_two_factor_options( $user ) {

$error.find('p').text( errorMessage );

$( '#enabled-Two_Factor_Totp' ).prop( 'checked', false );
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We now enable the TOTP method when the key has been registered by default.

$('#two-factor-totp-authcode').val('');
} ).then( function( response ) {
$( '#enabled-Two_Factor_Totp' ).prop( 'checked', true );
$( '#two-factor-totp-options' ).html( response.html );
} );
} );
Expand All @@ -407,6 +414,7 @@ public function user_two_factor_options( $user ) {
user_id: <?php echo wp_json_encode( $user->ID ); ?>,
}
} ).then( function( response ) {
$( '#enabled-Two_Factor_Totp' ).prop( 'checked', false );
$( '#two-factor-totp-options' ).html( response.html );
} );
} );
Expand Down
36 changes: 18 additions & 18 deletions tests/class-two-factor-core.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -818,52 +818,52 @@ public function test_show_password_reset_error() {
public function test_enable_disable_provider_for_user() {
$user = self::factory()->user->create_and_get();
$enabled_providers = Two_Factor_Core::get_enabled_providers_for_user( $user->ID );
$this->assertEmpty( $enabled_providers );
$this->assertEmpty( $enabled_providers, 'No providers are enabled by default' );
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These help us identify failing tests instead of just showing the test method name.


// Disabling one that's already disabled should succeed.
$totp_disabled = Two_Factor_Core::disable_provider_for_user( $user->ID, 'Two_Factor_Totp' );
$this->assertTrue( $totp_disabled );
$this->assertTrue( $totp_disabled, 'Disabling something that wasn\'t enabled should succeed' );

// Disabling one that doesn't exist should fail.
$nonexistent_enabled = Two_Factor_Core::enable_provider_for_user( $user->ID, 'Nonexistent_Provider' );
$enabled_providers = Two_Factor_Core::get_enabled_providers_for_user( $user->ID );
$this->assertFalse( $nonexistent_enabled );
$this->assertEmpty( $enabled_providers );
$this->assertNull( Two_Factor_Core::get_primary_provider_for_user( $user->ID ) );
$this->assertFalse( $nonexistent_enabled, 'Nonexistent shouldn\'t be allowed to be enabled' );
$this->assertEmpty( $enabled_providers, 'Nonexistent wasn\'t enabled' );
$this->assertNull( Two_Factor_Core::get_primary_provider_for_user( $user->ID ), 'Nonexistent wasn\'t set as primary' );

// Enabling a valid one should succeed. The first one that's enabled and configured should be the default primary.
$totp = Two_Factor_Totp::get_instance();
$totp->set_user_totp_key( $user->ID, 'foo' );
$totp_enabled = Two_Factor_Core::enable_provider_for_user( $user->ID, 'Two_Factor_Totp' );
$enabled_providers = Two_Factor_Core::get_enabled_providers_for_user( $user->ID );
$this->assertTrue( $totp_enabled );
$this->assertSame( array( 'Two_Factor_Totp' ), $enabled_providers );
$this->assertSame( 'Two_Factor_Totp', Two_Factor_Core::get_primary_provider_for_user( $user->ID )->get_key() );
$this->assertTrue( $totp_enabled, 'Can enable a valid provider' );
$this->assertSame( array( 'Two_Factor_Totp' ), $enabled_providers, 'Enabled provider is now listed as enabled' );
$this->assertSame( 'Two_Factor_Totp', Two_Factor_Core::get_primary_provider_for_user( $user->ID )->get_key(), 'Primary is now the only enabled provider' );

// Enabling one that's already enabled should succeed.
$totp_enabled = Two_Factor_Core::enable_provider_for_user( $user->ID, 'Two_Factor_Totp' );
$this->assertTrue( $totp_enabled );
$this->assertTrue( $totp_enabled, 'Can enable a provider that is already enabled' );

// Enabling another should succeed, and not change the primary.
$dummy_enabled = Two_Factor_Core::enable_provider_for_user( $user->ID, 'Two_Factor_Dummy' );
$enabled_providers = Two_Factor_Core::get_enabled_providers_for_user( $user->ID );
$this->assertTrue( $dummy_enabled );
$this->assertSame( array( 'Two_Factor_Totp', 'Two_Factor_Dummy' ), $enabled_providers );
$this->assertSame( 'Two_Factor_Totp', Two_Factor_Core::get_primary_provider_for_user( $user->ID )->get_key() );
$this->assertTrue( $dummy_enabled, 'Can enable valid provider' );
$this->assertSame( array( 'Two_Factor_Totp', 'Two_Factor_Dummy' ), $enabled_providers, 'Multiple can be enabled at the same time' );
$this->assertSame( 'Two_Factor_Totp', Two_Factor_Core::get_primary_provider_for_user( $user->ID )->get_key(), 'The primary not changed upon additional providers being enabled' );

// Disabling one that doesn't exist should fail.
$nonexistent_disabled = Two_Factor_Core::disable_provider_for_user( $user->ID, 'Nonexistent_Provider' );
$enabled_providers = Two_Factor_Core::get_enabled_providers_for_user( $user->ID );
$this->assertFalse( $nonexistent_disabled );
$this->assertSame( array( 'Two_Factor_Totp', 'Two_Factor_Dummy' ), $enabled_providers );
$this->assertSame( 'Two_Factor_Totp', Two_Factor_Core::get_primary_provider_for_user( $user->ID )->get_key() );
$this->assertFalse( $nonexistent_disabled, 'Unavailable provider can\'t be disabled' );
$this->assertSame( array( 'Two_Factor_Totp', 'Two_Factor_Dummy' ), $enabled_providers, 'Unavailable wasn\'t added to the list of enabled proviers' );
$this->assertSame( 'Two_Factor_Totp', Two_Factor_Core::get_primary_provider_for_user( $user->ID )->get_key(), 'The primary is still the same after unavailable disable attempt' );

// Disabling one that's enabled should succeed, and change the primary to the next available one.
$totp_disabled = Two_Factor_Core::disable_provider_for_user( $user->ID, 'Two_Factor_Totp' );
$enabled_providers = Two_Factor_Core::get_enabled_providers_for_user( $user->ID );
$this->assertTrue( $totp_disabled ); //todo enable and fix
$this->assertSame( array( 1 => 'Two_Factor_Dummy' ), $enabled_providers );
$this->assertSame( 'Two_Factor_Dummy', Two_Factor_Core::get_primary_provider_for_user( $user->ID )->get_key() );
$this->assertTrue( $totp_disabled, 'Can disable a provider that is enabled' );
$this->assertSame( array( 1 => 'Two_Factor_Dummy' ), $enabled_providers, 'The other providers are kept enabled' );
$this->assertSame( 'Two_Factor_Dummy', Two_Factor_Core::get_primary_provider_for_user( $user->ID )->get_key(), 'Primary is updated to the first available' );
}

/**
Expand Down