To reproduce:

1) Log in as an admin user, with privileges to "administer blocks" and "administer users".
2) Go to admin > site building > blocks, and configure the "Who's online" block so that "Hide this block by default but let individual users show it" is selected. Save these settings.
3) Go to admin > users > users, and edit a user other than yourself.
4) Under "Block Configuration", change the setting for the "Who's online" block. Save.
5) Edit the same user.

Expected behavior:

The form should reflect the change which was just made to the settings.

What happens instead:

The form displays your (the admin user's) settings every time.

Now, the admin user's settings aren't necessarily being saved to the other user's account -- unless you click the submit button at this point. But they will always be displayed.

I believe the problem is in block.module.

function block_user($type, $edit, &$user, $category = NULL) {
  global $user;
  switch ($type) {
    case 'form':
      if ($category == 'account') {
        $result = db_query("SELECT DISTINCT b.* FROM {blocks} b LEFT JOIN {blocks_roles} r ON b.module = r.module AND b.delta = r.delta WHERE b.status = 1 AND b.custom != 0 AND (r.rid IN (%s) OR r.rid IS NULL) ORDER BY b.weight, b.module", implode(',', array_keys($user->roles)));
        $form['block'] = array('#type' => 'fieldset', '#title' => t('Block configuration'), '#weight' => 3, '#collapsible' => TRUE, '#tree' => TRUE);
        while ($block = db_fetch_object($result)) {
          $data = module_invoke($block->module, 'block', 'list');
          if ($data[$block->delta]['info']) {
            $return = TRUE;
            $form['block'][$block->module][$block->delta] = array('#type' => 'checkbox', '#title' => check_plain($data[$block->delta]['info']), '#default_value' => isset($user->block[$block->module][$block->delta]) ? $user->block[$block->module][$block->delta] : ($block->custom == 1));
          }
        }
      # etc
      }
  # etc
  }
}

Wherever the function above says "$user", shouldn't it say "$edit" instead? Here, $user refers to the admin user (the currently active account), and $edit represents the user account being edited.

Comments

noah@groups.drupal.org’s picture

Or maybe the culprit is the first line of code inside the function. We should just get rid of that "global $user" -- it overwrites the correct $user, which is passed as a parameter. I tried this and it seemed to solve the problem.

beginner’s picture

Version: 5.1 » 6.x-dev
Priority: Normal » Critical
Status: Active » Needs review
StatusFileSize
new575 bytes

The bug still exist in D6.

To reproduce: set some blocks with the setting "Hide this block by default but let individual users show it.". Enable them in your admin account. Edit another user account, and see that the block seem to be already enabled. Save the user account without making any change. Log in as that user, and see that the blocks are now enabled on that account.

There is no way for the admin to see which blocks the user had enabled or not.

By definition, this bug is critical because some data is manipulated where it shouldn't, therefore loosing the real user data. The data can not be easily recovered, especially for sites with many roles and many blocks. E.g. at drupal.org, each time an admin edits a user account, he is enabling the "Contributor links" for that user, regardless of the original user setting.

webchick’s picture

Status: Needs review » Needs work

Yikes. Nasty bug.

However, in order to prevent this from happening again, let's do the best practice of naming that variable &$account rather than &$user. That way this won't bite us again in the future should we decide we actually do want the global user variable in there for some reason.

Steven Merrill’s picture

Status: Needs work » Needs review
StatusFileSize
new1.99 KB

Give this one a shot. It incorporates Angie's suggestion from above.

As mentioned above, the bug never prevented an administrator from saving the setting, the display on user/*/edit would be incorrect. This patch fixes the display, as well as changing the parameter name to $account.

killes@www.drop.org’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new1.99 KB

I've update Steve's patch to also remove the "global $user;" line. I've tested it and it works.

gábor hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Committed, thanks.

beginner’s picture

Version: 6.x-dev » 5.x-dev
Status: Fixed » Reviewed & tested by the community
StatusFileSize
new2.06 KB
dries’s picture

Status: Reviewed & tested by the community » Needs work

This patch doesn't seem to apply against CVS HEAD.

gábor hojtsy’s picture

Status: Needs work » Reviewed & tested by the community

Dries: this is a bug against D5, and one that was fixed in HEAD already. Let's get Niel to review this.

drumm’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 5.x.

Anonymous’s picture

Status: Fixed » Closed (fixed)

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