From 2fee0b2aa55d653386c51a07c7505e0908fa5734 Mon Sep 17 00:00:00 2001 From: Kaspars Dambis Date: Mon, 2 Dec 2024 14:00:24 +0200 Subject: [PATCH 01/27] Accept providers as input --- class-two-factor-core.php | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/class-two-factor-core.php b/class-two-factor-core.php index f0204378..440e01f6 100644 --- a/class-two-factor-core.php +++ b/class-two-factor-core.php @@ -218,17 +218,17 @@ public static function get_providers_registered() { } /** - * Get the classnames for all registered providers. + * Get the classnames for specific providers. * - * Note some of these providers might not be enabled. + * @param array $providers List of paths to provider class files indexed by class names. * * @return array List of provider keys and classnames. */ - private static function get_providers_classes() { - $providers = self::get_providers_registered(); - + private static function get_providers_classes( $providers ) { foreach ( $providers as $provider_key => $path ) { - require_once $path; + if ( ! empty( $path ) && is_readable( $path ) ) { + require_once $path; + } $class = $provider_key; @@ -287,15 +287,13 @@ public static function get_providers() { } // Map provider keys to classes so that we can instantiate them. - $providers = array_intersect_key( self::get_providers_classes(), $providers ); + $providers = self::get_providers_classes( $providers ); foreach ( $providers as $provider_key => $provider_class ) { - if ( method_exists( $provider_class, 'get_instance' ) ) { - try { - $providers[ $provider_key ] = call_user_func( array( $provider_class, 'get_instance' ) ); - } catch ( Exception $e ) { - unset( $providers[ $provider_key ] ); - } + try { + $providers[ $provider_key ] = call_user_func( array( $provider_class, 'get_instance' ) ); + } catch ( Exception $e ) { + unset( $providers[ $provider_key ] ); } } From fc771fb4ca8314029d07e6c8ddc0a178ee50c170 Mon Sep 17 00:00:00 2001 From: Kaspars Dambis Date: Mon, 2 Dec 2024 14:00:41 +0200 Subject: [PATCH 02/27] We should only care that the class exists --- class-two-factor-core.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/class-two-factor-core.php b/class-two-factor-core.php index 440e01f6..421d6787 100644 --- a/class-two-factor-core.php +++ b/class-two-factor-core.php @@ -241,9 +241,9 @@ private static function get_providers_classes( $providers ) { $class = apply_filters( "two_factor_provider_classname_{$provider_key}", $class, $path ); /** - * Confirm that it's been successfully included before instantiating. + * Confirm that it's been successfully included. */ - if ( method_exists( $class, 'get_instance' ) ) { + if ( class_exists( $class ) ) { $providers[ $provider_key ] = $class; } else { unset( $providers[ $provider_key ] ); From 93f561f7b0c60e6808f163244a58afbe02d9addc Mon Sep 17 00:00:00 2001 From: Kaspars Dambis Date: Mon, 2 Dec 2024 14:01:24 +0200 Subject: [PATCH 03/27] Only the uninstall logic should care about disabled providers --- class-two-factor-core.php | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/class-two-factor-core.php b/class-two-factor-core.php index 421d6787..1cee880d 100644 --- a/class-two-factor-core.php +++ b/class-two-factor-core.php @@ -149,7 +149,22 @@ public static function uninstall() { $option_keys = array(); - foreach ( self::get_providers_classes() as $provider_class ) { + $providers = self::get_default_providers(); + + /** + * Include additional providers to be uninstalled. + * + * @param array $providers A key-value array where the key is the class name, and + * the value is the path to the file containing the class. + */ + $additional_providers = apply_filters( 'two_factor_providers', $providers ); + + // Merge them with the default providers. + if ( ! empty( $additional_providers ) ) { + return array_merge( $providers, $additional_providers ); + } + + foreach ( self::get_providers_classes( $providers ) as $provider_class ) { // Merge with provider-specific user meta keys. if ( method_exists( $provider_class, 'uninstall_user_meta_keys' ) ) { try { @@ -192,29 +207,14 @@ public static function uninstall() { * * @return array List of provider keys and paths to class files. */ - public static function get_providers_registered() { - $providers = array( + private static function get_default_providers() { + return array( 'Two_Factor_Email' => TWO_FACTOR_DIR . 'providers/class-two-factor-email.php', 'Two_Factor_Totp' => TWO_FACTOR_DIR . 'providers/class-two-factor-totp.php', 'Two_Factor_FIDO_U2F' => TWO_FACTOR_DIR . 'providers/class-two-factor-fido-u2f.php', 'Two_Factor_Backup_Codes' => TWO_FACTOR_DIR . 'providers/class-two-factor-backup-codes.php', 'Two_Factor_Dummy' => TWO_FACTOR_DIR . 'providers/class-two-factor-dummy.php', ); - - /** - * Filter the supplied providers. - * - * @param array $providers A key-value array where the key is the class name, and - * the value is the path to the file containing the class. - */ - $additional_providers = apply_filters( 'two_factor_providers', $providers ); - - // Merge them with the default providers. - if ( ! empty( $additional_providers ) ) { - return array_merge( $providers, $additional_providers ); - } - - return $providers; } /** @@ -261,7 +261,7 @@ private static function get_providers_classes( $providers ) { * @return array */ public static function get_providers() { - $providers = self::get_providers_registered(); + $providers = self::get_default_providers(); /** * Filter the supplied providers. From 656772a4f897dd4f4c82a7533adf9de2eeb0032b Mon Sep 17 00:00:00 2001 From: Kaspars Dambis Date: Mon, 2 Dec 2024 14:31:42 +0200 Subject: [PATCH 04/27] We need to actually do something here --- class-two-factor-core.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/class-two-factor-core.php b/class-two-factor-core.php index 1cee880d..7c8cad14 100644 --- a/class-two-factor-core.php +++ b/class-two-factor-core.php @@ -161,7 +161,7 @@ public static function uninstall() { // Merge them with the default providers. if ( ! empty( $additional_providers ) ) { - return array_merge( $providers, $additional_providers ); + $providers = array_merge( $providers, $additional_providers ); } foreach ( self::get_providers_classes( $providers ) as $provider_class ) { From f275d7c207959a6e451feae945c8d27c6d441115 Mon Sep 17 00:00:00 2001 From: Kaspars Dambis Date: Mon, 2 Dec 2024 14:42:24 +0200 Subject: [PATCH 05/27] Add a unit test that disabled providers are still deleted during uninstall --- tests/class-two-factor-core.php | 37 +++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/tests/class-two-factor-core.php b/tests/class-two-factor-core.php index 5f6a15a2..a2bcaa57 100644 --- a/tests/class-two-factor-core.php +++ b/tests/class-two-factor-core.php @@ -1581,4 +1581,41 @@ public function test_uninstall_removes_user_meta() { 'Provider was disabled due to uninstall' ); } + + /** + * Plugin uninstall removes all user meta even for disabled providers. + * + * @covers Two_Factor_Core::uninstall + */ + public function test_uninstall_removes_disabled_provider_user_meta() { + $user = self::factory()->user->create_and_get(); + + // Enable a provider for the user. + Two_Factor_Core::enable_provider_for_user( $user->ID, 'Two_Factor_Totp' ); + + $totp_provider = Two_Factor_Totp::get_instance(); + + $totp_provider->set_user_totp_key( $user->ID, 'some_key' ); + + $this->assertEquals( 'some_key', $totp_provider->get_user_totp_key( $user->ID ), 'TOTP secret was set for user' ); + + add_filter( + 'two_factor_providers', + function( $providers ) { + return array_diff_key( $providers, array( 'Two_Factor_Totp' => null ) ); + } + ); + + $this->assertNotContains( + 'Two_Factor_Totp', + Two_Factor_Core::get_enabled_providers_for_user( $user->ID ), + 'TOTP provider is disabled for everyone via filter' + ); + + Two_Factor_Core::uninstall(); + + $this->assertEmpty( $totp_provider->get_user_totp_key( $user->ID ), 'TOTP secret was deleted during uninstall' ); + + remove_all_filters( 'two_factor_providers' ); + } } From e6c3ea1c1db9e6d13b9b7a73a1ed918a526da2a7 Mon Sep 17 00:00:00 2001 From: Kaspars Dambis Date: Mon, 2 Dec 2024 15:28:59 +0200 Subject: [PATCH 06/27] Allow filtering the email token length --- providers/class-two-factor-email.php | 4 +++- readme.txt | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/providers/class-two-factor-email.php b/providers/class-two-factor-email.php index da7a7c60..a3d86b43 100644 --- a/providers/class-two-factor-email.php +++ b/providers/class-two-factor-email.php @@ -72,7 +72,9 @@ public function get_alternative_provider_label() { * @return string */ public function generate_token( $user_id ) { - $token = $this->get_code(); + $token_length = (int) apply_filters( 'two_factor_token_length', 8 ); + + $token = $this->get_code( $token_length ); update_user_meta( $user_id, self::TOKEN_META_KEY_TIMESTAMP, time() ); update_user_meta( $user_id, self::TOKEN_META_KEY, wp_hash( $token ) ); diff --git a/readme.txt b/readme.txt index 8c2ff974..07cd5777 100644 --- a/readme.txt +++ b/readme.txt @@ -28,6 +28,7 @@ Here is a list of action and filter hooks provided by the plugin: - `two_factor_enabled_providers_for_user` filter overrides the list of two-factor providers enabled for a user. First argument is an array of enabled provider classnames as values, the second argument is the user ID. - `two_factor_user_authenticated` action which receives the logged in `WP_User` object as the first argument for determining the logged in user right after the authentication workflow. - `two_factor_token_ttl` filter overrides the time interval in seconds that an email token is considered after generation. Accepts the time in seconds as the first argument and the ID of the `WP_User` object being authenticated. +- `two_factor_token_length` filter overrides the default 8 character count for email tokens. == Frequently Asked Questions == From 1b103109da01b2f315f9b2355c53477461f15edd Mon Sep 17 00:00:00 2001 From: Kaspars Dambis Date: Mon, 2 Dec 2024 15:29:12 +0200 Subject: [PATCH 07/27] Allow filtering the backup code length --- providers/class-two-factor-backup-codes.php | 10 +++++++++- readme.txt | 1 + 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/providers/class-two-factor-backup-codes.php b/providers/class-two-factor-backup-codes.php index 12601d5c..89389238 100644 --- a/providers/class-two-factor-backup-codes.php +++ b/providers/class-two-factor-backup-codes.php @@ -239,8 +239,16 @@ public function generate_codes( $user, $args = '' ) { $codes_hashed = (array) get_user_meta( $user->ID, self::BACKUP_CODES_META_KEY, true ); } + /** + * Customize the character count of the backup codes. + * + * @var int $code_length Length of the backup code. + * @var WP_User $user User object. + */ + $code_length = (int) apply_filters( 'two_factor_backup_code_length', 8, $user ); + for ( $i = 0; $i < $num_codes; $i++ ) { - $code = $this->get_code(); + $code = $this->get_code( $code_length ); $codes_hashed[] = wp_hash_password( $code ); $codes[] = $code; unset( $code ); diff --git a/readme.txt b/readme.txt index 07cd5777..d0853539 100644 --- a/readme.txt +++ b/readme.txt @@ -29,6 +29,7 @@ Here is a list of action and filter hooks provided by the plugin: - `two_factor_user_authenticated` action which receives the logged in `WP_User` object as the first argument for determining the logged in user right after the authentication workflow. - `two_factor_token_ttl` filter overrides the time interval in seconds that an email token is considered after generation. Accepts the time in seconds as the first argument and the ID of the `WP_User` object being authenticated. - `two_factor_token_length` filter overrides the default 8 character count for email tokens. +- `two_factor_backup_code_length` filter overrides the default 8 character count for backup codes. Providers the `WP_User` of the associated user as the second argument. == Frequently Asked Questions == From bbcd0415f9b517d753520ed918d982b068332f72 Mon Sep 17 00:00:00 2001 From: Kaspars Dambis Date: Mon, 2 Dec 2024 15:40:57 +0200 Subject: [PATCH 08/27] Add tests for the backup code length filter --- .../class-two-factor-backup-codes.php | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/providers/class-two-factor-backup-codes.php b/tests/providers/class-two-factor-backup-codes.php index a926044e..fb134b60 100644 --- a/tests/providers/class-two-factor-backup-codes.php +++ b/tests/providers/class-two-factor-backup-codes.php @@ -194,4 +194,25 @@ public function test_delete_code() { $this->provider->delete_code( $user, $backup_codes[0] ); $this->assertEquals( 1, $this->provider->codes_remaining_for_user( $user ) ); } + + public function test_backup_code_length_filter() { + $user = new WP_User( self::factory()->user->create() ); + + $code_default = $this->provider->generate_codes( $user, array( 'number' => 1 ) ); + + add_filter( + 'two_factor_backup_code_length', + function() { + return 7; + } + ); + + $code_custom_length = $this->provider->generate_codes( $user, array( 'number' => 1 ) ); + + $this->assertNotEquals( strlen( $code_custom_length[0] ), strlen( $code_default[0] ), 'Backup code length can be adjusted via filter' ); + + $this->assertEquals( 7, strlen( $code_custom_length[0] ), 'Backup code length matches the filtered length' ); + + remove_all_filters( 'two_factor_backup_code_length' ); + } } From 61a74896b7ec5ebb1a207df104c01e78ee19d801 Mon Sep 17 00:00:00 2001 From: Kaspars Dambis Date: Mon, 2 Dec 2024 15:45:16 +0200 Subject: [PATCH 09/27] Add tests for email token length too --- tests/providers/class-two-factor-email.php | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/providers/class-two-factor-email.php b/tests/providers/class-two-factor-email.php index 93e5dc76..0b5a154c 100644 --- a/tests/providers/class-two-factor-email.php +++ b/tests/providers/class-two-factor-email.php @@ -352,4 +352,24 @@ public function test_tokens_can_expire() { ); } + public function test_custom_token_length() { + $user_id = self::factory()->user->create(); + + $default_token = $this->provider->generate_token( $user_id ); + + add_filter( + 'two_factor_token_length', + function() { + return 15; + } + ); + + $custom_token = $this->provider->generate_token( $user_id ); + + $this->assertNotEquals( strlen( $default_token ), strlen( $custom_token ), 'Token length is different due to filter' ); + $this->assertEquals( 15, strlen( $custom_token ), 'Token length matches the filter value' ); + + remove_all_filters( 'two_factor_token_length' ); + } + } From 702475de1cac7881ecaafbf919dcefb174c2b072 Mon Sep 17 00:00:00 2001 From: Kaspars Dambis Date: Wed, 18 Dec 2024 10:37:02 +0200 Subject: [PATCH 10/27] Test that a core provider can be disabled --- tests/class-two-factor-core.php | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/class-two-factor-core.php b/tests/class-two-factor-core.php index a2bcaa57..ad09f2c1 100644 --- a/tests/class-two-factor-core.php +++ b/tests/class-two-factor-core.php @@ -1582,6 +1582,21 @@ public function test_uninstall_removes_user_meta() { ); } + public function test_can_disable_default_providers() { + $this->assertContains( 'Two_Factor_Email', array_keys( Two_Factor_Core::get_providers() ), 'Email provider is enabled by default' ); + + add_filter( + 'two_factor_providers', + function( $providers ) { + return array_diff_key( $providers, array( 'Two_Factor_Email' => null ) ); + } + ); + + $this->assertNotContains( 'Two_Factor_Email', array_keys( Two_Factor_Core::get_providers() ), 'Default provider can be disabled via a filter' ); + + remove_all_filters( 'two_factor_providers' ); + } + /** * Plugin uninstall removes all user meta even for disabled providers. * From 2c2e7f31844d09e396c6256a42650e5337dfc112 Mon Sep 17 00:00:00 2001 From: Kaspars Dambis Date: Wed, 18 Dec 2024 10:37:57 +0200 Subject: [PATCH 11/27] Add a reminder about performance implications --- class-two-factor-core.php | 1 + 1 file changed, 1 insertion(+) diff --git a/class-two-factor-core.php b/class-two-factor-core.php index 7c8cad14..a0a0fb59 100644 --- a/class-two-factor-core.php +++ b/class-two-factor-core.php @@ -289,6 +289,7 @@ public static function get_providers() { // Map provider keys to classes so that we can instantiate them. $providers = self::get_providers_classes( $providers ); + // TODO: Refactor this to avoid instantiating the provider instances every time this method is called. foreach ( $providers as $provider_key => $provider_class ) { try { $providers[ $provider_key ] = call_user_func( array( $provider_class, 'get_instance' ) ); From ee03637a7ee0e6d99e34c60c9ca01d6b70834cf8 Mon Sep 17 00:00:00 2001 From: Kaspars Dambis Date: Wed, 18 Dec 2024 10:41:39 +0200 Subject: [PATCH 12/27] Per linter --- tests/class-two-factor-core.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/class-two-factor-core.php b/tests/class-two-factor-core.php index ad09f2c1..3d88dd40 100644 --- a/tests/class-two-factor-core.php +++ b/tests/class-two-factor-core.php @@ -1587,7 +1587,7 @@ public function test_can_disable_default_providers() { add_filter( 'two_factor_providers', - function( $providers ) { + function ( $providers ) { return array_diff_key( $providers, array( 'Two_Factor_Email' => null ) ); } ); @@ -1616,7 +1616,7 @@ public function test_uninstall_removes_disabled_provider_user_meta() { add_filter( 'two_factor_providers', - function( $providers ) { + function ( $providers ) { return array_diff_key( $providers, array( 'Two_Factor_Totp' => null ) ); } ); From 604ff9f034957dfcf571d200d1dc4e7cb602d6d4 Mon Sep 17 00:00:00 2001 From: Kaspars Dambis Date: Wed, 18 Dec 2024 19:14:45 +0200 Subject: [PATCH 13/27] Link to the original filter documentation to avoid repetition --- class-two-factor-core.php | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/class-two-factor-core.php b/class-two-factor-core.php index a0a0fb59..c9916d8f 100644 --- a/class-two-factor-core.php +++ b/class-two-factor-core.php @@ -151,12 +151,7 @@ public static function uninstall() { $providers = self::get_default_providers(); - /** - * Include additional providers to be uninstalled. - * - * @param array $providers A key-value array where the key is the class name, and - * the value is the path to the file containing the class. - */ + /** This filter is documented in the get_providers() method */ $additional_providers = apply_filters( 'two_factor_providers', $providers ); // Merge them with the default providers. From 6a9a1336c141a851e108f249417ccb8baebaabd9 Mon Sep 17 00:00:00 2001 From: Stefan Momm Date: Mon, 6 Jan 2025 18:07:03 +0100 Subject: [PATCH 14/27] Disable input autocompletion --- providers/class-two-factor-backup-codes.php | 2 +- providers/class-two-factor-email.php | 2 +- providers/class-two-factor-totp.php | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/providers/class-two-factor-backup-codes.php b/providers/class-two-factor-backup-codes.php index 12601d5c..3597d80b 100644 --- a/providers/class-two-factor-backup-codes.php +++ b/providers/class-two-factor-backup-codes.php @@ -330,7 +330,7 @@ public function authentication_page( $user ) {


- +

- +

diff --git a/providers/class-two-factor-totp.php b/providers/class-two-factor-totp.php index 003a9774..9d44b67b 100644 --- a/providers/class-two-factor-totp.php +++ b/providers/class-two-factor-totp.php @@ -349,7 +349,7 @@ public function user_two_factor_options( $user ) { /* translators: Example auth code. */ $placeholder = sprintf( __( 'eg. %s', 'two-factor' ), '123456' ); ?> - +

@@ -682,7 +682,7 @@ public function authentication_page( $user ) {

- +