Make WordPress Core

Opened 13 years ago

Last modified 5 months ago

#17904 assigned defect (bug)

Multisite has more restrictions on user login character set

Reported by: duck_'s profile duck_ Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.0
Component: Login and Registration Keywords: has-patch needs-testing-info needs-testing needs-unit-tests needs-dev-note needs-refresh
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 11 years ago.
17904.2.diff (12.7 KB) - added by jeremyfelt 11 years ago.
17904.unittests.patch (4.1 KB) - added by imath 10 years ago.
17904.3.patch (11.5 KB) - added by imath 10 years ago.
17904.4.patch (13.5 KB) - added by imath 9 years ago.
17904.5.patch (13.6 KB) - added by imath 9 years ago.
Moves the blogname characters check in signup_blog()
17904.3.diff (13.0 KB) - added by ericlewis 9 years ago.
17904.4.diff (17.4 KB) - added by ericlewis 9 years ago.
17904.5.diff (17.0 KB) - added by ericlewis 9 years ago.
17904.tests.diff (3.7 KB) - added by ericlewis 9 years ago.
17904.6.diff (17.6 KB) - added by FolioVision 8 years ago.
Making it possible to signup with user name with spaces and uppercase letters
17904.7.diff (19.3 KB) - added by FolioVision 8 years ago.
Making sure the blank spaces in wp signup are not stripped

Download all attachments as: .zip

Change History (105)

#1 @danielbachhuber
13 years ago

  • Cc d@… added

#3 @mercime
13 years ago

  • Cc mercijavier@… added

#4 @mordauk
12 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
12 years ago

  • Cc chriscct7@… added
  • Version 3.0 deleted

#6 @chriscct7
12 years ago

  • Version set to trunk

#7 follow-up: @SergeyBiryukov
12 years ago

  • Version changed from trunk to 3.0

#8 in reply to: ↑ 7 ; follow-up: @chriscct7
12 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
12 years ago

#23124 was marked as a duplicate.

#10 in reply to: ↑ 8 ; follow-up: @SergeyBiryukov
12 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
12 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
12 years ago

#23452 was marked as a duplicate.

#14 follow-up: @Ipstenu
12 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
12 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
12 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
11 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
11 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 11 years ago by martythornley (previous) (diff)

@martythornley
11 years ago

#19 @jeremyfelt
11 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
11 years ago

#20 @jeremyfelt
11 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
11 years ago

  • Cc xoodrew@… added

#22 @r-a-y
11 years ago

  • Cc r-a-y@… added

#23 @nacin
11 years ago

  • Milestone changed from 3.7 to 3.8

Per conversation with jeremyfelt, pushing this to 3.8.

#24 @jeremyfelt
11 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
11 years ago

  • Cc desrosj@… added

#26 @jeremyfelt
11 years ago

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

#27 @nafur
10 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
10 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
10 years ago

#31016 was marked as a duplicate.

#30 @ArkonLabs
10 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
10 years 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
10 years ago

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


10 years ago

#33 @boonebgorges
10 years ago

In 31970:

Unit tests for wpmu_validate_user_signup().

See #17904, #26784.

#34 @pento
10 years 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
10 years 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
9 years ago

  • Keywords needs-refresh added; dev-feedback removed

Someone please refresh this with gusto

#37 @wonderboymusic
9 years ago

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

#38 in reply to: ↑ 35 @obenland
9 years 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
9 years 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
9 years 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
9 years ago

@imath
9 years ago

Moves the blogname characters check in signup_blog()

#41 @ericlewis
9 years ago

#36286 was marked as a duplicate.

@ericlewis
9 years ago

#42 @ericlewis
9 years ago

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

#43 @ericlewis
9 years 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.


9 years ago

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


9 years ago

#46 @ericlewis
9 years 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
9 years ago

#47 @ericlewis
9 years 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
9 years 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
9 years ago

#49 follow-up: @ericlewis
9 years 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
9 years ago

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

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


9 years ago

#52 in reply to: ↑ 49 ; follow-ups: @jeremyfelt
9 years 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
9 years 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
9 years 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.


8 years ago

#56 @chriscct7
8 years ago

#37195 was marked as a duplicate.

#57 @ocean90
8 years ago

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

This should have landed before beta 1. Punting.

@FolioVision
8 years ago

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

#58 @FolioVision
8 years 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 8 years ago by FolioVision (previous) (diff)

#59 @Ipstenu
8 years 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
8 years 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
8 years ago

Making sure the blank spaces in wp signup are not stripped

#61 @FolioVision
8 years 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/

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


8 years ago

#63 @johnjamesjacoby
8 years ago

  • Keywords 2nd-opinion needs-refresh added; early removed

I just ran into another edge-case.

