From 253a823c9aa029794260bb4d211c90a9d49b1828 Mon Sep 17 00:00:00 2001 From: Kaspars Dambis Date: Thu, 19 Sep 2024 14:20:00 +0300 Subject: [PATCH 01/13] Introduce a matching disable provider for user method --- class-two-factor-core.php | 47 ++++++++++++++------------------------- 1 file changed, 17 insertions(+), 30 deletions(-) diff --git a/class-two-factor-core.php b/class-two-factor-core.php index 99033a70..cf5945e8 100644 --- a/class-two-factor-core.php +++ b/class-two-factor-core.php @@ -1889,30 +1889,18 @@ 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(); + $enabled_providers = self::get_enabled_providers_for_user( $user_id ); - if ( ! array_key_exists( $new_provider, $available_providers ) ) { - return false; + if ( ! in_array( $new_provider, $enabled_providers ) ) { + $enabled_providers[] = $new_provider; } - $user = get_userdata( $user_id ); - $enabled_providers = self::get_enabled_providers_for_user( $user ); - - if ( in_array( $new_provider, $enabled_providers ) ) { - return true; + // Add as primary if none set. + if ( ! self::get_primary_provider_for_user( $user_id ) ) { + update_user_meta( $user_id, self::PROVIDER_USER_META_KEY, $new_provider ); } - $enabled_providers[] = $new_provider; - $enabled = update_user_meta( $user_id, self::ENABLED_PROVIDERS_USER_META_KEY, $enabled_providers ); - - // Primary provider must be 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 ); } /** @@ -1929,23 +1917,22 @@ 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() ); + $enabled_providers = self::get_enabled_providers_for_user( $user_id ); - if ( ! $is_registered ) { - return false; + // Check if this is disabled already. + if ( ! in_array( $provider_to_delete, $enabled_providers ) ) { + return true; } - $old_enabled_providers = self::get_enabled_providers_for_user( $user_id ); - $is_enabled = in_array( $provider_to_delete, $old_enabled_providers ); + $enabled_providers = array_diff( $enabled_providers, array( $provider_to_delete ) ); - if ( ! $is_enabled ) { - return true; + // 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 ); } - $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 ); - - return (bool) $was_disabled; + return (bool) update_user_meta( $user_id, self::ENABLED_PROVIDERS_USER_META_KEY, $enabled_providers ); } /** From 7f05a5dab7669456d8bc247326d4da6a051086e4 Mon Sep 17 00:00:00 2001 From: Kaspars Dambis Date: Thu, 19 Sep 2024 14:20:18 +0300 Subject: [PATCH 02/13] Account for primary being disabled --- class-two-factor-core.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/class-two-factor-core.php b/class-two-factor-core.php index cf5945e8..8ec4aaf0 100644 --- a/class-two-factor-core.php +++ b/class-two-factor-core.php @@ -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; } From 4031d4d9d9162d764fc252e0680b7584e99a911e Mon Sep 17 00:00:00 2001 From: Kaspars Dambis Date: Thu, 19 Sep 2024 14:20:38 +0300 Subject: [PATCH 03/13] Enable by default --- providers/class-two-factor-totp.php | 1 + 1 file changed, 1 insertion(+) diff --git a/providers/class-two-factor-totp.php b/providers/class-two-factor-totp.php index 7bafe1a1..4229e82d 100644 --- a/providers/class-two-factor-totp.php +++ b/providers/class-two-factor-totp.php @@ -368,6 +368,7 @@ public function user_two_factor_options( $user ) { user_id: ID ); ?>, key: key, code: code, + enable_provider: true, } } ).fail( function( response, status ) { var errorMessage = response.responseJSON.message || status, From edf24d2d4eb417e6ed3383b70a781f43e5022350 Mon Sep 17 00:00:00 2001 From: Kaspars Dambis Date: Thu, 19 Sep 2024 14:20:48 +0300 Subject: [PATCH 04/13] Disable on deactivate --- providers/class-two-factor-totp.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/providers/class-two-factor-totp.php b/providers/class-two-factor-totp.php index 4229e82d..99786bd3 100644 --- a/providers/class-two-factor-totp.php +++ b/providers/class-two-factor-totp.php @@ -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' ) ) { + 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(); From d86f234a3aceb7998ce07daf524622717d2836f9 Mon Sep 17 00:00:00 2001 From: Kaspars Dambis Date: Thu, 19 Sep 2024 14:50:00 +0300 Subject: [PATCH 05/13] Describe tests to quickly identify failing tests --- tests/class-two-factor-core.php | 36 ++++++++++++++++----------------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/tests/class-two-factor-core.php b/tests/class-two-factor-core.php index 5f6a15a2..c1d06b72 100644 --- a/tests/class-two-factor-core.php +++ b/tests/class-two-factor-core.php @@ -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' ); // 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' ); } /** From 64c0fce35c4659d8c769a4126ed2a7250d9f2fca Mon Sep 17 00:00:00 2001 From: Kaspars Dambis Date: Thu, 19 Sep 2024 14:50:11 +0300 Subject: [PATCH 06/13] Maintain the previous logic --- class-two-factor-core.php | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/class-two-factor-core.php b/class-two-factor-core.php index 8ec4aaf0..398bb25f 100644 --- a/class-two-factor-core.php +++ b/class-two-factor-core.php @@ -1893,12 +1893,19 @@ 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 ) { + if ( ! in_array( $new_provider, self::get_providers_classes(), true ) ) { + return false; + } + $enabled_providers = self::get_enabled_providers_for_user( $user_id ); - if ( ! in_array( $new_provider, $enabled_providers ) ) { - $enabled_providers[] = $new_provider; + // Check if this is enabled already. + if ( in_array( $new_provider, $enabled_providers ) ) { + return true; } + $enabled_providers[] = $new_provider; + // Add as primary if none set. if ( ! self::get_primary_provider_for_user( $user_id ) ) { update_user_meta( $user_id, self::PROVIDER_USER_META_KEY, $new_provider ); @@ -1921,6 +1928,11 @@ 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 ) { + // Check if the provider is even enabled. + if ( ! in_array( $provider_to_delete, self::get_providers_classes(), true ) ) { + return false; + } + $enabled_providers = self::get_enabled_providers_for_user( $user_id ); // Check if this is disabled already. From faf80cb132fbeb9ed10cdc31e9a86018ceb9dba0 Mon Sep 17 00:00:00 2001 From: Kaspars Dambis Date: Thu, 19 Sep 2024 14:53:48 +0300 Subject: [PATCH 07/13] =?UTF-8?q?Don=E2=80=99t=20deal=20with=20setting=20t?= =?UTF-8?q?he=20primary?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- class-two-factor-core.php | 5 ----- 1 file changed, 5 deletions(-) diff --git a/class-two-factor-core.php b/class-two-factor-core.php index 398bb25f..eb5b2149 100644 --- a/class-two-factor-core.php +++ b/class-two-factor-core.php @@ -1906,11 +1906,6 @@ public static function enable_provider_for_user( $user_id, $new_provider ) { $enabled_providers[] = $new_provider; - // Add as primary if none set. - if ( ! self::get_primary_provider_for_user( $user_id ) ) { - update_user_meta( $user_id, self::PROVIDER_USER_META_KEY, $new_provider ); - } - return (bool) update_user_meta( $user_id, self::ENABLED_PROVIDERS_USER_META_KEY, $enabled_providers ); } From a8a6c4aabbb169d0a716410f7bca5bfda022d11f Mon Sep 17 00:00:00 2001 From: Kaspars Dambis Date: Thu, 19 Sep 2024 15:17:37 +0300 Subject: [PATCH 08/13] Toggle the checkbox outside of the control panel --- providers/class-two-factor-totp.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/providers/class-two-factor-totp.php b/providers/class-two-factor-totp.php index 99786bd3..74f0f2e6 100644 --- a/providers/class-two-factor-totp.php +++ b/providers/class-two-factor-totp.php @@ -384,8 +384,10 @@ public function user_two_factor_options( $user ) { $error.find('p').text( errorMessage ); + $( '#enabled-Two_Factor_Totp' ).prop( 'checked', false ); $('#two-factor-totp-authcode').val(''); } ).then( function( response ) { + $( '#enabled-Two_Factor_Totp' ).prop( 'checked', true ); $( '#two-factor-totp-options' ).html( response.html ); } ); } ); @@ -412,6 +414,7 @@ public function user_two_factor_options( $user ) { user_id: ID ); ?>, } } ).then( function( response ) { + $( '#enabled-Two_Factor_Totp' ).prop( 'checked', false ); $( '#two-factor-totp-options' ).html( response.html ); } ); } ); From a5b22146ed56108f18b44d5c58cc4fd19690b404 Mon Sep 17 00:00:00 2001 From: Kaspars Dambis Date: Thu, 19 Sep 2024 15:26:35 +0300 Subject: [PATCH 09/13] get_primary_provider_for_user() already ensures that primary is always one of the enabled options --- class-two-factor-core.php | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/class-two-factor-core.php b/class-two-factor-core.php index eb5b2149..9f143e35 100644 --- a/class-two-factor-core.php +++ b/class-two-factor-core.php @@ -1246,6 +1246,8 @@ public static function current_user_can_update_two_factor_options( $context = 'd return false; } + return true; + // If the current user is not using two-factor, they can adjust the settings. if ( ! self::is_user_using_two_factor( $user_id ) ) { return true; @@ -1794,15 +1796,10 @@ public static function user_two_factor_options( $user ) { $primary_provider = self::get_primary_provider_for_user( $user->ID ); $primary_provider_key = null; - if ( is_object( $primary_provider ) ) { + if ( ! empty( $primary_provider ) && is_object( $primary_provider ) ) { $primary_provider_key = $primary_provider->get_key(); } - // 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; - } - // This is specific to the current session, not the displayed user. $show_2fa_options = self::current_user_can_update_two_factor_options(); From ba5750d31b68a6a3e66cc051fb509d62961862c2 Mon Sep 17 00:00:00 2001 From: Kaspars Dambis Date: Thu, 19 Sep 2024 15:27:06 +0300 Subject: [PATCH 10/13] Remove debug logic --- class-two-factor-core.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/class-two-factor-core.php b/class-two-factor-core.php index 9f143e35..bf37eaef 100644 --- a/class-two-factor-core.php +++ b/class-two-factor-core.php @@ -1246,8 +1246,6 @@ public static function current_user_can_update_two_factor_options( $context = 'd return false; } - return true; - // If the current user is not using two-factor, they can adjust the settings. if ( ! self::is_user_using_two_factor( $user_id ) ) { return true; From 2a39f2aa2321a626ca348a00d40a110097e5274b Mon Sep 17 00:00:00 2001 From: Kaspars Dambis Date: Thu, 9 Jan 2025 13:22:39 +0200 Subject: [PATCH 11/13] Describe the output format --- class-two-factor-core.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/class-two-factor-core.php b/class-two-factor-core.php index cad24e48..dbe264c6 100644 --- a/class-two-factor-core.php +++ b/class-two-factor-core.php @@ -249,7 +249,8 @@ private static function get_providers_classes( $providers ) { } /** - * Get all enabled two-factor providers. + * Get all enabled two-factor providers with keys as the original + * provider class names and the values as the provider class instances. * * @since 0.1-dev * From 03c2cab9a3c07e9659b81957f6d44a7781d2014f Mon Sep 17 00:00:00 2001 From: Kaspars Dambis Date: Thu, 9 Jan 2025 13:22:56 +0200 Subject: [PATCH 12/13] Switch back to providers list due to API changes --- class-two-factor-core.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/class-two-factor-core.php b/class-two-factor-core.php index dbe264c6..64ebed8f 100644 --- a/class-two-factor-core.php +++ b/class-two-factor-core.php @@ -1896,7 +1896,10 @@ 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 ) { - if ( ! in_array( $new_provider, self::get_providers_classes(), true ) ) { + $available_providers = self::get_providers(); + + // Ensure the provider is even available. + if ( isset( $available_providers[ $new_provider ] ) ) { return false; } From ff7cfb5d0206e6ef259ba98a98bae060e34db5a5 Mon Sep 17 00:00:00 2001 From: Kaspars Dambis Date: Thu, 9 Jan 2025 13:30:57 +0200 Subject: [PATCH 13/13] Keep a consistent format --- class-two-factor-core.php | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/class-two-factor-core.php b/class-two-factor-core.php index 64ebed8f..61179cd9 100644 --- a/class-two-factor-core.php +++ b/class-two-factor-core.php @@ -1896,10 +1896,8 @@ 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(); - // Ensure the provider is even available. - if ( isset( $available_providers[ $new_provider ] ) ) { + if ( ! array_key_exists( $new_provider, self::get_providers() ) ) { return false; } @@ -1930,7 +1928,7 @@ public static function enable_provider_for_user( $user_id, $new_provider ) { */ public static function disable_provider_for_user( $user_id, $provider_to_delete ) { // Check if the provider is even enabled. - if ( ! in_array( $provider_to_delete, self::get_providers_classes(), true ) ) { + if ( ! array_key_exists( $provider_to_delete, self::get_providers() ) ) { return false; }