[2.x] Prevent race condition in account generation code by adding uid to name

I just posted a patch to another issue when I saw this and it got me thinking. Why not just replace the @ with an underscore and put the whole email address in. There shouldn't be any collisions at all that way, plus every user name could be computed from their email address without added information.

I agree with scottgifford - that we can't show the entire e-mail address. Even showing partial e-mail is a potential issue because spammers can guess the domain (yahoo, hotmail, gmail will cover 75% of the users on most sites).

What about #247717: provide a hook so other modules can help generate the user name which would let the admin define a style for the username generation including UID. On small sites the race condition will never be a problem and uids in the name are kind of annoying so they could be ommitted there.

This would fix also #421078 postgres compatibility issue. Adding _UID certainly would make the function less complicated although there would need to be a check for existing usernames for sites already using the module.

Drupal now has a semaphore system that we could potentially use instead of the UID.

Using the uid seems like a decent idea so we avoid the slowish semaphore system.

Attached is a patch to do this. I didn't want to do it in the uniqueness check nor the email_registration_cleanup_username because those have a specific purpose and this seems like it would be an unwanted side-effect in the use of those functions.

#8: 423920_prevent_race_with_uid.patch queued for re-testing.

Whoops. I committed that patch accidentally so it stopped applying - see https://drupal.org/node/1996664#comment-7823177

OK, that's a valid failure now though probably just the test needs to be updated.

ahem.

Here's a reroll/rethink.

1) this should be in the overridable default cleanup function
2) it should be keep the functions compatible with any current usage (e.g. I work on a codebase that uses the function directly without passing in uid)
3) for some reason $uid was in a function where it wasn't used - removed that

OK, let's try this.

One more try - somehow I missed the fact that $uid was being used in the unique check.

Yay testing!

And status to get it tested.

Great, fixed!

I don't plan to backport things to 6.x. If someone cares about that please reroll the patch and change the status to needs review.

Is there any way to turn this off? My clients are complaining about the appended uid :(

Of course! implement hook_email_registration_name and then this code gets skipped. Copy and paste what you like.

I'd also be open to making this optional if you want to write a patch for that instead.

> Using the uid seems like a decent idea so we avoid the slowish semaphore system.

Is the semaphore system that slow? Surely user registration is an event that doesn't need to be optimized for speed -- we're doing a load of DB inserts anyway.

And while I can write a patch to add an option to turn off the uid suffix:

a) it means anyone turning it off is then vulnerable to the race condition problem, which at least one person in this thread thinks is unwise even for small sites (plus I don't know what is meant by 'small'!)
b) it means that we still have the really ugly appending of the uid enabled by default, and the only available option for sites that are large enough to be concerned about the race condition

Can we rethink this?

In my experience the semaphore system is slow, yes. I guess that was using the default mysql-based lock which is slower than something like a redis/memcache lock, but most sites do use the default one.

I don't find the uid on the end to be particularly "ugly". I also believe that most people using this module have hidden the username on their site and are using something like https://drupal.org/project/realname

There's a proposal which addresses this in a slightly different way at #2273265: An interface to configure username. Maybe that interests you?

> In my experience the semaphore system is slow, yes

That's fair enough, but my point is, does performance matter for a process such as user registration? It happens relatively rarely (compared to things like page loads and user logins), and it's already a slow process as it involves a lot of DB writes.

> I don't find the uid on the end to be particularly "ugly".

My client's words, not mine! :)
On my project, we're just telling users to log in with the first part of their email address -- due to #657472: Add setting to allow users to login with email address or username. If they now have a UID appended, then that's no longer true, and they have to be aware of what their username is -- which this module is meant to hide!

> There's a proposal which addresses this in a slightly different way at #2273265: An interface to configure username: An interface to configure username. Maybe that interests you?

The thing is that that won't fix the race condition if you configure it to not append the UID -- you'll hit the problem that this issue was meant to tackle.

PS. In case it's not clear, I'm volunteering to work on changing this to use the semaphone system :) -- should I reopen this or file a new issue?

caught me off-guard (no I don't always read release notes :P); do find it unsightly; did not have realname in place... but expediting that now; thanks for the documentation! :)

Tests are fixed in commit 4278/57f, needs port to 8

Ported to D8, please test it.

I ported it but I can't reproduce because I can't create two user at time, only have 1 mouse ^^

But... if it's a condition to create a 8.0 version we must to fix it :P

+++ b/email_registration.module
@@ -108,6 +108,11 @@ function email_registration_cleanup_username($name) {
+	// Put uid on the end of the name.
+   	$name = $name . '_' . $uid;

tabs should be replaced with spaces

+++ b/email_registration.module
@@ -25,7 +25,7 @@ function email_registration_user_insert(&$edit, &$account, $category = NULL) {
-    $new_name = email_registration_cleanup_username($new_name);
+    $new_name = email_registration_cleanup_username($new_name, $account->uid);

@@ -67,7 +67,7 @@ function email_registration_user_insert(&$edit, &$account, $category = NULL) {
-function email_registration_unique_username($name, $uid = 0) {
+function email_registration_unique_username($name, $uid) {

@@ -92,7 +92,7 @@ function email_registration_unique_username($name, $uid = 0) {
-function email_registration_cleanup_username($name) {
+function email_registration_cleanup_username($name, $uid = NULL) {

not properly ported

Re-roll and fix nits, not sure it's ok to change it but no usages in contrib found
- http://grep.xnddx.ru/search?text=email_registration_cleanup_username
- http://grep.xnddx.ru/search?text=email_registration_unique_username

underscore expected

Needs work for the test fails.

This also changes the API, so is worth an extra big release note on that.

This can be still reproduced in 2.x, in "hook_ENTITY_TYPE_presave()" through commenting out $new_name = email_registration_unique_username($new_name, (int) $account->id()); in line 58 in the email_registration.module.

This is quite the edge case, but should be solved nonetheless. I am not a big fan of adding a UUID at the end of the username. Could this eventually solved by executing an SQL transaction?

> Why not just replace the @ with an underscore and put the whole email address in. There shouldn't be any collisions at all that way, plus every user name could be computed from their email address without added information.
> There is a privacy issue with revealing the user's entire email address as their username, if their username is ever visible.

Alternative idea: instead of appending the uid, or the email address, append a short hash of the username. That way we get something (probably!) unique, but we don't need the uid, and we don't expose the email address.

See https://roytanck.com/2021/10/17/generating-short-hashes-in-php/ for instance for creating short hashes.

Great idea @joachim! Anyone willing to implement that?