Make WordPress Core

Opened 15 years ago

Closed 8 years ago

Last modified 8 years ago

#9568 closed feature request (fixed)

Allow users to log in using their email address

Reported by: denis-de-bernardy's profile Denis-de-Bernardy Owned by: ocean90's profile ocean90
Milestone: 4.5 Priority: normal
Severity: normal Version: 2.8
Component: Users Keywords: has-patch has-unit-tests has-screenshots commit
Focuses: Cc:

Description

I've been looking into a few tickets and adding patches to things that were potentially problematic when implementing a membership plugin (#1626, #4170, #9563, #9564). The general idea being twofold: to avoid duplicate users at all costs -- because it's a bloody mess to merge users in a billing system.

I'm about to embark on a mid-sized patch that renders the WP username field optional. This is a natural next step from #9563 and #9564.

Users could then be allowed to specify an email only, when registering. And they could use their email to login -- in addition to their username if they specified one.

Attachments (18)

9568.diff (7.6 KB) - added by Denis-de-Bernardy 15 years ago.
updated patch, against 10966
9568.2.diff (7.6 KB) - added by Denis-de-Bernardy 15 years ago.
patch against 11077
9568.3.diff (9.5 KB) - added by Denis-de-Bernardy 15 years ago.
patch against 11130, using ID as user_nicename
9568.4.diff (9.7 KB) - added by Denis-de-Bernardy 15 years ago.
as 9568.3.diff with a different approach for extracting a user_nicename from the email
9568.5.diff (9.8 KB) - added by Denis-de-Bernardy 15 years ago.
yet another refresh, against 11256
9568.6.diff (9.8 KB) - added by Denis-de-Bernardy 15 years ago.
9568.7.diff (9.2 KB) - added by Denis-de-Bernardy 15 years ago.
refreshed patch
9568.8.diff (7.2 KB) - added by ericlewis 9 years ago.
9568.9.diff (7.8 KB) - added by vhomenko 9 years ago.
auth_with_login_tests.php (2.1 KB) - added by vhomenko 9 years ago.
add wp_authenticate_username_password and wp_authenticate_email_password unit tests to wordpress-develop/tests/phpunit/tests/auth.php
9568.10.diff (8.3 KB) - added by MikeHansenMe 9 years ago.
patch with simple test in auth.php
9568.11.diff (6.3 KB) - added by swissspidy 8 years ago.
Screen Shot 2016-01-19 at 9.10.09 AM.png (18.9 KB) - added by ryan 8 years ago.
wordpress.com
wordpressdotcom-registration.png (23.1 KB) - added by swissspidy 8 years ago.
Registration error on WordPress.com (for completeness)
9568-username.png (77.3 KB) - added by ocean90 8 years ago.
9568-email.png (86.4 KB) - added by ocean90 8 years ago.
9568.12.diff (6.1 KB) - added by swissspidy 8 years ago.
9568.13.diff (6.4 KB) - added by ocean90 8 years ago.

Download all attachments as: .zip

Change History (122)

#1 @Denis-de-Bernardy
15 years ago

  • Keywords has-patch tested added

patch is well tested, but it could always use a little more of it. it includes #9563 and #9564, which are prerequisites to make this work.

Detailed Changes:

  • Username is still available, but no longer is a required field
  • If left empty, the username will default to the email address (and change when the email address changes)
  • Note that internally, the username is still used, to ensure backward compatibility
  • The various form labels and error messages have been changed to reflect the optional nature of the username

#2 @Denis-de-Bernardy
15 years ago

  • Keywords needs-patch added; has-patch tested removed

patch no longer works against the current trunk

@Denis-de-Bernardy
15 years ago

updated patch, against 10966

#3 @Denis-de-Bernardy
15 years ago

tests conducted:

  • register user without a username from register form and users / add new
  • change user email from profile and users / edit
  • verified user is not logged out when changing email under profile

#4 @ryan
15 years ago

I like this in principle. I'll try out the patch after the weekend.

#5 @Denis-de-Bernardy
15 years ago

sigh... patch no longer applies cleanly :P

