Skip to content

Improve Higlight-MFA Module notice to specify the affected roles #28

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

Merged
merged 3 commits into from
Jun 9, 2025
Merged

Improve Higlight-MFA Module notice to specify the affected roles #28

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
49fcfeb
improve the text in the notice to highlight that the users missing th…
andrea-sdl Jun 6, 2025
00820d1
use unordered array comparison for default config check in MFA roles
andrea-sdl Jun 6, 2025
cf48a0d
fix indentation
andrea-sdl Jun 9, 2025
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
122 changes: 95 additions & 27 deletions modules/highlight-mfa-users/class-highlight-mfa-users.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@
use function Automattic\VIP\Security\Utils\get_module_configs;

class Highlight_MFA_Users {
const MFA_SKIP_USER_IDS_OPTION_KEY = 'vip_security_mfa_skip_user_ids';
const ROLE_COLUMN_KEY = 'role';
const MFA_SKIP_USER_IDS_OPTION_KEY = 'vip_security_mfa_skip_user_ids';
const ROLE_COLUMN_KEY = 'role';
const DEFAULT_ADMIN_EDITOR_ROLE_SLUGS = [ 'administrator', 'editor' ];

/**
* The roles used to highlight users without MFA.
Expand All @@ -17,15 +18,15 @@ class Highlight_MFA_Users {
public static function init() {
// Feature is always active unless specific users are skipped via option.
$highlight_mfa_configs = get_module_configs( 'highlight-mfa-users' );
self::$roles = $highlight_mfa_configs['roles'] ?? [ 'administrator', 'editor' ]; // Default to administrator and editor if not configured
self::$roles = $highlight_mfa_configs['roles'] ?? self::DEFAULT_ADMIN_EDITOR_ROLE_SLUGS; // Default to administrator and editor if not configured

if ( ! is_array( self::$roles ) ) {
self::$roles = [ self::$roles ];
}
self::$roles = array_filter( self::$roles );
// If after filtering, the array is empty, default back to administrator and editor
if ( empty( self::$roles ) ) {
self::$roles = [ 'administrator', 'editor' ];
self::$roles = self::DEFAULT_ADMIN_EDITOR_ROLE_SLUGS;
}

add_action( 'admin_notices', [ __CLASS__, 'display_mfa_disabled_notice' ] );
Expand Down Expand Up @@ -61,6 +62,14 @@ public static function display_mfa_disabled_notice() {
return;
}

// Determine notice text based on role configuration
// self::$roles is already an array and populated from init().
$configured_roles_for_comparison = self::$roles;
\sort( $configured_roles_for_comparison ); // Use global sort

// unordered array check with ==
$is_default_config = ( self::DEFAULT_ADMIN_EDITOR_ROLE_SLUGS === $configured_roles_for_comparison || empty( $configured_roles_for_comparison ) );

$skipped_user_ids = get_option( self::MFA_SKIP_USER_IDS_OPTION_KEY, [] );
if ( ! is_array( $skipped_user_ids ) ) {
$skipped_user_ids = [];
Expand Down Expand Up @@ -95,45 +104,104 @@ public static function display_mfa_disabled_notice() {
$is_filtered = isset( $_GET['filter_mfa_disabled'] ) && '1' === $_GET['filter_mfa_disabled'];

if ( $is_filtered ) {
// Display notice for when the list IS filtered
$show_all_url = remove_query_arg( 'filter_mfa_disabled', admin_url( 'users.php' ) );
// Display info notice for when the list IS filtered
$show_all_url = remove_query_arg( 'filter_mfa_disabled', admin_url( 'users.php' ) );
$notice_message_text = self::get_missing_mfa_notice_message_text( $mfa_disabled_count, $is_default_config );

printf(
'<div class="notice notice-info"><p>%s <a href="%s">%s</a></p></div>',
esc_html( sprintf(
/* Translators: %d is the number of users without 2FA enabled being shown in the filtered list. */
_n(
'Showing %d user without Two-Factor Authentication enabled.',
'Showing %d users without Two-Factor Authentication enabled.',
$mfa_disabled_count,
'wpvip'
),
number_format_i18n( $mfa_disabled_count )
) ),
esc_html( $notice_message_text ),
esc_url( $show_all_url ),
esc_html__( 'Show all users.', 'wpvip' )
);
} else {
// Display the original notice when the list is NOT filtered
$filter_url = add_query_arg( 'filter_mfa_disabled', '1', admin_url( 'users.php' ) );
$filter_url = add_query_arg( 'filter_mfa_disabled', '1', admin_url( 'users.php' ) );
$notice_message_text = self::get_filtering_mfa_info_message_text( $mfa_disabled_count, $is_default_config );

printf(
'<div class="notice notice-error"><p>%s <a href="%s">%s</a></p></div>',
esc_html( sprintf(
/* Translators: %d is the number of users without 2FA enabled. */
_n(
'There is %d user with Two-Factor Authentication disabled.',
'There are %d users with Two-Factor Authentication disabled.',
$mfa_disabled_count,
'wpvip'
),
number_format_i18n( $mfa_disabled_count )
) ),
esc_html( $notice_message_text ),
esc_url( $filter_url ),
esc_html__( 'Filter list to show these users.', 'wpvip' )
);
}
}
}

