Currently a valid email address can be registered as username without any ownership verification. This leads to a multitude of problems including consistency/uniqueness and spoofing.
#111317-83: Allow users to login using either their username OR their e-mail address:
At a minimum, I think we need validation when new users register that their username matches neither an existing username nor an existing e-mail address.
While a bit better than the status quo, this doesn't cut it leading to - em, sorry - superficial fixes like the proposal in #1359718: Allow password reset on account with the username matching another email; prevent registrations that match another account which introduces code bloat and new privacy problems, while still not solving the fundamental problems.
It neither is enough to check usernames for uniqueness upon account creation nor upon changing the account name.
Registering with a username that could be an email address always comes with a risk of identity fraud or blockade, if the potential email address isn't verified.
Imagine someone registering with the username 'bill.gates@microsoft.com' or 'dries.buytaert@drupal.org' with her real email address given as 'spoof@xxxhost.ru'.
Regarding the identity fraud case: Bill Gates might not be registered on our site yet, so the address is allowed. Still the fake Bill could post in the name of Bill Gates, and even with a (misleading) sign of being verified.
Regarding the blockade case: Bill Gates might not be registered on our site yet, so the address is allowed. But if tomorrow Bill Gates wants to register as 'Bill Gates' with his email address 'bill.gates@microsoft.com', he will be disallowed to do that because someone else fraudulently blocked Bill's real email address from being registered. Now Bill doesn't only have to live with the fact that there's someone else posting spam in his own name, but he can't even prove that the email address in reality belongs to him.
So what could be our options?
- 1: The most rigid solution:
- Disallow @ signs in usernames.
- 2: The minimum solution:
- Check for existence when registering but give the email address precedence to the username. So, if someone proves to be owner of say 'bill.gates@microsoft.com', he can register, but the preexisting account with 'bill.gates@microsoft.com' being username would be blocked or renamed.
- 3: The probably most diligent solution:
- Allow potentially valid email addresses only as username if at the time of registering or changing username, the username matches the current email address. If at a later point someone registers proving to be the new owner of say 'ceo@microsoft.com', a preexisting account with the username 'ceo@microsoft.com' might be blocked, but would in any case be renamed and the owner informed.
- 3a: The most rigid solution:
- Block, rename and inform users with a user name which might potentially be an email address, but doesn't match the currently given email address.
- 3b: The minimum solution:
- Rename and inform only users which user name matches the email address of another account. Leave all other accounts alone someone else proves ownership of the email address.
- 3c: The probably most diligent solution:
- Rename and inform users which user name matches the email address of another account. Inform and require all other users with a username being a potential email address to prove ownership within some time, otherwise rename the account.
And what to do with existing usernames on existing sites?
Taking the latter solution from above, we could adopt:
Opinions?
Comment | File | Size | Author |
---|---|---|---|
#78 | check_usernames_that-2014185-78.patch | 8.84 KB | yogeshmpawar |
#74 | check_usernames_that-2014185-74.patch | 8.78 KB | hitesh-jain |
#65 | 2014185-65.patch | 8.78 KB | mgifford |
#60 | 2014185-60.patch | 8.91 KB | rpayanm |
Comments
Comment #1
no2e CreditAttribution: no2e commentedAsking bluntly: Do we really need to allow two login names (email adress or username) in D8? Couldn’t it be an option to remove this feature altogether and stick to one way?
Then we’d only have to solve "And what to do with existing usernames on existing sites?".
Comment #2
PanchoThanks for bringing this up again! However:
There's nothing to remove. It's not yet implemented.
However #111317: Allow users to login using either their username OR their e-mail address might add login via email address, which is a very common and therefore much requested feature. And removing login via username is an absolute no-go for existing sites.
Also, why would we want to do that? Because:
No, this issue is independent from that question. If #111317: Allow users to login using either their username OR their e-mail address lands, the problem would only become more apparent.
Comment #3
YesCT CreditAttribution: YesCT commentedproposed solution 3 seems like the best one.
We only allow email addresses as a username if the username matches the email address.
----
I think what to do about existing data (the upgrade path) is a hard problem that should be a separate issue.
For this issue, we dont do anything with existing data, our upgrade path is that the data stays the same. And then it is only a problem when people try and save some change in their account.
----
Note, this has email verification for sites that *have* email verification turned on.
Comment #4
ZenDoodles CreditAttribution: ZenDoodles commented@YesCT, @Les Lim, @SeanKelly, and I discussed this at a code sprint (in Twin Cities) and we believe this issue and #1359718: Allow password reset on account with the username matching another email; prevent registrations that match another account are distinct issues. We like the following workflow:
Comment #4.0
ZenDoodles CreditAttribution: ZenDoodles commentedlinkfix
Comment #5
tim.plunkettSorry, not a bug.
Comment #6
ZenDoodles CreditAttribution: ZenDoodles commentedThis patch is tests-only and should fail.
It tests:
Also, @tim.plunkett I disagree. I think allowing a user to set their username to an email address which is not their own is a bug, and arguably a significant privacy issue. @See also #1359718-138: Allow password reset on account with the username matching another email; prevent registrations that match another account
Comment #7
ZenDoodles CreditAttribution: ZenDoodles commentedComment #10
damiankloip CreditAttribution: damiankloip commentedBug, feature, whatever :) I think this is a handy thing to have.
How about something like this? I went with PHO's email validation as I assume if it is just something like john@smith we don't really care? Only a legit email address is being used for the user name?
Had the change the tests a little, I think it was just some stuff that didn't quite get changed from copy pasting.
EDIT: although seems like d.o thinks that's an email address and has added a mailto: to it. HA..
Comment #11
catchWhat about existing users like https://drupal.org/user/227 ?
Comment #12
damiankloip CreditAttribution: damiankloip commentedGood point. As this is 8.x won't this only be an issue for user migrations?
Comment #13
catchIt looks from the patch like the validation runs whenever the account is updated via the form. So if we retain usernames during the migration you'd still need to be able to re-save your account without changing the username.
The other option would be forcing people to update their username when they edit their account, but that feels like a choice for site-admins.
Comment #14
damiankloip CreditAttribution: damiankloip commentedSo what are we saying? Wes should just make this a setting? Seems it will get hard to do otherwise.
Comment #15
mgiffordIf it is a setting, can we ensure it is enabled by default? Where would be the best place to place this option?
Comment #16
damiankloip CreditAttribution: damiankloip commentedSpoke to catch about this and decided it would suffice to just add a settings to enable/disable this functionality would be fine. Defaulting to on. Additional tests added too.
Advice on the wording for the form etc.. most welcome.
Comment #17
damiankloip CreditAttribution: damiankloip commentedRerolled.
Comment #19
damiankloip CreditAttribution: damiankloip commentedComment #20
damiankloip CreditAttribution: damiankloip commentedReroll.
Comment #21
damiankloip CreditAttribution: damiankloip commentedSome text tweaks after speaking to catch.
Comment #23
damiankloip CreditAttribution: damiankloip commentedCatch also suggested just removing the description, which is 100% fine with me.
Tagging with 'Needs usability review' so one of the UX folks can cast their eyes on the new option. Screenshots attached too.
Comment #24
damiankloip CreditAttribution: damiankloip commentedForgot to fix the tests too.
Comment #25
Bojhan CreditAttribution: Bojhan commentedInteresting, I like the solution that is shown here. But a couple of questions:
1) Shouldn't we enable this by default? In 99% of the cases, we are helping the webmaster by mitigating this problem.
2) The label could be more clear. I am just not sure how, because you mention e-mail and username each twice, which makes it harder to scan.
Comment #26
damiankloip CreditAttribution: damiankloip commentedThanks for looking over this.
This does default to true. The screenshot I took just didn't have it on.
Here is a new patch based on the discussion about the option text.
Comment #27
Bojhan CreditAttribution: Bojhan commentedThat looks better! I really love our attention to detail on these kind of things.
"When an e-mail address is used as username, require a matching e-mail address."
Its a little verbose since it needs to match both registration and usage after registration. However it is something that can be scanned more quickly now.
Comment #30
damiankloip CreditAttribution: damiankloip commentedRerolled. Mostly form state object changes.
Comment #31
damiankloip CreditAttribution: damiankloip commentedIgnore the whitespace in the diff, not in the patch. Not sure what happened there..
Comment #33
LinL CreditAttribution: LinL commented+1 for this change.
Haven't fully tested it, but a nitpick - since #950534: [policy] Consistently use "email" instead of "e-mail" in Drupal we're using email instead of e-mail.
Comment #34
damiankloip CreditAttribution: damiankloip commentedNice, thanks for linking that issue. Also fixed the fatals now randomName() doesn't exist anymore.
Comment #36
damiankloip CreditAttribution: damiankloip commentedMissed one of the form state changes.
Comment #38
damiankloip CreditAttribution: damiankloip commentedMissed a s/e-mail/email change.
Comment #39
LinL CreditAttribution: LinL commentedStill a couple more e-mails! 2 in the yml file and 1 in test comments:
Comment #40
damiankloip CreditAttribution: damiankloip commentedGood spot! Forgot about the schema files.
Comment #41
dawehnerGeneral question: Shouldn't we adapt the description of the name field to explain that you can actually enter an email address? In case the verify_email_match is set
the text could explain that in case you specify a mail, it should be the same?
I could imagine that users might not have the idea to try out their email adress as well.
* You can just use $this->config() instead.
* Don't we have abstracted away the validation using symfony validator? Could we use that to replace that filter_var call? Read more about this mess in https://www.drupal.org/node/2313669
Comment #42
damiankloip CreditAttribution: damiankloip commentedThanks for the review! So:
- I would like to punt any other user facing changes to a follow up if we can. That seems like a discussion by itself, and this issue has been hanging around long enough.
- User $this->config()
- Added @todo to use Symfony validator when #2313669: Bring in egulias/EmailValidator for RFC compliant email address validation is committed.
How about that?
Comment #43
dawehnerI'm cool with it
Comment #45
Dries CreditAttribution: Dries commentedI'm moving this to 'Normal'.
Comment #46
alexpottIs this really a feature we need to add to core? This is a problem - https://www.drupal.org/u/driesbuytaert.net but is has been this way for years. Interesting the d.o user registration form says "Spaces are allowed; punctuation is not allowed except for periods, hyphens, apostrophes, and underscores." as help text for the user name - this is obviously incorrect.
The comment is incorrect - we're changing to a random non matching email address here.
Comment #47
David_Rothstein CreditAttribution: David_Rothstein commented??? - (fix this on commit if there were nothing else wrong)
Why isn't this just using valid_email_address()? Any @todo can be on that function directly.
This wording doesn't make a whole lot of sense when an administrator is editing someone else's account...
This is asserting the absence of a message that is not actually used anywhere in the actual code.
---
Overall looks nice. This seems like it should be backportable to Drupal 7 (especially with the optional setting).
Comment #48
damiankloip CreditAttribution: damiankloip commentedThanks for the feedback. I don't think this should be demoted to normal but meh.
@alexpott, I think this is a useful core feature, something that we should definitely have. Precisely for the reason outlined in the account you created. You could now go and cause havoc in the queue as dries@buytaert.net very easily. This is also a better fix IMO to some of the related issues that try to dance around with checking usernames and emails when doing password reset. Plus, the fact that this has been the case for some time is not a good reason to not fix it :) Fixed the test comment also.
@David_Rothstein, Yes I would like to see this backported. As it's is controlled by an option. Should not break anything. Fixed the comments from #47. Let me know what you think of the text change. This should make more sense in both contexts now.
Comment #49
LinL CreditAttribution: LinL commentedPatch missing from #48?
Comment #50
damiankloip CreditAttribution: damiankloip commentedYep!
Comment #51
damiankloip CreditAttribution: damiankloip commentedRerolled.
Anyone?
Comment #52
dawehnerFeedback got adressed.
Comment #54
YesCT CreditAttribution: YesCT commentedWe still need a separate issue to correct the help text as identified by @alexpott in #46
Here is one #2354129: correct help text for account creation email address field.
I think it will need correcting wether or not this gets in.
rerolled for #1802128: Replace "user name" with "username" in UI text
Comment #55
mgiffordOk, so this passes the bot. It has been RTBC'd a few times so I assume it's just fine now. Still, what other testing should be done (if any)?
I'm not clear from the summary (or the last issue) which solution is being pursued, so looking for a summary update.
Comment #56
kaareComment #59
Anonymous (not verified) CreditAttribution: Anonymous at Druid commentedComment #60
rpayanmTrying...
Comment #62
vermauv CreditAttribution: vermauv commentedComment #65
mgiffordJust a straight re-roll of #60.
patching file core/modules/user/src/Tests/UserRegistrationTest.php
Hunk #1 succeeded at 223 with fuzz 1 (offset 73 lines).
Comment #68
damiankloip CreditAttribution: damiankloip commentedThe code that actually does the validation is missing :) (Missing from #60 too). We can probably implement this using some validator now too?
Comment #69
AaronBaumanI'll volunteer to re-roll a patch that includes the validation mechanism.
I can think of at least 3 ways to do this:
* add a validate() method on user entity
* update UserNameConstraint to conditionally verify email based on user setting config
* create a new constraint which is conditionally added to user field definitions
Any of these make the most sense?
A different approach?
Comment #72
jonathanshawComment #73
hitesh-jain CreditAttribution: hitesh-jain at Acquia commentedComment #74
hitesh-jain CreditAttribution: hitesh-jain at Acquia commentedRerolling against 8.3.x branch.
Comment #77
yogeshmpawarComment #78
yogeshmpawarI have rerolled the patch against #74.
Comment #80
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedComment #94
AnybodyThis is still an important improvement and security precondition for #111317: Allow users to login using either their username OR their e-mail address. The current situation also allows for easier social engineering, if an email address doesn't match the username or even the email address or username of someone else.