Opened 5 years ago
Closed 4 years ago
#41101 closed enhancement (fixed)
Add filter to prevent the addition of a user to a blog
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 4.9 | Priority: | normal |
Severity: | normal | Version: | 4.9 |
Component: | Users | Keywords: | has-patch has-unit-tests commit |
Focuses: | multisite | Cc: |
Description
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.
Attachments (9)
Change History (24)
#2
@
5 years ago
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.
#3
@
5 years ago
@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.
#4
@
5 years ago
I've updated the diff to include better cleanup of individual tests as seen in test_add_user_to_blog_subscriber()
.
#5
@
5 years ago
- Keywords needs-testing added
@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.
#6
@
5 years ago
@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()
andremove_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.
#7
@
5 years ago
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.
#8
@
4 years ago
@flixos90 @desrosj : mind taking a peek at @jmdodd's latest patch, please? Seems pretty solid to me.
#9
@
4 years ago
- Milestone changed from Awaiting Review to 4.9
- Owner set to flixos90
- Status changed from new to reviewing
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.
#10
@
4 years ago
- Keywords commit added; needs-testing removed
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 whetheradd_user_to_blog()
fails fromwpmu_activate_signup()
. I added a checkis_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!
#11
@
4 years ago
41101.4.diff adds an extra test for the new add_user_to_blog()
failure behavior through the REST API.
#13
@
4 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
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).
Proposed filter and unit tests