@Denis-de-Bernardy
15 years ago

patch against 11077

#6 @Denis-de-Bernardy
15 years ago

  • Keywords has-patch tested added; needs-patch removed

actually, it did work:

patching file wp-login.php
patching file wp-includes/registration.php
Hunk #1 succeeded at 188 with fuzz 2 (offset 12 lines).
Hunk #2 succeeded at 266 (offset 12 lines).
patching file wp-admin/includes/user.php
patching file wp-admin/user-new.php
patching file wp-admin/user-edit.php
Hunk #1 succeeded at 246 (offset 8 lines).

but refreshed it nonetheless.

#7 @Denis-de-Bernardy
15 years ago

  • Keywords commit added

Please? :-)

#8 @misterbisson
15 years ago

+1

I've been working on this same issue for a while myself (implementing it entirely as a plugin). Though I'm about to go live with a solution, core support would be great.

Thinking forward a bit, this has some implications for MU and BuddyPress (and it's inside BP that I'm trying to solve this). How would BP construct a persistent user profile URL if not by user_login (as it currently does), for instance. Still, there's an easy answer to that (use the users.ID, I guess), and being able to use an email address as username is an important big step.

#9 @ryan
15 years ago

I'm curious about user_nicename impact as well. I'd like to do this, but maybe we should wait for 2.9.

#10 @Denis-de-Bernardy
15 years ago

@misterbisson: Agreed. What prompted me to write the patch was that this wasn't trivial to implement with a plugin, and is a much needed functionality when you're dealing with a membership site.

@Ryan: foo[at]bar.com gets sanitized as foo-bar-com (or foobar-com) as the user_nicename. It presumably doesn't get changed afterwards due to this check in wp_insert_user():

if ( empty($user_nicename) )
  $user_nicename = sanitize_title( $user_login );

It's the only place where it actually gets written. Else, it's behaving as a read-only field. If you'd rather see it changed as the username changes, just let me know and I'll adjust the patch accordingly.

#11 @Denis-de-Bernardy
15 years ago

Thinking out loud... Another possibility could be to leave an empty login field around that is actually writable in the profile/edit user page. Upon filling it once, it would become read-only and change the username accordingly at that time.

Whichever option feels best to you will work for me. Just let me know how you'd prefer this to get implemented and/or what you are dreading so I can check and test, so we can get this into 2.8.

#12 @ryan
15 years ago

Also thinking out loud... user_nicename is exposed in various places (author links). Having it be based on email means exposing an email address.

#13 @Denis-de-Bernardy
15 years ago

  • Keywords needs-patch added; has-patch tested commit removed

sounds reasonable. I'll refresh the patch and make it use the user's ID instead for those who are using their email address as a login.

@Denis-de-Bernardy
15 years ago

patch against 11130, using ID as user_nicename

#14 @Denis-de-Bernardy
15 years ago

  • Keywords has-patch tested added; needs-patch removed

uploaded two patches against the current trunk.

  • 9568.3.diff post-processes inserts so as to use the ID instead of the sanitized email
  • 9568.4.diff takes another approach, and uses preg_replace("/@.*/", "", $user_email) as the user_name

If you've other suggestions/potential objections, just let me know and I'll get them sorted out.

@Denis-de-Bernardy
15 years ago

as 9568.3.diff with a different approach for extracting a user_nicename from the email

#15 follow-up: @misterbisson
15 years ago

I'm concerned that using preg_replace("/@.*/", "", $user_email) as the username creates a large opportunity for duplicate usernames. Also, the idea of building permalinks against an email address (which will necessarily change in my application as applicants become students at my university) seems off.

This isn't the solution, but the plugin I developed filters sanitize_user (which is used for all sorts of things other than actually sanitizing usernames/aside) and replaces it with a random string (pattern: 'p'. rand( 0, 9 ) . randomstring( 7, 'abcdefghijklmnopqrstuvwxyz' )). I then replaced wp_authenticate() so it tests against get_user_by_email().

