Implement caching for the masquerade block and links with a 'masquerade' cache context

So now that rendered menus/menu blocks are finally correctly cached, this is no longer blocked.

Right now, it is always caching per user: http://cgit.drupalcode.org/masquerade/tree/src/Access/SwitchAccessCheck.... — but that is not correct. Because it actually is happening on a per-permissions basis. And yes, even the UID==1 edge case is wrong/unnecessary, because Drupal already takes care of that for you: it grants user 1 *all* permissions. See https://api.drupal.org/api/drupal/core%21modules%21user%21src%21Entity%2....

So: SwitchAccessCheck needs to be updated to fix these bugs. It will be simpler in the end :)

Finally, to answer the key question here: how to implement a cache context for Masquerade?. Usually the answer is trivial. But here it is not, because what Masquerade does, affects the entire response, and even affects other cache contexts.

If you haven't yet, read https://www.drupal.org/developing/api/8/cache/contexts.

So, let's analyze it at a UI level:

1. You want to be able to show a block or some other UI element that says that you're currently masquerading. So you want a is_masquerading cache context for that purpose, which just checks for the presence of that $_SESSION['some key'] array value.
2. For the Start masquerading menu link(s): it depends solely on the current user's permissions (whether masquerading or not, i.e. when masquerading the masqueraded user is the current user and hence that user's permissions matter) whether they see that link or not. (I'm assuming Masquerade protects against recursive masquerading — if I'm UID 1 and I masquerade as user 3, and then as user 3 I masquerade as user 5, then if I stop masquerading, I should just go back to UID 1, not 3 and then 1 — unless you're actually tracking it as a stack. But that'd get very complicated.)
3. For the Stop masquerading menu link(s), its access check is very simple: it depends solely on whether masquerading is currently happening, and if so, access is always granted. Currently, that sets max-age = 0, but once there is a is_masquerading cache context, the max-age = 0 can be removed in favor of varying by that cache context.

That way, everything is perfectly cacheable, and simple & clear to understand :)

Is there still a question for me here or not?

Here you go :)

1. +++ b/masquerade.services.yml
   @@ -14,3 +14,8 @@ services:
   +  cache_context.is_masquerading:
   

   This should be renamed to cache_context.session.is_masquerading, because it's stored in the session, and thus if something already varies by session, it already varies by whether it's masquerading or not.

   See https://www.drupal.org/developing/api/8/cache/contexts#optimizing

2. +++ b/src/Access/UnmasqueradeAccessCheck.php
   @@ -40,7 +40,7 @@ class UnmasqueradeAccessCheck implements AccessInterface {
   +      ->addCacheContexts(['is_masquerading']);
   

   Then this would become 'session.is_masquerading'.

3. +++ b/src/Plugin/Block/MasqueradeBlock.php
   @@ -34,6 +36,13 @@ class MasqueradeBlock extends BlockBase implements ContainerFactoryPluginInterfa
      /**
   +   * The permission handler.
   +   *
   +   * @var \Drupal\user\PermissionHandlerInterface
   +   */
   +  protected $permissionHandler;
   

   Oh, hah, I not even know this service existed!

4. +++ b/src/Plugin/Block/MasqueradeBlock.php
   @@ -78,16 +91,25 @@ class MasqueradeBlock extends BlockBase implements ContainerFactoryPluginInterfa
   +    if ($this->masquerade->isMasquerading()) {
   +      return AccessResult::forbidden()->addCacheContexts(['is_masquerading']);
   +    }
   +    $permissions = [];
   +    foreach ($this->permissionHandler->getPermissions() as $name => $permission) {
   +      if ($permission['provider'] === 'masquerade') {
   +        // Filter only module's permissions.
   +        $permissions[] = $name;
   +      }
   +    }
   +    // Display block for all users that has any of masquerade permissions.
   +    return AccessResult::allowedIfHasPermissions($account, $permissions, 'OR');
   

   So, if the user is masquerading, we DO NOT want to show this block.

   Which means the access result depends on session.is_masquerading.

   It is handled correctly for the case where the user is masquerading. But it's not handled correctly in the other case. In the other case, we only vary by whether the user has permission. But we need that to also vary by session.is_masquerading! Because the return value of this method always depends on session.is_masquerading.

   So, you want to add addCacheContexts(['session.is_masquerading']) to that last line also.

Looks good :) Test coverage should prove that the earlier patch has fails, and the latest patch does not.

I started writing tests for this, but applying the most recent patch against Masquerade-8.x-2.x-dev did not yield the expected result. Steps to reproduce:

1. Enable Dynamic Page Caching
2. Create two accounts, each in the authenticated role, and give that role "masquerade as authenticated" permission
3. Log in as user A and visit the homepage: the switch block is shown, as expected
4. Masquerade as user B and visit the homepage: the switch block is shown, while it should not be (since the block should be hidden while masquerading)

This is one reproducible scenario. While playing around, it frequently occurred that the block was unexpectedly shown or hidden.

I tracked this down to MasqueradeCacheContext::getContext(), which has 2 issues:

1. (minor) The return value is cast to string along the way, and casting FALSE to string yields an empty string, so cids contained "[session.is_masquerading]=" as part of the key (note the missing value;
2. Masquerade is using $_SESSION['masquerading'] everywhere, except in the MasqueradeCacheContext::getContext(), where it uses \Symfony\Component\HttpFoundation\Session\Session::has(). The latter is not a wrapper around $_SESSION, and thus always returns FALSE, even when masquerading.

2 patches attached: one is the unaltered patch from #13, but with a specific test for this behaviour, the second patch fixes the session context return value (in a follow-up issue, Masquerade should probably switch from using $_SESSION to using the Symfony Session.