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
Comment | File | Size | Author |
---|---|---|---|
#5 | patch-load-by-email.patch | 2.69 KB | eiriksm |
Comments
Comment #1
bakr CreditAttribution: bakr commentedThe 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:
Comment #2
johnbarclay CreditAttribution: johnbarclay commentedThe 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 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.
Comment #3
bakr CreditAttribution: bakr commentedThanks 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
Comment #4
johnbarclay CreditAttribution: johnbarclay commentedYeah. 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.
Comment #5
eiriksmI 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
Comment #6
bakr CreditAttribution: bakr commentedI am very happy with where this is going.
Comment #7
eiriksmMarking this as needs review, to try to get it commited.
Comment #8
cgmonroe CreditAttribution: cgmonroe commentedIn 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.
Comment #9
cgmonroe CreditAttribution: cgmonroe commentedThe dev version now has the fixes listed in #8.