Plugins that implement their own user registration functionality are highly likely to use validate_username() directly (because there really isn't an easier way to customize it all) but the validate_username filter does not enforce multisite's user_login rules.

This means if a plugin like Easy Digital Downloads is active on 1 site of a multisite install, and someone buys something, and registration is enabled, anonymous users can successfully sign-up to a multisite install the username - which is totally valid for single-site but invalid for multisite.

I think it's safe to assume this concern is pandemic across the plugins repository, though a deeper dive would be revealing. If that's the case, deprecating validate_username alone is no longer a complete solution.


In light of this new information, I'd like to propose the opposite approach.

  • Continue to use validate_username() as is
  • Introduce a wpmu_validate_username() filter to enforce the multisite rules when validate_username() is called
  • Introduce the wp_validate_user_login() function as discussed, but have it use validate_username() internally, and only use it in places where WP_Error results are desirable.
  • In addition, the patches above don't use the return values of wp_validate_user_login() so they wouldn't actually work as intended, so it needs a refresh regardless

#64 @flixos90
8 years ago

@johnjamesjacoby and I had a chat on the behavior of username validation (and a tiny bit of email validation) in today's bug scrub (see https://wordpress.slack.com/archives/core-multisite/p1489430014794142). Here's a summary of what we agreed on:

Functions for username validation:

  1. validate_username(): Will be modified to accept a second parameter $wp_error = false to make the function return error objects instead of false. If is_multisite(), the function will include the restrictions for multisite (illegal names, minimum and maximum length, illegal logins, all numeric).
  2. username_exists(): In addition for what this function currently does, it can accept a second parameter $check_reserved = true. If that parameter is true (default), the function also calls is_username_reserved() (see below).
  3. is_username_reserved(): New method that performs the wp_signups query for existing username that currently happens in wpmu_validate_user_signup(). Returns true/false.
  4. wp_validate_user_login(): New method that acts as a wrapper calling validate_username(), username_exists() with second parameter false and is_username_reserved(). Returns true/WP_Error.

The $check_reserved = true parameter of username_exists() makes sense so that by default the check is enforced, but for more granular checking by calling the functions manually it can be disabled (see wp_validate_user_login() that calls them separately to provide distinct error messages). This part was not discussed in the chat, but I figured it would make sense. Correct me if there are concerns with that. :)

On a side note, once we get to a patch and closer to a commit here, we can open a ticket to perform equivalent changes for email validation.

Version 0, edited 8 years ago by flixos90 (next)

#65 @FolioVision
8 years ago

This is a fantastic improvement Felix.

With the prevalence of multisite these days, it would be very efficient to see the same validation rules apply to usernames single site as multisite, including not allowing duplicates in the whole database. Of course in cases like WordPress.com, that's inconvenient but I'd say installs of that magnitude are more the exception than the rule. We run several multisite networks where we expressly don't want to allow duplicate usernames among the sites.

While applying the rules of multisite to single site in the future would leave invalid usernames in old WordPress sites, at least there would not be additional problematic usernames created and eventually (in a few years) one could sanitise existing user databases without too much trouble.

#66 @SergeyBiryukov
8 years ago

#40169 was marked as a duplicate.

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


7 years ago

#68 @sethcarstens
7 years ago

I have interest in this bug as well, I'd love to help but I've never patched, and I'm not sure what "needs refresh" means exactly for this longrunning ticket. If anyone can point me in the right direction, I'll try to put time into it over the next few weeks.

#69 @jadeweb
7 years ago

Seven years, and still active. Yikes! I feel like I may be trying to move a mountain by chiming in here, but I'll give it a go anyhow. :)

I've come to this thread while searching for a way around the strictness of multisite username validation. We have setup a multisite install for a client's network of ecommerce stores, where the vast bulk of users created are WooCommerce customers. Enforcing these restrictions on customers has proved to be a terrible experience. I think this is a sensible use-case and legitimate UX concern.

The clear assumption by some in this thread is that these restrictions on the username are a hard requirement and usernames created on single-site installs should also adhere to them at some point down the road.

On the contrary though, what strikes me is how torturously contrived the restrictions seem to be. The problem boils down to conflating two necessary pieces of data in Multisite: a username for the purposes of authentication, and a slug for a multisite sub-domain. I agree whole-heartedly with @FolioVision's initial position that this is a huge UX loss. If the app were refactored to use two pieces of data, the extra multisite restrictions would be unnecessary. Whilst setting up a new blog, simply offer a slugified version of the username as a default option for the subdomain. In so doing, you could:

  • consolidate and simplify username validation logic across single and multisite installs, while maintaining backwards compatibility
  • avoid the problem alluded to of having existing usernames become invalid
  • existing multisite usernames would continue to work without issue
  • effectively retain the current UX of defaulting to have the domain (now, closely) match the user's name
  • allow more human-friendly usernames in all cases

/end 2¢.

Cheers,
David

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


5 years ago

#72 @tofuSCHNITZEL
4 years ago

any news? seems like an easy fix to me?
is there a reason why it is stricter with multisites?

Last edited 4 years ago by tofuSCHNITZEL (previous) (diff)

#73 @jeremyfelt
4 years ago

  • Owner jeremyfelt deleted

#74 @desrosj
3 years ago

#55831 was marked as a duplicate.

This ticket was mentioned in PR #3153 on WordPress/wordpress-develop by petitphp.