/**
* Get the notice message text for when users with missing MFA are shown.
*
* @param int $mfa_disabled_count The number of users with missing MFA.
* @param bool $is_default_config Whether the default roles are being used.
* @return string The notice message text.
*/
protected static function get_missing_mfa_notice_message_text( $mfa_disabled_count, $is_default_config ) {
$notice_message_text = '';
if ( $is_default_config ) {
// Default roles: Administrator, Editor
$notice_message_text = sprintf(
/* Translators: %d is the number of users with Administrator or Editor roles without 2FA enabled being shown. */
_n(
'Showing %d user with Administrator or Editor roles without Two-Factor Authentication enabled.',
'Showing %d users with Administrator or Editor roles without Two-Factor Authentication enabled.',
$mfa_disabled_count,
'wpvip'
),
number_format_i18n( $mfa_disabled_count )
);
} else {
// Custom roles
$notice_message_text = sprintf(
/* Translators: %d is the number of users with high-privileges without 2FA enabled being shown. */
_n(
'Showing %d user with high-privileges without Two-Factor Authentication enabled.',
'Showing %d users with high-privileges without Two-Factor Authentication enabled.',
$mfa_disabled_count,
'wpvip'
),
number_format_i18n( $mfa_disabled_count )
);
}

return $notice_message_text;
}

/**
* Get the notice message text for when users with missing MFA are filtered.
*
* @param int $mfa_disabled_count The number of users with missing MFA.
* @param bool $is_default_config Whether the default roles are being used.
* @return string The notice message text.
*/
protected static function get_filtering_mfa_info_message_text( $mfa_disabled_count, $is_default_config ) {
$notice_message_text = '';
if ( $is_default_config ) {
$notice_message_text = sprintf(
/* Translators: %d is the number of users with Administrator or Editor roles and 2FA disabled. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: Indentation looks off here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brunobasto Agree. I wonder why it wasn't caught by the linting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed indentation in cf48a0d

_n(
'There is %d user with Administrator or Editor roles with Two-Factor Authentication disabled.',
'There are %d users with Administrator or Editor roles with Two-Factor Authentication disabled.',
$mfa_disabled_count,
'wpvip'
),
number_format_i18n( $mfa_disabled_count )
);
} else {
$notice_message_text = sprintf(
/* Translators: %d is the number of high-privilege users with 2FA disabled. */
_n(
'There is %d user with high-privileges with Two-Factor Authentication disabled.',
'There are %d users with high-privileges with Two-Factor Authentication disabled.',
$mfa_disabled_count,
'wpvip'
),
number_format_i18n( $mfa_disabled_count )
);
}
return $notice_message_text;
}

