Allow the admin email verification capability to be filtered

[desrosj]

Currently, the capability used to determine if the admin email verification screen should be displayed is hard coded as manage_options.

There are scenarios where a plugin or site owner would want to change this capability.

[desrosj]

48153.diff​ introduces a new, admin_email_check_cap filter, and consolidates some documentation for the admin_email_check_interval filter.

[desrosj]

48153.2.diff​ fixes an opening parenthesis that should have been closing.

[Clorith]

This seems like a reasonable approach, and for many it would be expected that they could control this behavior from the start.

+1 to go ahead from me.

[desrosj]

In 46323:

Site Health: Allow the capability required for the site admin email verification screen to be filtered.

Props desrosj, Clorith.
Fixes #48153.

[desrosj]

In 46324:

Site Health: Consolidate documentation for the admin_email_check_interval filter.

See #48153.

[desrosj]

In 46325:

Docs: Correct typos introduced in [46324]

Props TimothyBlynJacobs.
See #48153.

[azaozz]

Looking at admin_email_check_cap filter, there is a somewhat high possibility for plugins to be "doing-it-wrong" there? All users that are redirected will have to have the manage_options capability or they won't be able to update the email address.

The intended functionality can be done by using the admin_email_check_interval filter and looking at the current user. If additional capabilities are required, the standard current_user_can() can be used as well. Then the redirect can be disabled on per-user basis by returning a "falsey" value, see https://core.trac.wordpress.org/browser/trunk/src/wp-login.php?rev=46325#L1246.

Perhaps would be better to update the docs and describe the user case in more details instead of a new filter?

[Clorith]

My thinking that had this make sense was that many folks do interesting things with filters and permissions as is, so having a filter to allow granular control of the warning was not unreasonable.

And having someone then use a time-related filter intended for an integer value, which admin_email_check_interval is, feels like a hacky solution.

[azaozz]

Replying to Clorith:

...having a filter to allow granular control of the warning was not unreasonable.

OK, lets add a direct "show/do not show" filter, after we check that the current user has the capability to change the email. Imho that will be a lot clearer.

With the current filter it is not clear what happens if the manage_options cap is changed. Would all users that have the other/custom cap also have manage_options? Would that ever change (i.e. a user with the custom cap may stop having access to manage options)?

Perhaps a "boolean" filter, something like current_user_can_update_admin_email or maybe show_admin_email_verification returning true by default?

[Clorith]

Very good thought, I like the suggestion of the last proposed filter there.

[afragen]

Do we need a refreshed patch before 5.3beta2?

[azaozz]

Yeah, [46323] needs to be reverted and new filter added. Also perhaps [46324] needs to be reverted. Having more inline comments is always better? :)

[azaozz]

Actually, if we have a dedicated filter it doesn't make sense to (re)use admin_email_check_interval for the same thing.

Patch coming up.

[azaozz]

In 48153.3.diff:

• Replace admin_email_check_cap with show_admin_email_verification filter.
• Update use and docs for admin_email_check_interval. No need to be able to use it for disabling the redirect to the verification screen.

[desrosj]

Minor docblock formatting changes

[desrosj]

48153.4.diff​ only contains a few documentation formatting fixes.

I like the general direction of this, but because the new filter is nested within a $user->has_cap( 'manage_options' ) check, a site owner is still unable to change the required capability for this feature.

I'm thinking for now, we revert [46323-46325] and solve after Beta 2 so a solid patch can be discussed. @azaozz does that work for you?

[azaozz]

Replying to desrosj:

Sure. Lets do that.

Generally we need to match the capabilities required to access Settings->General when redirecting to the email confirmation screen. That can be a bit tricky...

[desrosj]

In 46361:

Site Health: Revert [46323-46325] for further discussion of how the admin email verification should be filtered.

See #48153.

[azaozz]

Looking at this a bit more: we are also checking current_user_can( 'manage_options' ) before outputting the email verification screen. Even if we redirect a user without that capability (as in [46323]), they will not be able to see that screen, and will be redirected again to wp-admin/.

ToDo:

• Match the capability required to access Settings->General before redirecting to the email verification screen. This is just for the redirect, the user capabilities will be checked again before showing the verification screen and again before letting them change the admin email address (if selected).
• Add another filter for more granular control of who sees that screen. Something like show_admin_email_verification from 48153.4.diff would work well. Then plugins will be able to do additional capabilities checks before redirecting and limit access for users, or to completely disable showing of this screen. The same filter will need to be checked at the top before outputting the verification screen.

[peterwilsoncc]

Since we're nearing the 5.3 RC and it looks like there is a bit to discuss on this ticket, I'm moving it off the milestone.

I've added needs-unit-tests as the faux primitive will need to be added to the roles and caps unit tests. It looks like another test or two would be helpful to make sure users with the verification capability can access the admin screen too.