Skip to content

Show the MFA notice only for users with manage_options capabilities#27

Merged
andrea-sdl merged 3 commits intoproductionfrom
add/PLTFRM-973-notice-admins
Jun 9, 2025
Merged

Show the MFA notice only for users with manage_options capabilities#27
andrea-sdl merged 3 commits intoproductionfrom
add/PLTFRM-973-notice-admins

Conversation

@andrea-sdl
Copy link
Copy Markdown
Contributor

Description

Currently, the Highlight MFA module is displaying the notice for every user landing on the user list.
This could create confusion because that's not something that every user (think subscribers) can act upon.

This PR changes the logic so that we will show the notice only if the logged in user has manage_options capabilities.

Changelog Description

Added

  • MFA Notice is now only displayed for users with manage_options capabilities ("admins")

Pre-review checklist

Please make sure the items below have been covered before requesting a review:

  • This change works and has been tested locally or in Codespaces (or has an appropriate fallback).
  • This change has relevant unit tests (if applicable).
  • This change has relevant documentation additions / updates (if applicable).
  • I've created a changelog description that aligns with the provided examples.

Pre-deploy checklist

  • VIP staff: Ensure any alerts added/updated conform to internal standards (see internal documentation).

Steps to Test

  1. Check out PR.
  2. Log in as admin and go to the user list, ensure you see the notice (if you have users without MFA)
  3. Log in as editor and the notice shouldn't be there anymore.

Copilot AI review requested due to automatic review settings June 6, 2025 13:16
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR restricts the display of the MFA notice to only users with the "manage_options" capability, ensuring that subscribers or non-admin users are not confused by seeing an inapplicable notice.

  • Updated the logic in Highlight_MFA_Users::display_mfa_disabled_notice() to check for admin capabilities.
  • Added a new PHPUnit test (test_display_mfa_disabled_notice_does_not_show_when_not_admin) to verify that non-admin users do not see the notice.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
tests/phpunit/test-highlight-mfa-users.php Added test for non-admin behaviour and updated current user reset in tearDown.
modules/highlight-mfa-users/class-highlight-mfa-users.php Implemented a capability check to display the MFA notice only for admins.
Comments suppressed due to low confidence (1)

tests/phpunit/test-highlight-mfa-users.php:199

  • The doc comment for test_display_mfa_disabled_notice_does_not_show_when_not_admin() should clearly indicate that it verifies the notice is not shown for non-admin users. Consider updating the comment to accurately reflect the test scenario.
/** Test that the admin notice is displayed correctly when MFA-disabled admins exist.

@andrea-sdl andrea-sdl requested a review from a team June 6, 2025 13:18


/**
* Test that the admin notice is displayed correctly when MFA-disabled admins exist.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Minor: I think you meant "when the user is not an admin" since we're testing with the editor role here.

Copy link
Copy Markdown
Contributor

@brunobasto brunobasto left a comment

Choose a reason for hiding this comment

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

Looks great!

@andrea-sdl andrea-sdl added this pull request to the merge queue Jun 9, 2025
Merged via the queue into production with commit 64491a6 Jun 9, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants