From 2fee0b2aa55d653386c51a07c7505e0908fa5734 Mon Sep 17 00:00:00 2001 From: Kaspars Dambis Date: Mon, 2 Dec 2024 14:00:24 +0200 Subject: [PATCH 1/9] 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 2/9] 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 3/9] 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 4/9] 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 5/9] 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 702475de1cac7881ecaafbf919dcefb174c2b072 Mon Sep 17 00:00:00 2001 From: Kaspars Dambis Date: Wed, 18 Dec 2024 10:37:02 +0200 Subject: [PATCH 6/9] 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 7/9] 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 8/9] 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 9/9] 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.