Last access by a user should not change after masquerade as that user (D8)

Verified. It works!
Thanks

Reviewed and tested. It works!
Thanks

Ghanks for the patch, this addition require tests and steps to reproduce, also code needs a bot of cleanup

1. +++ b/src/MasqueradeServiceProvider.php
   @@ -0,0 +1,26 @@
   +  /**
   +   * Alter definitions.
   +   *
   +   * @inheritdoc
   +   */
   

   I would note here which definitions are altered and why.

   I'm not sure if using @inheritdoc and a describing text may be used together. I remember that I've read somewhere you should use one or the other.
   Anyway, I believe the @inheritdoc should always be defined within curly brackets:
   {@inheritdoc}

2. +++ b/src/EventSubscriber/MasqueradeUserRequestSubscriber.php
   @@ -0,0 +1,26 @@
   +  /**
   +   * On kernel terminate trigger parent in case session variable is not set.
   +   *
   +   * @inheritdoc
   +   */
   

   Same. Not sure if {@inheritdoc} should be used here if the method already has a description.

Since {@inheritdoc} is required, extra comment has been removed.

Working on adding a test for this.

Just added an assert for this, in MasqueradeTest.php.

Getting the service provider to actually register in the test proved challenging, that problem was caused by the new event subscriber not being declared in masquerade.services.yml (hint: checking core/modules/system/tests/modules/service_provider_test was very helpful); the full patch includes that. Also, using $account->getLastAccessedTime() was always returning 0, so I had to fetch the value directly from users_field_data.

Let's see if the testbot likes this...

Alright! the test works.