2 years ago
#75

  • Keywords needs-refresh removed

This PR implement changes proposed inticket 17904#comment:64

The goal is to align how single and multisite check the validity of a username.

Changes in PR :

  • Update validate_username() to include all restrictions from multisite,
  • Introduce new function is_username_reserved(), this function performs the wp_signups query for existing username that was previously done in wpmu_validate_user_signup(),
  • Introduce new function wp_validate_user_login(), that acts as a wrapper calling validate_username(), username_exists() and is_username_reserved(),
  • Update codebase to use the new wp_validate_user_login() where applicable and remove code that was merge in validate_username(),
  • Update User REST endpoint following the refactoring of validate_username(),
  • Update wpmu_validate_blog_signup() to allow dashes in blogname.

Changes not in PR :

  • Change username_exists() to accept a second parameter $check_reserved = true, this seem like a tricky change to me. Currently the function will return either a user id matching the username or false. Returning a true when the username is reserved could break other code casting the return of username_exists() to int.

Trac ticket: https://core.trac.wordpress.org/ticket/17904

audrasjb commented on PR #3153:


2 years ago
#76

Thanks for the PR, I added a commit to fix a WPCS issue.
It replaces current_time() with time(), as per WordPress Coding Standards.

#77 @audrasjb
2 years ago

  • Keywords 2nd-opinion removed
  • Milestone changed from Future Release to 6.1

Thanks for the PR! Moving for 6.1 consideration.

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


2 years ago

petitphp commented on PR #3153:


2 years ago
#79

@dream-encode Thanks for taking the time to look at the PR. I've applied all your suggestions.

#80 @audrasjb
2 years ago

  • Milestone changed from 6.1 to 6.2

With WP 6.1 RC 1 scheduled today (Oct 11, 2022), there is not much time left to address this ticket. Let's move it to the next milestone.

Ps: if you were about to send a patch and if you feel it is realistic to commit it in the next one or two hours, please feel free to move this ticket back to milestone 6.1.

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


22 months ago

#82 @costdev
22 months ago

  • Keywords needs-testing-info needs-testing needs-unit-tests added; has-unit-tests removed

This ticket was discussed during the recent bug scrub. This ticket is in good hands with JB and David active in the PR.

Just adding a few keywords to note some needs for this ticket that were raised during the bug scrub.

  • Needs testing instructions and manual testing. Adding needs-testing-info and needs-testing.
  • Needs unit tests for the new functions being introduced. Removing has-unit-tests and adding needs-unit-tests.

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


22 months ago

#84 @costdev
22 months ago

  • Milestone changed from 6.2 to Future Release

This ticket was discussed in the bug scrub and it was agreed to move this ticket to Future Release.

#85 @rajinsharwar
13 months ago

  • Milestone changed from Future Release to 6.5

Hey all, this is a long long ticket, and awaiting unit tests, and testing. So, I am putting this for 6.5 considering that Beta one has a lot of time left to come.

#86 @rajinsharwar
13 months ago

  • Keywords needs-dev-note added

So, considering the patch of the PR: 3153, we have got 2 new functions introduced:

  1. wp_validate_user_login() | wp-includes/user.php
  2. is_username_reserved() | wp-includes/user.php

I am afraid we might still need the unit tests for these two new functions.

We have got the validate_username()function modified, with an additional parameter being passed. So, we need to update the developer blog as well for that function.

We got 4 NEW strings introduced, and 2 OLD strings being removed. Added:

  1. Site names can only contain lowercase letters (a-z), numbers, and hyphens.
  1. Site names can not begin or end with a hyphen.
  1. Sorry, that username is invalid. (NOTE: We can use the "Sorry, that username is not allowed" instead of introducing this.
  1. That username is currently reserved, but it may be available in a couple of days.

Removed:

  1. This username is already registered. Please choose another one.
  1. Site names can only contain lowercase letters (a-z) and numbers.

As this change includes major changes for the login and registration functions for multi-site, we will need a DEV note for this as well.

Last edited 13 months ago by rajinsharwar (previous) (diff)

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


10 months ago

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


10 months ago

#89 @audrasjb
10 months ago

As per today's bug scrub:
The PR has some conflicts with current trunk.

@petitphp is it still in your radar?
We'll give it another week to see if the PR can be merged :)

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


10 months ago

#91 @rajinsharwar
10 months ago

  • Keywords needs-refresh added
  • Milestone changed from 6.5 to Future Release

This is pending some unit tests, as well, and needs a refresh with the current trunk. Let's put this in the Future Release milestone unless we have some progress.

#92 @petitphp
10 months ago

@audrasjb @rajinsharwar thanks for the ping.

I've refreshed the PR against trunk and will look at adding the missing tests.

If it's too short for this release cycle, feel free to punt to 6.6.

#93 @rajinsharwar
5 months ago

Hi @petitphp, good day! Would you be willing to help with adding the missing unit tests to the PR? If you could take the leap, maybe we can add this to the 6.7 release, and commit early in the release. :)

Note: See TracTickets for help on using tickets.