/**
* Modify the user query on the Users page to filter by MFA status if requested.
* @param \WP_User_Query $query The WP_User_Query instance (passed by reference).
Expand Down
109 changes: 105 additions & 4 deletions tests/phpunit/test-highlight-mfa-users.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,8 +212,8 @@ public function test_display_mfa_disabled_notice_shows_when_needed() {
sprintf(
// translators: %d: Number of users.
_n(
'There is %d user with Two-Factor Authentication disabled.',
'There are %d users with Two-Factor Authentication disabled.',
'There is %d user with Administrator or Editor roles with Two-Factor Authentication disabled.',
'There are %d users with Administrator or Editor roles with Two-Factor Authentication disabled.',
$expected_count,
'wpvip'
),
Expand Down Expand Up @@ -248,8 +248,8 @@ public function test_display_mfa_disabled_notice_shows_correct_message_when_filt
sprintf(
// translators: %d: Number of users.
_n(
'Showing %d user without Two-Factor Authentication enabled.',
'Showing %d users without Two-Factor Authentication enabled.',
'Showing %d user with Administrator or Editor roles without Two-Factor Authentication enabled.',
'Showing %d users with Administrator or Editor roles without Two-Factor Authentication enabled.',
$expected_count,
'wpvip'
),
Expand Down Expand Up @@ -308,6 +308,107 @@ public function test_display_mfa_disabled_notice_hides_when_no_disabled_users()
$this->assertEmpty( $output );
}

/**
* Test that the admin notice (filtered view) displays correctly for custom high-privilege roles.
*/
public function test_display_mfa_disabled_notice_shows_correct_filtered_message_for_custom_roles() {
$this->set_admin_screen_users();
$_GET['filter_mfa_disabled'] = '1'; // Activate the filter

// Set custom roles using reflection
$reflection_class = new \ReflectionClass( Highlight_MFA_Users::class );
$roles_property = $reflection_class->getProperty( 'roles' );
$roles_property->setAccessible( true );
$custom_roles = [ 'author', 'contributor' ];
$roles_property->setValue( null, $custom_roles );

// Create users with custom roles
$author_user_id = $this->factory()->user->create( [ 'role' => 'author' ] );
$contributor_user_id = $this->factory()->user->create( [ 'role' => 'contributor' ] );

// Existing users (admin_user_mfa_disabled_id, editor_user_id, user ID 1) should not be counted
// as 'administrator' and 'editor' are not in $custom_roles for this test.

$expected_count = 2; // The author and contributor created above.
$show_all_url = remove_query_arg( 'filter_mfa_disabled', admin_url( 'users.php' ) );
$expected_output = sprintf(
'<div class="notice notice-info"><p>%s <a href="%s">%s</a></p></div>',
sprintf(
// translators: %d: Number of users.
_n(
'Showing %d user with high-privileges without Two-Factor Authentication enabled.',
'Showing %d users with high-privileges without Two-Factor Authentication enabled.',
$expected_count,
'wpvip'
),
number_format_i18n( $expected_count )
),
esc_url( $show_all_url ),
esc_html__( 'Show all users.', 'wpvip' )
);

ob_start();
Highlight_MFA_Users::display_mfa_disabled_notice();
$output = ob_get_clean();

$this->assertEquals( $expected_output, $output );

unset( $_GET['filter_mfa_disabled'] ); // Clean up
}

/**
* Test that the admin notice displays correctly for custom high-privilege roles.
*/
public function test_display_mfa_disabled_notice_shows_for_custom_roles() {
$this->set_admin_screen_users();

// Set custom roles using reflection
$reflection_class = new \ReflectionClass( Highlight_MFA_Users::class );
$roles_property = $reflection_class->getProperty( 'roles' );
$roles_property->setAccessible( true );
// Highlight_MFA_Users::init() in setUp already set roles to default.
// We are overriding them here for this specific test.
$custom_roles = [ 'author', 'contributor' ];
$roles_property->setValue( null, $custom_roles );

// Create users with custom roles
$author_user_id = $this->factory()->user->create( [ 'role' => 'author' ] );
// Ensure this user is MFA disabled (default for mock unless added to Two_Factor_Core::$mock_enabled_user_ids)

$contributor_user_id = $this->factory()->user->create( [ 'role' => 'contributor' ] );
// Ensure this user is MFA disabled

// Existing users (admin_user_mfa_disabled_id, editor_user_id, user ID 1) should not be counted
// as 'administrator' and 'editor' are not in $custom_roles for this test.

$expected_count = 2; // The author and contributor created above.
$filter_url = add_query_arg( 'filter_mfa_disabled', '1', admin_url( 'users.php' ) );
$expected_output = sprintf(
'<div class="notice notice-error"><p>%s <a href="%s">%s</a></p></div>',
sprintf(
// translators: %d: Number of users.
_n(
'There is %d user with high-privileges with Two-Factor Authentication disabled.',
'There are %d users with high-privileges with Two-Factor Authentication disabled.',
$expected_count,
'wpvip'
),
number_format_i18n( $expected_count )
),
esc_url( $filter_url ),
esc_html__( 'Filter list to show these users.', 'wpvip' )
);

ob_start();
Highlight_MFA_Users::display_mfa_disabled_notice();
$output = ob_get_clean();

$this->assertEquals( $expected_output, $output );

// No need to explicitly clean up $author_user_id, $contributor_user_id as WP_UnitTestCase handles factory users.
// No need to restore Highlight_MFA_Users::$roles as the next test's setUp() will call init() again.
}

/**
* Test that the init function hooks actions correctly.
*/
Expand Down