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.

Sign up for GitHub

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

Check user has a desired provider set #24

Closed
wants to merge 1 commit into from
Closed

Check user has a desired provider set #24

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion wporg-two-factor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,14 @@ function two_factor_providers( array $providers ) : array {
$desired_providers = array(
'Two_Factor_WebAuthn' => '',
'Two_Factor_Totp' => '',
'Two_Factor_Backup_Codes' => '',
Copy link
Member

Choose a reason for hiding this comment

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

Will this cause any problems w/ the flow in #18? The provider wouldn't be loaded when the user visits the page, so we might not be able to generate the backup codes during activation, unless we explicitly require it in the Ajax callback.

The two_factor_enabled_providers_for_user might solve any issues there, and seems a bit more appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly - although for the activation flow we could add in the backup codes through hooking in later to two_factor_providers since we'd be manipulating the standard flow anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just revisited this, and not sure if either are a good option - if we remove the option to allow backup codes until totp or webathn are enabled, we'd have to re-trigger the code to allow using this method. If we use two_factor_enabled_providers_for_user do we want to disable a method that could potentially lock someone out, for example they try to reenable a method and it fails?

Copy link
Member

@iandunn iandunn Jan 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

🤔 yeah, fair point. what problem is this trying to solve? maybe there's another way to do it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what problem is this trying to solve?

In #21 we were trying to avoid users setting up backup codes without another preferred method, but maybe it's better just to prompt them to do so, rather than force them to using code?

The plugin does have logic that if you only have one provider it becomes the primary, so potentially in this scenario should there be an error in setup for a preferred provider the user could become unnecessarily locked out.

Copy link
Member

Choose a reason for hiding this comment

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

Would the "enabled" vs "available" distinction might be useful in avoiding locking the user out?

);

// Check current user has a primary method setup, before allowing backup codes
$user_providers = Two_Factor_Core::get_enabled_providers_for_user();
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
$user_providers = Two_Factor_Core::get_enabled_providers_for_user();
$user_providers = Two_Factor_Core::get_available_providers_for_user();

i didn't realize that this until recently, but the enabled/available functions are confusingly named. A provider can be "enabled", but not it's "available" until it's been configured. I think in this context we want the latter.

Copy link
Member

Choose a reason for hiding this comment

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

Should we pass bbp_get_displayed_user_id() to the function, for the the situations where an admin is editing another user?

if ( ! empty( array_intersect_key( $user_providers, $desired_providers ) ) ) {
$desired_providers['Two_Factor_Backup_Codes'] = '';
}

Copy link
Member

Choose a reason for hiding this comment

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

Do you mind adding some basic tests to the suite? composer run test:watch and test:coverage

return array_intersect_key( $providers, $desired_providers );
}

Expand Down