As noted above, that isn't a solution for anybody but me, but it does result in slightly more friendly author URLs than numbers alone. Still, I'd prefer using the user ID over using anything based on email address (other than, perhaps, an md5 of the email address).

#16 in reply to: ↑ 15 ; follow-up: @Denis-de-Bernardy
15 years ago

Replying to misterbisson:

I'm concerned that using preg_replace("/@.*/", "", $user_email) as the username creates a large opportunity for duplicate usernames.

It's the user_nicename, and there is a function further down that ensures it's unique.

Also, the idea of building permalinks against an email address (which will necessarily change in my application as applicants become students at my university) seems off.

Once set, the user_nicename isn't changed.

Still, I'd prefer using the user ID over using anything based on email address (other than, perhaps, an md5 of the email address).

Oh, that's what the replace is about. john.doe@… would have a nicename of john-doe (or john-doe-2 if there is one already). if this is a problem on some servers, one could use a plugin to change it as needed.

#17 in reply to: ↑ 16 @misterbisson
15 years ago

Replying to Denis-de-Bernardy:
True. I can set user_nicename to whatever I want with the pre_user_nicename filter. I'm good with either patch.

#18 @Denis-de-Bernardy
15 years ago

  • Keywords commit added

#19 @Denis-de-Bernardy
15 years ago

see also #9577 if this makes it into 2.8. hoping it does...

@Denis-de-Bernardy
15 years ago

yet another refresh, against 11256

#20 @Denis-de-Bernardy
15 years ago

@ryan: could this get committed before the patch needs yet another refresh? :-|

#21 @Denis-de-Bernardy
15 years ago

  • Priority changed from normal to high

#22 @Denis-de-Bernardy
15 years ago

tests done:

  • create a user with no username (succeeds), from register form and edit user form
  • create a user with existing email (fails), from register form and edit user form
  • update user to new email (succeeds)
  • update user to existing email (fails)
  • create user with dup user_nicename based on email (succeeds with a suffix)

dunno how to test this any further...

#23 @Denis-de-Bernardy
15 years ago

oh, and the latest patch users the email up to the @ as a nicename, so as to not reveal it in the user_nicename.

#24 @Denis-de-Bernardy
15 years ago

  • Severity changed from normal to major

#25 @markjaquith
15 years ago

I really like this idea, especially since the e-mail address exposure issue has been mitigated. I'm all for this as an early 2.9 feature addition.

#26 @Denis-de-Bernardy
15 years ago

Are you sure you can't add this to 2.8? I'd really hate to need to override entire pages in WP to make my plugin work? :-|

#27 @Denis-de-Bernardy
15 years ago

  • Status changed from new to accepted

#28 @westi
15 years ago

  • Keywords early added
  • Milestone changed from 2.8 to 2.9

This is a big change and I agree with Mark that it should wait for early in 2.9

#29 @Denis-de-Bernardy
15 years ago

  • Keywords needs-patch added; has-patch tested commit removed

needs a new refresh

#30 @Denis-de-Bernardy
15 years ago

  • Keywords has-patch added; needs-patch removed

#31 @Denis-de-Bernardy
15 years ago

haven't tested the refresh yet.

#32 @westi
15 years ago

Not sure that making username optional is the right way to go.

It feels like it would be nicer to work like quite a few other sites do and that is required you to pick a username on signup but then let you log in with your username or your email address at a later date.

#33 @Denis-de-Bernardy
15 years ago

Sometimes, you need to register users without prompting them to enter a username. When importing data from a separate application, for instance. How about I change it as follows:

  • Require the username on signup
  • If username = email, allow user to edit it and refuse to save until he picks one
  • Once username <> email, refuse to edit username

That way, we'd have the best of both worlds. Thoughts?

#34 @aaroncampbell
15 years ago

  • Cc aaroncampbell added

#35 follow-ups: @aaroncampbell
15 years ago

I haven't gone through everything here yet (there's a lot of patches here), but I definitely have to say that logging in with an E-Mail is a big deal, and implemented on almost all the sites I do. I'd love to see this taken a little further, because right now most of what I do is simply:

