Check user has a desired provider set #24
Conversation
Disallow backup codes unless a desired provider exists
| @@ -35,9 +35,14 @@ function two_factor_providers( array $providers ) : array { | |||
| $desired_providers = array( | |||
| 'Two_Factor_WebAuthn' => '', | |||
| 'Two_Factor_Totp' => '', | |||
| 'Two_Factor_Backup_Codes' => '', | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
🤔 yeah, fair point. what problem is this trying to solve? maybe there's another way to do it?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Would the "enabled" vs "available" distinction might be useful in avoiding locking the user out?
| if ( ! empty( array_intersect_key( $user_providers, $desired_providers ) ) ) { | ||
| $desired_providers['Two_Factor_Backup_Codes'] = ''; | ||
| } | ||
|
|
There was a problem hiding this comment.
Do you mind adding some basic tests to the suite? composer run test:watch and test:coverage
| ); | ||
|
|
||
| // Check current user has a primary method setup, before allowing backup codes | ||
| $user_providers = Two_Factor_Core::get_enabled_providers_for_user(); |
There was a problem hiding this comment.
| $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.
| ); | ||
|
|
||
| // Check current user has a primary method setup, before allowing backup codes | ||
| $user_providers = Two_Factor_Core::get_enabled_providers_for_user(); |
There was a problem hiding this comment.
Should we pass bbp_get_displayed_user_id() to the function, for the the situations where an admin is editing another user?
|
I think this was superseded by #75, but please reopen if there's anything from it that we still want to merge. |
Disallow backup codes unless a desired provider exists