1. +++ b/src/EventSubscriber/MasqueradeUserRequestSubscriber.php
   @@ -0,0 +1,24 @@
   +    if (!isset($_SESSION['masquerading'])) {
   

   not sure session should be red from global, there's a request object with session inside passed via event object

   PS: https://github.com/symfony/symfony/issues/10557

2. +++ b/src/MasqueradeServiceProvider.php
   @@ -0,0 +1,24 @@
   +    $definition->setClass('Drupal\masquerade\EventSubscriber\MasqueradeUserRequestSubscriber');
   

   could use MasqueradeUserRequestSubscriber::class syntax to explicitly define usage

Thx for reviewing @andypost! here's the improved patch.

+++ b/src/MasqueradeServiceProvider.php
@@ -0,0 +1,25 @@
+  public function alter(ContainerBuilder $container) {
+    $definition = $container->getDefinition('user_last_access_subscriber');
+    $definition->setClass(MasqueradeUserRequestSubscriber::class);

Maybe allow to configure it?

Sure! would a simple checkbox do it? I'm thinking something like "Prevent changing last access" with description "Prevents modifying the last access timestamp for an account, when masquerading as that user".

As for implementation, I'm not sure it's possible to read config from the service provider alter() call; if that's the case, the next idea I have is to just go ahead with the override, but check for config in MasqueradeUserRequestSubscriber itself.

1. +++ b/src/EventSubscriber/MasqueradeUserRequestSubscriber.php
   @@ -0,0 +1,26 @@
   +    if (!isset($_SESSION['masquerading'])) {
   

   should use service

2. +++ b/src/MasqueradeServiceProvider.php
   @@ -0,0 +1,26 @@
   +    $definition->setClass('Drupal\masquerade\EventSubscriber\MasqueradeUserRequestSubscriber');
   

   should be configurable and use `MasqueradeUserRequestSubscriber::class` syntax

@andypost: #14 already fixes #17.1 and #17.2.b. As for #17.2.a, can you please have a look at #16? I just want to be sure my approach is sound before I put more work into it :).

Thanks!

Hey! The example of it is core"s configurable language manager so it's doable at least)

On other hand I prefer to keep default behaviour and get opinions from distributions and GDPR POV(

Tl'dr opt-in vs opt-out

I suppose the GDPR would be a wider issue if that was a concern? Eg, an opt-in on a user profile / registration with something like "Allow administrators to masquerade as me in order to support me", but that would be out of scope of this issue.

I personally cannot think of a case where a site builder would have an issue where masquerading no longer changes the last accessed timestamp on their users.

Still no UI and GDPR could be a reason to change port update hook, any way needs review

+++ b/masquerade.post_update.php
@@ -0,0 +1,16 @@
+    ->set('update_user_last_access', TRUE)

this affects upgrade path, probably safer to keep previous behavior for already installed sites

+++ b/src/EventSubscriber/MasqueradeUserRequestSubscriber.php
@@ -0,0 +1,48 @@
+    if (!$session->has('masquerading') || $force) {

Just debuged this:
- session reports that it's not started! (that's why this check in isMasquerading() method)
- this call to has() throws warning for every page because of previous

RuntimeException: Failed to start the session because headers have already been sent by "/var/www/html/web/core/modules/big_pipe/src/Render/BigPipe.php" at line 264. in Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage->start() (line 145 of /var/www/html/web/vendor/symfony/http-foundation/Session/Storage/NativeSessionStorage.php)

#0 /var/www/html/web/core/lib/Drupal/Core/Session/SessionManager.php(164): Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage->start()
#1 /var/www/html/web/core/lib/Drupal/Core/Session/SessionManager.php(118): Drupal\Core\Session\SessionManager->startNow()
#2 /var/www/html/web/vendor/symfony/http-foundation/Session/Storage/NativeSessionStorage.php(299): Drupal\Core\Session\SessionManager->start()
#3 /var/www/html/web/vendor/symfony/http-foundation/Session/Session.php(256): Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage->getBag('attributes')
#4 /var/www/html/web/vendor/symfony/http-foundation/Session/Session.php(280): Symfony\Component\HttpFoundation\Session\Session->getBag('attributes')
#5 /var/www/html/web/vendor/symfony/http-foundation/Session/Session.php(65): Symfony\Component\HttpFoundation\Session\Session->getAttributeBag()
#6 /var/www/html/web/modules/masquerade/src/EventSubscriber/MasqueradeUserRequestSubscriber.php(43): Symfony\Component\HttpFoundation\Session\Session->has('masquerading')
#7 [internal function]: Drupal\masquerade\EventSubscriber\MasqueradeUserRequestSubscriber->onKernelTerminate(Object(Symfony\Component\HttpKernel\Event\PostResponseEvent), 'kernel.terminat...', Object(Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher))
#8 /var/www/html/web/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php(111): call_user_func(Array, Object(Symfony\Component\HttpKernel\Event\PostResponseEvent), 'kernel.terminat...', Object(Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher))
#9 /var/www/html/web/vendor/symfony/http-kernel/HttpKernel.php(88): Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.terminat...', Object(Symfony\Component\HttpKernel\Event\PostResponseEvent))
#10 /var/www/html/web/vendor/stack/builder/src/Stack/StackedHttpKernel.php(32): Symfony\Component\HttpKernel\HttpKernel->terminate(Object(Symfony\Component\HttpFoundation\Request), Object(Drupal\big_pipe\Render\BigPipeResponse))
#11 /var/www/html/web/core/lib/Drupal/Core/DrupalKernel.php(672): Stack\StackedHttpKernel->terminate(Object(Symfony\Component\HttpFoundation\Request), Object(Drupal\big_pipe\Render\BigPipeResponse))
#12 /var/www/html/web/index.php(22): Drupal\Core\DrupalKernel->terminate(Object(Symfony\Component\HttpFoundation\Request), Object(Drupal\big_pipe\Render\BigPipeResponse))
#13 {main}

Looks this way it works fine, needs test for decorators

I gonna add this to next beta #2982514: Masquerade 8.x-2.1 stable release plan

Added basic kernel test for service ovverrides and a bit of cleanup
The key should remain "masquerading" to stay compatible with previous sessions

Fix typo

Use to load user entity to be sure it is not cached in test runner

Added change record https://www.drupal.org/node/3108941 (would be great to update it with details about decorations)

Fix CS issue in last patch

updated summary

This would be a very good fix. Few remarks:

1. +++ b/masquerade.services.yml
   @@ -20,3 +20,15 @@ services:
   +      - [setMasquerade, ['@masquerade', '@config.factory']]
   

   setMasquerade() sounds like we set only the @masquerade service. Either create another setConfigFactory() or rename to setServices() (or injectServices()).

2. +++ b/src/EventSubscriber/MasqueradeUserRequestSubscriber.php
   @@ -0,0 +1,57 @@
   +    $force = (bool) $this->configFactory
   ...
   +    if (!$this->masquerade->isMasquerading() || $force) {
   

   Why casting to (bool), shouldn't that work out-of-the-box with our config system?

   Also, if !$this->masquerade->isMasquerading() is TRUE we don't need to compute $force anymore. So, probably, something like:

   if ($this->masquerade->isMasquerading()) {
     $force = $this->configFactory
       ->get('masquerade.settings')
       ->get('update_user_last_access');
     if (!$force) {
       return;
     }
   }
   
   parent::onKernelTerminate($event);
   

   ...would be better.

   I would also rename $force to $update_user_last_access to be consistent with the config key.

3. +++ b/src/Session/MetadataBag.php
   @@ -0,0 +1,49 @@
   +class MetadataBag extends CoreMetadataBag {
   

   I'm not sure I get why we need this decorator. Could you, please, create a regression test for #23 so we can prove how this decorator is solving the problem?

4. +++ b/tests/src/Functional/MasqueradeTest.php
   @@ -43,6 +45,13 @@ class MasqueradeTest extends MasqueradeWebTestBase {
   +    $auth_user = \Drupal::entityTypeManager()
   +      ->getStorage('user')
   +      ->loadUnchanged($this->auth_user->id());
   +    $this->assertEquals($original_last_access, $auth_user->getLastAccessedTime(), 'Last access timestamp for impersonated user was not changed.');
   

   This tests the default setting (FALSE). Let's extend and test also with update_user_last_access: true

5. +++ b/tests/src/Kernel/ServiceDecoratorsTest.php
   @@ -0,0 +1,58 @@
   +  public static $modules = [
   

   s/public/protected

6. +++ b/tests/src/Kernel/ServiceDecoratorsTest.php
   @@ -0,0 +1,58 @@
   +    $this->assertTrue(method_exists($bag, 'setMasquerade'));
   +    $this->assertTrue(method_exists($bag, 'getMasquerade'));
   +    $this->assertTrue(method_exists($bag, 'clearMasquerade'));
   

   Could we replace the 3 lines with: $this->assertInstanceOf()?

7. +++ b/tests/src/Kernel/ServiceDecoratorsTest.php
   @@ -0,0 +1,58 @@
   +    $this->assertIdentical($bag->getMasquerade(), $uid);
   

   s/assertIdentical/assertSame

8. +++ b/tests/src/Kernel/ServiceDecoratorsTest.php
   @@ -0,0 +1,58 @@
   +    $this->assertTrue(method_exists($service, 'setMasquerade'));
   

   Same, I think assertInstanceOf() is more appropriate.

The core issue may help to move override flag

+++ b/src/Session/MetadataBag.php
@@ -0,0 +1,49 @@
+class MetadataBag extends CoreMetadataBag {
...
+  const MASQUERADE = 'masquerading';

needs polishing after https://www.drupal.org/node/3109877 and may require 8.9 core next release

Rerolled patch in #28 against current HEAD of 8.x-2.x.

This intentionally has not addressed the feedback in #30 and #32 at this point.

Setting to Needs Review anyway to kick off tests.

Running drush cr on Drupal 9.1.5 after installing the patch in #33 gives the following warning:

[warning] Declaration of Drupal\masquerade\EventSubscriber\MasqueradeUserRequestSubscriber::onKernelTerminate(Symfony\Component\HttpKernel\Event\PostResponseEvent $event) should be compatible with Drupal\user\EventSubscriber\UserRequestSubscriber::onKernelTerminate(Symfony\Component\HttpKernel\Event\TerminateEvent $event) MasqueradeUserRequestSubscriber.php:16

This also causes the test failures in https://www.drupal.org/pift-ci-job/2000260

The patch in #28 adds a class MasqueradeUserRequestSubscriber which extends Drupal\user\EventSubscriber\UserRequestSubscriber

This class is not covered by Drupal's BC policy: https://www.drupal.org/about/core/policies/core-change-policies/drupal-8...

In Drupal 9, the method signature for UserRequestSubscriber::onKernelTerminate() has changed, replacing the PostResponseEvent with a KernelEvent, in line with the former being deprecated since Symfony 4.3.

So as it stands I'm not sure how to make this patch compatible with both Drupal 8 and Drupal 9 without some kind of shim.

As we make use of the masquerade module in order to support users of our patform we would really appreaciate this functionality to be able to see if an user really did something on our platform on his own. I did not test the patch yet - before I dig in deeper I'd like to know if there are any plans for a new release of this module which may only be compatible with Drupal 9 to solve the problem mentioned above bei ELI-T?!

The patch in #36 just works fine for Drupal 9.4, after changing ‘true’ to ‘false’ in masquerade.settings.yml. I would not be too worried about breaking backwards compatibility with Drupal 8. And possible future BC breaks might happen, so that is something to test once in a while.

Agreed, now Drupal 8 is no longer supported, we no longer need a fix that works for 8 and 9.

RTBC +1

Patch #36 doesn't apply to the 8.x-2.x branch anymore. Here's an adjusted version without any functional changes.

Patch looks good after running db update on latest 10.0.9!

Adjusted patch for rc3

Aren't you mixing up $this->authUser and $this->auth_user in Functional/MasqueradeTest.php?
$this->auth_user is undefined in the first line of function testMasquerade().

Might be. I didn't care about the tests and just adjusted the existing patches here to by able to be applied against the latest versions.

Thanks for adjusting the patch for masquerade 8.x-2.0-rc3. Let us see what the attached patch does in the tests.

Well. 2 failing tests fixed. A bit better.

I took a quick look at the failing tests.

RTBC +1

@Jan-E you can set the issue status to RTBC.

The last patch erroneously modified the info.yml

Thank you! Pushed to -dev version as rc4 bugfix just rolled out

Missed one coding standards message but fine

tests/src/Kernel/ServiceDecoratorsTest.php ✗ 1 more
line 11 The class short comment should describe what the class does and not simply repeat the class name

For the record: #56 applies without problems to masquerade 8.x-2.0-rc4.

Should a new issue be opened for adding a configuration form for updating this setting? It's not a big need for me, as I can set the config value. But others might not be able to do that.

I think it needs some readme update as it will be confusing if masquerade will update last accessed

There needs to be at least an item in the README file and preferably a config form.