Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem
- When creating a user account on a Drupal site, then the built-in login form autocompletion (password manager) feature of web browsers (at least Firefox) identifies the email field as the username (instead of the username field).
- If I let my browser/password-manager save my credentials, then a wrong username is stored and I cannot log in with the saved credentials.
Cause
- Built-in detection of login fields in forms is a best guess of web browsers, not an official HTML standard (nor a feature of the "autocomplete" attribute).
- The email field appears directly before the password field. → The email field is identified as the username field.
Goal
- As an end-user who creates a user account on a Drupal site (and who is using a password manager), I want my browser to correctly identify my user credentials, so I can save them and use them the next time I need to log in.
Security
- Several security articles on the net advise web site owners to disable the autocomplete functionality for user registration/login forms. — Even though they are just pointing out weaknesses in individual browsers that simply ought to be fixed by the respective browser vendors.
- The trumping counter-argument is that force-disabling password managers encourages bad practices (using a similar password on multiple websites, writing it down or typing it into an insecure document, etc).
-
Drupal core did not care for any of these security concerns in the past.
→ The only purpose of this issue is to ensure that a password manager works correctly, if the user chooses to use one (which is not something Drupal should decide in the first place).
Proposed solution
-
Change the order of the username and email fields in the user account registration/creation form, so that the username field appears before the password field and is correctly identified by password managers:
Screenshots
→ The order of the Username and E-mail fields is swapped, so that the Username field appears right above the password element:
| Comment | File | Size | Author |
|---|---|---|---|
| #39 | interdiff.txt | 1.08 KB | sun |
| #39 | user.form_.39.patch | 10.08 KB | sun |
| #37 | user.form_.37.patch | 10.09 KB | sun |
| #34 | interdiff.txt | 872 bytes | sun |
| #34 | user.form_.34.patch | 10.13 KB | sun |
Comments
Comment #1
sunComment #2
dawehnerGiven that this affects quite a lot of users (everyone using a browser) I would argue that this is major.
Comment #3
larowlanSad that we can't use form display mode settings here to put the decision in hands of site builder
Comment #4
sunI wonder what else is actually needed for this issue?
I considered to perhaps add an assertion to an existing user registration test, so as to ensure that the order of fields is not changed/broken in the future, but then again, I'm not sure whether that is really necessary?
Lastly, please note that this patch essentially reverts parts the patch of #1525640: Disable autocompletion for email/password field when editing user profile, because that issue seemingly just worked around the actual root cause (as outlined here).
Comment #5
BerdirThe main problem there IMHO was editing an account of *someone else*, where it also auto completed it with your own information and overwrites the existing. Needs to be confirmed how that exactly behaves now with your changes...
Comment #6
sunI'm afraid I'm not able to follow. — The browser's autocomplete + password manager behavior only kicks in for empty form input fields.
→ When editing an existing user account, then the user name + email fields already contain values. No autocompletion.
I can happily add some extra code to add an autocomplete="off" attribute when
$account->id() > 0, but I honestly don't see the point of that. ;-)Comment #7
sunComment #8
sunRe-rolled against HEAD.
Comment #9
sun8: drupal8.user-login-autocomplete.8.patch queued for re-testing.
Comment #10
sunAs an attempt to move this issue forward, here are two screenshots of the two forms that are primarily affected by this change.
→ The order of the Username and E-mail fields is swapped, so that the Username field appears right above the password element.
Comment #11
sun8: drupal8.user-login-autocomplete.8.patch queued for re-testing.
Comment #12
sun8: drupal8.user-login-autocomplete.8.patch queued for re-testing.
Comment #13
tstoecklerHaven't experienced the problem reported in #5 either. The problem this issue fixes is a pain, however, and has been bugging me for a while. Code-wise it's RTBC, but I suppose we need some more opinions before this can go in?!
Comment #14
sun8: drupal8.user-login-autocomplete.8.patch queued for re-testing.
Comment #15
sunI think I've provided everything that I can to make reviews of this change as easy as possible.
Thus far, I've the impression that no one raised objections to swapping the two input fields. Shall we move forward here?
If people think it is necessary, I'm also happy to add a quick test assertion for this somewhere; thus far, I merely thought that might be overkill.
Comment #16
sunComment #17
sunWhat can I do to move this forward?
Comment #18
mgiffordThis patch still applies nicely on SimplyTest.me
So what are the simplest steps to replicate this problem. The goal of the code is just to re-order the form so that the browser makes a better choice about what to remember, right?
1) Spin up a new instance.
2) Create a new account & verify that your browser has "Remembered" them.
3) Look in your browser, look at the security preferences & verify that the saved password is under the username and not the email address.
Would be nice to get this fixed up.
Comment #19
sunAdded test coverage.
Comment #20
sun19: user.form_.19.patch queued for re-testing.
Comment #22
sunUpdated for EntityManager::getStorageController() renamed to getStorage().
Comment #23
sun22: user.form_.22.patch queued for re-testing.
Comment #24
Bojhan CreditAttribution: Bojhan commentedThis looks like a very good idea, I expect few to no usability issues will arise from this.
Comment #25
sunAdd autocomplete=off if operation is not 'register' + added test coverage.
This (hopefully final) patch adds proper test coverage for the changes performed in the following issues (which should have added test coverage on their own):
#1525640: Disable autocompletion for email/password field when editing user profile
#787876: Edit "My Account" fills the first password field
#2219443: Autocomplete in Safari 7 overwrites email address on user profile edit form
The revised test not only asserts the field order, but additionally asserts the expected appearance of the autocomplete=off attribute on both the user registration and the user edit/profile form (as well as the maintenance user account form in the installer).
Comment #26
sun25: user.form_.25.patch queued for re-testing.
Comment #27
sun25: user.form_.25.patch queued for re-testing.
Comment #28
sunWhat can I do that allows us to move forward here?
Comment #29
mgiffordA clear testing process for this patch (#18?), someone to review the code and someone to mark it RTBC.
Think that's all.
Comment #30
sunYup, the procedure in #18 is accurate.
Comment #31
Anonymous (not verified) CreditAttribution: Anonymous commentedI started reviewing this and can confirm that my email address was stored instead of my username.
Then I wanted to test the patch, but it doesn't apply anymore. Tagging this for a reroll.
Comment #32
sunResolved merge conflicts.
Comment #34
sunUpdated UserAccountFormFieldsTest for modernized installer forms.
Comment #35
sun34: user.form_.34.patch queued for re-testing.
Comment #37
sunMerged HEAD
Comment #39
sunUpdated for #2238077: Rename EntityFormController to EntityForm
Comment #40
Anonymous (not verified) CreditAttribution: Anonymous commentedCodewise this seems ok and the patch works as expected. The username is saved instead of the email address, and thus the autofill works right.
I wasn't able to reproduce the issue as described in #5.
However, after playing around with this, I found another case where the change could be needed:
- register a new account and save the credentials
- logout
- login as an admin
- add a new user
The credentials that are stored while registering for an account are filled in here. I would think that, similar to the user edit form, autocomplete should be set explicitely to "off" here as well.
Comment #41
sunre: #40: @pjonckiere:
Hm. I started to look into that, but then I questioned whether that expectation is actually universally true.
The counter-expectation would be:
These are two conflicting expectations. (In fact, that's how I generated some of the screenshots in the OP.)
It additionally depends on the password manager's behavior whether the form fields on the user creation form are automatically filled out or not. Some password managers always require you to use autocomplete or click a button to fill in your credentials. And of course, if multiple credentials are stored, then you always need to choose one.
Since no clear/sane/universal expectation can be made, I'd rather leave the administrative user creation form alone; i.e., deferring the choice to users/agents.
Thoughts?
Comment #42
Anonymous (not verified) CreditAttribution: Anonymous commentedAn argument could be that an admin will use the "add a user" functionality more often to create new accounts for other people and not for him-/herself. But I can agree that there is no solution that will satisfy all use cases, and for that reason I think it's reasonable to leave the form as-is.
Since the initial problem (storage of email iso user) is resolved and the test coverage seems sufficient, I think this patch is good to go.
Comment #43
sunI guess you meant this status then ;-)
Comment #44
sunSlightly more accurate issue title, hopefully.
Comment #45
webchickDang. It's really too bad that browsers are so stupid. :( (Seriously, on what planet is an input field with an ID of "mail" a username field?!)
This is going to be totally disruptive to people switching between Drupal 7 and 8 who've memorized the tab strokes to get to one field versus another, ugh. OTOH I can't really envision a different fix. We certainly don't want to disable password managers altogether, and it sounds like our options for putting additional attributes are limited since this autocomplete stuff isn't a formal W3C specification.
Therefore, committed and pushed to 8.x.
Comment #47
sunHm. To clarify:
That is actually the case on probably 98% of all web sites. More or less just the 2% powered by Drupal have a separate notion of username vs. email.
For that reason, there are a range of contributed modules that are removing/replacing the username concept with just the email address. (Which, FWIW, simplifies things a lot and improves the first-time/sign-up user experience; just one form field less means 33% less input.)
I'd totally +1,000 an effort to remove the (enforced) username concept from core and make it optional instead. Everyone expects real names to begin with; a separate "login" name is pointless to have. Wherever we're currently showing this name, we could as well chop-off the alias/mailbox name from the email; i.e., sun@drupal.org turns into "sun", done.
Lastly, to clarify further:
Those modules are not negatively affected by this change. They are removing either the username or the email form input field anyway, so the net result for them is the same: Password managers will identify the input field above the password field as the login username to be stored.
Comment #49
sunBelated auto-re-test result.