LDAP User: User pictures disappearing on login

Problem/Motivation

Whenever a user with a picture (e.g. $user->picture != 0) logs in, their picture is removed. Specifically, the image file remains intact, but the file ID ($user->picture and/or $user->picture->fid, depending on the context) is set to 0.

Note that this is using the built-in user picture feature and not a 3rd-party module.

Steps to reproduce using 7.x-2.x-dev and 7.x-2.x-beta5:

1. Log into a site with ldap_user enabled. (on my test rig, role and field provisioning to Drupal accounts is enabled, but the picture field is not setup for provisioning)
2. Ensure that user pictures are enabled via admin/config/people/accounts.
3. Upload a user picture.
4. Verify that /user displays the uploaded picture.
5. Log out.
6. (Optional) Verify that the user's picture is still displayed at /user/uid.
7. Log back in as the same user.
8. The user's picture is no longer set.

Proposed resolution

It appears that the call to user_save() in the ldap_user_user_login(&$edit, $account) hook is to blame. Specifically, the value of $account->picture is merely the file ID rather than the full picture object that is normally returned by user_load(). This causes user_save() to set the user's picture to 0 (see Issue #935592: User picture is deleted after calls to user_save()).

The best solution I've found so far is to call user_load() prior to the save. I looked through the ldap_user_user_login(&$edit, $account) function and did not see any changes being made to $account prior to the call to user_save(); it also seems unlikely that provisioning to LDAP would modify the Drupal user in the future, so it appears to not cause issues with potential future changes.

Thus,

function ldap_user_user_login(&$edit, $account) {
  // Code for provisioning from a Drupal user/account to LDAP.

  // Provision from LDAP to a Drupal user.
  if (/* configured to provision to Drupal users */) {
    // Code to populate $user_edit with the information from LDAP.

    $account = user_save($account, $user_edit, 'ldap_user');
  }
}

becomes

function ldap_user_user_login(&$edit, $account) {
  // Code for provisioning from a Drupal user/account to LDAP.

  // Provision from LDAP to a Drupal user.
  if (/* configured to provision to Drupal users */) {
    // Load the user before saving to fully populate the $account->picture field.
    $account = user_load($account->uid);

    // Code to populate $user_edit with the information from LDAP.

    $account = user_save($account, $user_edit, 'ldap_user');
  }
}

Remaining tasks

(reviews needed, tests to be written or run, documentation to be written, etc.)
I have done a brief scan through the rest of the code base for other places that may be calling user_save() on user objects without fully-loaded picture objects and didn't spot any obvious ones. There are, however, several functions that are passed a user object as a parameter that do not load the user object prior to saving it, so the safety of those methods depends on the calling function. Here's a rough example of the functions that are potentially unsafe for the user's picture:

function myFunction($account, &$user_edit) {
  // Code that adds to and/or changes $user_edit.

  user_save($account, $user_edit);
}

The functions I've identified as at-risk are as follows:

• _ldap_authorizations_user_authorizations_set
• revokeSingleAuthorization
• grantsAndRevokes
• provisionDrupalAccount
• ldapAssociateDrupalAccount
• _ldap_user_orphans

I may have missed some, and these functions may not actually cause any problems; I just am not (yet) familiar enough with the flow of execution within these modules and consequently cannot easily verify if they will break the user's picture.

Tests should be added to check for user objects with gettype($account->picture) == 'string' being passed to user_save().

Related Issues

Issue #935592: User picture is deleted after calls to user_save()