WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 4 years ago

#27317 closed enhancement (fixed)

Update wp_insert_user function to return 'pre_user_login' filter WP_Errors

Reported by: crazycoolcam Owned by: SergeyBiryukov
Milestone: 4.4 Priority: normal
Severity: normal Version:
Component: Users Keywords: has-patch commit
Focuses: Cc:
PR Number:

Description

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.

Attachments (12)

27317.patch (585 bytes) - added by crazycoolcam 6 years ago.
Patch to user.php
admin-includes-user.php.patch (520 bytes) - added by crazycoolcam 6 years ago.
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.
27317.2.patch (1.2 KB) - added by crazycoolcam 6 years ago.
27317.3.patch (1.2 KB) - added by crazycoolcam 6 years ago.
27317.4.diff (4.5 KB) - added by danielbachhuber 6 years ago.
Add illegal_user_logins check to more places, and add tests
27317.4.patch (5.4 KB) - added by chriscct7 4 years ago.
27317.5.patch (5.5 KB) - added by chriscct7 4 years ago.
27317.6.patch (4.5 KB) - added by chriscct7 4 years ago.
27317.7.patch (4.7 KB) - added by SergeyBiryukov 4 years ago.
27317.diff (2.4 KB) - added by markjaquith 4 years ago.
27317.2.diff (1.9 KB) - added by boonebgorges 4 years ago.
27317.3.diff (3.8 KB) - added by markjaquith 4 years ago.

Download all attachments as: .zip

Change History (45)

@crazycoolcam
6 years ago

Patch to user.php

@crazycoolcam
6 years ago

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.

#1 @danielbachhuber
6 years ago

  • Keywords reporter-feedback added
  • Version 3.8.1 deleted

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.

#2 @crazycoolcam
6 years ago

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

#3 @crazycoolcam
6 years ago

  • Keywords reporter-feedback removed

#4 @danielbachhuber
6 years ago

  • Keywords needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to Future Release

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.

#5 @crazycoolcam
6 years ago

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.

#6 @crazycoolcam
6 years ago

  • Keywords needs-patch removed

What are unit tests?

#7 @danielbachhuber
6 years ago

  • Keywords needs-patch added

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.

#8 @crazycoolcam
6 years ago

  • Keywords needs-patch removed

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

This ticket was mentioned in IRC in #wordpress-dev by danielbachhuber-. View the logs.


6 years ago

@danielbachhuber
6 years ago

Add illegal_user_logins check to more places, and add tests

#10 @danielbachhuber
6 years ago

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

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.

#11 @crazycoolcam
6 years ago

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
4 years ago

@chriscct7
4 years ago

#12 @chriscct7
4 years ago

  • Milestone changed from Future Release to 4.4
  • Owner set to chriscct7
  • Status changed from new to assigned

Refreshed the patch and fixed an error in the unit test

#13 @DrewAPicture
4 years ago

  • Keywords needs-refresh needs-docs added

@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 */
    

@chriscct7
4 years ago

#14 @chriscct7
4 years ago

  • Keywords commit added; dev-feedback needs-refresh needs-docs removed

#15 @SergeyBiryukov
4 years ago

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

#16 @SergeyBiryukov
4 years ago

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

In 35189:

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

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

#17 @SergeyBiryukov
4 years ago

In 35190:

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

See #27317.

#18 @juliobox
4 years ago

  • Keywords 2nd-opinion close added; has-patch removed

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 ) ) { 

#19 @juliobox
4 years ago

  • Keywords has-patch added; close removed

bad keywords sorry

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


4 years ago

#21 @SergeyBiryukov
4 years ago

  • Keywords needs-patch added; commit has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

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


4 years ago

#23 @SergeyBiryukov
4 years ago

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

In 35629:

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

Props juliobox.
Fixes #27317.

#24 @SergeyBiryukov
4 years ago

In 35630:

Cast 'illegal_user_logins' filter result to array.

See #27317.

#25 @SergeyBiryukov
4 years ago

In 35631:

Fix failing multisite test after [35629].

See #27317.

#26 @GregLone
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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()).

#27 @wonderboymusic
4 years ago

  • Owner changed from chriscct7 to SergeyBiryukov
  • Status changed from reopened to assigned

@markjaquith
4 years ago

@boonebgorges
4 years ago

#28 @markjaquith
4 years ago

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.

@markjaquith
4 years ago

#29 @boonebgorges
4 years ago

  • Keywords has-patch commit added; 2nd-opinion needs-patch removed

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


4 years ago

#31 @wonderboymusic
4 years ago

+1 27317.3.diff looks good to me

#33 @boonebgorges
4 years ago

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

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.

Note: See TracTickets for help on using tickets.