Add filter to prevent the addition of a user to a blog

[jmdodd]

In some multisite instances, we may want to establish a policy that only certain users may be added to a given blog.

The current option available for preventing the addition of a user to a given blog is hooking into the add_user_to_blog action and undoing the user addition after it has already taken place. This is problematic because the user has already been added to the blog and any related actions may have already fired, depending on their priority.

This introduces a filter to allow a plugin to override the addition of a user to a given blog. There is already precedent in add_user_to_blog() for returning a WP_Error if the user does not exist.

[jmdodd]

Proposed filter and unit tests

[jorbin]

Should the filters be removed after being used in the test to prevent leakage between tests? Right now, it all works because these two tests run back to back, but I don't think that's something we should rely on.

[flixos90]

@jorbin As far as I'm aware, the core test case class resets all hooks automatically, so that shouldn't be a problem. However the calls to wpmu_delete_blog() can be removed since the addition queries are not committed by MySQL. Might need to double-check that, so correct me if I'm wrong.

[jmdodd]

Proposed filter and updated unit tests

[jmdodd]

I've updated the diff to include better cleanup of individual tests as seen in test_add_user_to_blog_subscriber().

[desrosj]

@flixos90 looks like that is correct. See https://core.trac.wordpress.org/browser/trunk/tests/phpunit/includes/testcase.php#L301. Not sure if we should always remove the hooks in the test function, or just rely on that function and avoid unneeded calls to remove_action() and remove_filter().

Looking at the most recent patch @jmdodd, the filter needs a PHPDocBlock.

I did some testing, and found a few issues with how it works. To test, I used add_filter( 'pre_add_user_to_blog', '__return_false', 10, 4 );.

Add Existing User Form

• In the network site admin add user screen, the filter correctly prevents the user from being added. However, a success message is incorrectly displayed in the admin Shot 2017-07-01 at 12.20.19 PM.png.
• The add user screen in the site admin displays a similar success message, even though the user is not added to the site Shot 2017-07-01 at 12.26.20 PM.png. This is when "Add the user without sending an email that requires their confirmation." is checked. When it is not checked, the activation email is sent out. Clicking the link brings you to a page with this warning: Shot 2017-07-01 at 12.29.00 PM.png. Ideally, this email should not go out.

Add New User Form

• In the network admin, submitting this form shows a success message. The user does not show in the table. An activation email is sent out, and the user is created when the link is clicked, but the user is never added to the site.
• In the site admin, the experience is the same.

[flixos90]

@desrosj

Not sure if we should always remove the hooks in the test function, or just rely on that function and avoid unneeded calls to remove_action() and remove_filter().

I know several tests do that manually, but that might be old code or have been overlooked. Removing hooks should be omitted as it's an unnecessary step in tests.

Thanks for testing this in detail. I agree that the functionality related to adding a user to a site needs to be adjusted as well in order to for this change to be viable for core.

[jmdodd]

Site-level user additions/edits can already be blocked using the existing action user_profile_update_errors in edit_user() (wp-admin/includes/user.php) with code like this:

<?php
function dev_user_profile_update_errors( $errors, $update, $user ) {
        $errors->add( 'nope', 'Not happening.' );
}
add_action( 'user_profile_update_errors', 'dev_user_profile_update_errors', 10, 3 );

I've added additional logic around multisite network additions to allow plugins to either display a custom error message, or to use a more generic default message.

[stephdau]

@flixos90 @desrosj : mind taking a peek at @jmdodd's latest patch? Seems pretty solid to me.

[flixos90]

The patch looks good indeed at first sight, I'll make time for a more thorough review a bit later. Also moving this to 4.9, since it looks close to ready.

[flixos90]

Thanks for working on this @jmdodd. The patch works well, I tested it thoroughly. There are only a few things I tweaked in 41101.3.diff​ - most of them minor, though particularly the user-facing messages needed a few adjustments (due to some terrible WP Admin code that was there before):

• Ensure that the error message when adding an existing user to a site (in wp-admin/user-new.php) shows with the error color (red) instead of success color (green). This was actually wrong for a few other existing messages as well, so I adjusted this too.
• Correctly handle the case where a new user is being created that cannot be added to the site though (in wp-admin/user-new.php). This was a bit tricky since there is no way to detect whether add_user_to_blog() fails from wpmu_activate_signup(). I added a check is_user_member_of_blog() afterwards to detect possible errors. It's hacky, but probably the best we can do at this point. A message "User has been created, but could not be added to this site." is shown under these circumstances.
• Use the correct term "site" instead of "blog" in the default error message in add_user_to_blog().
• Remove the statements to delete temporary sites and users from the tests, since that happens automatically as these SQL queries are not committed.
• Add 4.9 to the new filter doc block.
• Minor changes in the new filter documentation.
• Minor code style tweaks to make the code better readable, particularly when using is_wp_error() around function calls.

Let's get this merged!

[flixos90]

41101.4.diff​ adds an extra test for the new add_user_to_blog() failure behavior through the REST API.

[flixos90]

In 41225:

Multisite: Introduce a can_add_user_to_blog filter to prevent adding a user to a site.

Under certain circumstances, it can be necessary that a user should not be added to a site, beyond the restrictions that WordPress core applies. With the new can_add_user_to_blog filter, plugin developers can run custom checks and return an error in case of a failure, that will prevent the user from being added.

The user-facing parts and the REST API route that interact with add_user_to_blog() have been adjusted accordingly to provide appropriate error feedback when a user could not be added to a site. Furthermore, two existing error feedback messages in the site admin's "New User" screen have been adjusted to properly show inside an error notice instead of a success notice.

Props jmdodd.
Fixes #41101.

[flixos90]

Per @johnjamesjacoby (​https://twitter.com/JJJ/status/902722123898253314), there is an action hook wrapped in a ! is_wp_error() check, although it also accepts a WP_Error.

How do we solve this? Does it make more sense removing the clause, or only running the hook when there isn't a WP_Error. The first is more backward-compatible, the latter makes more sense for what the hook name indicates IMO (if there's an error, the user was _not_ added to the site).

[jeremyfelt]

I think we're okay removing the wrap and passing $result to the action whether it's an error or not. It's been like that since MU, so back-compat is good to keep here even though the name is a little strange. :)

[flixos90]

In 41324:

Multisite: Remove an unnecessary if clause wrapping the added_existing_user filter.

This was accidentally introduced in [41125]. Since the added_existing_user filter has historically accepted either true or a WP_Error object, the clause is not necessary here. The doc block has been adjusted to account for the new possible WP_Error condition.

Fixes #41101.