Allow password reset on account with the username matching another email; prevent registrations that match another account

A test to demonstrate the issue (should fail). Do we seriously not have a test for duplicate usernames yet, or am I just missing it?

A patch to crosscheck the username against the email field and vice-versa. Also includes the aforementioned tests.

Didn't test, but looking over the patch it looks good.

See also #111317: Allow users to login using either their username OR their e-mail address.

Switching to User.module. To clarify, this patch places a constraint on the User.module, not on the user system. Important distinction should the User.module become plugable in the future.

Patch looks right.

Only a minor formatting thing:

+++ b/core/modules/user/user.module
@@ -1137,12 +1137,15 @@ function user_validate_current_pass(&$form, &$form_state) {
+

Is that extra line needed?

-27 days to next Drupal core point release.

Applied changes per #6

hmmm - reverting to original patch from #2. Please use patch #2 above.

Trying to get it back to #2 then will change to needs work (for #6).

#2: username_email_crosscheck-1359718-2.patch queued for re-testing.

This should only need to change the formatting detail mentioned on #6 and I would say it's ready.

BTW this affects directly one core functionality, so moving to normal.

I think I've squashed the single space issue that #15 is talking about.

turning to needs review so test bot wakes up

Thanks!

Sorry to set this back on these minors:

+++ b/core/modules/user/user.testundefined
@@ -133,6 +133,29 @@ class UserRegistrationTestCase extends DrupalWebTestCase {
+    $this->assertText(t('The name @name is already taken.', array('@name' => $duplicate_user->name)), t('Supplying an exact duplicate username displays an error message'));

The assertion message 'Supplying an exact ...' should not be wrapped in t().

+++ b/core/modules/user/user.testundefined
@@ -133,6 +133,29 @@ class UserRegistrationTestCase extends DrupalWebTestCase {
+    $this->assertText(t('The name @name is already taken.', array('@name' => $duplicate_user->mail)), t('Supplying a username equal to an email address of an already registered user displays an error message'));

Same thing here.

Without t() what would be the best way to do keyword replacement? preg_replace()?

@cosmicdreams: It's only about the second parameter, not the first one ;-)

Here's the cleaned up patch

switch to patch review mode.

Thanks!

BTW Thanks to Niklas Fiekas for the review too.

Thanks @Niklas Fiekas, @marvil07, and @cosmicdreams. Few more minor points for cleanup:

+++ b/core/modules/user/user.moduleundefined
@@ -1137,12 +1137,14 @@ function user_validate_current_pass(&$form, &$form_state) {
+    elseif ((bool) db_select('users')->fields('users', array('uid'))->condition('uid', $account->uid, '<>')->condition(db_or()->condition('name', db_like($form_state['values']['name']), 'LIKE')->condition('mail', db_like($form_state['values']['name']), 'LIKE'))->range(0, 1)->execute()->fetchField()) {

@@ -1152,11 +1154,12 @@ function user_account_form_validate($form, &$form_state) {
+  elseif ((bool) db_select('users')->fields('users', array('uid'))->condition('uid', $account->uid, '<>')->condition(db_or()->condition('mail', db_like($form_state['values']['mail']), 'LIKE')->condition('name', db_like($form_state['values']['mail']), 'LIKE'))->range(0, 1)->execute()->fetchField()) {

Egad. I know it was already this way, but damn are these hard to read.

+++ b/core/modules/user/user.testundefined
@@ -124,13 +124,42 @@ class UserRegistrationTestCase extends DrupalWebTestCase {
+  function testRegistrationUsernameDuplicates() {

This test method needs a docblock. Reference: http://drupal.org/node/1354#functions

+++ b/core/modules/user/user.testundefined
@@ -124,13 +124,42 @@ class UserRegistrationTestCase extends DrupalWebTestCase {
+    $this->assertText(t('The name @name is already taken.', array('@name' => $duplicate_user->name)), t('Supplying an exact duplicate username displays an error message'));
...
+    $this->assertText(t('The name @name is already taken.', array('@name' => $duplicate_user->mail)), t('Supplying a username equal to an email address of an already registered user displays an error message'));

The assertion messages here (final arguments) should not be translated with t(). Reference: http://drupal.org/simpletest-tutorial-drupal7#t

Also, we should backport this fix.

+++ b/core/modules/user/user.testundefined
@@ -124,13 +124,42 @@ class UserRegistrationTestCase extends DrupalWebTestCase {
-    $this->assertText(t('The e-mail address @email is already registered.', array('@email' => $duplicate_user->mail)), t('Supplying an exact duplicate email address displays an error message'));
+    $this->assertText(
+      t('The e-mail address @email is already registered.', array('@email' => $duplicate_user->mail)),
+      'Supplying an exact duplicate email address displays an error message'
+    );
 
     // Attempt to bypass duplicate email registration validation by adding spaces.
     $edit['mail'] = '   ' . $duplicate_user->mail . '   ';
 
     $this->drupalPost('user/register', $edit, t('Create new account'));
-    $this->assertText(t('The e-mail address @email is already registered.', array('@email' => $duplicate_user->mail)), t('Supplying a duplicate email address with added whitespace displays an error message'));
+    $this->assertText(
+      t('The e-mail address @email is already registered.', array('@email' => $duplicate_user->mail)),
+      'Supplying a duplicate email address with added whitespace displays an error message'

We shouldn't touch this just for cleaning it up. I suppose you did this cleanups because I wasn't very clear about the t(...) thing.

+++ b/core/modules/user/user.testundefined
@@ -124,13 +124,42 @@ class UserRegistrationTestCase extends DrupalWebTestCase {
+    $this->assertText(t('The name @name is already taken.', array('@name' => $duplicate_user->name)), t('Supplying an exact duplicate username displays an error message'));

Actually I meant this one.

+++ b/core/modules/user/user.testundefined
@@ -124,13 +124,42 @@ class UserRegistrationTestCase extends DrupalWebTestCase {
+    $this->assertText(t('The name @name is already taken.', array('@name' => $duplicate_user->mail)), t('Supplying a username equal to an email address of an already registered user displays an error message'));

And this one. Sorry.

#1: username_email_crosscheck-1359718-1.patch queued for re-testing.

I'll have an opportunity to fix the patch later tonight.

Ah, I spaced out and didn't notice those lines weren't part of the fix. @Niklas Fiekas is correct; we should not have any changes in user.test outside of the new test method.

@Niklas Fiekas I've read and reread you post at #27 and I'm confused. You seem to be objecting to the two changes I made in the patch and then you are telling me that I should have made those very same changes.

If I'm not adhering to a coding standard by ensuring that the line of code stays less that 80 characters wide please let me know. But I think you should reread what you post and let me know if you're seeing the same thing I am.

To be clear, The reason why the patch is effecting more lines of code that may seem necessary is because I am separting the parameters given to the assertion statement onto different lines. This was done to ensure that I didn't produce a line of code that was greater than the outer limit of characters.

@cosmicdreams, the changes are incorrect because they are outside the lines being added by the patch. They already exist in the codebase and are not being altered functionally. (Compare with your previous patch in #16.) We don't add cleanups that are not specifically a part of the functional patch, because it causes scope creep, makes the patch harder to review, and makes it more likely to collide with other issues.

Also, the 80-character rule is only 100% required for comments. It is just strongly recommended for executable code, and one common exception is assertions. IMO the assertions should either be all on one line, or have all the parameters on separate lines similar to our multi-line array formatting. (Reference: http://drupal.org/coding-standards#array) Edit: But, again, the lines Niklas pointed out should not be changed at all in this patch.

Thanks for your work on this!

thanks for the clarification. If I cleanup what is itemized in #25, can we call this done?

here's the corrected patch:

1. only cleaned up the previous modified section of the user tests
2. included the doc block

Looks like the patch has a syntax error.

+++ b/core/modules/user/user.testundefined
@@ -133,6 +133,32 @@ class UserRegistrationTestCase extends DrupalWebTestCase {
+   * Tests if one can register using a previously registered email address as a username

Additionally, this should be less than 80 chars and end in a period. Reference: http://drupal.org/node/1354#functions

+++ b/core/modules/user/user.testundefined
@@ -133,6 +133,32 @@ class UserRegistrationTestCase extends DrupalWebTestCase {
+    $this->drupalPost('user/register', $edit, t('Create new account'));S

Yes, the trailing S.

Eeesh, that's embarrasing.

1. fixed the syntax error that I should have caught before submitting
2. Discovered my IDE had it's right text boundary set to 120 characters instead of 80. Fixed the length of the comment.

escalating to review mode.

Above corrects the function summary to be one line less than 80 chars ending in a period.

The failure of the test without the fix is exposed in #1. Coding standards are met, patch resolves the issue, etc. So pending testbot, I think this is RTBC. Thanks for your patience with all the rerolls!

For existing users where this is already the case, what are we going to do?

I don't see any discussion in this issue on how to deal with that. While the upgrade path would need to happen in Drupal 7, it'd be good to have some kind of plan on how to deal with it before this gets committed to 8.x - seems like at the moment it'd force someone to change their username - maybe that's OK since it's an edge case, but worth asking.

Well, per the code comments and user_account_form_validate(), it's only validating:

• New or changed usernames
• Email addresses on all account form submissions (I think)

So existing users wouldn't be forced to change their usernames. However, they'd be forced to change their email addresses if someone else has the same email as a username, if I'm understanding correctly.

Reformatting the queries because they are so darn hard to read. This happens only if $form_state['values']['name'] is set and the name does not fail validation:

db_select('users')
->fields('users', array('uid'))
->condition('uid', $account->uid, '<>')
->condition(
  db_or()
  ->condition('name', db_like($form_state['values']['name']), 'LIKE')
  ->condition('mail', db_like($form_state['values']['name']), 'LIKE')
)->range(0, 1)
->execute()
->fetchField()

And this is always:

db_select('users')
->fields('users', array('uid'))
->condition('uid', $account->uid, '<>')
->condition(
  db_or()
  ->condition('mail', db_like($form_state['values']['mail']), 'LIKE')
  ->condition('name', db_like($form_state['values']['mail']), 'LIKE')
)->range(0, 1)
->execute()
->fetchField()

Probably a good idea to have some manual testing for what happens to existing username/email collisions before and after this patch is applied.

1. Create a user with username a@example.com and email b@example.com
2. Create a user with mail a@example.com and an arbitrary username.
3. Try to modify an unrelated field (not username or password) on user A's form, submit, and screenshot what happens.
4. Try to modify an unrelated field (not username or password) on user B's form, submit, and screenshot what happens.
5. Apply the patch and clear the site cache
6. Try to modify an unrelated field (not username or password) on user A's form, submit, and screenshot what happens.
7. Try to modify an unrelated field (not username or password) on user B's form, submit, and screenshot what happens.

And let's please reformat the queries as above in the patch.

Probably we'll have to do something like catch's suggestion. Requiring users to change the email adress is not acceptable - usernames can be changed with a lot less trouble. Also, we're targeting #111317: Allow users to login using either their username OR their e-mail address, which would require us to eliminate all of those occurences one way or another.

I'm pretty sure user_account_form_validate() is lying, and that it validates any username that gets submitted whether it's new, changed or the same.

So maybe this is just not backportable? Or for backport we do it on the registration form only? Edit: and be sure we are only validating changes, not every submission.

An aside, I've run into user form validation getting stuck on the unchanged email field in the past for existing users, and it's a real UX WTF.

Even if it's not backportable, if we apply it to Drupal 8, then any site that upgrades to Drupal 7 that has this problem is going to run into it.

Greetings from Core Office Hours! :)

This patch needs a re-roll after #1174620: Add new HTML5 FAPI element: email. I attempted to do that. :)

I decided to test catch's very rational-sounding theory:

I created two users:

User 1: username: bananas@mailinator.com, e-mail: not-bananas@mailinator.com
User 2: username: bananas, e-mail: bananas@mailinator.com

Confirmed that both users could log in/log out fine, and that a password reset on 'bananas@mailinator.com' went to user 2, not user 1.

Applied the patch.

I was still able to log in as both users fine, so no change there.

I go to user 1's account edit page and save it. I get error: "The name bananas@mailinator.com is already taken."
I go to user 2's account edit page and save it. I get error: "The e-mail address bananas@mailinator.com is already taken."

user 2 is basically fine, because they can choose a different e-mail address (but it creates a huge panic-y WTF: "How did someone steal my e-mail address?!"). But user 1 is screwed, unless "change username" permissions are on for auth users, which they are not by default. In either case, the site maintainer's probably getting an email about it.

An upgrade path for this is tricky... we don't really want to inject legacy checks for this condition on every single login from now until forever. Ideally, it'd be something like we show the list of affected users during update.php and kick to the site maintainer to deal with by hand (I wouldn't think there's more than a handful of these types of accounts on a handful of Drupal sites).

To me that feels more appropriate for D8 than D7. But a D8 upgrade is going to include 80 bazillion other things, so any messages are likely to get lost. :\ Hrm. Will do some thinkering.

Oops. patch.

Another thought. Don't change the validation at all. Instead change the password reset form so that if it detects a conflict with username/email, it allows the user to pick which account they meant (or has them type in the exact username or whatever). That would be safe to backport.

Another thought. Don't change the validation at all. Instead change the password reset form so that if it detects a conflict with username/email, it allows the user to pick which account they meant (or has them type in the exact username or whatever). That would be safe to backport.

This. I like this!

New novice task: let's try to make a new patch like webchick describes in #53. We can even reuse the same test from #52.

I think I've got #53 working. If the entered email address is matched to one user's name and another's email address, the form is redisplayed asking the user to pick between the two accounts. I made sure to not display another user's email address to the user attempting a password reset, but both usernames are still displayed which might be a privacy issue. Anyone want to weigh in on what the appropriate labels to display would be?

Great patch! I love the thorough test coverage. Thanks @adharris.

Regarding the privacy question, I think it is not a problem because we are already implicitly confirming an account exists when we allow the password reset email to be sent. Also, webchick has pointed out before that usernames are not secret. However, tagging for security review just to get confirmation that this workflow is OK.

I did a code review and have a couple questions plus a number of stylistic cleanups:

1. +++ b/core/modules/user/user.pages.incundefined
   @@ -27,62 +27,132 @@ function user_autocomplete($string = '') {
   +function user_pass($form, &$form_state) {
   

   I'm concerned that this looks like an API change, even though these arguments are already implicitly available to the form builder. Core never calls user_pass() directly, but theoretically some code somewhere could do so and drupal_render() it, right? (Or am I totally mistaken?) And if so, could we get around it by factoring the logic into a helper for backport?

2. +++ b/core/modules/user/user.pages.incundefined
   @@ -27,62 +27,132 @@ function user_autocomplete($string = '') {
   +  if (empty($form_state['step'])) {
   +    $form_state['step'] = 1;
   +  }
   ...
   +  if ($form_state['step'] == 1) {
   ...
   +  if ($form_state['step'] == 2) {
   ...
   +  if ($form_state['step'] == 1) {
   ...
   +  if ($form_state['step'] == 1) {
   ...
   +      $form_state['step'] = 2;
   

   I'd suggest adding a couple inline comments explaining the multistep form. (A couple other comments in this function might be good, too.)

3. +++ b/core/modules/user/user.pages.incundefined
   @@ -27,62 +27,132 @@ function user_autocomplete($string = '') {
   +        $label .= '/ ' . t('Email Address: @email', array('@email' => $account->mail));
   ...
   +      '#title' => t('Choose Account'),
   

   These labels should be sentence-cased. Reference: http://drupal.org/node/604342#capitalization

4. +++ b/core/modules/user/user.pages.incundefined
   @@ -27,62 +27,132 @@ function user_autocomplete($string = '') {
   +      '#prefix' => "<p>" . t("The email address @email was matched to to multiple accounts.  Please select which account's password should be reset.", array('@email' => $form_state['storage']['name'])) . "</p>",
   

   Typo: The word "to" is repeated twice. It would also be good to make the text a little shorter, though I'm not sure how. It might be good to take some screenshots of the workflow and ask for a UX review. (Shrink browser windows to 600px & crop screenshots to the relevant portions.)

5. +++ b/core/modules/user/user.pages.incundefined
   @@ -27,62 +27,132 @@ function user_autocomplete($string = '') {
   +    $form['actions']['cancel'] = array(
   ...
   +  $form['actions']['submit'] = array(
   

   We might consider separate submit handlers for these rather than the old "clicked button" stuff. I think we could retain the previous submit handler as a wrapper for backport, and remove it in D8?

6. +++ b/core/modules/user/user.pages.incundefined
   @@ -27,62 +27,132 @@ function user_autocomplete($string = '') {
   +    // Mail one time login URL and instructions using current language.
   

   "one-time" should be hyphenated.

7. +++ b/core/modules/user/user.testundefined
   @@ -439,6 +439,86 @@ class UserLoginTestCase extends DrupalWebTestCase {
   + * Test Resetting a user's password.
   ...
   +   * Attempt to reset a password when an email address matches two accounts.
   

   Minor nitpick: The first word should be a third-person verb ("Tests", "Attempts"). Reference: http://drupal.org/node/1354#functions

   Also, "resetting" should not be capitalized.

8. +++ b/core/modules/user/user.testundefined
   @@ -439,6 +439,86 @@ class UserLoginTestCase extends DrupalWebTestCase {
   +    // To maintain upgradability, registration with an account name that is
   +    // used as the email adderess as another account should be allowed.
   

   I was a little confused by this comment... perhaps "For backwards compatibility" rather than "To maintain upgradability"?

9. +++ b/core/modules/user/user.testundefined
   @@ -439,6 +439,86 @@ class UserLoginTestCase extends DrupalWebTestCase {
   +    // Select the account with the username matching the entered email;
   

   Typo here (semicolon).

10. +++ b/core/modules/user/user.testundefined
    @@ -439,6 +439,86 @@ class UserLoginTestCase extends DrupalWebTestCase {
    +    $this->assertNoField('name', 'Duplicate user is not asked for a name when resetting password while logged in');
    

    Missing period at the end of the assertion message here.

11. +++ b/core/modules/user/user.testundefined
    @@ -439,6 +439,86 @@ class UserLoginTestCase extends DrupalWebTestCase {
    +    // Make sure the user was sent an email.
    

    Here I'd specify "the user with the matching username".

12. +++ b/core/modules/user/user.testundefined
    @@ -439,6 +439,86 @@ class UserLoginTestCase extends DrupalWebTestCase {
    +    // Make sure that the other user was not sent an email. (Remembering that
    +    // one has already been sent to this user earlier)
    

    I'd reword this slightly as:

    Make sure that the user with the matching email address was not sent an email. (An email was already sent to this user earlier.)

It would be a fairly simple novice task to reroll this patch and clean up the simpler points above. Another good task would be to do manual testing similar to what webchick did in #51 and take screenshots of each stage in the workflow to facilitate some UX review and help us bikeshed the UI text. :)

I manually tested and it worked as expected but I will not mark RTBC because my email server was not configured correctly so the emails ended up not being sent. I did confirm that the appropriate email addresses were shown in the error log though so I'm 99.999% confident that it works as expected.

I'm going to update the patch from #56 (taking into account the feedback from @xjm) and add some screenshots... everybody stand back!

(but please call an ambulance if you see core holding me down and kicking me in the guts).

LOL. :) Godspeed, cossovich!

Re-rolled patch from #56, here's a summary of the changes I made based on feedback from #57:

1. Not sure about the API concerns, maybe others can offer a more informed opinion.
2. I've (sparingly) added some inline comments to the user_pass() function. It might also make sense to add comments to the user_pass_validate() function where the check for the conflict occurs.
3. Changed labels to sentence case.
4. I've rephrased the language when a conflict is detected to make it shorter, not sure if it improves meaning at all: "There is a username conflict with the email address @email. Please select which account password to reset."
5. Wasn't sure about this... @xjm do you mean one submit handler for the first step of the form and a separate submit handler for the second step?
6. Hyphen added.
7. Changed the verbs and the capitalisation nitpicks. I found other examples in user.test where function comments aren't following the third-person verb convention... I wasn't sure whether to change them all or not (I didn't in the end).
8. Updated inline comment to: "For backward compatibility, registration with a username that conflicts with an email address of another account should be allowed."
9. Semicolon fixed.
10. Period added.
11. Comment amended: "Make sure the user with the matching username was sent an email."
12. Comment amended: "Make sure that the user with the matching email address was not sent an email. (An email was already sent to this user earlier.)"

Tested the re-roll with the following setup (following #51).

User 1: username: thisuser@mailinator.com, email: notthisuser@mailinator.com
User 2: username: user3 email: thisuser@mailinator.com

Screenshots attached showing the request new password UI when a conflict occurs.

The API change concern is the change in the function signature. Anywhere in contrib that calls user_pass() would do so without passing the form and form_state arguments. I'm not sure on the right fix here; to the best of my knowledge a multi-stage form needs to have form_state available. So if someone calls user_pass() and then drupal_render()s it, would they lose the multi-stage functionality?

Perhaps the work around is to just func_get_args() and set $form_state = form_state_defaults()? Not sure what the effect of that would be. We could also just change the signature to function user_pass($form = NULL, &$form_state = NULL) though once again I'm not sure what the effects of that would be.

For #5, @xjm was saying we should attach a separate submit handler to the Cancel button. Then the go back functionality is in it's own handler and the button check here:

+++ b/core/modules/user/user.pages.inc
@@ -27,62 +27,136 @@ function user_autocomplete($string = '') {
+    if ($form_state['clicked_button']['#name'] == 'submit') {
+      $chosen_account = $form_state['values']['choose_account'];
+      $account = $form_state['storage']['accounts'][$chosen_account];
+    }
+    else {
+      $form_state['redirect'] = 'user/password';
+    }

would not be necessary.

Nice thorough job with the cleanup and the manual testing. Thanks @cossovich!

Thanks also for the screenshots with the patch applied. I've cropped the second screenshot here to show the form for reviewers. (In general, when creating screenshots, it's a good idea to use a narrow browser window and crop the image to only the relevant portion. That way they can be neatly embedded in issue comments and handbook pages using an <img> tag.)

From this screenshot, I notice that the formatting for separating the username and email address is a little weird. At the least, there should be a space before the slash, though maybe we can improve it in other ways.

I found other examples in user.test where function comments aren't following the third-person verb convention... I wasn't sure whether to change them all or not (I didn't in the end).

That's the correct choice. We should adhere to the standard in the lines added by our patch, but also keep within the scope of the issue. (The existing and unchanged summaries are being cleaned up in #1326666: Clean up API docs for user module.)

Attached interdiff shows the changes from #56 for easier code review.

I feel like step 2 (from #62) could be a privacy problem.

If you register an account with a certain mail adress as nickname and use the form to request your password:

1. You get to know that there is a user with that certain email adress registered at this site.
"Oh look, my fiancée has an account at adult-dating.example.com"

2. You get to know the UID of that user. Now you can visit her user page, see her nickname and identify that user.
"Oh look, my fiancée is actually the user named alice23 – let me check her profile …"

Regarding #65, #1 is not a regression--we already confirm that implicitly when we let them use the password reset form. Usernames and UIDs are not considered secret. #2 is more of a concern because, as you say, we are displaying the relationship between the account name and email, which is a regression. Though, the chances that a person could exploit this are small considering they'd have to stumble on this weird edge case to begin with. But still needs some review, I think.

That said, though, maybe we should add back something to enforce that you can't create new accounts with a username or email address that is someone else's username or email. We just leave existing accounts alone with some workaround like the above.

@ #66: You are right, xjm: the first one is a separate issue. I created a feature request for that:
#1521996: Password reset form reveals whether an email or username is in use

@no2e's second point demonstrates well what I was feeling yet could not figure out how to articulate. Furthermore, it seems extremely exploitable to me because these type of account collisions can be intentionally created:

1. Know someone's email
2. Register for a new account using that email as the username
3. request password reset on the new account
4. You now have (in the current patch) the username and if you look at source, the user's id
5. browser to user/###, commence stalking

It seems to me (3) can be fixed with two easy changes:

• Change the second step of the form to chose from:
• • The account with email address duplicate@example.com
  • The account with username duplicate@example.com
• Use a different id for the form values than uid. Random number, a hash, anything would work, as the accounts themselves can be stored on $form_state

This of course wouldn't fix @no2e's concerns in #1521996: Password reset form reveals whether an email or username is in use, but it seems to at least not be a regression: the only information that the user gets out of the reset password process is whether or not an email is associated with an account.

That does seem like a much better approach. Let's update the patch with those changes, plus add some validation to disallow creating new accounts (leave existing alone) where the username duplicates an existing email address or vice versa.

Here's a reroll along with the text changes. We still need to use a different id for the form elements than uid, I couldn't quite figure that one out.

Sending to the bot. Thanks @Cottser!

D'oh, I missed a bracket. This should pass.

Looks like this needs another reroll.

user.test has now been split up into many smaller test files under core/modules/user/lib/Drupal/user/Tests.

Here's a reroll. No code changes, just moved the test to its new home.

Here's a start at obfuscating the user IDs. I also tried drupal_get_token($uid), but that passed manual testing and failed automated testing. If we do stick with the method in this patch, would it make sense to break this out into a function in user.module since it is repeated 5 times?

@xjm also mentioned in IRC that someone at the DrupalCamp Twin Cities core contribution sprint had a suggestion for an additional approach to validation. Paraphrasing: When creating new accounts, add validation so that if your username is an email address, it must also match your email address.

Forgot the interdiff for #80.

Another update:

1. Added validation for new users only, to ensure their chosen username is not already registered as an email, and vice versa (per #69).
2. Added tests for this newly added validation.
3. Revised the testUserPasswordResetDuplicateUsers() test because we are no longer allowing new registrations that would create username/email conflicts.
4. Added tests to ensure that existing users who have conflicting username/email combinations can still edit their accounts normally.

+++ b/core/modules/user/user.moduleundefined
@@ -982,7 +985,18 @@ function user_account_form_validate($form, &$form_state) {
-      if ($name_taken) {
+      // For new registrations, make sure the username does not conflict with an
+      // existing user's email address.
+      if (empty($account->uid)) {
+        $mail_taken = (bool) db_select('users')
+          ->fields('users', array('uid'))
+          ->condition('mail', db_like($form_state['values']['name']), 'LIKE')
+          ->range(0, 1)
+          ->execute()
+          ->fetchField();
+      }
+
+      if ($name_taken || $mail_taken) {

I'm thinking now that perhaps there should be two different queries here. For new registrations, use a db_or() to check both user fields, for existing users just check the relevant field. This would eliminate two queries for new registrations.

+++ b/core/modules/user/user.moduleundefined
@@ -966,6 +966,9 @@ function user_validate_current_pass(&$form, &$form_state) {
+  $name_taken = FALSE;
+  $mail_taken = FALSE;

It would also eliminate this ugliness.

Sorry for the noise folks! I went ahead and made the change proposed in #83. I realize now the validation was very similar to this back in #2, minus the check between registered and non-registered users.

I got sidetracked away from this issue, but actually just had this issue come up legitimately on one of my sites. We decided that allowing one user to use an email address that is not their own was not desirable for our use case, so we added validation to the user form so that if the user name is a email, it must be the email associated with that account. This allows us to avoid this issue cleanly, and assert the user "owns" the email they are trying to use as a username.

To me, this seems like a better way than just checking against existing users, but I might be missing a valid use case for some people.

Also, what happens in the existing patch if a user creates an account with foo@example.com as the username, and then later someone registers an account with that email? It seems that that would create this duplicate user situation as well.

@adharris - The approach you mention in your first paragraph was touched on in #80, and should certainly be considered.

The current patch validates that any new users registering cannot use a username OR email that is not already registered as a username OR email. So in the example you gave, the person who tried to register with an email of foo@example.com would not be allowed to register with that email since foo@example.com was already used as a username. If foo@example.com was registered first as an email, then a second user cannot use that as their username. This does not affect existing users, and there are tests to verify that existing users are not forced to change their email address or username.

Also… rerolled.

#86: 1359718-86.patch queued for re-testing.

Another reroll for #1499596: Introduce a basic entity form controller and #1184272: Remove deprecated $conditions support from entity controller.

#88: 1359718-88-tests.patch queued for re-testing.

For some reason I'm seeing two of the assertions failing twice in the test-only patch, let's try that one again.

#88: 1359718-88-tests.patch queued for re-testing.

Sorry for the noise, not sure what's up. 1359718-88-tests.patch on first test showed 8 fails (should have been 6), now testbot seems to hate it.

Rerolled.

Any update on this? Feature freeze is getting closer and closer... (I'm creating a small inventory with possible wanna-haves, just like this one.) Thanks!

This needs another reroll, I'll work on that in the next few days. I think this could still get committed after feature freeze though.

Note that this is just a bug, not a new feature, so we can commit it at any time.

Seems related to, and potentially conflicting with #849602: Update 'username' theme template to use 'view label' operation.

Great Cottser! And got it webchick, but i wonder about this (and couple things more), because:

I like this patch, but i wonder if we can implement some configurable option? Just thinking about user cases where this could be an issue / flexibility. I already demo'ed hefoxed my demo site where i render a config option called:
"Prevent identical username and e-mail address". (description: "Enable this to make sure people do not submit identical username and e-mail address during registration.")

(In length of that idea we could also create some function to parse accounts to scan for usernames where this is an issue and force them to rename their account.) (maybe can do this in contrib space)

See this issue for a link to screenshots, demo page, lots of details.
#1837054: [META] Refactor account workflow (and config)

This all also made me think of some new options, the one related:
"Prevent people from using their e-mail address in the "username" field during registration."
I wonder if an issue about this exists, if not, i can create a patch of what i have working already. (that's concept patch-stage, but working)

[edit: fix oopse]

Thanks for the clarification, @webchick!

Here's the rerolled patch.

#98: 1359718-98.patch queued for re-testing.

See #1812636: Remove deprecated $form_state['clicked_button'] for the test failure.

tagging.

updating issue summary

Thanks @xjm and @YesCT! I'll take a look at updating the patch over the weekend, unless someone else wants to tackle it sooner :)

Here's the revised patch, just updates clicked_button to triggering_element.

@YesCT: I looked through the issue summary, what is the interdiff for? The past handful of patches have just been rerolls. Attached an interdiff here for the heck of it.

Interdiffs dont make sense sometimes... something about merges ..
But as a general rule they are super helpful for allowing someone to review something.
You'll get more participation when you provide them. :)

#106: 1359718-106.patch queued for re-testing.

I'll reroll this.

This issue exposes the fact that a given email address is in use by a user on the site. I can't think of anywhere else that currently happens, and does seem like a gray area. Once you have that information, you can try to see if there are any users that have the local name of their email address as their username on the site.

Updated issue summary with remaining tasks to make it easier for new people

@Grayside - For that issue, see #1521996: Password reset form reveals whether an email or username is in use.

Updating tags per issue summary, thanks @YesCT!

Straight reroll, no changes to the patch.

#112/@Cottser:
That issue is specific to the new password form, but you are right, the type of exposure is the same.

However, where that issue is simply going about the business of not admitting to any specific email address, this issue is relying on doing so to create an actionable UI.

It took a while to read everyone's comments and understand this issue, but I can confirm some things:

On a fresh Drupal 8.x-dev install from March 6th, 2013, I:
1) created a user with username b**@gmail.com and email a**@yahoo.com
2) created a user with username a**@yahoo.com and email b**@gmail.com

On the request new password screen at user/password, if you requested the password for username b**@gmail.com it will send the password to the email of b**@gmail.com (which in this case is a different user).

After applying patch 1359718-114.patch in #114 by Cottser, on the request new password screen at user/password, if you requested the password for username b**@gmail.com, Drupal will stop and show a confirm screen "There is a username conflict with the email address b**@gmail.com. Please select which account password to reset." Also, after applying the patch, if I completed step #1 above, I am blocked from completing step #2 above. See screenshots. Hopefully the screenshots will add some clarity to this issue :)

embedding screen shot
Before Shot

Before Shot 2

After Shot

After Shot 2

adding crop image for issue summary

Remove interdiff task, we've just been chasing HEAD and test changes recently.

Thanks for all the work on this. I like everything about this patch, except the new 'Request new password' screen. The UI appears to show the username associated with an e-mail address. This could be a security breach in my opinion, as it could potentially be exploited to identify who is behind a specific username. Thoughts on this?

Issue summary is updated with issue summary template

@Dries - Thanks for reviewing this.

With the latest patch, the only time the "Choose account" UI is displayed is when an email address is entered in the "Username or e-mail address" text field and the entered email address matches two users. Even then, the UI only displays the email address the user entered, no username information (other than exposing that a user with a username matching the entered email address exists). Earlier patches used the usernames as keys in the form but those were obfuscated in #80.

So as far as I can tell we are not introducing any security regressions, see #1521996: Password reset form reveals whether an email or username is in use for some privacy concerns around the existing password reset form that would apply here as well.

Someone at our sprint also suggested another possibility for handling the edge cases which I don't believe has been brought up in this issue - just email both users. Not sure what I think about that just yet.

@adharris articulated this way better than me in #68 :)

This of course wouldn't fix @no2e's concerns in #1521996: Password reset form reveals whether an email or username is in use, but it seems to at least not be a regression: the only information that the user gets out of the reset password process is whether or not an email is associated with an account.

Edit: added quote.

Looking at the screenshots, it probably doesn't make sense to have the 'Cancel' button first on the "Choose account" form. CNW for that, will reroll this weekend.

@snowpeas_60609 thanks for embedding those screenshots. It really helps speed reviewing.

Re-titling for clarity. Thanks @designfitsu, @andymartha, and @snowpeas_60609 for moving this forward :)

Trying again.

Added a weight to the Cancel button, new patch and screenshots:

Correct path to request new password form

Updating UI screenshot

#126: 1359718-126.patch queued for re-testing.

http://drupal.org/patch/reroll (how to reroll)

Thanks @YesCT! Unassigning so this can be rerolled.

Drupalcon Portland sprint: working on a reroll.

A reroll, also changes uses of drupal_hash_base64() to Crypt::hashBase64().

Thanks @rbayliss! I tried applying #132 locally but it needed another reroll, see attached.

Sorry for chiming in after 134 comments, but I believe that this approach doesn't really cut it.

@Dries stated in #119:

This could be a security breach in my opinion, as it could potentially be exploited to identify who is behind a specific username. Thoughts on this?

@Cottser answered in #120:

With the latest patch, the only time the "Choose account" UI is displayed is when an email address is entered in the "Username or e-mail address" text field and the entered email address matches two users. Even then, the UI only displays the email address the user entered, no username information (other than exposing that a user with a username matching the entered email address exists). Earlier patches used the usernames as keys in the form but those were obfuscated in #80.

So as far as I can tell we are not introducing any security regressions see #1521996: Password reset form reveals whether an email or username is in use: anonymous can check if someone is registered for some privacy concerns around the existing password reset form that would apply here as well.

Now while it is true that we're not introducing a completely new privacy regression, we're adding another instance of the same privacy breach. While we already need to get past the existing privacy breach, we can't reinforce it at another place. This IMHO isn't acceptable.
It really doesn't help that it's only displayed in specific situations if the specific situation can be diligently induced by anyone.
Also, while already possible, this is an open invitation to reset the password of someone else and exhibits our weak security level at this specific point.

At the same time, this introduces considerable complexity (additional code, a multi-step form) to solve an edge case which actually shouldn't exist, so it is a quite elaborate stop-gap which doesn't solve any of the underlying consistency (uniqueness) and validity (spoofing) problems.

Please don't be offended by my rant, I really feel sorry for the amout of work that already went into this, but I really can't imagine seeing this in D8 core. Might be okay as a D7 stop-gap, but I'd rather propose digging a bit deeper #2014185: Check usernames that are email addresses more rigidly, only allow if matches email to get the underlying deficiencies eradicated.

Thanks @Pancho, great to hear some feedback. Working on this issue for over a year has taught me a lot about the changes in Drupal 8, so regardless of what happens with this patch I'm not worried and won't be offended. I do want to clear up one thing though:

It really doesn't help that it's only displayed in specific situations if the specific situation can be diligently induced by anyone.

I don't think this is true - the patch here prevents any new user registrations that would meet these conditions. The multi-step form and (most of) the additional code are there for the sole purpose of handling existing data.

Edit: clarified last paragraph.

Hey @Cottser,
and thanks for not being offended!

I need to clarify and specify my statement about the privacy aspect:
The patch really doesn't introduce an additional instance of a privacy breach that wouldn't have already existed. You even takes a long way to not expose more data than necessary.
Still it makes fixing the preexisting privacy problem not just a bit harder, it even makes it impossible.
I need to explain this in some length:

If someone tries to register an email address that has already been registered before, we somehow have to reject this registration. And it's really hard to reject a registration without disclosing the reason. But let's look into the actual cases:
The fact that email address already exists can only occur in the following situations:

1. the user re-registers after being already registered (we're currently assuming this and as a nice service we ask if the user has forgotten the password)
2. the email address has changed the owner (infrequent, but possible, especially with small corporate email addresses)
3. someone mistypes his/her own address and by accident hits the address of someone else (infrequent)
4. someone tries to spy upon someone else trying to find out if the other person is registered (probably infrequent, but to be avoided by all means)
5. spammers try to read out existing email addresses by brute force or walking through an existing list in order to get more specific target data (not so infrequent and to be avoided by all means)

So while for the user in case 1 it is nice to be reassured that the account already exists, it's simply not worth the privacy breach, especially because it is possible to inform the user via the existing email address that s/he can access.
Same holds for the second case where the address has changed the owner: the new owner has access to the email account.
Now the third case is harmless and getting immediate feedback that the address is already registered would be very nice, but again is not worth the privacy breach. What we can do is display the entered email address and hope the registrant sees the typing error. As in the latter two cases the registrant doesn't have access to the entered email address.
The latter two cases are dangerous and any disclosure has to be avoided.

So in order to stay on the safe side, we have one possibility to not disclose any information:
Don't directly notify the registrant that the account creation is impossible! Generally just give feedback that an email has been send to the given address. And yes, we can always send the email, because we can assume that only legitimate account owners get the email and thereby get informed that possibly someone else tried to register with his/her email address.

Now what changes does this patch introduce to this situation? Imagine your patch is committed:
Now we're checking the chosen username against existing email addresses. Now we somehow need to inform the registrant that the chosen username is not possible. We can only do that by email, but in this case things are diffferent:
If we're sending the email to the email address of the preexisting account (the username of the newly registered account), the new registrant gets it only if s/he is legitimate owner.
But let's rethink this from the perspective of the two "dangerous" registrants. Imagin, they entered 'bill.gates@microsoft.com' as their username and 'spammer696@xxxhost.ru' as email address. The email address is both valid and can be expected to be unknown to the system. So if they don't receive an email at 'spammer696@xxxhost.ru', they know that the registration has failed and if they know only a bit about Drupal, the can induce that this is because 'bill.gates@microsoft.com' is already registered.
Goal accomplished: Now they know Bill Gates is user of this website and can add his email to their targeted address list.

Now this is only about the privacy problem which is indeed getting worse by this patch.

Add to that the fact that this doesn't help with spoofing.

And yet another aspect would be the fact that now that you're doing this here:

// For new registrations, make sure the email address does not conflict
// with an existing user's username.

means anybody can now abuse your email address as username in order to keep you from registering with your own email address.

In spite of the very best intentions, all of this unfortunately really suggests that this approach is broken by design. Still we probably had to go this way to eventually come to the insight that it's simply not possible to alleviate this problem just a little bit, without this or the other way making it even worse.

I'm really tempted to mark this approach "won't fix", and restart on #2014185: Check usernames that are email addresses more rigidly, only allow if matches email.
Still I'm not claiming to be 100% sure that the proposals over there fully work out. They simply seem to be slightly more promising.

#2014185: Check usernames that are email addresses more rigidly, only allow if matches email hasn't budged so tagging this for another reroll.

@YesCT, @Les Lim, @SeanKelly, and I discussed this at a code sprint (in Twin Cities) and we believe this issue and #2014185: Check usernames that are email addresses more rigidly, only allow if matches email are distinct problems. Let's move forward with this as is... rerolled of course.

I think fixing the last part of comment #138

And yet another aspect would be the fact that now that you're doing this here:...

in #2014185: Check usernames that are email addresses more rigidly, only allow if matches email makes sense. Lets keep that separate from this.

Summarizing.

Prevent people registering with a username that matches some user's existing email address.
Prevent people registering with a email address that matches some user's existing username.

When requesting a password reset, and the info entered matches both a username and an email address, ask which. See #126.

adding related issue (to motivation)

Yet another reroll

*d'oh*

I somewhat agree that creating this UI is problematic. It is now possible to create duplicate users and if we land a patch that disallows that we should just be fine. Existing sites are broken and will simply continue to be broken. If people actually suffer by this we would see forum posts on stackexchange or drupal.org. It's better to not be able to reset your password and have to contact administrator rather than exposing yet another UI.

I rerolled the last patch in #143

I think we may have lost some changes between #135 and #143:

#135: 5 files changed, 280 insertions, 61 deletions.
#143: 5 files changed, 310 insertions, 16 deletions.

updated remaining tasks. Tried to clarify the motivation and correct grammar.

Attempting patch reroll from #135

Re-rolled patch. I ran a local SimpleTest and it failed on the mail() function but I think that is because I don't have a local mail server setup.

Will see how the online test goes.

@Cottser

Looking at the validate() function in the /core/modules/user/lib/Drupal/user/AccountFormController.php file.

Line: 305

$name_taken = (bool) db_select('users')
  ->fields('users', array('uid'))
  ->condition('uid', (int) $account->id(), '<>')
  ->condition('name', db_like($form_state['values']['name']), 'LIKE')
  ->range(0, 1)
  ->execute()
  ->fetchField();

I am getting an error that it is trying to convert a FieldListItem to an int as $account->id() returns NULL when the user is creating an account.

So, I tried the below which removed the error.

$query = db_select('users')
  ->fields('users', array('uid'))
  ->condition('name', db_like($form_state['values']['name']), 'LIKE');

  if (!$account->isNew()) {
    $query->condition('uid', (int) $account->id(), '<>');
  }

  $name_taken = (bool) $query->range(0, 1)
    ->execute()
    ->fetchField();

I haven’t changed the patch as I wanted @Cottser to take a look and get his thoughts.

Changed status to Needs Review.

#111317: Allow users to login using either their username OR their e-mail address is pending on this one now.

@pancho is totally right in #138 I think, this approach is just not sufficient. This would not stop me registering yourname@emailaddress.com as my username, when you came along to register an account, you would not be able to because I had stolen your email address as my username. This is why #2014185: Check usernames that are email addresses more rigidly, only allow if matches email is more important to start with IMO.

Plus this patch is really out of date.. :)

Just adding related issues...

Reroll of #151. Including several required changes due to changes in $form_state, select queries, deprecated and removed functions. The interdiff contains the changes after the git reroll.

[edit] The #154 comments are not included.

Rerolled the #159 patch

What happened to core/modules/user/user.pages.inc?

@mgifford #2407489: Remove user.pages.inc.