Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#38526 closed defect (bug) (fixed)

REST Multisite new user is not assigned to any site

Reported by: compute's profile Compute Owned by: rmccue's profile rmccue
Milestone: 4.7 Priority: normal
Severity: major Version: 4.7
Component: REST API Keywords: has-patch has-unit-tests commit
Focuses: multisite Cc:

Description

Hi there

Creating a new user through the API on a multisite results in a user not assigned to any website. I know the focus on the REST API have not focused mainly on the multisite part of things but would the expected behavior not be to assign the user to the site that the request has been passed to?

Example:

POST http://example.com/wp-json/wp/v2/users will add the user to the example.com site
POST http://test example.com/wp-json/wp/v2/users will add the user to the test.example.com site

And so on.

As users are the thing shared across multisite I think that this would be an important change as otherwise creating new users on multisites will be kinda meaningless at the moment.

Attachments (3)

multisite-rest-user.png (153.3 KB) - added by Compute 8 years ago.
new user through rest api on multisite
38526.diff (817 bytes) - added by jeremyfelt 8 years ago.
38526.2.diff (6.7 KB) - added by jeremyfelt 8 years ago.

Download all attachments as: .zip

Change History (23)

@Compute
8 years ago

new user through rest api on multisite

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


8 years ago

#3 @ocean90
8 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.7
  • Severity changed from normal to major
  • Type changed from enhancement to defect (bug)
  • Version 4.6.1 deleted

Moving to 4.7 because I think that this needs to be fixed. At a glance it looks like a add_user_to_blog() call is missing in WP_REST_Users_Controller::create_item().

Noted that already on Oct 19th, see https://wordpress.slack.com/archives/core-multisite/p1476912979001644.

cc: @jeremyfelt, @flixos90

#4 @jeremyfelt
8 years ago

Agreed. I had first thought that it would be confusing to provide add_user_to_blog() in create_item(), but after looking closer at delete_item(), the user is only removed from a site (via wp_delete_user() and not deleted from the network. It probably makes sense to align the expected behaviors here.

The harder part will be figuring out (in 4.x) the right endpoint to assign existing network users to individual sites. Right now if you use create_item() on an existing username, wpmu_validate_user_signup() will kick back an error.

#5 @jeremyfelt
8 years ago

In WP_REST_Users_Controller::create_item(), on single site, a user is first created and then any roles passed with the REST request are assigned.

In add_user_to_blog(), a single role is expected with the user.

We could shift $request['roles'] and pass that to add_user_to_blog() and then let the current loop handle any remaining roles.

#6 follow-up: @jeremyfelt
8 years ago

I think I misunderstood the original issue at some level.

In the current state, if I create a new user via POST src.wordpress-develop.dev/site-two/wp-json/wp/v2/users, the user is added to the network and the site as expected.

If I attempt to use a create request for a user that already exists on the network, an error is returned.

This seems like something that should then defer to WP_REST_Users_Controller::update_item(), which can be used to update the user's roles on a site from none to whatever is passed.

So for the client:

  • Request 1: Create user, error response - user_name
  • Request 2: Update existing user with roles, success.

Though the request is successful, there are some side effects from add_user_to_blog() never actually firing:

  1. The primary_blog and source_domain meta for a user may not be set.
  2. The add_user_to_blog action does not fire.
  3. The users and _user_count cache are not cleared.

We can probably make use of add_to_blog() in both create_item() and update_item() without much trouble.

#7 in reply to: ↑ 6 @rmccue
8 years ago

Our consensus here was to punt this to 4.8; as @jeremyfelt notes, creating a user does work, it's only adding them to a site that doesn't. This is at least somewhat intentional: the user already exists on the network (with an ID), what you're really doing to trying to grant them a role on a site. That's an update, which needs to be a PUT to the resource, not a POST.

Replying to jeremyfelt:

This seems like something that should then defer to WP_REST_Users_Controller::update_item(), which can be used to update the user's roles on a site from none to whatever is passed.

This should work, I think? Apart from:

Though the request is successful, there are some side effects from add_user_to_blog() never actually firing:

  1. The primary_blog and source_domain meta for a user may not be set.
  2. The add_user_to_blog action does not fire.
  3. The users and _user_count cache are not cleared.

We can probably make use of add_to_blog() in both create_item() and update_item() without much trouble.

I'd be happy to do that here, although we need to consider the API in the context of multisite more thoroughly in the next release. I don't think we'd be locking ourselves down by doing this.

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


8 years ago

#9 @rmccue
8 years ago

  • Owner set to jeremyfelt
  • Status changed from new to assigned
  • Version set to trunk

@jeremyfelt
8 years ago

#10 @jeremyfelt
8 years ago

In 38526.diff:

  • In create_item(), when is_multisite(), fire add_user_to_blog() with an empty string for role. This allows the existing logic to add any passed roles.
  • In update_item(), when is_multisite() and not a member of the site, fire add_user_to_blog() with an empty string for role.

I have not added any tests yet. Is this an appropriate approach?

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


8 years ago

@jeremyfelt
8 years ago

#12 @jeremyfelt
8 years ago

38526.2.diff adds tests asserting current expectations.

In the process of creating these I ran into a quirk with how get_rest_url() handles the $path argument and how WP_Test_REST_Controller_Testcase filters it on every setup. For now I've set it to skip filtering when is_multisite() as our tests should be written to include the leading slash anyway.

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


8 years ago

#14 @jeremyfelt
8 years ago

  • Keywords has-patch has-unit-tests added; needs-patch removed

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


8 years ago

#16 @jeremyfelt
8 years ago

  • Keywords commit added
  • Owner changed from jeremyfelt to rmccue
  • Status changed from assigned to reviewing

Handing back to @rmccue for review/commit.

#17 @rmccue
8 years ago

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

In 39177:

REST API: Fire correct hooks when creating users on multiste.

add_user_to_blog() is now called, ensuring the correct hooks are called, along with setting the primary blog and clearing relevant caches.

Props jeremyfelt.
Fixes #38526.

#18 @Compute
8 years ago

@jeremyfelt Is there a reason for using a mix of get_site()->id and get_current_blog_id() in https://core.trac.wordpress.org/attachment/ticket/38526/38526.2.diff?

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


8 years ago

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


8 years ago

Note: See TracTickets for help on using tickets.