function allow_email_login($user, $pass) {
     global $wpdb;
     if (is_email($user)) {
          $user = $wpdb->get_var("SELECT user_login FROM $wpdb->users WHERE user_email = '$user'");
     }
     return;
}
add_action('wp_authenticate', 'allow_email_login', 0, 2);

The problem is that when we import users (for example from an existing fulfillment or billing system), we still have to create some sort of (rather pointless) username that isn't used anywhere.

#36 in reply to: ↑ 35 @Denis-de-Bernardy
15 years ago

Replying to aaroncampbell:

The problem is that when we import users (for example from an existing fulfillment or billing system), we still have to create some sort of (rather pointless) username that isn't used anywhere.

Hehe. That's the very reason I opened it in the first place. :-)

#37 follow-up: @Denis-de-Bernardy
15 years ago

@aaron: thoughts on this?

http://core.trac.wordpress.org/ticket/9568#comment:33

I'm still hesitating on whether to make it a mandatory field (in which case you'd *need* to fill it in) or an optional one.

#38 in reply to: ↑ 37 @aaroncampbell
15 years ago

Replying to Denis-de-Bernardy:

@aaron: thoughts on this?

http://core.trac.wordpress.org/ticket/9568#comment:33

I'm still hesitating on whether to make it a mandatory field (in which case you'd *need* to fill it in) or an optional one.

The problem still lies in the automated imports. I've found that a lot of places (Harvard for instance) tend to collect very little info on customers. We had to import a lot of users where they had little more than an E-Mail address, so we had to create a bogus username (which was ugly). I'd like to see it be an "either or" kind of thing, and I see two ways to do that:

  1. Make a setting to change from using usernames for logging in (and for permalinks etc) to using E-Mail addresses. This would mean that when the blog owner tried to change from one to the other that we need to do some error checking to make sure that the chosen column has unique values (This is because I envision being able to have empty usernames).
  1. Simply open it up and make it so a user can log in with either a username OR email, and that only one is required (although both could be filled in). The difficulty here is that you need to enforce a multi-column unique key that allows for empty values (which there could be plenty of).

I'm thinking less on the user's end when they sign up, and more about the back end for when they're imported.

#39 @Denis-de-Bernardy
15 years ago

Well, what the patch currently does is this:

  • set user_login to user_email when no user_login is supplied
  • for such users, work out a user_nicename (used in permalinks) based on the first part of the email (the stuff up to the @, basically), which is left unchanged from that point forward
  • for users where user_email = user_login, change user_login along with user_email; for users where user_email <> user_login, keep user_login
  • allow to log in with either of user_login or user_email

the only real remaining question are:

  • per westi's suggestion, whether we should allow users to pick a user_login when none is present
  • per simonwheatley's question in IRC yesterday, whether we should allow users to change their user_login when one is present

#40 @dd32
15 years ago

per westi's suggestion, whether we should allow users to pick a user_login when none is present

If one is auto-generated from the email.. Nah.. well.. Not unless every user can change their usernames - Which would be a nice feature, But permalink history'ness and all that..

My main question is:

  • What happens when a user changes their email, Does their user_login change to reflect it? IMO, It should stay static, So that behaviour will need to be documented for developers who may rely on it for internal systems/custom integrations.

#41 @Denis-de-Bernardy
15 years ago

@DD32: To the end user, it looks as if there is no user_login field at all. In the background, I keep user_login in sync with the email without ever displaying the login field.

Westi's question goes down to do whether to show a blank login field that the user can then fill out to choose a (final) user_login when he currently has none. I like the idea, personally.

#42 @dd32
15 years ago

Ah, just looked at the patch. Looks like thats a nice way to go about it.

Westi's idea of letting the user choose a username later on sounds like a good idea, Just a extra explanation of "You currently do not have a username, You may optionally create one now. Please note that once your username is chosen, You cannot change it"

#43 @Denis-de-Bernardy
15 years ago

Adding this for reference, by DD32 in IRC:

I just noticed that bbPress allows email login too now, You might want to check up on their implementation, since bbpress/wordpress are integratable

#44 @aaroncampbell
15 years ago

