WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 7 weeks ago

#17904 assigned defect (bug)

Multisite has more restrictions on user login character set

Reported by: duck_ Owned by: jeremyfelt
Milestone: Future Release Priority: normal
Severity: normal Version: 3.0
Component: Login and Registration Keywords: has-patch has-unit-tests early
Focuses: multisite Cc:

Description

Multisite has more restrictions on the characters allowed in a user's login name compared to single site. This seems unnecessary and confusing. It was also the root of a recent bug in the importer, see this forum thread and the workaround.

I haven't worked up a patch yet since there seem to be a few locations where these restrictions are enforced and I don't know if I have found them all yet:

  • wpmu_validate_user_signup() uses the regex /[a-z0-9]+/
  • ms-default-filters.php adds strtolower to sanitize_user

Relevant: http://mu.trac.wordpress.org/changeset/1689 [12948]

Attachments (12)

17904.diff (1.1 KB) - added by martythornley 3 years ago.
17904.2.diff (12.7 KB) - added by jeremyfelt 3 years ago.
17904.unittests.patch (4.1 KB) - added by imath 23 months ago.
17904.3.patch (11.5 KB) - added by imath 23 months ago.
17904.4.patch (13.5 KB) - added by imath 17 months ago.
17904.5.patch (13.6 KB) - added by imath 17 months ago.
Moves the blogname characters check in signup_blog()
17904.3.diff (13.0 KB) - added by ericlewis 7 months ago.
17904.4.diff (17.4 KB) - added by ericlewis 7 months ago.
17904.5.diff (17.0 KB) - added by ericlewis 7 months ago.
17904.tests.diff (3.7 KB) - added by ericlewis 7 months ago.
17904.6.diff (17.6 KB) - added by FolioVision 4 months ago.
Making it possible to signup with user name with spaces and uppercase letters
17904.7.diff (19.3 KB) - added by FolioVision 7 weeks ago.
Making sure the blank spaces in wp signup are not stripped

Download all attachments as: .zip

Change History (73)

#1 @danielbachhuber
5 years ago

  • Cc d@… added

#2 @boonebgorges
5 years ago

See also #BP2207.

Version 0, edited 5 years ago by boonebgorges (next)

#3 @mercime
5 years ago

  • Cc mercijavier@… added

#4 @mordauk
4 years ago

  • Cc pippin@… added

I'd love to get some more traction on this. It's causing an issue for me with several plugins when running them on MS installs. Here's a bug report for one of the plugins: https://github.com/pippinsplugins/Easy-Digital-Downloads/issues/643

#5 @chriscct7
4 years ago

  • Cc chriscct7@… added
  • Version 3.0 deleted

#6 @chriscct7
4 years ago

  • Version set to trunk

#7 follow-up: @SergeyBiryukov
4 years ago

  • Version changed from trunk to 3.0

#8 in reply to: ↑ 7 ; follow-up: @chriscct7
4 years ago

Replying to SergeyBiryukov:Isn't the version supposed to be the latest version the bug applies to? Or at least that's what I always assumed it was for....

#9 @SergeyBiryukov
4 years ago

#23124 was marked as a duplicate.

#10 in reply to: ↑ 8 ; follow-up: @SergeyBiryukov
4 years ago

Replying to chriscct7:

Isn't the version supposed to be the latest version the bug applies to?

Version number indicates when the issue was initially introduced/reported.

#11 in reply to: ↑ 10 @chriscct7
4 years ago

Oh, thats the total opposite of what I thought it was for. Good to know!

Replying to SergeyBiryukov:

Replying to chriscct7:

Isn't the version supposed to be the latest version the bug applies to?

Version number indicates when the issue was initially introduced/reported.

#12 @SergeyBiryukov
4 years ago

#23452 was marked as a duplicate.

#14 follow-up: @Ipstenu
4 years ago

I was under the understanding that the restrictions are in place because the username is used for the default sitename slug. So to prevent bad URLS, we sanitized everything to match.

If that can be broken out of the username check and only applied to the sitename, that would be okay.

#15 follow-up: @boonebgorges
4 years ago

I was under the understanding that the restrictions are in place because the username is used for the default sitename slug.

Ah, I didn't know that - can you point to documentation of that fact, or are you hypothesizing?

If that can be broken out of the username check and only applied to the sitename, that would be okay.

It'd be straightforward to use, eg, the user_nicename for slugs (that's what it's for, after all). The only potential problem I see with that is that when there are user_nicename clashes (ip@stenu and ipsten@u, for example), they're cleared up with trailing counters (so the second one to get there is ipstenu2, etc). That might be less than ideal from a UX point of view, because URLs would not match usernames. But this is an edge case, and seems to me less important than consistency and flexibility in user_login.

