WordPress.org

Make WordPress Core

Opened 3 weeks ago

Last modified 5 days 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: trunk
Component: Site Health Keywords: has-patch needs-refresh needs-unit-tests
Focuses: administration Cc:
PR Number:

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 3 weeks ago.
48153.2.diff (1.5 KB) - added by desrosj 3 weeks ago.
48153.3.diff (2.8 KB) - added by azaozz 2 weeks ago.
48153.4.diff (4.0 KB) - added by desrosj 2 weeks ago.
Minor docblock formatting changes

Download all attachments as: .zip

Change History (29)

@desrosj
3 weeks ago

#1 @desrosj
3 weeks ago

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

@desrosj
3 weeks ago

#2 @desrosj
3 weeks 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.


3 weeks ago

#4 @Clorith
3 weeks 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
3 weeks ago

  • Keywords commit added; 2nd-opinion removed

#6 @desrosj
3 weeks 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
3 weeks ago

In 46324:

Site Health: Consolidate documentation for the admin_email_check_interval filter.

See #48153.

#8 @desrosj
3 weeks ago

In 46325:

Docs: Correct typos introduced in [46324]

Props TimothyBlynJacobs.
See #48153.

#9 @azaozz
3 weeks 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 3 weeks ago by azaozz (previous) (diff)

#10 follow-up: @Clorith
3 weeks 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
3 weeks 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 3 weeks ago by azaozz (previous) (diff)

#12 @Clorith
2 weeks ago

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

#13 @afragen
2 weeks 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.


2 weeks ago

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


2 weeks ago

#16 @azaozz
2 weeks 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
2 weeks 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
2 weeks ago

#18 @azaozz
2 weeks 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 2 weeks ago by azaozz (previous) (diff)

@desrosj
2 weeks ago

Minor docblock formatting changes

#19 follow-up: @desrosj
2 weeks 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
2 weeks 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
2 weeks ago

In 46361:

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

See #48153.

#22 @azaozz
2 weeks 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 2 weeks ago by azaozz (previous) (diff)

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


8 days ago

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


5 days ago

#25 @peterwilsoncc
5 days 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.