I looked at the patch, and I like what's going on. I agree with dd32 that a little extra instruction would be good, but it also looks like you lost the "<strong>ERROR</strong>: Please enter a username." error that currently shows if no username is entered for login. And else added to your if ( !empty($user_login) ) could easily bring it back though.

Also, I really like the idea of letting users change their login, but maybe we can implement that after we have permalink history (and probably from another ticket).

@Denis-de-Bernardy
15 years ago

refreshed patch

#45 @Denis-de-Bernardy
15 years ago

  • Keywords needs-testing added

refreshed patch attached, that takes comments from above into account.

  • registration form -> unchanged (requires a username)
  • login form -> allow to log in using username or email
  • internal functions -> mostly changed as was previously done: if no username is present, use the email instead (this is to allow for imports from 3rd party software that lack a username)
  • edit user form -> it now requires the user to fill in a username if it was lacking
  • new user form -> it allows to fill in no username (this is mostly for testing purposes, but this also allows users registered by the admin to choose one to their liking)

needs testing.

#46 @Denis-de-Bernardy
15 years ago

reposting previous comment with formatting:

  • registration form -> unchanged (requires a username)
  • login form -> allow to log in using username or email
  • internal functions -> mostly changed as was previously done: if no username is present, use the email instead (this is to allow for imports from 3rd party software that lack a username)
  • edit user form -> it now requires the user to fill in a username if it was lacking *new user form -> it allows to fill in no username (this is mostly for testing purposes, but this also allows users registered by the admin to choose one to their liking)

#47 @Denis-de-Bernardy
15 years ago

  • Owner Denis-de-Bernardy deleted
  • Status changed from accepted to assigned

#48 @ryan
14 years ago

  • Milestone changed from 2.9 to Future Release

#49 @asbjornu
14 years ago

What's the plan with applying this patch? There was mentioning of including it in 2.9, but that obviously didn't happen and now it's set to "Future Release". Does that mean it will be included in a dot release of 2.9 or do we have to wait until 3.0 to see this implemented?

I'm having lots of customers that would love to just eradicate the user name from all registration and login forms, but at the moment that requires a heap of custom code fiddling, core file patching and whatnot. Having this supported in the core is very, very welcome!

#50 @asbjornu
14 years ago

  • Cc asbjornu added

#51 @nacin
14 years ago

Future Release means it is currently not slated for any milestone. You won't see it before 3.1.

See also http://wordpress.org/extend/plugins/wp-email-login/

#52 in reply to: ↑ 35 @aaroncampbell
14 years ago

  • Milestone changed from Future Release to 3.1

It doesn't take a lot of code or any modification of core files to allow E-Mail login, just 8 lines of code posted above. You will have to make sure that you collect the E-Mail address on registration though.

This is definitely something I'd like to see though, so I'm going to bump it up to 3.1 so I can take a look at it after the 3.0 release.

#53 @mikeschinkel
14 years ago

  • Cc mikeschinkel@… added

I can't tell from reading through the comments but I hope that if this patch is applied a site owner can still require a user to provide a username while still allowing them to login with an email address.

#54 @F J Kaiser
14 years ago

  • Cc 24-7@… added

#56 @nacin
13 years ago

  • Milestone changed from Awaiting Triage to Future Release

#57 @norbertm
13 years ago

  • Cc norbert@… added

#58 @johnbillion
13 years ago

  • Cc johnbillion@… added

#59 @gruvii
12 years ago

  • Cc gruvii added

#60 @webord
11 years ago

  • Cc bordoni.dev@… added

#61 @aniketpant
11 years ago

  • Cc me@… added

#62 @mau
11 years ago

  • Cc ngomau@… added

#63 @ocean90
9 years ago

#30626 was marked as a duplicate.

#64 @gputignano
9 years ago

Is it time to consider this enhancement?

#65 @ericlewis
9 years ago

  • Milestone changed from Future Release to 4.2

Let's get into this for 4.2. Anyone want to check up on the patch attachment:9568.7.diff and make sure it's fresh and still makes sense?

