WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 3 weeks ago

#17904 new defect (bug)

Multisite has more restrictions on user login character set

Reported by: duck_ Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.0
Component: Login and Registration Keywords: dev-feedback has-patch
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 (4)

17904.diff (1.1 KB) - added by martythornley 21 months ago.
17904.2.diff (12.7 KB) - added by jeremyfelt 20 months ago.
17904.unittests.patch (4.1 KB) - added by imath 3 months ago.
17904.3.patch (11.5 KB) - added by imath 3 months ago.

Download all attachments as: .zip

Change History (38)

comment:1 @danielbachhuber4 years ago

  • Cc d@… added

comment:2 @boonebgorges3 years ago

See also #BP2207.

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

comment:3 @mercime3 years ago

  • Cc mercijavier@… added

comment:4 @mordauk2 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

comment:5 @chriscct72 years ago

  • Cc chriscct7@… added
  • Version 3.0 deleted

comment:6 @chriscct72 years ago

  • Version set to trunk

comment:7 follow-up: @SergeyBiryukov2 years ago

  • Version changed from trunk to 3.0

comment:8 in reply to: ↑ 7 ; follow-up: @chriscct72 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....

comment:9 @SergeyBiryukov2 years ago

#23124 was marked as a duplicate.

comment:10 in reply to: ↑ 8 ; follow-up: @SergeyBiryukov2 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.

comment:11 in reply to: ↑ 10 @chriscct72 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.

comment:12 @SergeyBiryukov2 years ago

#23452 was marked as a duplicate.

comment:14 follow-up: @Ipstenu2 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.

comment:15 @boonebgorges2 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.

comment:16 @Ipstenu2 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.)

comment:17 @jeremyfelt21 months ago

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

Moving to 3.7 for discussion

comment:18 in reply to: ↑ 14 @martythornley21 months 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 21 months ago by martythornley (previous) (diff)

@martythornley21 months ago

comment:19 @jeremyfelt20 months 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

@jeremyfelt20 months ago

comment:20 @jeremyfelt20 months 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.

comment:21 @DrewAPicture20 months ago

  • Cc xoodrew@… added

comment:22 @r-a-y20 months ago

  • Cc r-a-y@… added

comment:23 @nacin19 months ago

  • Milestone changed from 3.7 to 3.8

Per conversation with jeremyfelt, pushing this to 3.8.

comment:24 @jeremyfelt17 months 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.

comment:25 @desrosj16 months ago

  • Cc desrosj@… added

comment:26 @jeremyfelt15 months ago

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

comment:27 @nafur8 months 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...

comment:28 @jdaviescoates7 months 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.

comment:29 @boonebgorges3 months ago

#31016 was marked as a duplicate.

comment:30 @ArkonLabs3 months 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?

comment:31 @imath3 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.

@imath3 months ago

@imath3 months ago

comment:32 @slackbot3 months ago

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

comment:33 @boonebgorges3 weeks ago

In 31970:

Unit tests for wpmu_validate_user_signup().

See #17904, #26784.

comment:34 @pento3 weeks ago

In 31978:

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

See #17904, #26784.

Note: See TracTickets for help on using tickets.