WordPress.org

Make WordPress Core

Opened 5 months ago

Last modified 4 months ago

#48153 reopened defect (bug)

Allow the admin email verification capability to be filtered

Reported by: desrosj Owned by: desrosj
Milestone: Future Release Priority: normal
Severity: normal Version: 5.3
Component: Site Health Keywords: has-patch needs-refresh needs-unit-tests
Focuses: administration Cc:

Description

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.

Attachments (4)

48153.diff (1.5 KB) - added by desrosj 5 months ago.
48153.2.diff (1.5 KB) - added by desrosj 5 months ago.
48153.3.diff (2.8 KB) - added by azaozz 5 months ago.
48153.4.diff (4.0 KB) - added by desrosj 5 months ago.
Minor docblock formatting changes

Download all attachments as: .zip

Change History (29)

@desrosj
5 months ago

#1 @desrosj
5 months ago

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

@desrosj
5 months ago

#2 @desrosj
5 months ago

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

This ticket was mentioned in Slack in #core by desrosj. View the logs.


5 months ago

#4 @Clorith
5 months ago

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.

#5 @desrosj
5 months ago

  • Keywords commit added; 2nd-opinion removed

#6 @desrosj
5 months ago

  • Owner set to desrosj
  • Resolution set to fixed
  • Status changed from new to closed

In 46323:

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

Props desrosj, Clorith.
Fixes #48153.

#7 @desrosj
5 months ago

In 46324:

Site Health: Consolidate documentation for the admin_email_check_interval filter.

See #48153.

#8 @desrosj
5 months ago

In 46325:

Docs: Correct typos introduced in [46324]

Props TimothyBlynJacobs.
See #48153.

#9 @azaozz
5 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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?

Last edited 5 months ago by azaozz (previous) (diff)

#10 follow-up: @Clorith
5 months ago

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.

#11 in reply to: ↑ 10 @azaozz
5 months ago

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?

Last edited 5 months ago by azaozz (previous) (diff)

#12 @Clorith
5 months ago

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

#13 @afragen
5 months ago

Do we need a refreshed patch before 5.3beta2?

This ticket was mentioned in Slack in #core-site-health by afragen. View the logs.


5 months ago

This ticket was mentioned in Slack in #core by afragen. View the logs.


5 months ago

#16 @azaozz
5 months ago

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

#17 @azaozz
5 months ago

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
5 months ago

#18 @azaozz
5 months ago

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.
Last edited 5 months ago by azaozz (previous) (diff)

@desrosj
5 months ago

Minor docblock formatting changes

#19 follow-up: @desrosj
5 months ago

  • Keywords commit removed

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?

#20 in reply to: ↑ 19 @azaozz
5 months ago

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...

#21 @desrosj
5 months ago

In 46361:

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

See #48153.

#22 @azaozz
5 months ago

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.
Last edited 5 months ago by azaozz (previous) (diff)

This ticket was mentioned in Slack in #core-site-health by afragen. View the logs.


5 months ago

This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.


4 months ago

#25 @peterwilsoncc
4 months ago

  • Keywords needs-refresh needs-unit-tests added
  • Milestone changed from 5.3 to Future Release

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.

Note: See TracTickets for help on using tickets.