#66 @pento
9 years ago

  • Keywords needs-refresh needs-unit-tests added; early removed
  • Priority changed from high to normal
  • Severity changed from major to normal

9568.7.diff doesn't apply against trunk at all, and has no unit tests. Fixing that up is probably a good place to start, for discussing putting it into 4.2.

@ericlewis
9 years ago

#67 @ericlewis
9 years ago

attachment:9568.8.diff is a fresh approach here. Rather than usernames to be a non-required field, modify the login process to allow for logging in with user email.

wp_signon() is wp-login.php's form submission handler, which calls wp_authenticate(), the canonical "authenticate username and password" function, which is also used by XMLRPC. This patch changes the $username parameter to $user_identifier, which supports usernames for back-compat, and now also emails.

Authentication logic (i.e. is this username and password valid?) actually takes place in a callback on the authenticate filter. wp_authenticate_username_password() is the hooked function for checking usernames and passwords. I've added another callback for wp_authenticate_email_password(). The two share a good amount of logic. We could create another function like wp_authenticate_user_field_and_password( $field_name, $field_value, $password ), which could share almost all this logic, and then the username and email functions would proxy to this function.

I've modified the two functions to coallate error messages, so we can figure out what the best way to present error messages to the user (this needs ironing out). e.g. so if you enter "eric@…," we might say "The username or email address and password combination was incorrect." rather than just the last-in error "The email address and password combination was incorrect."

#68 @vhomenko
9 years ago

In case of wrong username or password:

Notice: Undefined variable: username in /var/www/html/wordpress-develop/src/wp-includes/pluggable.php on line 573

Should be $user_identifier instead as both are strings?

@vhomenko
9 years ago

#69 @vhomenko
9 years ago

I discovered that the following part of the wp-login.php wasn't considering the possibility of an email based login:

case 'login' :
default:

