[wp-trac] [WordPress Trac] #17904: Multisite has more restrictions on user login character set

[WordPress Trac]

#17904: Multisite has more restrictions on user login character set
--------------------------------------+-------------------------
 Reporter:  duck_                     |       Owner:  jeremyfelt
     Type:  defect (bug)              |      Status:  assigned
 Priority:  normal                    |   Milestone:  4.6
Component:  Login and Registration    |     Version:  3.0
 Severity:  normal                    |  Resolution:
 Keywords:  has-patch has-unit-tests  |     Focuses:  multisite
--------------------------------------+-------------------------

Comment (by jeremyfelt):

 Replying to [comment:49 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
 [https://core.trac.wordpress.org/ticket/17904#comment:20 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?

--
Ticket URL: <https://core.trac.wordpress.org/ticket/17904#comment:52>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform