I am using: Feeds Module (User Importer from CSV), which imports employees from HRMS into Drupal. ... this is related to some workflow logic that is working fine for us.

The same site is also using LDAP Integration Mixed-Mode authentication and configure to create a local account is not exists earlier

The user-importer feed is configured to treat the "mail" as a unique entry.

To describe the issue; let us assume a user with the following attributes:

Name: James Bond
SAMAccount: James.Bond
mail: Jbond@domain.com

Also, assume that we start with clean drupal, not user accounts, all clean fresh installtion, this applies to bo scenarios below...

Scenario 1: (Normal Behaviour)

1) Login to Drupal with James Account.

2) LDAP Integration will take care of authenticating against Active Directory and create a local drupal user account.

3) next Day I decide to run the 'Feeds-User-Importer' ... it will not create another local account for James Bond, as it will detect that such email exists already...
GREAT!

Now let us have a look unto second scenario where things become interesting:

Scenario 2:

1) Login to Drupal as admin.

2) Run the 'Feeds-User-Importer' ... it will create a new local account for James Bond.

3) Log out from Drupal, and relogin again as James Account ....

4) LDAP Integration will take care of authenticating against Active Directory and create a local drupal user account. CRITICAL BUG

The above should not happen, as the LDAP Intergration Module was not smart enough to realize that an already existing drupal user account with the same mail already exists.

The end result is that I endup with two account for the same person!!!

--------------

Final Verdict:
* Feeds Importer is working fine as expected.
* LDAP Auth is missing mail matching for potenially existing local accounts

I hope this gets good attention.

Best Regards
Bakr

CommentFileSizeAuthor
#5 patch-load-by-email.patch2.69 KBeiriksm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bakr’s picture

The problem is that the matching mechanism is relying on the ldapauth_user_lookup() function is mathing based on $name

whereas there should be a function to match with $mail

ldapauth.module file:

  // Authenticate LDAP user.
  if (!($dn = _ldapauth_auth($name, $pass)))
    return;

  if (!$account) {
    // Register this new user.
    if ($ldap_user = _ldapauth_user_lookup($name)) {
      // If mail attribute is missing, set the name as mail.
      $init = $mail = key_exists(($_ldapauth_ldap->getOption('mail_attr') ? $_ldapauth_ldap->getOption('mail_attr') : LDAPAUTH_DEFAULT_MAIL_ATTR), $ldap_user) ? $ldap_user[$_ldapauth_ldap->getOption('mail_attr')][0] : $name;

      // Check if the e-mail is not denied.
      if (drupal_is_denied('mail', $mail)) {
        form_set_error('name', t('The name %name is registered using a reserved e-mail address and therefore could not be logged in.', array('%name' => $name)));
        return;
      }

      // Generate a random drupal password. LDAP password will be used anyways.
      $pass_new = (LDAPAUTH_LOGIN_PROCESS == LDAPAUTH_AUTH_EXCLUSIVED || !LDAPAUTH_SYNC_PASSWORDS) ? user_password(20) : $pass;

      $userinfo = array('name' => $name, 'pass' => $pass_new, 'mail' => $mail, 'init' => $init, 'status' => 1, 'authname_ldapauth' => $name, 'ldap_authentified' => TRUE, 'ldap_dn' => $ldap_user['dn'], 'ldap_config' => $_ldapauth_ldap->getOption('sid'));
      $user = user_save('', $userinfo);
      watchdog('ldapauth', 'New external user %name created from the LDAP server %server.', array('%name' => $name, '%server' => $_ldapauth_ldap->getOption('name')), WATCHDOG_NOTICE, l(t('edit'), 'user/'. $user->uid .'/edit'));
    }
johnbarclay’s picture

Title: Clash with Feeds-User Importer » Need to check for existing emails in ldapauth.module
Priority: Critical » Normal

The problem is not the search for the user's ldap entry, its that it doesn't deny the user if the email has already been taken. this check in ldapauth.module would take care of this case:

      if (drupal_is_denied('mail', $mail)) {
        form_set_error('name', t('The name %name is registered using a reserved e-mail address and therefore could not be logged in.', array('%name' => $name)));
        return;
      }

 +    if ($existing_user_by_email = user_load(array('mail' => $mail)) ) {
 +       form_set_error('name', t('The mail %mail is already in use.', array('%mail' => $mail)));
 +       return;       
 +     }

If the username and mail derivations are different, using both will only create security problems.

The solution is for you to derive username from mail attribute or use username as unique attribute in feeds. The error catching above will only prevent the user from logging in and creating a second account.

bakr’s picture

Thanks to you.

I liked the above 'if' statement.

And so far, inline with your advice, I have been actually dervining the username from the mail attribute, which is the SAMAccount in essence.

Potentially, you could parametrize this in future.

Great Module!

Best Regards

johnbarclay’s picture

Yeah. I'm proposing that and dealing with additional identifier issues here: #638798: Associate LDAP & Drupal user accounts via email

Functionally 99% of use cases have one to one mapping of usernames and emails, but you can't be too careful with authentication modules.

eiriksm’s picture

FileSize
2.69 KB

I had this exact same problem. I solved it by doing the following modification, which could be committed without breaking the existing functionality. What it does is add an options to the sync module to load users by email, thus not creating the duplicate account.

Furthermore i wrote a small module (to avoid hacking the module too much) that is almost a copy/paste from the _ldapsync_sync function. It just adds LDAP auth to all Drupal users also existing in the LDAP result. Then it syncs data too (if set up LDAP data settings). The function is run on cron atm. The function is run on enabling the module, and since the LDAP auth is now enabled for the users, LDAP sync will then start syncing their account info, so the module is also automatically disabled once the auth-mapping has run.

Patch attached.

Sandbox with LDAP sync existing users module: http://drupal.org/sandbox/eiriksm/1435922

bakr’s picture

I am very happy with where this is going.

eiriksm’s picture

Status: Active » Needs review

Marking this as needs review, to try to get it commited.

cgmonroe’s picture

In looking through the code and comments on this, I think the underlying need is really to support the Drupal user "standard" that all user id and e-mail pairs need to be unique. This is assumed in LOTS of places like the forgot password processing, notifications, and the like.

The extra code in #2 does this for ldapauth. However, the patch for ldapsync does not completely follow this. If you load by e-mail, there is still a chance a user with the imported name exists.

So, I'm going to add an extra check into the "// User does not exist in Drupal. Let's create it" area of the code to verify that both e-mail and user id are unique.

Speak now (and soon) if this will cause undo pain for some reason I haven't considered.

Just to verbally recap what I'm going to be committing soon around this issue:

- Ldapauth will have the extra check for e-mail in use.
- Ldapsync will have the "What to use for testing for existing users option and related code
- The ldapsync user create area will check the uniqueness of the "other" test item... e.g. if email is the primary test/check name and vice versa.

cgmonroe’s picture

Version: 6.x-1.0-beta2 » 6.x-1.x-dev
Status: Needs review » Fixed

The dev version now has the fixes listed in #8.

Status: Fixed » Closed (fixed)

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