...

	// If the user wants ssl but the session is not ssl, force a secure cookie.
	if ( !empty($_POST['log']) && !force_ssl_admin() ) {
		$user_name = sanitize_user($_POST['log']);

		if ( $user = get_user_by('login', $user_name) ) {
...

I updated the patch with a fix.

The Undefined variable: username in wp_authenticate() is also fixed.

Newbie question: Could anyone give me a few pointers, as to what kind of tests are necessary for the wp_authenticate_email_password()? I was hoping there'd be a test case for wp_authenticate_username_password() that I could bootstrap from. Does it suffice to write some unit tests for wp_authenticate()?

@vhomenko
9 years ago

add wp_authenticate_username_password and wp_authenticate_email_password unit tests to wordpress-develop/tests/phpunit/tests/auth.php

#70 @vhomenko
9 years ago

Please review the unit tests.

Also, there is an UX issue: if the user enters a wrong email address at the login screen, there are two error messages displayed:

ERROR: Invalid username. Lost your password?
ERROR: Invalid email address.

While the wp_authenticate_email_password() returns if the given email doesn't validate, making wp_authenticate_username_password() do a similar check seems non-trivial to me.

I'd propose using the wp_login_errors filter, that's applied in the end of the wp-login.php, to purge the 'invalid_username' entry, if there is 'invalid_email', in the WP_Error object.

If that's acceptable, then where would be a good place to put the callback function?

#71 @vhomenko
9 years ago

Oops. auth_with_login_tests.php attachment is actually a diff file.

#72 @vhomenko
9 years ago

A snippet to reduce the noise level of the errors, in case of entering a wrong email address into the login form.

add_filter( 'wp_login_errors', 'consolidate_wrong_user_id_errors', 20 );

function consolidate_wrong_user_id_errors( $errors ) {
	if ( ! is_wp_error( $errors ) ) return $errors;
	if ( 2 !== count( $errors->errors ) ) return $errors;

	$codes = $errors->get_error_codes();
	$is_invalid_username = in_array( 'invalid_username', $codes );
	$is_invalid_email = in_array( 'invalid_email', $codes );

	if ( $is_invalid_username && $is_invalid_email ) {
		$errors->remove( 'invalid_username' );
	}

	return $errors;
}

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


9 years ago

#74 @DrewAPicture
9 years ago

  • Priority changed from normal to low

@MikeHansenMe
9 years ago

patch with simple test in auth.php

#75 @MikeHansenMe
9 years ago

  • Keywords needs-refresh needs-unit-tests removed

#76 @DrewAPicture
9 years ago

9568.10.diff still applies. Some feedback:

  • Any of the hooks where the $username parameter is changed, the parameter description should reflect that it could be a username or email and a changelog entries should be added to the hook docs to reflect that emails are now also possible
  • "Collate" is misspelled as "Coallate" in a couple of places
  • Coding standards (mostly spacing) should be applied to changed or new code
  • The test method should have a docblock with @ticket 9568 so that test can be targeted by group (test passes)

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


9 years ago

#78 @helen
9 years ago

  • Milestone changed from 4.2 to Future Release

Seems like this still needs a little work, and definitely needs more review.

#79 @swissspidy
8 years ago

  • Keywords needs-refresh added

@swissspidy
8 years ago

#80 @swissspidy
8 years ago

  • Keywords has-unit-tests dev-feedback added; needs-testing needs-refresh removed
  • Milestone changed from Future Release to 4.5
  • Priority changed from low to normal

I just added 9568.11.diff which applies cleanly against trunk again. It's what I'd call a minimum viable patch:

  • No unnecessary variable changes
  • Adds @ticket 9568 to the unit test
  • Updates docs, removes duplicated hook docs

The goal is simple: Do not change any behaviour regarding user registration, just allow logging in using your email address for more comfort.

Changing milestone for consideration for 4.5. Happy to work more on this patch if necessary.

#81 @swissspidy
8 years ago

  • Focuses docs added

#82 @johnbillion
8 years ago

Important consideration: the error message reported when a valid email address is entered can lead to information disclosure. It allows an attacker to determine which email addresses are in use on the site. (The same issue does not apply to usernames as usernames are not considered private information.)

This change should not be implemented until it has been cleared by the WordPress Security Team.

#83 @DrewAPicture
8 years ago

  • Focuses docs removed

#84 @swissspidy
8 years ago

Important consideration: the error message reported when a valid email address is entered can lead to information disclosure. It allows an attacker to determine which email addresses are in use on the site. (The same issue does not apply to usernames as usernames are not considered private information.)

Absolutely valid point. Perhaps it's time to get rid of the detailed error message and just show a "Incorrect email or password" message. That would fix the information disclosure issue. See also #12129.

#85 @ryan
8 years ago

The security team discussed this here (a rare private channel, sorry): https://wordpress.slack.com/archives/security/p1453132031000512

I'll summarize.

otto42 did a survey of other sites. They all leak.

Facebook: "The email you’ve entered doesn’t match any account. Sign up for an account."
gmail: "Sorry, Google doesn't recognize that email. Create an account using that address?"
iCloud: "xxxx@… is not an Apple ID"
Microsoft: "That Microsoft account doesn't exist. Enter a different account or get a new one."

He also reminded us that email is discoverable via signup. https://kev.inburke.com/kevin/invalid-username-or-password-useless/

nacin reminded that these massive sites have network level monitoring and ways of mitigating brute force. Individual WordPress sites don't have that. We must keep that in mind when surveying the field.

Log in is important threshold flow that should be friendly as can be. Given that everyone leaks and that emails are discoverable in other ways, my inclination is to optimize for usability.

@ryan
8 years ago

wordpress.com

@swissspidy
8 years ago

Registration error on WordPress.com (for completeness)

#86 follow-up: @swissspidy
8 years ago

Thanks for this summary!

nacin reminded that these massive sites have network level monitoring and ways of mitigating brute force. Individual WordPress sites don't have that. We must keep that in mind when surveying the field.

What can we do for WordPress sites in that regard to keep them safe when we'd implement this? The Two-Factor feature plugin comes to mind, as well es dozens of plugins limiting login attempts, banning IPs, etc.

#87 in reply to: ↑ 86 ; follow-up: @dd32
8 years ago

Replying to swissspidy:

Thanks for this summary!

nacin reminded that these massive sites have network level monitoring and ways of mitigating brute force. Individual WordPress sites don't have that. We must keep that in mind when surveying the field.

What can we do for WordPress sites in that regard to keep them safe when we'd implement this? The Two-Factor feature plugin comes to mind, as well es dozens of plugins limiting login attempts, banning IPs, etc.

I don't think there's really any way for core to do the same as the larger sites, they've got infrastructure that individual WordPress sites simply don't have.
So I think limiting login attempts / enumeration is best left to plugins for the time being.

I agree with @ryan that we should optimise for usability, if that means the email address becomes slightly easier to verify, that's the price we pay for a smoother friendlier experience for our users.

#88 in reply to: ↑ 87 @aaroncampbell
8 years ago

Just weighing in and agreeing with @ryan and @dd32. I think the benefits of a smoother, more user-friendly flow outweigh the increase in E-Mail discoverability. I'm all for optimizing for usability in this case.

#89 @swissspidy
8 years ago

  • Keywords dev-feedback removed

#90 @swissspidy
8 years ago

  • Keywords needs-screenshots added
  • Owner set to swissspidy

Gonna take some screenshots with the latest patch applied.

#91 follow-up: @ocean90
8 years ago

9568.11.diff In wp_authenticate_email_password() is_email( $email ) is used which returns false in case $email is empty. So the following empty( $email ) checks seem to be unnecessary.

@ocean90
8 years ago

@ocean90
8 years ago

#92 @ocean90
8 years ago

  • Keywords has-screenshots needs-refresh added; needs-screenshots removed

#93 in reply to: ↑ 91 @swissspidy
8 years ago

Replying to ocean90:

9568.11.diff In wp_authenticate_email_password() is_email( $email ) is used which returns false in case $email is empty. So the following empty( $email ) checks seem to be unnecessary.

Thanks for the heads up. That means I can simplify the condition there a bit, which I'll do over the coming days.

@swissspidy
8 years ago

#94 @swissspidy
8 years ago

  • Keywords needs-refresh removed

9568.12.diff is a new patch which simplifies the logic inside wp_authenticate_email_password() and greatly improves the error messages after having been recently improved in wp_authenticate_username_password().

I think that's it. @ocean90 wanna do some final review?

#95 @ocean90
8 years ago

  • Owner changed from swissspidy to ocean90
  • Status changed from assigned to reviewing

@ocean90
8 years ago

#96 @ocean90
8 years ago

  • Keywords commit added

In 9568.13.diff I restored the empty email and password conditions. This improves the auth handling if the wp_authenticate_username_password() handler is removed.

This looks ready to go.

#97 @ocean90
8 years ago

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

In 36617:

Authentication: Allow users to log in using their email address.

Introduces wp_authenticate_email_password() which is hooked into authenticate after wp_authenticate_username_password().

Props Denis-de-Bernardy, ericlewis, vhomenko, MikeHansenMe, swissspidy, ocean90.
Fixes #9568.

#98 @ocean90
8 years ago

In 36620:

Template: Update label for the username field in wp_login_form() after [36617].

See #9568.

#99 @pento
8 years ago

See #35909 for performance chat.

#100 @ocean90
8 years ago

In 36654:

Schema: Add an index to wp_users.user_email.

Improves lookup of an email address on large user tables.

See #9568.
Fixes #33376.

#101 @johnbillion
8 years ago

In 36992:

Users: Add @since entries to wp_authenticate() and its filters now that the $username parameter can also be an email address.

See #9568, #35986

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


8 years ago

#103 @DrewAPicture
8 years ago

In 37007:

Docs: Use a third-person singular verb in the DocBlock summary for wp_authenticate_email_password(), introduced in [36617].

See #9568. See #35986.

#104 @DrewAPicture
8 years ago

In 37030:

Docs: Improve 4.5 changelog entries introduced in [36992] for wp_authenticate(), and the authenticate and wp_login_failed hooks.

See #9568. See #35986.

Note: See TracTickets for help on using tickets.