Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#40263 closed defect (bug) (fixed)

REST API: Allow site admins to edit user roles in multisite

Reported by: flixos90's profile flixos90 Owned by: flixos90's profile flixos90
Milestone: 4.9 Priority: normal
Severity: normal Version:
Component: REST API Keywords: has-patch, has-unit-tests, ms-roadmap, commit
Focuses: multisite Cc:

Description

In multisite, only network administrators can edit users. The REST API has that restriction in place implicitly, since the edit_user capability is automatically handled in map_meta_cap(), so that's good as is.

However, on multisite a site administrator should still be able to modify the roles of a user from their site. This is currently not possible through the REST API.

The dedicated capability promote_user (which is a meta capability that maps to promote_users by default) should be used to implement this functionality. This will return true for both site administrators and network administrators, while edit_user only works for the latter.

I suggest to adjust WP_REST_Users_Controller::update_item_permissions_check() as described:

  • Move the check for editing the user's roles above the check for editing the user and use current_user_can( 'promote_user', $user->ID ) instead of current_user_can( 'edit_users' ).
  • Only run the regular current_user_can( 'edit_user', $user->ID ) check if more parameters than id and roles are attached to the request. Otherwise we should be able to safely assume that this request is only for adjusting roles.

Some parts of the WP_REST_Users_Controller::update_item() method might need to be adjusted accordingly, but we can figure this out while working on a patch. Maybe that method won't even require any changes.

This ticket is part of the task defined in #39544.

Attachments (2)

40263.diff (2.5 KB) - added by flixos90 7 years ago.
40263.2.diff (4.4 KB) - added by jnylen0 7 years ago.
Moar tests; minor logic clarification

Download all attachments as: .zip

Change History (19)

#1 @flixos90
7 years ago

  • Milestone changed from Future Release to 4.8

I will put this to 4.8, but it may be possible that this is even eligible for a minor release since it's essentially a bugfix to the REST API. While other improvements to the users endpoint need to be implemented first and should be considered an enhancement, this one here appears to be a more straightforward issue.

It needs to be discussed thoroughly in combination with the other planned improvements though, so let's give it some time; with an option to bring it into 4.7.5 or similar. :)

#2 follow-up: @jnylen0
7 years ago

  • Keywords 2nd-opinion removed

I would like to continue to follow a simple rule when dealing with the users endpoint and multisite:

By default (in the absence of a special parameter like global=true), the REST API should behave as though multisite does not exist, and clients should not have to care about it.

So I agree with this change, because in general "a site administrator should be able to edit capabilities of a user on their site", even if behind the scenes this actually implies some "global" operations in a multisite context.

While other improvements to the users endpoint need to be implemented first ...

I don't understand that bit - can you clarify why this change can't be done in isolation from others discussed at #39544?

I agree with getting this change in sooner rather than later, but at least it's a broadening of existing functionality rather than a restriction, which helps with any backwards compatibility concerns.

#3 in reply to: ↑ 2 @flixos90
7 years ago

Replying to jnylen0:

While other improvements to the users endpoint need to be implemented first ...

I don't understand that bit - can you clarify why this change can't be done in isolation from others discussed at #39544?

I only meant that the other enhancements (mostly discussed in #39544) need more thought and time. This one here is different, since it allows for an easy fix - and it can also be considered a bug in the current implementation, thus qualifying for a 4.7.x release. To summarize, I think it _can_ happen in isolation from #39544. :)

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


7 years ago

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


7 years ago

@flixos90
7 years ago

#6 @flixos90
7 years ago

  • Keywords has-patch has-unit-tests added

40263.diff is a possible solution for the problem, that was previously discussed during multisite office-hours. The patch also includes a unit test that should actually pass, but fails due to another REST API bug that I just discovered while working on it. I opened #40704 for investing that bug.

I think we should either wait for that bug to be fixed (hopefully) or work our way around it in this very ticket (if the bugfix takes longer).

This ticket was mentioned in Slack in #core-restapi by flixos90. View the logs.


7 years ago

#8 @flixos90
7 years ago

  • Owner set to flixos90
  • Status changed from new to assigned

#9 @flixos90
7 years ago

  • Milestone changed from 4.8 to 4.8.1

As this is blocked by the bug in #40704, which will likely require some thorough investigation to be properly fixed, let's move this ticket to 4.8.1. It is still a 4.7 regression, so we should be able to deal with it in a minor milestone.

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


7 years ago

#11 @flixos90
7 years ago

  • Keywords ms-roadmap added

These tickets belong to our planned roadmap (a few of them not per final decision), so flagging with a keyword for better overview.

#12 @flixos90
7 years ago

  • Keywords changed from has-patch, has-unit-tests, ms-roadmap to has-patch has-unit-tests ms-roadmap
  • Milestone changed from 4.8.1 to 4.9

This won't get ready for 4.8.1, but we should have a look soon.

@jnylen0
7 years ago

Moar tests; minor logic clarification

#13 @jnylen0
7 years ago

  • Keywords commit added
  • Milestone changed from 4.9 to 4.8.1

40263.2.diff adds a couple more tests for the errors and for the moderately tricky logic branch that verifies that only the id and roles parameters are set, and also clarifies this logic and adds a comment.

The updated patch runs the new tests in single-site mode as well, which we can and should do. The behavior in single-site mode before and after this patch is almost exactly the same (rest_cannot_edit changes to rest_cannot_edit_roles if only a roles update is attempted, which is fine).

I think this is fine to ship in 4.8.1 (as noted earlier in the ticket, #40704 needs to be committed first).

Last edited 7 years ago by jnylen0 (previous) (diff)

#14 @jnylen0
7 years ago

  • Milestone changed from 4.8.1 to 4.9

Moving back to 4.9 along with #40704; see comment:5:ticket:40704

#15 @flixos90
7 years ago

  • Keywords needs-dev-note added

This should be part of the multisite dev note for 4.9.

#16 @flixos90
7 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 41226:

REST API: Allow site administrators to edit user roles in multisite.

While site administrators cannot generally edit users in multisite, they have always been able to change the roles of users on their site. In the REST API however, this has not been possible so far. This changeset brings parity with how it is handled in the administration panel: A REST request to edit only a user's roles succeeds correctly, while a REST request to edit any further details of a user fails.

Props jnylen0.
Fixes #40263.

#17 @jeremyfelt
6 years ago

  • Keywords needs-dev-note removed
Note: See TracTickets for help on using tickets.