#16 @Ipstenu
4 years ago

I swear I read it somewhere, or it was mentioned on IRC, but I can;t find where I saw it. Maybe I got that idea from Otto? (Blame Otto!)

I think the UX loss is more important than mucking with user_login too, though. If there's no other check on sitename validations, we'll have to make sure something's put into place :)

(I swear this came up when someone was kvetching that he couldn't name a site foo.bar for a subfolder site, since that gets wiggy with subdomains.)

#17 @jeremyfelt
3 years ago

  • Keywords dev-feedback added
  • Milestone changed from Future Release to 3.7

Moving to 3.7 for discussion

#18 in reply to: ↑ 14 @martythornley
3 years ago

Replying to Ipstenu:

I was under the understanding that the restrictions are in place because the username is used for the default sitename slug. So to prevent bad URLS, we sanitized everything to match.

If that can be broken out of the username check and only applied to the sitename, that would be okay.

The create_new_blog() function uses sanitize_user() to sanitize the sitename, so the same filters used for users would be used for sites.

So it might be a question of whether you would want to use the same validations for sites as users? Or should there be a separate sanitize_sitename()?

But I am not seeing anywhere that there is a fallback to the actual username. During wpmu_validate_blog_signup() it checks new site names and does its own separate lower string check.

It seems that removing the strtolower from sanitize user does the trick as long as that is the desired effect?

Last edited 3 years ago by martythornley (previous) (diff)

@martythornley
3 years ago

#19 @jeremyfelt
3 years ago

Ran into this today and compared what rules single site and multisite enforced. I'm wondering if there's a way we can abstract the username logic into a common function and plug it into both edit_user() and wpmu_validate_user_signup(). It's entirely possible to filter the final results with wpmu_validate_user_signup, but that seems ugly.

Single Site edit_user()

When adding a user in single site through wp-admin/user-new.php, edit_user() is used and does the following:

  1. Process with sanitize_user(), but do not compare to original POST data
  2. Check if ( $user_login == '' )
  3. Check validate_username()
  4. Check username_exists()

Multisite wpmu_validate_user_signup()

When adding a user in multisite through wp-admin/user-new.php or wp-admin/network/user-new.php, wpmu_validate_user_signup() is used and does the following:

  1. Process with preg_replace( '/\s+/', '', sanitize_user( $user_name, true ) ) and compare the result to original POST data
  2. Accept only a-z, 0-9 with preg_match( '/[^a-z0-9]/', $user_name )
  3. Check if empty()
  4. Check if in_array( $username, $illegal_names ) to filter out www, web, root, etc...
  5. Check if strlen( $user_name ) < 4 )
  6. Check `if strpos( ' '. $user_name, '_' ) != false )
  7. Check if ( preg_match( '/^[0-9]*$/', $user_name ) )
  8. Check username_exists()
  9. Check DB tables for any matching, pending signups

@jeremyfelt
3 years ago

#20 @jeremyfelt
3 years ago

  • Keywords has-patch added; needs-patch removed

17904.2.diff introduces wp_validate_user_login() in an attempt to consolidate the user login restrictions between single site and multisite installations.

New wp_validate_user_login() requirements:

  • minimum of 4 characters
  • only contains (case-insensitive) characters: a-z 0-9 _ . - @
  • no whitespace
  • not on blacklist of illegal names
  • contains at least one letter
  • must be unique
  • not pending signup

The original criteria for wpmu_validate_user_signup() was a character set of a-z and 0-9. This has been loosened to match the criteria in edit_site().

register_new_user(), edit_site(), and wpmu_validate_user_signup() have all been modified to use the new wp_validate_user_login() function rather relying on their individual (and different) logic.

I've pieced together the original error strings from edit_user() and wpmu_validate_user_signup(), though I'm a bit confused on the naming convention between user_name and user_login and some others. It may be worth revisiting the messaging here?

It's entirely possible that we can deprecate validate_username() as it is no longer used anywhere in core as of this patch and does not exactly perform the described function. We may want to replace with an option to return bool from wp_validate_user_login() instead or ...

No changes in single site or multisite tests at the moment, though it looks like not much is actually tested around this.

#21 @DrewAPicture
3 years ago

  • Cc xoodrew@… added

#22 @r-a-y
3 years ago

  • Cc r-a-y@… added

#23 @nacin
3 years ago

  • Milestone changed from 3.7 to 3.8

Per conversation with jeremyfelt, pushing this to 3.8.

#24 @jeremyfelt
3 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from 3.8 to Future Release

Probably too late for the 3.8 cycle. My latest patch introduced wp_validate_user_login(), which should definitely have unit tests before being used.

#25 @desrosj
3 years ago

  • Cc desrosj@… added

#26 @jeremyfelt
3 years ago

  • Component changed from Multisite to Login and Registration
  • Focuses multisite added

#27 @nafur
2 years ago

I just stumbled about this issue and it seems that the patch did not made it into any release yet.

Is anyone taking care of this one? I'd really like to see this patch in the upstream version...

#28 @jdaviescoates
2 years ago

I would LOVE this issue to be resolved asap. This has been reported since 3.0 and now we're on 4.0! When setting up stand alone installs of wordpress I nearly always use email addresses for usernames when setting new users because they tend to be both a lot longer and more varied (and hence a lot harder to guess/ more secure) than the short a-z 0-9 restricted user names currently possible with mulitisite installs. I just tried to create new user on a multisite install using an email address but hit this issue. My work around for now is to use this plugin that requires people use their email address instead of their username to login.

#29 @boonebgorges
2 years ago

#31016 was marked as a duplicate.

#30 @ArkonLabs
2 years ago

This is still an issue in 4.1.

I would like to have the choice of allowing users to register with first.last or first.last@… to match AD.

According to previous replies, this fix almost made it into version 3.7 and 3.8... are there any updates?

#31 @imath
23 months ago

  • Keywords needs-unit-tests removed

17904.3.patch is a bit different than 17904.2.diff.
I've included some suggested unit tests in 17904.unittests.patch that can be used for 17904.2.diff in case my suggested patch is doing something wrong.

@imath
23 months ago

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


23 months ago

#33 @boonebgorges
21 months ago

In 31970:

Unit tests for wpmu_validate_user_signup().

See #17904, #26784.

#34 @pento
21 months ago

In 31978:

Unit Tests added in [31970] need to be restricted to run in Multisite only.

See #17904, #26784.

#35 in reply to: ↑ 15 ; follow-up: @jeremyfelt
19 months ago

  • Milestone changed from Future Release to 4.3

Replying to boonebgorges:

I was under the understanding that the restrictions are in place because the username is used for the default sitename slug.

Ah, I didn't know that - can you point to documentation of that fact, or are you hypothesizing?

Continued hypothesizing only because I recently learned... In b2++, the username/sitename slug was first used as part of the table name (e.g. wp_jeremyfelt_posts) before it was switched to blog ID. This may have been a driver in some of the original sanitization.

Moving this to 4.3 because we have tests, a patch, and a full cycle ahead of us.

#36 @wonderboymusic
18 months ago

  • Keywords needs-refresh added; dev-feedback removed

Someone please refresh this with gusto

#37 @wonderboymusic
18 months ago

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

#38 in reply to: ↑ 35 @obenland
17 months ago

  • Milestone changed from 4.3 to Future Release

Replying to jeremyfelt:

Moving this to 4.3 because we have tests, a patch, and a full cycle ahead of us.

The cycle is almost over and there hasn't been movement outside of wonderboymusic's comments. Feel free to re-milestone one there is a refreshed patch and this has a chance to go in.

#39 @jeremyfelt
17 months ago

In 33083:

Usernames in multisite should be restricted to 60 characters or fewer.

Only 60 characters can be stored in the database for a username, which could cause lookup issues when attempting to use similar usernames of extreme length.

Props @DJPaul.
See #17904, Fixes #26784.

#40 @imath
17 months ago

  • Keywords needs-refresh removed

17904.4.patch is a refreshed version of the patch to r33083 + some adjustments :

  • If i understand well, these characters _ - @ . are now allowed for the user login, so i've removed the contains_underscores index from the dataset of Tests_Multisite_WpmuValidateUserSignup->test_user_name
  • When a user signups with a blog, i suggest to make sure the _ - @ . characters are removed from the blogname input in show_blog_form() as the user_name is used as the default value of the blog's name.

@imath
17 months ago

@imath
17 months ago

Moves the blogname characters check in signup_blog()

#41 @ericlewis
7 months ago

#36286 was marked as a duplicate.

@ericlewis
7 months ago

#42 @ericlewis
7 months ago

attachment:17904.3.diff is a refresh after r35189 and related commits from #27317.

#43 @ericlewis
7 months ago

  • Keywords needs-unit-tests added
  • Milestone changed from Future Release to 4.6

The current patch enforces the multisite user_login limitations onto single site, which @jeremyfelt described in comment:19. These new limitations include:

  • user_login is forced to at least 4 characters. Why do we enforce that limitation in multisite? Looks like it was a decision made early on.
  • user_login cannot only be numbers. With the introduction of newly allowed characters, this means user_logins like @@@@ and .... are legal. These user_logins translate into odd user_nicenames because they are run through sanitize_title and the rest of the business logic that happens in wp_insert_user().

Moving this to 4.6 for consideration. We'll want some unit tests for wp_validate_user_login(), depending on what we expect to be allowed.

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


7 months ago

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


7 months ago

#46 @ericlewis
7 months ago

  • Keywords needs-patch added; has-patch removed

We outlined next steps for this ticket during multisite office hours.

  • Remove the 4 character minimum limit.
  • Leave illegal_names, but only apply it to multisite.
  • Sandbox the restriction that a user login must have one letter to multisite, and modify the regex appropriately.

Let's try writing the unit tests first to meet these conditions for wp_validate_user_login() ;)

@ericlewis
7 months ago

#47 @ericlewis
7 months ago

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

In attachment:17904.4.diff,

  • Explicitly return an error if one is produced in wp_validate_user_login(). Previously just the user_login was being returned.
  • unit tests for wp_validate_user_login() with pretty good coverage.
  • Remove the 4 character minimum limit.
  • The site option illegal_names check should only happen in multisite, but the illegal_logins filter should stay as that is used in single site.
  • Sandbox the restriction that a user login must have one letter to multisite, and modified the regex to require an alphabetical letter.

#48 @ericlewis
7 months ago

Oh, I also suggest that we change the username away from admin when installing WordPress in unit tests so that some tests can pass.

A few other unit tests fail because of these changes, as well as new WP_Error return codes.

@ericlewis
7 months ago

#49 follow-up: @ericlewis
7 months ago

More in attachment:17904.5.diff.

I've brought over the validate_username filter from validate_username() for backwards compatibility. This filter provides a generic true/false way to invalidate a username. If a developer wants to add a more explicit WP_Error code, they can use the wp_validate_user_login action. We should deprecate validate_username() and perhaps have it call wp_validate_user_login() directly.

wp_validate_user_login() should stay true to its intent, and exclusively handle validation. The function could return true or WP_Error. The wp_validate_user_login filter shouldn't allow for filtering of the user_login, and rather filter the WP_Error, with the user_login passed in for context.

wp_insert_user() has some duplicate logic. Should we call wp_validate_user_login() in here? The WP_Error response codes are incompatible with the ones merged in from wpmu_validate_user_signup() (e.g. user_name vs. empty_user_login).

I've moved the "is this username pending sign-up?" logic from wpmu_validate_user_signup() into wp_validate_user_login(), which was intended previously.

Oh, I also suggest that we change the username away from admin when installing WordPress in unit tests so that some tests can pass.

I'm going to hold back on this recommendation as it breaks other tests and would hamstring our work here. Commenting out the problematic code in the unit test for now.

Other small fixes to get all unit tests passing. All unit tests now pass :D

#50 @ericlewis
7 months ago

Forgot tests previously, in attachment:17904.tests.diff

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


7 months ago

#52 in reply to: ↑ 49 ; follow-ups: @jeremyfelt
7 months ago

Replying to ericlewis:

More in attachment:17904.5.diff.

I've brought over the validate_username filter from validate_username() for backwards compatibility. This filter provides a generic true/false way to invalidate a username. If a developer wants to add a more explicit WP_Error code, they can use the wp_validate_user_login action.

The validate_username filter is currently only used when checking the string against sanitize_user to see if it changed. Moving it to its new location in wp_validate_user_login() gives it more authority and removes the direct true/false response it has now. I'd prefer finding a way to apply the filter only in the case of the sanitize_user() mismatch further up.

We should deprecate validate_username() and perhaps have it call wp_validate_user_login() directly.

I think deprecating is the right answer, as core will no longer use it, though I'm not sure the internals should change immediately. It performs a very direct task at the moment.

wp_validate_user_login() should stay true to its intent, and exclusively handle validation. The function could return true or WP_Error. The wp_validate_user_login filter shouldn't allow for filtering of the user_login, and rather filter the WP_Error, with the user_login passed in for context.

Agreed, true/WP_Error makes sense.

wp_insert_user() has some duplicate logic. Should we call wp_validate_user_login() in here? The WP_Error response codes are incompatible with the ones merged in from wpmu_validate_user_signup() (e.g. user_name vs. empty_user_login).

It's interesting that we don't already do more validation in wp_insert_user(). I don't think we should make adjustments there right now. There are quite a few additional restrictions in wp_validate_user_login() that may not jive with non-UI facing code that has been using wp_insert_user() for a while.

I've moved the "is this username pending sign-up?" logic from wpmu_validate_user_signup() into wp_validate_user_login(), which was intended previously.

+1

Would anything break if we adjusted the order of the is_multisite() checks a bit for readability? Illegal names could go after illegal user logins. Username exists could go after the pending signups. It may be easier then to spot the single site/multisite differences.

Oh, I also suggest that we change the username away from admin when installing WordPress in unit tests so that some tests can pass.

I'm going to hold back on this recommendation as it breaks other tests and would hamstring our work here. Commenting out the problematic code in the unit test for now.

This seems like a good idea. Should we move that specific change to another ticket?

#53 in reply to: ↑ 52 @ericlewis
7 months ago

Replying to jeremyfelt:

I think deprecating validate_username() is the right answer, as core will no longer use it, though I'm not sure the internals should change immediately. It performs a very direct task at the moment.

This sounds good. Let's add a deprecated call to point developers to the wp_validate_user_login() in the next patch, and move it into deprecated.php.

Would anything break if we adjusted the order of the is_multisite() checks a bit for readability? Illegal names could go after illegal user logins. Username exists could go after the pending signups. It may be easier then to spot the single site/multisite differences.

Good question. Error ordering in the resulting WP_Error would change.

On a related note, the current patch changes error codes that would be returned by register_new_user(), edit_user() or wpmu_validate_user_signup(). Previously they perform the same checks and return different error codes. E.g. the validate_username() check in edit_user() would return the error code user_login, and the validate_username() check in register_new_user() would return the error code invalid_username . The current patch changes this error code to user_name. Is changing error code responses okay, or should we consider some backward compat shim for these cases?

#54 in reply to: ↑ 52 @ericlewis
7 months ago

Replying to jeremyfelt:

The validate_username filter is currently only used when checking the string against sanitize_user to see if it changed. Moving it to its new location in wp_validate_user_login() gives it more authority and removes the direct true/false response it has now. I'd prefer finding a way to apply the filter only in the case of the sanitize_user() mismatch further up.

validate_username() is currently used to check the username when the user is created in edit_user() and in register_new_user(), so I don't think we should put it behind a sanitize_user() mismatch. The implementation in attachment:17904.5.diff keeps the boolean validation nature of sanitize_user(), and if the username is invalid adds an error to the WP_Error.

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


6 months ago

#56 @chriscct7
5 months ago

#37195 was marked as a duplicate.

#57 @ocean90
5 months ago

  • Keywords early added
  • Milestone changed from 4.6 to Future Release

This should have landed before beta 1. Punting.

@FolioVision
4 months ago

Making it possible to signup with user name with spaces and uppercase letters

#58 @FolioVision
4 months ago

It's great to see the development here as we would like to customize the username restrictions present in WordPress Multisite signups. When you enter anything except a-z or 0-9 you get:

Only lowercase letters (a-z) and numbers are allowed.

As @Ipstenu said, these restrictions are there to make sure the user blogs have nice URLs, but the UX loss is too big.

attachment:17904.6.diff removes the user name lowercase and no spaces requirements of Multisite (in wp_validate_user_login() and wp-includes/ms-default-filters.php), but still respects that when creating the new blog URL (wp-signup.php).

Last edited 4 months ago by FolioVision (previous) (diff)

#59 @Ipstenu
4 months ago

If we're going down the 'allowing usernames but not blog names' UX route, then we need to factor in a notification.

Like.. "Notice: Your username contains characters that cannot be used in your blog name. We have adjusted your blog name for you, but please review and confirm before you submit."

A check box "Yes, I like this name" or something to make sure they know. Otherwise we'll have an unintended result (the blog not matching the username) as we've changed the status quo. Not saying it's bad, not at all, just that we have to remember to keep existing users used to multisite as informed as the newbies :)

#60 @FolioVision
4 months ago

I found a bug in my fix. While it makes it possible to signup with username like Firstname Lastname (exactly what we need) once the signup is confirmed and activated, the username becomes firstnamelastname.

I'll fix it and post another patch.

@Ipstenu actually when you say you want to get a blog there is a second screen to which you get once you send the desired username and password. There you can see what's your blog URL going to look like and you can customize it further. I think that's enough already.

@FolioVision
7 weeks ago

Making sure the blank spaces in wp signup are not stripped

#61 @FolioVision
7 weeks ago

I fixed the bug with take user name blank spaces being removed later in the process. Over 17904.6.diff it's just 2 more lines changed in wp-includes/ms-functions.php - wpmu_signup_user() and wpmu_create_user().

This was tested on latest WordPress 4.7-alpha-38842 from http://svn.automattic.com/wordpress/trunk/

Note: See TracTickets for help on using tickets.