Update wp_insert_user function to return 'pre_user_login' filter WP_Errors

[crazycoolcam]

Currently, if I want to filter 'pre_user_login' and return a WP Error -- blocking people from using certain usernames or characters, there is no check for this and it returns the empty_user_login error instead.

This patch adds a quick check to $user_login variable to see if it is a WP-Error after applying the 'pre_user_login' filter, it returns the error instead of continuing processing the function.

~Cam
P.S. Apologies if I missed something in this patch/enhancement request. It is my first time adding a patch.

[crazycoolcam]

Patch to user.php

[crazycoolcam]

With the above patch, creating a user via the dashboard generates a blank email if the user is blocked. This patch adds the appropriate test to stop the email if it's returning an error.

[danielbachhuber]

Thanks for the patch, crazycoolcam.

Could you describe your use case in a bit more detail? Specifically, I'd be interested to know why you're blocking certain usernames, and how users are permitted to enter these usernames in the first place.

You might be able to perform this check higher up the stack, and return an empty value in pre_user_loginas the final stop-gap.

[crazycoolcam]

Thanks for asking Daniel,

The main use for this would be to check against a list of usernames I would like to ban/block, such as: 'admin', 'root', 'test', 'user', 'guest', etc. and not allow them to register at all.

Some of my sites run buddypress, which uses wp_insert_user and not the login/?action=register action, and others may have users who have permission to add users but who I would not want adding users with these names.

Does this make sense, and/or am I missing a really plain method of rejecting usernames for processes that don't use login/?action=register error checks?

Thanks for your help.
~Cam

[danielbachhuber]

I think the use case makes a lot of sense. Core already has the concept of "Banned Names" for sites, and wp_insert_user() already returns WP_Error in some cases.

Rather than bolting this onto pre_user_login though, I think I'd recommend checking if user_login is in an array of banned usernames. If it is, then return WP_Error. Something like this:

if ( in_array( $user_login, apply_filters( 'illegal_user_logins', array() ) ) ) {
    return new WP_Error( 'illegal-user-login', __( "User login value is not permitted." ) );

A few more things to note:

• Please create your patches from the WordPress root.
• It would be good to have unit tests included with the patch. If you use the ​developer checkout, you can include the changes and the tests in the same patch file.
• Your second patch should be included as well — edit_user() should handle cases in which the user isn't actually created.

[crazycoolcam]

Thanks for the update. My apologies for not creating the patch from the WP root. Still getting use to SVN as this is my first patch.

Is there a tutorial for using Developer Checkout? I am using TortoiseSVN and am happy to include any additional testing.

I like the idea of what you're suggesting. It would definitely add an extra layer of flexibility.

[crazycoolcam]

What are unit tests?

[danielbachhuber]

It looks like the patch you've just uploaded is missing a bracket — make sure you test fully before uploading.

Using the developer checkout is as simple as svn co https://developer.svn.wordpress.org/trunk (or the equivalent for Tortoise). ​Here is the documentation on writing tests.

[crazycoolcam]

Sorry for the extra bracket (Copy/Paste strikes again).

I tested the latest patch code (3) on my local environment and it works. I would love to include unit-tests but I am completely confused on everything related to running/generating them and have spent the last 2 and a half hours trying to figure it out with no success.

I'd love additional help testing this.

Thank you in advance.
~Cam

[danielbachhuber]

Add illegal_user_logins check to more places, and add tests

[danielbachhuber]

Using validate_username() and username_exists() as corollaries, 27317.4.diff applies the illegal_user_logins filter in a few more places and adds unit tests.

My concern is now that this code isn't very DRY. In diving into this, it appears our user login validation code isn't very DRY either. I don't think this filter is an easy win now — needs more consideration.

[crazycoolcam]

Hi Daniel,
Thank you for the updated patch.

I agree, the current code isn't very DRY in this area. It would make sense to simplify this (at least the validation checks) under one, maybe two, functions.

~Cam

[chriscct7]

Refreshed the patch and fixed an error in the unit test

[DrewAPicture]

@chriscct7 Some notes on 27317.5.patch​:

• Patch needs a refresh
• The summary for the illegal_user_logins filter should describe what is filterable, rather than what it allows. And it should end with a period
• For the parameter, we should use a "fake" variable, maybe $usernames, instead of array(), and the parameter description should mention the default value and end with a period
• Subsequent duplicates of illegal_user_logins should have a single-line comment written in the following style:

  /** This filter is documented in wp-admin/includes/user.php */
  

[SergeyBiryukov]

27317.7.patch​ fixes syntax errors and merges new string with an existing one.

[SergeyBiryukov]

In 35189:

Users: Add 'illegal_user_logins' filter to allow certain usernames to be blacklisted.

Props danielbachhuber, chriscct7, crazycoolcam, SergeyBiryukov.
Fixes #27317.

[SergeyBiryukov]

In 35190:

Remove a one-time variable from edit_user() added in [35189].

See #27317.

[juliobox]

Hello,

i just tested it and i'm feeling confused ...
Here's my simple code, the one you'll see on 100 websites at day 1 after 4.4 release:

<?php
add_filter( 'illegal_user_logins', 'admin_is_an_illegal_user_login' );
function admin_is_an_illegal_user_login() {
        return array( 'admin' );
}

So now, i'm running a fresh 4.4-b3 (monosite) with this mu-plugin and i try to add a user named "admin", it works fine, great job i lmike that.
But ... if i try to add a user named "Admin", it's ok(!) this is why i'm feeling confused, for me, "Admin", "admin", "aDmIn", "AdMiN" is the same forbidden user login, see what i mean?

It's ok on multisite because we use strtolower() on that.

So i would rather prefer that:

<?php
if ( in_array( strtolower( $user->user_login ), $usernames ) ) { 

[juliobox]

bad keywords sorry

[SergeyBiryukov]

In 35629:

Users: After [35189], make 'illegal_user_logins' check case-insensitive.

Props juliobox.
Fixes #27317.

[SergeyBiryukov]

In 35630:

Cast 'illegal_user_logins' filter result to array.

See #27317.

[SergeyBiryukov]

In 35631:

Fix failing multisite test after [35629].

See #27317.

[GregLone]

Hello.
Shouldn't this be also available in register_new_user()?
All other "user" functions uses it but not this one (wp_insert_user(), edit_user(), wpmu_validate_user_signup()).

[markjaquith]

So, on multisite, usernames are rejected with uppercase characters.

So on multisite, those usernames will be rejected with another error code. @boonebgorges suggested we collapse down onto invalid_username which already exists.

[boonebgorges]

+1 to 27317.3.diff​

[wonderboymusic]

+1 27317.3.diff​ looks good to me

[ocean90]

27317.3.diff​ works for me.

[boonebgorges]

In 35772:

Use 'invalid_username' error code when tripping 'illegal_user_logins'.

This gives us better compatibility with existing errors thrown by
sanitize_user(), especially in Multisite, where user_login has more
restrictions on allowed characters.

Props markjaquith.
Fixes #27317.