Registering account with already existing name will bump up sequences entry, and show SQL error (because name column is unique - and good :). Of course user will not be added to users table.

Comments

Bèr Kessels’s picture

I can confirm the bug.

Bèr Kessels’s picture

A heads up.
Line ~1200 user.module in _user_edit_validate states:

 if (user_access('change own username') || user_access('administer users')) {
    watchdog('debug', "Entering _validate with $uid ", WATCHDOG_WARNING);
    if ($error = user_validate_name($edit['name'])) {
      form_set_error('name', $error);
    }
    else if (db_num_rows(db_query("SELECT uid FROM {users} WHERE uid != %d AND LOWER(name) = LOWER('%s')", $uid, $edit['name'])) > 0) {
      form_set_error('name', t('The name %name is already taken.', array('%name' => theme('placeholder', $edit['name']))));
    }
    else if (drupal_is_denied('user', $edit['name'])) {
      form_set_error('name', t('The name %name has been denied access.', array('%name' => theme('placeholder', $edit['name']))));
    }
  }

Obviously on registration, a user has no rights yet. So user_access('change own username') || user_access('administer users') always FALSE.

A quick solution would be to add a third item to the if,

 if (variable_get('user_register', 1) || user_access('change own username') || user_access('administer users')) { 

But that is no *real* solution. The problem lies in the very badly coded op-in security. WE validate only if Foo or bar are treu. We *should* *always* validate. And optionally skip validation if, and only if certain settings explicitly tell us to. Opt-out security.

Bèr Kessels’s picture

StatusFileSize
new1.36 KB

Here is a patch that fixes this using abovementioned solution. Note that, however this fixes this, by no means I think this is the route to take. The problem, as stated above, lies deeper.

Bèr Kessels’s picture

Title: User's sequences are bumped by registering duplicated account » validation on registration is skipped (causing no check for name duplicates!)
Status: Active » Needs work
merlinofchaos’s picture

I'm not completely sure, but wouldn't it be better to check if the $user record has no uid? In that case, the name *always* needs validation because we know that if there is no uid, it's not already in the database.

Bèr Kessels’s picture

@merlin. That would be a step closer to tighter secure programming. But I would say turning around the loc and rewriting that validate function completely is the best solution:
* always return TRUE, so always return an error, or non validating part.
* If there is a uid, then do specific checking for an existing account
* else (default) check for validated email addies, blocked users, and names.
Ber

Bèr Kessels’s picture

The patch I supplied breaks the edit-user fields.

killes@www.drop.org’s picture

Status: Needs work » Fixed

This is fixed.

killes@www.drop.org’s picture

This is fixed.

Anonymous’s picture

Status: Fixed » Closed (fixed)