Negate role permissions

Providing a patch that allows masquerading access if a user has access to all the target account roles.

Unrelated test fails are getting fixed in #2448707: Fix masquerade tests

+++ b/masquerade.module
@@ -136,19 +136,13 @@ function masquerade_masquerade_access($user, UserInterface $target_account) {
+  $target_roles_access = [];
   foreach ($target_account_roles as $role_id) {
-    if ($user->hasPermission('masquerade as ' . $role_id)) {
-      if ($role_id === UserInterface::AUTHENTICATED_ROLE) {
-        // Make sure that this is the only role user has to prevent collision
-        // with 'masquerade as any user' permission.
-        if (count($target_account_roles) == 1) {
-          return TRUE;
-        }
-      }
-      else {
-        return TRUE;
-      }
-    }
+    $target_roles_access[$role_id] = $user->hasPermission("masquerade as $role_id") ? TRUE : FALSE;
+  }
+  // Only allow masquerade if a user has access to all the target account roles.
+  if (!in_array(FALSE, $target_roles_access, TRUE)) {
+    return TRUE;
   }

It might be easier to do a return FALSE in case one permission is missing?

Also, I'm wondering what "masquerade as authenticated" then exactly means in this context.

Probably allow to masquerade as user that does not have any other permission. But do we require that permission explicitly if a user has roles, do you always need authenticated and the other? seems a bit weird? Maybe we could special case that, if a user only has that role, we specifically check for that permission, else the loop with return false if !hasPermission()?

It might be easier to do a return FALSE in case one permission is missing?

I thought the same until I saw the doc block:

/**
 * Implements hook_masquerade_access().
 *
 * This default implementation only returns TRUE and never FALSE, since
 * alternative access implementations could not work otherwise.
 */

Also, I'm wondering what "masquerade as authenticated" then exactly means in this context.

Probably allow to masquerade as user that does not have any other permission.

Yeah, it allows masquerading as normal users, users who have no roles (authenticated role by default).

well, then we can simply return NULL instead of FALSE, although I find that a bit strange :)

also, if it's apparently using the true/false/null pattern then it should be using AccessResult but that's a too big change for this issue obviously.

^ Addressing above. Doing an early NULL return in case of a missing permission.

Hiding duplicated uploads.

Ok, that doesn't include a special case for authenticed permission yet, but lets see what @andypost thinks if and how we need that.

I think in almost all cases, you will need to select "masquerade as authenticated" permission anyway because you want to authenticate as users without additional roles as well. Means you can't configure to masquerade only users with a specific role and *not* those that are just authenticated. There might be some use cases around that..

Users with a specific role are technically specific role + authenticated role users (at least by the default implementation of Drupal\user\Entity\User::getRoles()), which is a bit misleading in UI as "authenticated" role is not displayed...

1. kind of, but it's a bit different than before. the question is wheter we want to allow masquerade to *only* a specific role but not authenticed users without a role. I guess so. that's also what the 7.x patch does I think. lets extend that tests that a user has masquerade as some permission but not authenticated can authenticate as a user with that permission but not as one that does not have any permissions other than authenticated.

2. & 3. Nope, as we discussed, role weight is pretty much meaningless, it's not possible to rely on it. that's how it has to work to be secure. However, we possibly need to check documentation and explain that a user must have permission for all the roles that another user has.

Rebasing against latest dev version.

I'd suggest to change:

'Users with the Masquerade as any user permission are able to masquerade as someone else. Users may only masquerade if they have the same or more permissions as the user they want to switch to. Users are never able to escalate their privileges by masquerading as someone else.'

to:

'A user may only masquerade as another user if he has the Masquerade as any user permission or if he has all the Masquerade as @role permissions for all the target account roles.

A user may only masquerade as another user if they have the "Masquerade as any user" permission or if they have all the "Masquerade as ROLE" permissions for all the target account's roles.

I think the suggestion makes it more accurate. I just de-gendered the sentence and removed developery "@role" to try and make it a bit more site-builder accessible.

Updated the help text with the latest suggestion above from @frob. I slightly adapted it however, since "A user" is he/she and afterwards you used "they". Here is the patch with the updated help text.

This is a very critical issue. Thank you for maintainers & supporters addressing this.

Any update on when this will be rolled out with module update?

Best,
Senthil Kumar Kumaran

We do have to make it a bit inconsistent with existing code to avoid these, defined the properties separtely and made them camelCase.

+++ b/tests/src/Functional/MasqueradeWebTestBase.php
@@ -37,14 +37,35 @@ abstract class MasqueradeWebTestBase extends BrowserTestBase {
+   * Supoer User.

Uh oh, we missed a typo here. :)

I think it'd make sense to add a note/change record about this to the Release Notes page for beta3 - I had a client contact me after masquerade stopped working (although in Permissions I'd selected the roles they needed to masquerade as, such as one called 'Applicant' which most accounts on the site have, I hadn't ticked "Masquerade as Authenticated User"). And I hadn't picked it up myself as I'd only been testing as user 1.

I suspect quite a few people could have ticked custom roles but NOT "as Authenticated User".

wturrell in #33:

I suspect quite a few people could have ticked custom roles but NOT "as Authenticated User".

Yes, we had the exact same problem on two sites: masquerade stopped working because we had not checked "Masquerade as Authenticated User" (which worked with beta2).

And since this issue is 7.x-1.x-dev (#29 changed the version for backport) it is harder to find if looking for 8.x issues.

So we agree with the idea of adding an explicit note to the 8.x-2.0-beta3 release notes (something more than just linking "#2688499 by mbovan, Berdir, sasanikolic, yongt9412: Negate role permissions").

I found out that a masquerading user now really needs all the "Masquerade as ROLE" permissions for all the target account's roles.

With beta2 it was enough to have just one matching permission (besides "as Authenticated User").
This is also mentioned in the change of the help text.

So maybe something like:

If you are using "Masquerade as ROLE" permissions check if you have permissions for all the "Masquerade as ROLE" permissions for all the target account's roles (in beta2 it was enough to have just one matching permission (besides "as Authenticated User").

Maybe better:

If you are using "Masquerade as ROLE" permissions check if you have permissions for all the "Masquerade as ROLE" permissions for all the target account's roles including "as Authenticated User".
In beta2 it was enough to have just one matching permission (besides "as Authenticated User").