Changing Email address via LDAP Data deletes value in database
| Project: | LDAP integration |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | miglius |
| Status: | closed |
| Issue tags: | ldapdata |
Changing the Email address via the account page works. Both values in my OpenLDAP server and the mysql database are changed. Changing the Email address on the LDAP entry page saves the changes to the LDAP server, but deletes the mail field value from the user table in the database (not the complete record). There is only a success message. When I go to the account page, the E-mail address field is empty.
After that, it is not possible to change the password (E-mail address field is required). After Log out, password request is not possible. After the next logon, the password is resynced with the LDAP server.
This does not depend on the setting of Alter user email field on user edit form, except that changing email address via the account page is only possible with the do nothing setting. So the only working setup is do nothing there and disable email editing on the LDAP entry page.
The status page on the help module is completely green.
#1
Can you please test with the dev version if the issue is still there?
#2
It is still there.
I have debugged through the code and found:
In function user_profile_form_submit() (file user.pages.inc) there is an array $form_state['values']. It contains the names and values of the form fields. First all the hooks are called via user_module_invoke(). Before the call, all values are as I entered them in the form, after the call they are all uninitialized and that happens in the lpdap modules.
After calling the hooks, Drupal performs it's own saving through user_save(). Every array element, which has a name, which is a field name of the user table, is written to the database. For me this is the case for the mail field. Since the value is now uninitialized, the field in the database is empty after that.
Sorry, I don't know your code deep enough to develop and supply a patch, but I hope, this helps to fix the problem.
#3
arnd,
what did you set for "Alter email field on user edit form" at "admin/settings/ldap/ldapauth"?
#4
When I set Do nothing, then the content of the mail field in the user table of the SQL database is deleted. This happens in both versions, alpha and dev.
When I set Remove email field from form, then the same happens in the alpha version. In the dev version the content of the mail field is deleted both in the LDAP directory and the SQL database!
#5
A workaround with the current dev version would be *not* to provide the E-mail field as an editable LDAP field, and to keep the "Alter email field on user edit form" on "Do nothing".
#6
I digged a little bit deeper into the code. Currently the logic is as follows:
user.pages.inc (core file)
<?phpuser_module_invoke('submit', $form_state['values'], $account, $category);
user_save($account, $form_state['values'], $category);
?>
Which means:
The first error has happened before this code is executed: In the dev-version with Remove email field from form set, the value of the mail field is already NULL (don't know why).
ldapdata.module in _ldapdata_user_submit()
<?phpif ($category == LDAPDATA_USER_DATA && $editables) {
...}
else if ($category == 'account') {
...}
?>
Which means: Do different things with the data, depending on wether they were entered on the account page or the LDAP entry page.
ldapdata.module in _ldapdata_user_update_drupal_account()
This is the account page handling: Find out, which of the values are stored in LDAP, prepare this data for LDAP write and keep the form data, so that the user_save() core function can save the data to the database. This happens to mail address changes on the account page, it just works.
ldapdata.module in _ldapdata_user_update_ldap_attributes()
This is the LDAP entry page handling: Prepare the data for LDAP write and
<?php$edit[$edit_attr] = NULL;
?>
Which means: keep the key, but delete the value. So the user_save() core function sees the key, checks, whether the key belongs to the user database table and if so, it will save the NULL value there. This happens for the mail field.
The correct handling for the LDAP data page should be: Check whether the LDAP field is synchronized to a field in the database (like mail). If so, keep key and value. If the field is present in LDAP and database, but the value shall be kept only in LDAP, then set the value to NULL (delete value from database), otherwise unset the key.
#7
#8
This might be a duplicate for a #380348: Value of E-mail field gets deleted from DB. Could you check if the patch attached to the mentioned issue would also solve this problem?
#9
The problem there has certainly the same reason. The problem also happens, when I change my given name. It just happens always, when you save data, except on the acount page.
But the patch is a quick hack, which does the wrong thing at the wrong place. The code there is also executed during login, the correct place is the function _ldapdata_user_update_ldap_attributes. If you just want to fix it quickly, fix it there. But it is not correct to unset the mail key. That way a changed email address will not be written to the SQL database. You have to keep key and value in order to keep email addresses in sync.
Also the problem is not restricted to the mail field. If you add other fields to function ldapdata_attributes in ldapdata.conf.php and if the name of the added field in the LDAP directory happens to be the same as a field name in the SQL user table, then the error will happen with that field too. It is a name clash from SQL and LDAP name spaces.
Finally: The patch does its job only with Remove email field from form. The error will also happen with disabled mail field and it also happens, if you keep both mail fields.
I reported above, that in the dev version with Remove email field from form there happens another error. What I didn't see last time is: due to this error, the mail value is deleted from the LDAP directory. But this is not related to the patch.
#10
I don't consider the patch in #380348: Value of E-mail field gets deleted from DB to be a quick hack. When email field is removed from the form, then the mail field should be removed from the $edit array to make sure that an old mail is preserved in both drupal and LDAP. This is exactly what the patch does.
I agree that there is possibility of the other names clash. Will address it shortly.
#11
The attach patch should fix the name clash issues and probably the deleting from the db issue as well as a consequence.
#12
Ok, this way there are no more name clashes and the value from the database isn't deleted. But a changed email address isn't saved to the database any more, only to LDAP.
#13
Are Drupal and LDAP mails mapped to each other at the 'admin/settings/ldap/ldapdata/edit/1' -> "Drupal-LDAP fields mapping"?
Do you save the "Account" or "LDAP entry" tab?
#14
Yes they are.
In my debugger I can see at (user.pages.inc, line 287)
<?phpuser_module_invoke('submit', $form_state['values'], $account, $category);
user_save($account, $form_state['values'], $category);
?>
that in the first line $form_state['values'] contains keys like ldap_givenName, ldap_sn. When I step over user_module_invoke, then $form_state['values'] is empty. But in that call to user_save, the data would be written to SQl (at least this happens, when I save on the account page).
Where should the synchronization happen otherwise?
#15
Maybe this is a dumb question, because I missed something:
I started playing with profile fields and now I don't understand anymore, why there is this LDAP Entry page, which makes so much trouble and requires a configuration file written in php. The only point it offers additionally (as far as I can see now) is, that I can show LDAP values on the View page, without the user being able to edit that values. It has a lot more functionality, but all of that can also be done with profile fields and standard drupal techniques.
Profile fields are not affected by this issue. Profile fields can be configured without a configuration file and I can name the pages and fields myself.
Maybe there is a simpler way to only show LDAP data. That would probably take some of the complexity out of the code and also makes this module a little easier to understand.
#16
Some installations needs to allow for users to display/edit their ldap data which is not mapped to the profile. This page does that. You can always uncheck all checkboxes in the settings page and this page will not be shown.
#17
I don't understand. You can make profile fields private. So where is the difference, except the view only mode?
#18
Committed the patch to the cvs.
#19
Automatically closed -- issue fixed for 2 weeks with no activity.