-
Notifications
You must be signed in to change notification settings - Fork 177
Remove duplicate two_factor_providers filter calls to allow disabling core providers
#651
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
Changes from all commits
2fee0b2
fc771fb
93f561f
656772a
f275d7c
702475d
2c2e7f3
ee03637
604ff9f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -149,7 +149,17 @@ public static function uninstall() { | |
|
|
||
| $option_keys = array(); | ||
|
|
||
| foreach ( self::get_providers_classes() as $provider_class ) { | ||
| $providers = self::get_default_providers(); | ||
|
|
||
| /** This filter is documented in the get_providers() method */ | ||
| $additional_providers = apply_filters( 'two_factor_providers', $providers ); | ||
|
|
||
| // Merge them with the default providers. | ||
| if ( ! empty( $additional_providers ) ) { | ||
| $providers = array_merge( $providers, $additional_providers ); | ||
| } | ||
|
|
||
| foreach ( self::get_providers_classes( $providers ) as $provider_class ) { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here we still pass the provider classes through the filters so any provider adjustments are accounted for. |
||
| // Merge with provider-specific user meta keys. | ||
| if ( method_exists( $provider_class, 'uninstall_user_meta_keys' ) ) { | ||
| try { | ||
|
|
@@ -192,43 +202,28 @@ public static function uninstall() { | |
| * | ||
| * @return array List of provider keys and paths to class files. | ||
| */ | ||
| public static function get_providers_registered() { | ||
|
kasparsd marked this conversation as resolved.
|
||
| $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; | ||
| } | ||
|
|
||
| /** | ||
| * 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 ) ) { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Previously this wasn't checking if the file path even exists. If the path isn't valid, we still attempt to check if the class exists and include it. This was always a private method so we don't need to account for anyone referencing this. |
||
| require_once $path; | ||
| } | ||
|
|
||
| $class = $provider_key; | ||
|
|
||
|
|
@@ -241,9 +236,9 @@ private static function get_providers_classes() { | |
| $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 ) ) { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
| $providers[ $provider_key ] = $class; | ||
| } else { | ||
| unset( $providers[ $provider_key ] ); | ||
|
|
@@ -261,7 +256,7 @@ private static function get_providers_classes() { | |
| * @return array | ||
| */ | ||
| public static function get_providers() { | ||
| $providers = self::get_providers_registered(); | ||
| $providers = self::get_default_providers(); | ||
|
|
||
| /** | ||
| * Filter the supplied providers. | ||
|
|
@@ -287,15 +282,14 @@ 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 ); | ||
|
|
||
| // TODO: Refactor this to avoid instantiating the provider instances every time this method is called. | ||
| foreach ( $providers as $provider_key => $provider_class ) { | ||
| if ( method_exists( $provider_class, 'get_instance' ) ) { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Previously it wouldn't actually remove the provider from the list if it didn't have the |
||
| 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' ) ); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will fail both when (1) |
||
| } catch ( Exception $e ) { | ||
| unset( $providers[ $provider_key ] ); | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1581,4 +1581,56 @@ public function test_uninstall_removes_user_meta() { | |
| 'Provider was disabled due to uninstall' | ||
| ); | ||
| } | ||
|
|
||
| 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' ); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
|
||
| remove_all_filters( 'two_factor_providers' ); | ||
| } | ||
|
|
||
| /** | ||
| * 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' ); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.