Regression: RC6 checking for users in list prevents admin masqurade

I have applied #2. Thank you.

I have carefully read the code in #2, as well as in commit 79e5e5 which implemented the security change for RC6. Studying it carefully, I could find no problems.

I then applied the patch, set the masquerade as any user for the administrator role. Using the admin account, I could use the auto-complete form in the block and the links on the target users' profile page to masquerade.

I then logged in as a non-admin user and confirmed that I could no longer do either, as desired and as in accord with RC6.

I observed only two minor things:

1) The indentation of the db_select would be better as follows:

  if ($to_uid !== FALSE) {
    $allowed = user_access('masquerade as any user') ||
               db_select('masquerade_users', 'm')
                  ->fields('m', array('uid_to'))
                  ->condition('m.uid_to', $to_uid, '=')
                  ->condition('m.uid_from', $user->uid, '=')
                  ->execute()
                  ->fetchField();
  }

2) The permission is not initially set for the Administrator role, as is the custom. This could be done in an update routine. Or it could be argued that in light of the RC6 security release, it is best left as a strictly opt-in feature.

I leave 1) to the maintainer to format to his/her preference. I leave 2) as it on the theory it is the most conservative approach.

Maybe someone else can test this too and then we can mark it RTBC.

Test failing are unrelated to this patch. RC6 will fail as well.

The tests, unchanged by this patch, create a user with permissions administer site configuration, administer permissions, and masquerade as user. It then tries to masquerade and fails because it can't (and then can't switch back).

It is is desired to fix these tests, the user should be created with the masquerade as any user permission. It would also be wise to test that it can't masquerade when it doesn't have this permission.

The attached patch is a straight re-roll of mrmikedewolf's fine (RTBC) patch, with the indentation change that I mentioned, plus extensively updated tests.

The previous tests were completely out-of-date. RC6 would not pass a single one. These tests are now:

• Create the admin user with sufficient privileges to execute the tests.
• Validate that the masquerade configuration page is actually reached.
• Validate that the masquerade block can be enabled.
• Validate that the administrator can masquerade using the user profile page.
• Validate that switch back works.
• Validate that the Masquerade block can switch using the text field.

The tests did not work because a) not enough privileges (even for RC6) b) text messages for successful masquerade and switch back changed, and c) masquerade links now have tokens.

I am leaving this RTBC, but it would be wise to ensure that testbot likes these tests. They ran successfully on my development machine.

Thank you for your work on this module. I find it indispensable.

@David_Rothstein - Definitely on the 'restrict access'. I would also set that for 'masquerade as admin'. If you can masquerade as admin, you can set anything.

@deekayen - I suggest you add the same 'restrict access' to 'masquerade as admin' for both safety and consistency.