Don't allow users to register with name the same as the Anonymous name

I think the original post is actually a symptom of a larger issue. We should not let users register using the name that we are using for the Anonymous user. Similarly, we should not be able to set the anonymous user name to the name of a user that already exists.

This patch fixes this (and thus fixes the original issue).

so would the patch in #1 + a case insensitive comment name check solve this?

Will play with this a bit later

rayasa: Actually, I think the patch in #1 does completely address the issue.

Lets say we have a default drupal install... the anon user is called 'Anonymous'. With the patch in #1 a user cannot register with the name "Anonymous" therefore a user commenting with "Anonymous" in the name field is fine. If a user registers as "anonymous" then there is still no problem because if someone tries to comment as "anonymous" they will get an error that there is a user registered as "anonymous".

In another case, lets say that our anonymous user name is set as "Bob"... The comment form will be pre-populated with "Bob" in the name field which is fine. And again, if a user tries to register as "Bob" he will get an error and not be able to.

Finally if the site admin tries to change the anonymous user name to "George" and there is already a registered username with that name, the patch in #1 throws an error and tells the site admin to pick something else.

I think that is every possible case covered ...

Am I missing anything?

This patch does not allow a user to register as "Anonymous" or "anoNymous" (assuming that is the value of variable_get('anonymous') ... so you can never run into the situation you describe in #7

Patch has some code style issues; tabs, abbreviation of 'anonymous', etc.

blaaarg ... been testing out Firefox4 and there is no Dreditor yet :(

How's this

linclark ... where is anonymous abbreviated? I think I got them all.

+++ modules/user/user.admin.inc	8 Sep 2010 12:08:50 -0000
@@ -639,6 +639,15 @@ function user_admin_settings() {
+ * A FAPI validate handler.

"Form validation handler for the user settings form."

+++ modules/user/user.admin.inc	8 Sep 2010 12:08:50 -0000
@@ -639,6 +639,15 @@ function user_admin_settings() {
+  if ((bool) db_select('users')->fields('users', array('uid'))->condition('name', db_like($form_state['values']['anonymous']), 'LIKE')->range(0, 1)->execute()->fetchField()) {

Have a look at http://drupal.org/node/310075 and other code in core to see how we write dynamic database queries.

+++ modules/user/user.admin.inc	8 Sep 2010 12:08:50 -0000
@@ -639,6 +639,15 @@ function user_admin_settings() {
+    form_set_error('anonymous', t('There is a registered user with the name %anonymous. You must choose a unique name for anonymous users.', array('%anonymous' => $form_state['values']['anonymous'])));

The error message looks a bit lengthy to me. Please try to shorten it without losing information.

+++ modules/user/user.module	8 Sep 2010 12:08:50 -0000
@@ -607,6 +607,9 @@ function user_validate_name($name) {
+  if (strtolower($name) == strtolower(variable_get('anonymous', t('Anonymous')))) {

1) Read http://drupal.org/node/473460 about Drupal's PHP Unicode wrapper functions. Full list of wrappers: http://api.drupal.org/api/group/php_wrappers/7

2) When a user registers in a non-English language, this does not check for the non-localized version of "Anonymous".

+++ modules/user/user.module	8 Sep 2010 12:08:50 -0000
@@ -607,6 +607,9 @@ function user_validate_name($name) {
+    return t("The username '%anonymous' is reserved. Please choose a different username.", array('%anonymous' => $name));

1) Quotes around %anonymous need to be removed. Afterwards, the surrounding double-quotes can be turned into single-quotes.

2) %anonymous should be %name, for consistency with the other messages.

3) Speaking of, the other messages do not seem to use any second "Please..." sentence.

Powered by Dreditor.

This patch takes care of all the suggestion from sun in #18 except for changing the dynamic query. I basically took that query from here:

function user_account_form_validate($form, &$form_state) {
  if ($form['#user_category'] == 'account' || $form['#user_category'] == 'register') {
    $account = $form['#user'];
    // Validate new or changing username.
    if (isset($form_state['values']['name'])) {
      if ($error = user_validate_name($form_state['values']['name'])) {
        form_set_error('name', $error);
      }
      elseif ((bool) db_select('users')->fields('users', array('uid'))->condition('uid', $account->uid, '<>')->condition('name', db_like($form_state['values']['name']), 'LIKE')->range(0, 1)->execute()->fetchField()) {
        form_set_error('name', t('The name %name is already taken.', array('%name' => $form_state['values']['name'])));
      }
    }
...

Can you be more specific about how I should do this differently

Rerolled for Drupal 8, added tests and took the opportunity to convert Drupal\user\Tests\UserValidationTest to DrupalUnitTestBase

+++ b/core/modules/user/lib/Drupal/user/Tests/UserAdminSettingsFormTest.php
@@ -41,4 +41,18 @@ public function setUp() {
+    $admin_user = $this->drupalCreateUser(array('administer users'));
+    $this->drupalLogin($admin_user);

Hat tip: Whenever user permissions are irrelevant:

$this->drupalLogin($this->root_user);

That logs in uid 1. Only available in D8 web tests.

+++ b/core/modules/user/lib/Drupal/user/Tests/UserAdminSettingsFormTest.php
@@ -41,4 +41,18 @@ public function setUp() {
+    $this->assertRaw(t('There is already a registered user %anonymous. You must choose an unused name.', array('%anonymous' => $user->name)), "Can not change the anonymous name to an existing user's name");

Can we remove the custom assertion message here? It duplicates the raw text assertion.

+++ b/core/modules/user/user.admin.inc
@@ -612,6 +613,21 @@ function user_admin_settings($form, &$form_state) {
+function user_admin_settings_validate($form, &$form_state) {
...
+    form_set_error('anonymous', t('There is already a registered user %anonymous. You must choose an unused name.', array('%anonymous' => $form_state['values']['anonymous'])));

If I'd get that error message, I'd like to see that user account. I think it would be helpful to turn the placeholder into a link.

To make that work, we could use user_load_by_name() instead of the $name_taken query.

I don't think we have to care for user-view permissions, since the administrative user who has access to these account settings most likely also has access to view user accounts.

+++ b/core/modules/user/user.module
@@ -353,6 +353,9 @@ function user_validate_name($name) {
+  if (drupal_strtolower($name) == drupal_strtolower(t('!anonymous', array('!anonymous' => config('user.settings')->get('anonymous'))))) {

Hm. I thought the t('!anonymous') is obsolete in D8?

Granted, there's no config translation system yet, but at least I was fairly sure that we've removed t() from all dynamic/config strings in the meantime.

Implemented suggestions from #21.

Really like the use of user_load_by_name() in admin_settings_validate() - makes the patch nice and tidy :)

+++ b/core/modules/user/user.admin.inc
@@ -616,14 +616,9 @@
+    form_set_error('anonymous', t('There is already a registered user <a href="!user_view">%user</a>. You must choose an unused name.',  array('%user' => $account->name, '!user_view' => url("user/$account->uid"))));

I think we need to use $account->uri() for the URL here, like this:

$uri = $account->uri();
url($uri['path'], $uri['options'])

Ahhh,,, learnt a new thing today :) thanks for the review.

Thanks!

This is a good idea, but what's the upgrade path look like? From what I can tell, someone who was already pre-registered with the name "Anonymous" (or whatever) can no longer log in / edit their profile / etc. with this change?

Erm. That's what support@example.com is for. I don't think we should care for that 0.01% case. Those sites have a registered user who posts as Anonymous in the first place, ... and by the looks of it, that's the bug which triggered this very issue, so ???

:)

Well the only way of doing this is probably something like this - which I'm not sure I particularly like but will prevent sites from getting in the situation @webchick describes in #26.

--- a/core/includes/update.inc
+++ b/core/includes/update.inc
@@ -161,8 +161,22 @@ function update_prepare_d8_bootstrap() {
         'description' => $has_required_schema ? '' : 'Please update your Drupal 7 installation to the most recent version before attempting to upgrade to Drupal 8',
       ),
     );
-    update_extra_requirements($requirements);
 
+    // Ensure there is no user that matches the anonymous name.
+    $anonymous_name = update_variable_get('anonymous', 'Anonymous');
+    $uid = db_select('users')->fields('users', array('uid'))->condition('name', db_like($anonymous_name), 'LIKE')->range(0, 1)->execute()->fetchField();
+    if ((bool) $uid) {
+      $requirements = array(
+        'username and anonymous conflict' => array(
+          'title' => 'Username and anonymous variable conflict.',
+          'value' => 'User (uid: <em>' . $uid . '</em>) has the same name as the anonymous user <em>' .$anonymous_name . '</em>',
+          'severity' => REQUIREMENT_ERROR,
+          'description' => 'Please either change the user\'s name or the anonymous name',
+        ),
+      );
+    }
+
+    update_extra_requirements($requirements);

I'm really not sure I understand the logic for needing extra care for existing sites. Here's why:

A) Sites that are not experiencing this bug do not care.

B) Sites that are experiencing this bug want a straight fix.

C) Sites that are experiencing this bug, but are not aware of it, will face one of these two situations:

C.1) The site administrator attempts to update the administrative user settings form. An error is displayed that the existing (unchanged) value for "Anonymous" conflicts with an existing username. The site administrator needs to fix it in whichever way he prefers.

C.2) The user with the username that clashes with "Anonymous" attempts to update his own user profile. Only in case users are permitted to change their username, the existing (unchanged) username will be validated upon form submission. The user is asked to choose a different name. The user will either do that, or consult the site administrator to learn what's going on. The displayed error message clarifies that the chosen username clashes with a reserved username.

Am I missing something?

#28: 903606.28.patch queued for re-testing.

This is covered in #472202: 'Name' value for anonymous comments can conflict with registered usernames. Closing this and will transfer credit over there.

In #472202-41: 'Name' value for anonymous comments can conflict with registered usernames @joachim suggested this problem is wider than comments, e.g. node author.

Did a bit of testing and there is no error on node creation by an anonymous if there is a registered user Anonymous. I think this is because it specifically references 'Anonymous (0)' so it's not just validating the plain text against the list of users.

I also don't think it's that confusing because as I pointed out in #472202-35: 'Name' value for anonymous comments can conflict with registered usernames the registered username is linked and the anon one says (not verified) and the same is true for nodes.

Creating a node as anon with author 'Anonymous'

Node created by anonymous user vs. node created by user 'Anonymous'

So unless there are other issues arising from having a user with the anonymous username, I think this can stay closed. The error that occurs on the comment form is covered in the other issue.

I think you're misunderstanding the bug.

This bug is not to do with content creation by anonymous users.

This bug is that you should not be allowed to register a user account with the username 'Anonymous', or 'Anonymous (not verified)' because it's confusing to see user labels on the site that look identical but refer to 2 totally different things.

(Conversely, you should not be allowed to change the configuration for anonymous user label (at admin/config/people/accounts) to a string that matches an existing username.)

I am reopening this because it really is a different bug from #472202: 'Name' value for anonymous comments can conflict with registered usernames. This bug occurs without comment module enabled. The other bug is only to do with comment module.

I understand but I don’t agree that it’s confusing. Fair enough though, it’s OK to disagree.

But this needs an issue summary update at least to clarify that the intention is not to resolve a specific error but prevent the possibility of a clash.

I've implemented following changes:

• Re-roll the patch.
• Update code as per constraint approach for user name validation.
• Update test cases accordingly.
• Added validation in account settings form.

Other changes related to hook_requirement():

+++ b/core/includes/update.inc
@@ -161,8 +161,22 @@ function update_prepare_d8_bootstrap() {
+    // Ensure there is no user that matches the anonymous name.
+    $anonymous_name = update_variable_get('anonymous', 'Anonymous');
+    $uid = db_select('users')->fields('users', array('uid'))->condition('name', db_like($anonymous_name), 'LIKE')->range(0, 1)->execute()->fetchField();
+    if ((bool) $uid) {
+      $requirements = array(
+        'username and anonymous conflict' => array(
+          'title' => 'Username and anonymous variable conflict.',
+          'value' => 'User (uid: <em>' . $uid . '</em>) has the same name as the anonymous user <em>' .$anonymous_name . '</em>',
+          'severity' => REQUIREMENT_ERROR,
+          'description' => 'Please either change the user\'s name or the anonymous name',
+        ),
+      );
+    }
+
+    update_extra_requirements($requirements);

I haven't included above hook_requirement() implementation as I am not sure if it is required.
@alexpott, can you please confirm whether these changes are required.

Let's see how many test cases get affected.

For IS update

Updated IS but leaving the tag for someone to confirm.

Also MR needs to be updated to point to 9.5.x please

Rebased the MR #909 against 11.x branch and pushed changes.

Left some comments on the MR.