Add a submodule for using the full email address as username and add a batch action to modify the existing usernames

Would be really nice to hear from a maintainer, if there's any good reason for the required cleanup, which I didn't find yet.

If the maintainers like the idea from this issue, we should switch this to a feature request and I'd be willing to provide a MR.

In Drupal 8+ there doesn't seem to be any problem in setting an email address as username!

Yes, this works in Drupal 7 and likely earlier versions as well down to 4.6 and earlier.

why Email Registration forces a cleanup of usernames

One good reason I know is that usernames are "leaked" from Drupal core and contrib in a variety of spots. In my work on the security team we regularly get reports of a site that "leaks email addresses" and the maintainer of the site is very worried. But the realization hits that the users are entering email AS username not realizing that usernames will get leaked.

Here is where it's documented that usernames will be leaked https://www.drupal.org/drupal-security-team/security-team-procedures/dis... →

And here's some discussion to change that, but it's not done yet https://www.drupal.org/project/drupal/issues/3241232 🌱 [policy] Treat username enumerations as security bugs that require Security Advisories Active

Thank you very very much for your feedback @greggles! :)

So if I get this right, these are no technical, but security / information exposure reasons?
That's a very good point to not use the email addresses as usernames by default!

Still I'm thinking if a setting with, a clear explanation why you should take care, to disable the email to username sanitation, might be helpful. It should only be used of course, if user accounts can't be accessed and are not listed anywhere in public!

In our case it's a company with a client login. The email address tells the site owners which company the client belongs to, so it's not deserved to have the domain removed from the username.
Accounts are only accessible for site owner / team.

How would you rate that? Would it be fine for you to opt-out of the default sanitation?

Thank you once again, this is super helpful feedback!

Is that site accessible on the internet? If so...usernames are very likely to leak out. In my mind it would only be suitable for an intranet that is accessible on a private network.

But I can see having a setting to let it happen that is very hard to enable, e.g. it's in settings.php and the README says how to do it and explains the risks.

Thank you @greggles!

Is that site accessible on the internet? If so...usernames are very likely to leak out.

So I think the next step would be to look for examples in the mentioned issue and in general to see the cases exactly.

But I can see having a setting to let it happen that is very hard to enable, e.g. it's in settings.php and the README says how to do it and explains the risks.

Not a bad idea, if there's a realistic risk! In that case better than a checkbox (which might be added readonly as indicator if the setting is set so people down't wonder about what's happening).

Let's see if I or someone else comes back here or use a different method, for example by exposing the email address in an administrative view then instead. So I'll postpone it for now.

As of #7+#8 changing this into a feature request and our team is planning to work on this. Please care about the related issues, which are also relevant to this feature.

Eventually, instead of implementing this in the module (which is still my favorite way here), one of the following module could also solve this in combination. I haven't tried yet!

• https://www.drupal.org/project/auto_username →
• https://www.drupal.org/project/realname →

Add an option to use the E-mail address as username without cleanup

and

Add an option to sync the username to the email address

Sound, to me, as it should be combined into one option (of course as read-only and configurable through the settings.php as discussed here) ? As using the mail address as the username without syncing the fields won't make much sense! I'll update the issue summary accordingly.

We just discussed it might be better to implement this as a submodule to keep that part out of the core code. That submodule could also carry the security risk information.

Just discussed this implementation internally with @Anybody, and we came to the conclusion to implement this feature through a submodule. This way we can easily let the user know about the security risk implications and split the logic inside its own space!

Yes, I think a submodule is a good way to separate out the risk. It should be documented in the README of the main module and submodule that this can be a risk and also documented in the README and maybe module description that shows on the admin/extend page that there is a risk worth reading the README to learn about.

Alright, the code is finished! Now time to test and maybe add a few additional test cases!

I'll open it for review RN.

@greggles, seems like I missed your comment!

It should be documented in the README of the main module and submodule that this can be a risk and also documented in the README and maybe module description that shows on the admin/extend page that there is a risk worth reading the README to learn about.

I totally agree! I haven't touched the main module's README yet, but I can additionally provide this information there as well if desired! 🙂

$user->original is undefined in hook_ENTITY_TYPE_presave(). I'll test and finish this tomorrow.

VERY well done @Grevil! I left some comments. Perhaps @greggles would also like to take a look?

Looking forward to the testing and tests tomorrow :)
But perhaps let's better finish here first.

Some testcases we need with the new options:
- New account registration
- Existing account save without email change
- Existing account save with email change
- Existing account save with email change but overridden username
- Batch updating overridden / legacy usernames
- Tests with the several related core permissions
- Test with static obfuscation value
- Test with empty obfuscation value
- Test with core tokens
- Watch out for possible information disclosure tests, like:
- Author information on nodes, but missing "View user email address" or "View user information" permission?
- User profile pages without "View user email address" permission, but allowed to see the profiles
...

Ok all done, for some reason the "testNoObfuscationWithCorrectPermission" test still fails... no idea why, manually testing it works as expected...

@Grevil: Nice! I left some comments.

Please also add the other tests from my list. I think for some of them Functional tests would definitely make sense to check the results in the UI. Especially for information disclosure.

And perhaps add a Functional test to check the settings form fields appear and save as expected? (bonus, not important)

Alright, all "email_registration_username" succeed now! Ready for final review! 🙂

Great work! LGTM! :)

@greggles would you like to take a final look? We tried to compromise the security implications as much as possible, by implementing configurable user display name obfuscation through "hook_user_format_name_alter()".

Additional note: @Grevil also implemented tests to check typical places, where the username appears are obfuscated as expected!
By default, the same obfuscation as in the main module is used, but you may override it by a static string like ************ or use tokens :)

Next week we're planning to fix the other test issues and add some UX improvements.
Of course your review is optional @greggles but we'd be happy to have your feedback and bless ;)

Sorry, changes based on the last comment got a bit out of hand. Meaning this issue is a bit overloaded now. I'll avoid this in future issues!

RTBC from my side. No reply from @greggles yet, presumably he doesn't have the time currently, so let's proceed and just wait for his feeback before the next release.

Agreed! Merging then. :)

Small follow-up issue, while implementing the action test in 📌 Fix broken tests (D10.1) Active , I found the following issue: 🐛 Newly added batch action will throw an error on duplicate uid Active .

Automatically closed - issue fixed for 2 weeks with no activity.