Skip to content

Commit c8c9488

Browse files
andrea-sdlandrea-sdl
authored
Merge pull request #28 from Automattic/add/PLTFRM-962-notice-text
Improve Higlight-MFA Module notice to specify the affected roles
2 parents 64491a6 + cf48a0d commit c8c9488

File tree

2 files changed

+200
-31
lines changed

2 files changed

+200
-31
lines changed

‎modules/highlight-mfa-users/class-highlight-mfa-users.php

Lines changed: 95 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,9 @@
44
use function Automattic\VIP\Security\Utils\get_module_configs;
55

66
class Highlight_MFA_Users {
7-
const MFA_SKIP_USER_IDS_OPTION_KEY = 'vip_security_mfa_skip_user_ids';
8-
const ROLE_COLUMN_KEY = 'role';
7+
const MFA_SKIP_USER_IDS_OPTION_KEY = 'vip_security_mfa_skip_user_ids';
8+
const ROLE_COLUMN_KEY = 'role';
9+
const DEFAULT_ADMIN_EDITOR_ROLE_SLUGS = [ 'administrator', 'editor' ];
910

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

2223
if ( ! is_array( self::$roles ) ) {
2324
self::$roles = [ self::$roles ];
2425
}
2526
self::$roles = array_filter( self::$roles );
2627
// If after filtering, the array is empty, default back to administrator and editor
2728
if ( empty( self::$roles ) ) {
28-
self::$roles = [ 'administrator', 'editor' ];
29+
self::$roles = self::DEFAULT_ADMIN_EDITOR_ROLE_SLUGS;
2930
}
3031

3132
add_action( 'admin_notices', [ __CLASS__, 'display_mfa_disabled_notice' ] );
@@ -66,6 +67,14 @@ public static function display_mfa_disabled_notice() {
6667
return;
6768
}
6869

70+
// Determine notice text based on role configuration
71+
// self::$roles is already an array and populated from init().
72+
$configured_roles_for_comparison = self::$roles;
73+
\sort( $configured_roles_for_comparison ); // Use global sort
74+
75+
// unordered array check with ==
76+
$is_default_config = ( self::DEFAULT_ADMIN_EDITOR_ROLE_SLUGS === $configured_roles_for_comparison || empty( $configured_roles_for_comparison ) );
77+
6978
$skipped_user_ids = get_option( self::MFA_SKIP_USER_IDS_OPTION_KEY, [] );
7079
if ( ! is_array( $skipped_user_ids ) ) {
7180
$skipped_user_ids = [];
@@ -100,45 +109,104 @@ public static function display_mfa_disabled_notice() {
100109
$is_filtered = isset( $_GET['filter_mfa_disabled'] ) && '1' === $_GET['filter_mfa_disabled'];
101110

102111
if ( $is_filtered ) {
103-
// Display notice for when the list IS filtered
104-
$show_all_url = remove_query_arg( 'filter_mfa_disabled', admin_url( 'users.php' ) );
112+
// Display info notice for when the list IS filtered
113+
$show_all_url = remove_query_arg( 'filter_mfa_disabled', admin_url( 'users.php' ) );
114+
$notice_message_text = self::get_missing_mfa_notice_message_text( $mfa_disabled_count, $is_default_config );
115+
105116
printf(
106117
'<div class="notice notice-info"><p>%s <a href="%s">%s</a></p></div>',
107-
esc_html( sprintf(
108-
/* Translators: %d is the number of users without 2FA enabled being shown in the filtered list. */
109-
_n(
110-
'Showing %d user without Two-Factor Authentication enabled.',
111-
'Showing %d users without Two-Factor Authentication enabled.',
112-
$mfa_disabled_count,
113-
'wpvip'
114-
),
115-
number_format_i18n( $mfa_disabled_count )
116-
) ),
118+
esc_html( $notice_message_text ),
117119
esc_url( $show_all_url ),
118120
esc_html__( 'Show all users.', 'wpvip' )
119121
);
120122
} else {
121123
// Display the original notice when the list is NOT filtered
122-
$filter_url = add_query_arg( 'filter_mfa_disabled', '1', admin_url( 'users.php' ) );
124+
$filter_url = add_query_arg( 'filter_mfa_disabled', '1', admin_url( 'users.php' ) );
125+
$notice_message_text = self::get_filtering_mfa_info_message_text( $mfa_disabled_count, $is_default_config );
126+
123127
printf(
124128
'<div class="notice notice-error"><p>%s <a href="%s">%s</a></p></div>',
125-
esc_html( sprintf(
126-
/* Translators: %d is the number of users without 2FA enabled. */
127-
_n(
128-
'There is %d user with Two-Factor Authentication disabled.',
129-
'There are %d users with Two-Factor Authentication disabled.',
130-
$mfa_disabled_count,
131-
'wpvip'
132-
),
133-
number_format_i18n( $mfa_disabled_count )
134-
) ),
129+
esc_html( $notice_message_text ),
135130
esc_url( $filter_url ),
136131
esc_html__( 'Filter list to show these users.', 'wpvip' )
137132
);
138133
}
139134
}
140135
}
141136

137+
/**
138+
* Get the notice message text for when users with missing MFA are shown.
139+
*
140+
* @param int $mfa_disabled_count The number of users with missing MFA.
141+
* @param bool $is_default_config Whether the default roles are being used.
142+
* @return string The notice message text.
143+
*/
144+
protected static function get_missing_mfa_notice_message_text( $mfa_disabled_count, $is_default_config ) {
145+
$notice_message_text = '';
146+
if ( $is_default_config ) {
147+
// Default roles: Administrator, Editor
148+
$notice_message_text = sprintf(
149+
/* Translators: %d is the number of users with Administrator or Editor roles without 2FA enabled being shown. */
150+
_n(
151+
'Showing %d user with Administrator or Editor roles without Two-Factor Authentication enabled.',
152+
'Showing %d users with Administrator or Editor roles without Two-Factor Authentication enabled.',
153+
$mfa_disabled_count,
154+
'wpvip'
155+
),
156+
number_format_i18n( $mfa_disabled_count )
157+
);
158+
} else {
159+
// Custom roles
160+
$notice_message_text = sprintf(
161+
/* Translators: %d is the number of users with high-privileges without 2FA enabled being shown. */
162+
_n(
163+
'Showing %d user with high-privileges without Two-Factor Authentication enabled.',
164+
'Showing %d users with high-privileges without Two-Factor Authentication enabled.',
165+
$mfa_disabled_count,
166+
'wpvip'
167+
),
168+
number_format_i18n( $mfa_disabled_count )
169+
);
170+
}
171+
172+
return $notice_message_text;
173+
}
174+
175+
/**
176+
* Get the notice message text for when users with missing MFA are filtered.
177+
*
178+
* @param int $mfa_disabled_count The number of users with missing MFA.
179+
* @param bool $is_default_config Whether the default roles are being used.
180+
* @return string The notice message text.
181+
*/
182+
protected static function get_filtering_mfa_info_message_text( $mfa_disabled_count, $is_default_config ) {
183+
$notice_message_text = '';
184+
if ( $is_default_config ) {
185+
$notice_message_text = sprintf(
186+
/* Translators: %d is the number of users with Administrator or Editor roles and 2FA disabled. */
187+
_n(
188+
'There is %d user with Administrator or Editor roles with Two-Factor Authentication disabled.',
189+
'There are %d users with Administrator or Editor roles with Two-Factor Authentication disabled.',
190+
$mfa_disabled_count,
191+
'wpvip'
192+
),
193+
number_format_i18n( $mfa_disabled_count )
194+
);
195+
} else {
196+
$notice_message_text = sprintf(
197+
/* Translators: %d is the number of high-privilege users with 2FA disabled. */
198+
_n(
199+
'There is %d user with high-privileges with Two-Factor Authentication disabled.',
200+
'There are %d users with high-privileges with Two-Factor Authentication disabled.',
201+
$mfa_disabled_count,
202+
'wpvip'
203+
),
204+
number_format_i18n( $mfa_disabled_count )
205+
);
206+
}
207+
return $notice_message_text;
208+
}
209+
142210
/**
143211
* Modify the user query on the Users page to filter by MFA status if requested.
144212
* @param \WP_User_Query $query The WP_User_Query instance (passed by reference).

‎tests/phpunit/test-highlight-mfa-users.php

Lines changed: 105 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -228,8 +228,8 @@ public function test_display_mfa_disabled_notice_shows_when_needed() {
228228
sprintf(
229229
// translators: %d: Number of users.
230230
_n(
231-
'There is %d user with Two-Factor Authentication disabled.',
232-
'There are %d users with Two-Factor Authentication disabled.',
231+
'There is %d user with Administrator or Editor roles with Two-Factor Authentication disabled.',
232+
'There are %d users with Administrator or Editor roles with Two-Factor Authentication disabled.',
233233
$expected_count,
234234
'wpvip'
235235
),
@@ -264,8 +264,8 @@ public function test_display_mfa_disabled_notice_shows_correct_message_when_filt
264264
sprintf(
265265
// translators: %d: Number of users.
266266
_n(
267-
'Showing %d user without Two-Factor Authentication enabled.',
268-
'Showing %d users without Two-Factor Authentication enabled.',
267+
'Showing %d user with Administrator or Editor roles without Two-Factor Authentication enabled.',
268+
'Showing %d users with Administrator or Editor roles without Two-Factor Authentication enabled.',
269269
$expected_count,
270270
'wpvip'
271271
),
@@ -324,6 +324,107 @@ public function test_display_mfa_disabled_notice_hides_when_no_disabled_users()
324324
$this->assertEmpty( $output );
325325
}
326326

327+
/**
328+
* Test that the admin notice (filtered view) displays correctly for custom high-privilege roles.
329+
*/
330+
public function test_display_mfa_disabled_notice_shows_correct_filtered_message_for_custom_roles() {
331+
$this->set_admin_screen_users();
332+
$_GET['filter_mfa_disabled'] = '1'; // Activate the filter
333+
334+
// Set custom roles using reflection
335+
$reflection_class = new \ReflectionClass( Highlight_MFA_Users::class );
336+
$roles_property = $reflection_class->getProperty( 'roles' );
337+
$roles_property->setAccessible( true );
338+
$custom_roles = [ 'author', 'contributor' ];
339+
$roles_property->setValue( null, $custom_roles );
340+
341+
// Create users with custom roles
342+
$author_user_id = $this->factory()->user->create( [ 'role' => 'author' ] );
343+
$contributor_user_id = $this->factory()->user->create( [ 'role' => 'contributor' ] );
344+
345+
// Existing users (admin_user_mfa_disabled_id, editor_user_id, user ID 1) should not be counted
346+
// as 'administrator' and 'editor' are not in $custom_roles for this test.
347+
348+
$expected_count = 2; // The author and contributor created above.
349+
$show_all_url = remove_query_arg( 'filter_mfa_disabled', admin_url( 'users.php' ) );
350+
$expected_output = sprintf(
351+
'<div class="notice notice-info"><p>%s <a href="%s">%s</a></p></div>',
352+
sprintf(
353+
// translators: %d: Number of users.
354+
_n(
355+
'Showing %d user with high-privileges without Two-Factor Authentication enabled.',
356+
'Showing %d users with high-privileges without Two-Factor Authentication enabled.',
357+
$expected_count,
358+
'wpvip'
359+
),
360+
number_format_i18n( $expected_count )
361+
),
362+
esc_url( $show_all_url ),
363+
esc_html__( 'Show all users.', 'wpvip' )
364+
);
365+
366+
ob_start();
367+
Highlight_MFA_Users::display_mfa_disabled_notice();
368+
$output = ob_get_clean();
369+
370+
$this->assertEquals( $expected_output, $output );
371+
372+
unset( $_GET['filter_mfa_disabled'] ); // Clean up
373+
}
374+
375+
/**
376+
* Test that the admin notice displays correctly for custom high-privilege roles.
377+
*/
378+
public function test_display_mfa_disabled_notice_shows_for_custom_roles() {
379+
$this->set_admin_screen_users();
380+
381+
// Set custom roles using reflection
382+
$reflection_class = new \ReflectionClass( Highlight_MFA_Users::class );
383+
$roles_property = $reflection_class->getProperty( 'roles' );
384+
$roles_property->setAccessible( true );
385+
// Highlight_MFA_Users::init() in setUp already set roles to default.
386+
// We are overriding them here for this specific test.
387+
$custom_roles = [ 'author', 'contributor' ];
388+
$roles_property->setValue( null, $custom_roles );
389+
390+
// Create users with custom roles
391+
$author_user_id = $this->factory()->user->create( [ 'role' => 'author' ] );
392+
// Ensure this user is MFA disabled (default for mock unless added to Two_Factor_Core::$mock_enabled_user_ids)
393+
394+
$contributor_user_id = $this->factory()->user->create( [ 'role' => 'contributor' ] );
395+
// Ensure this user is MFA disabled
396+
397+
// Existing users (admin_user_mfa_disabled_id, editor_user_id, user ID 1) should not be counted
398+
// as 'administrator' and 'editor' are not in $custom_roles for this test.
399+
400+
$expected_count = 2; // The author and contributor created above.
401+
$filter_url = add_query_arg( 'filter_mfa_disabled', '1', admin_url( 'users.php' ) );
402+
$expected_output = sprintf(
403+
'<div class="notice notice-error"><p>%s <a href="%s">%s</a></p></div>',
404+
sprintf(
405+
// translators: %d: Number of users.
406+
_n(
407+
'There is %d user with high-privileges with Two-Factor Authentication disabled.',
408+
'There are %d users with high-privileges with Two-Factor Authentication disabled.',
409+
$expected_count,
410+
'wpvip'
411+
),
412+
number_format_i18n( $expected_count )
413+
),
414+
esc_url( $filter_url ),
415+
esc_html__( 'Filter list to show these users.', 'wpvip' )
416+
);
417+
418+
ob_start();
419+
Highlight_MFA_Users::display_mfa_disabled_notice();
420+
$output = ob_get_clean();
421+
422+
$this->assertEquals( $expected_output, $output );
423+
424+
// No need to explicitly clean up $author_user_id, $contributor_user_id as WP_UnitTestCase handles factory users.
425+
// No need to restore Highlight_MFA_Users::$roles as the next test's setUp() will call init() again.
426+
}
427+
327428
/**
328429
* Test that the init function hooks actions correctly.
329430
*/

0 commit comments

Comments
 (0)