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

Add a filter to limit two-factor providers available to each user #669

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

Conversation

kasparsd
Copy link
Collaborator

@kasparsd kasparsd commented Feb 14, 2025

What?

  1. Add a filter to allow limiting the available providers per user.
  2. Skip rendering the provider table if no providers are available for a user.

Why?

Fixes #647, #662.

How?

  1. Introduce a new method get_supported_providers_for_user( $user ) which wraps the output of get_providers() in a filter two_factor_providers_for_user.
  2. Use the new helper whenever we're looking for two-factor methods that should be available to a user.

Note that we already similar methods:

  • get_enabled_providers_for_user( $user = null ) for the provider keys that are enabled in the user profile but might not be configured, yet.
  • get_available_providers_for_user( $user = null ) for the provider keys that are both enabled and configured.

so I chose the name supported to designate if a provider is available to a specific user.

Testing Instructions

Unit tests have been added to cover the filtering scenario. There are no user settings to toggle this.

Screenshots or screencast

If all providers are disabled for a user we now display a notice instead of an empty table:

two-factor-options-no-providers

Changelog Entry

Add a filter two_factor_providers_for_user to disable providers for specific users.

@kasparsd kasparsd requested a review from ocean90 February 14, 2025 15:21
Copy link
Collaborator Author

@kasparsd kasparsd left a comment

Choose a reason for hiding this comment

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

@ocean90 Would this approach provide you with the required filtering as described in #647?

* provider class names and the values as the provider class instances.
*
* @see Two_Factor_Core::get_enabled_providers_for_user()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cross-linked the related methods for easier discovery and comparison.

* @param WP_User|int|null $user User ID.
* @return array List of provider instances indexed by provider key.
*/
public static function get_supported_providers_for_user( $user = null ) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the new method and the associated filter.

$show_2fa_options ? '' : 'disabled="disabled"'
);
if ( empty( $providers ) ) {
$notices['notice two-factor-notice-no-providers-supported'] = esc_html__( 'No providers are available for your account.', 'two-factor' );
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Display a notice if no providers are available for user.

I'm assuming that with the plugin enabled it would be more confusing to hide the section completely. With the section being present users can at least know the option should be available and is simply disabled.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, perhaps iterating on the copy to provide a user with some insight on what to do next? Eg. "No providers are available for your account. Please contact the site administrator for more details." or whatever verbage is used elsewhere in core to point folks to the admin (either site or network)?


if ( 1 === count( $enabled_providers ) ) {
// Suggest enabling a backup method if only method is enabled and there are more available.
if ( count( $providers ) > 1 && 1 === count( $enabled_providers ) ) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Display this suggestion only if at least two methods are available.

<fieldset id="two-factor-options" <?php echo $show_2fa_options ? '' : 'disabled="disabled"'; ?>>
<?php
if ( $providers ) {
self::render_user_providers_form( $user, $providers );
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To avoid wrapping a large block into this conditional, I've moved the form to a helper method.

@@ -164,6 +164,10 @@ protected static function asset_version() {
* @param WP_User $user WP_User object of the logged-in user.
*/
public static function show_user_profile( $user ) {
if ( ! Two_Factor_FIDO_U2F::is_supported_for_user( $user ) ) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Avoid rendering the FIDO U2F settings if the provider is not "supported".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants