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.

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

Ensure that doesn't 'fail open' if existing providers poof. #586

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

georgestephanis
Copy link
Collaborator

This also ensures if a user only had U2F enabled, and it's deprecated and removed, that it won't "fail open" for lack of any available methods.

If Email is available, shove it in. If not, return an error.

This also ensures if a user only had U2F enabled, and it's deprecated and removed, that it won't "fail open" for lack of any available methods.

If Email is available, shove it in.  If not, return an error.
@@ -413,15 +413,37 @@ public static function get_enabled_providers_for_user( $user = null ) {
if ( empty( $enabled_providers ) ) {
$enabled_providers = array();
}
$enabled_providers = array_intersect( $enabled_providers, array_keys( $providers ) );
$enabled_existing_providers = array_intersect( $enabled_providers, array_keys( $providers ) );
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$enabled_existing_providers = array_intersect( $enabled_providers, array_keys( $providers ) );
$enabled_registered_providers = array_intersect( $enabled_providers, array_keys( $providers ) );

Would this be more clear that we are talking about enabled providers that are registered?

@georgestephanis
Copy link
Collaborator Author

Possible chance with bad input this changes some test results -- I need to add a test for the use case anyway.

The return value uses the class as keys, and the object as the value.
TOTP being unconfigured stripped it out otherwise.
@georgestephanis
Copy link
Collaborator Author

Once this is in, purging u2f from the plugin won't leave users who only had that enabled wide open with no two-factor protection.

Copy link
Member

@iandunn iandunn left a comment

Choose a reason for hiding this comment

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

Didn't test manually, but the code LGTM 👍🏻

class-two-factor-core.php Show resolved Hide resolved
@jeffpaul jeffpaul modified the milestones: 0.9.0, 0.10.0 May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants