Session of (UI) test runner leaks into web tests

Let's try a different approach: Prevent that session data is leaked into the UI test runner by closing (saving) the session in TestBase::prepareEnvironment() and then setting the session-name (i.e. the name of the session cookie) to a random value. If a test starts a new session, then it will be associated with a different session-name. After the test-run (TestBase::restoreEnvironment() it can be safely destroyed.

Another benefit of not using SessionManager::disable() in the test-runner is that we can implement the temporary session write-protection in a better way. I would like to remove that responsibility from the session-manager and instead implement it directly in the session-handler (or even better, a session-handler proxy).

But when doing so, we'd only "officially accept/endorse" that the session does leak into tests... which doesn't really resolve the stated problem of this issue :-/

All that the patch is supposed to do is to ensure that test code (DUTB as well as the parent site when running WTB) has no way to clobber a) in-memory session data ($_SESSION) and b) on-disk session data of the execution environment.

It is indeed unfortunate that the WTB child site also reuses the session-name of the execution environment, but that is a different problem. We can solve that by making the session-name configurable from within settings.php. Given that this will involve changing non-test code, I'd prefer to not extend the scope of this issue here.

All of the original* properties are supposed to be private and not supposed to be available/reachable within tests. [...] I don't know why the cURL code takes over the session name of the test runner by default.

Fixed. In order to keep track of changes to the session, the WTB parent site needs to know the session name of the child site.

Also I found that we need to make sure that a session cookie set during test-execution is deleted when the session is destroyed.

8: 2239969-fix-session-leak-8.diff queued for re-testing.

What are the symptoms of this bug? I have a feeling this may be related to session issues I have been having since #1289536: Switch Watchdog to a PSR-3 logging framework was committed. That one causes SQL errors in the installer and maybe LogicException: Cannot change the ID of an active session in Symfony\Component\HttpFoundation\Session\Storage\Proxy\AbstractProxy->setId() as well.

@@ -1107,7 +1123,6 @@ protected function rebuildContainer($environment = 'prod') {
     else {
       $this->container->get('request_stack')->push($request);
     }
-    $this->container->get('current_user')->setAccount(\Drupal::currentUser());
 
     // The request context is normally set by the router_listener from within
     // its KernelEvents::REQUEST listener. In the simpletest parent site this

This fixes the LogicException: Cannot change the ID of an active session in most if not all cases. Setting the account on the current_user service is in fact a noop here (because $this->container is set to Drupal::getContainer() a couple of lines before). However since #2238087: Rebase SessionManager onto Symfony NativeSessionStorage it may cause side-effects.

Other than that, this patch only affects testing through the UI (command line testing is not affected).

8: 2239969-fix-session-leak-8.diff queued for re-testing.

I haven't dug into this a lot, but removing that line from WebTestBase does not prevent the errors in my particular case.

Back to RTBC, test failures from #10 and #12 were bogus.

Nice - this fixes a bug in KernelTestBase tests (eg TextPlainUnitTest) that could not be run through the UI.

Committed 5a3ef30 and pushed to 8.x. Thanks!

This causes:

Drupal core - 8.x fail: https://qa.drupal.org/8.x-status

Overall Summary
==============================================

FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,771 pass(es), 1 fail(s), and 0 exception(s).

Individual Environment Summaries
==============================================

-- [[SimpleTest]]: [PHP 5.4 MySQL] --

* Drupal\simpletest\Tests\InstallationProfileModuleTestsTest (27 pass(es), 1 fail(s), and 0 exception(s))
   - [fail] [Other] ""SystemListingCompatibleTest test executed." found" in InstallationProfileModuleTestsTest.php on line 63 of Drupal\simpletest\Tests\InstallationProfileModuleTestsTest->testInstallationProfileTests().

Reverting it resolved the failure for me locally.

Reverted 5a3ef30 for now.

8: 2239969-fix-session-leak-8.diff queued for re-testing.

And actually whilst it fixes KernelTestBase tests being run from the UI, in fact all WebTestBase tests appear to by broken with this patch applied.

Oh, fun. Seems like this is in fact incompatible with #1289536: Switch Watchdog to a PSR-3 logging framework. #8 only green turned green again because the watchdog patch was reverted intermittently.

Symfony\Component\HttpFoundation\Session\Storage\Proxy\AbstractProxy->setId('oDa6BpniF7lKzhBL2R58P22TaIf_bt8sTy8enMZhXBk')
Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage->setId('oDa6BpniF7lKzhBL2R58P22TaIf_bt8sTy8enMZhXBk')
Drupal\Core\Session\SessionManager->initialize()
Drupal\Core\Authentication\Provider\Cookie->authenticate(Object)
Drupal\Core\Authentication\AuthenticationManager->authenticate(Object)
Drupal\Core\Session\AccountProxy->getAccount()
Drupal\Core\Session\AccountProxy->id()
Drupal\Core\Logger\LoggerChannel->log(6, '%module module installed.', Array)
watchdog('system', '%module module installed.', Array, 6)
Drupal\Core\Extension\ModuleHandler->install(Array, 1)
Drupal\simpletest\WebTestBase->setUp()
Drupal\simpletest\Tests\InstallationProfileModuleTestsTest->setUp()
Drupal\simpletest\TestBase->run()
_simpletest_batch_operation(Array, '3', Array)
call_user_func_array('_simpletest_batch_operation', Array)
_batch_process()
_batch_do()
_batch_page(Object)
Drupal\system\Controller\BatchController->batchPage(Object)
call_user_func_array(Array, Array)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1)
Drupal\Core\HttpKernel->handle(Object, 1, 1)
Drupal\Core\DrupalKernel->handle(Object)
drupal_handle_request()

I see two options on how this could be fixed.

1. Easy: Fix SessionManager::initialize() such that it can be called multiple times, i.e. when initializing a lazy session for anonymous users, do not attempt to set the session-id if it already exists.
2. Align the session with the current user service, i.e. do not persist the session_manager service. This means we'd need to properly close/write the session before constructing a new container and restart it afterwards (if appropriate), similar to how the request is handled in DrupalKernel::initializeContainer()

Option 1 would be easy to implement, but I fear we'd just hide the problem. Option 2 could be tricky but it would also help with #2256257: Move token seed in SessionManager in isSessionObsolete() and regenerate() to the MetadataBag I think.

And actually whilst it fixes KernelTestBase tests being run from the UI, in fact all WebTestBase tests appear to by broken with this patch applied.

Uh. How on earth do we not have test coverage for that?

So this is yet another example of why persisted services are a bad idea. +1 for option 2

I've opened #2272879: Can not run a WebTestBase or KernelTest base tests from inside a simpletest to test running tests through the UI

I think we should postpone this on #2272879: Can not run a WebTestBase or KernelTest base tests from inside a simpletest as that contains a commented out test for KernelTestBase and an actual test and fix for WebTestBase - so we can commit this patch knowing we have not broken anything and proving that we're fixing stuff :)

Filed #2272987: Do not persist session manager

re #32 I'm not sure about that since the errors are caused by not having a clear separation of sessions between the test runner and the test.

Just a quick note: with #2272987-9: Do not persist session manager and #19 reapplied, both of the following tests pass on the command line as well as in the UI test-runner:

• Formatter\TextPlainUnitTest
• InstallationProfileModuleTestsTest

We should be testing our UI test runner better... see #2272879: Can not run a WebTestBase or KernelTest base tests from inside a simpletest

Simple reroll. Should fail the same way as #8.

The failing test creates a site with the testing profile. It then logs in and submits the web test runner with a test provide by a module in that profile. The error I got was in the test results of the test:

The test did not complete due to a fatal error.

LogicException: Cannot change the ID of an active session in Symfony\Component\HttpFoundation\Session\Storage\Proxy\AbstractProxy->setId() (line 123 of /.../d8/core/vendor/symfony/http-foundation/Symfony/Component/HttpFoundation/Session/Storage/Proxy/AbstractProxy.php).
Symfony\Component\HttpFoundation\Session\Storage\Proxy\AbstractProxy->setId('iYbw8hs_k-nhcQSTpoVOrjRDUJzBtw43RAXO6In8rU8')
Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage->setId('iYbw8hs_k-nhcQSTpoVOrjRDUJzBtw43RAXO6In8rU8')
Drupal\Core\Session\SessionManager->initialize()
Drupal\Core\Authentication\Provider\Cookie->authenticate(Object)
Drupal\Core\Authentication\AuthenticationManager->authenticate(Object)
Drupal\Core\Session\AccountProxy->getAccount()
Drupal\Core\Session\AccountProxy->id()
Drupal\Core\Logger\LoggerChannel->log(6, '%module module installed.', Array)
watchdog('system', '%module module installed.', Array, 6)
Drupal\Core\Extension\ModuleHandler->install(Array, 1)
Drupal\simpletest\WebTestBase->setUp()
Drupal\simpletest\TestBase->run()
_simpletest_batch_operation(Array, '1', Array)
call_user_func_array('_simpletest_batch_operation', Array)
_batch_process(Array)
_batch_progress_page()
_batch_page(Object)
Drupal\system\Controller\BatchController->batchPage(Object)
call_user_func_array(Array, Array)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1)
Drupal\Core\HttpKernel->handle(Object, 1, 1)
Drupal\Core\DrupalKernel->handle(Object)

The executed test doesn't do anything strange. just install a minimal profile and a single test module and then execute:

$this->pass(__CLASS__ . ' test executed.');

Its all a pretty strange thing to be the only failure.

Wow, stepped through this and it is nasty. Sort of in the scope of what we're trying to fix though.

So the big problem is how we run through the installer which is working hard on the session and expecting multiple request from inside the setUp method, all in one request.

First, our session initialize() method gets called repeatedly through watchdog calls in the installer. This turns out is fine because the session is shut down.

Then we hit this code in the instaler:

  $account = user_load(1);
  user_login_finalize($account);

This logs the admin user in, or at least it should. It also calls SessionStorageInterface::regenerate() which actually triggers the session to be started. This is also where the first actual weird thing happens because the actual session handler registered with php has a reference to the parent site request stack which doesn't see simpletest session name cookie so when we hit SessionHandler::read() actually logs out the user back out.

Now, we hit initialize() from another watchdog request. This time from webtest installing extra modules (not that it really matters where) and we have a logged out user so we skip to the lazy anon block where we indescriminately call setId() and boom, and exception is thrown because the session is already started.

So the easy fix is to close the session after the installer runs. It seems like we should probably push a request on to the stack and maybe log uid 1 out.

Take a look test bot. see how we're doing.

because the actual session handler registered with php has a reference to the parent site request stack

This is exactly the problem with persist-services. #2272987: Do not persist session manager will fix that problem, see #35.

I guess it isn't clear but I'm actually okay rtbc'ing this.

Reuploading #37 after #2272987: Do not persist session manager went in.

Turns out that I forgot one hunk when rerolling #2272987: Do not persist session manager after the bootstrap-patch landed. Let's see what happens when it is restored, will #45 afterwards.

One note though:

@@ -907,11 +907,33 @@ protected function setUp() {
...
     $this->kernel = DrupalKernel::createFromRequest($request, drupal_classloader(), 'prod', TRUE);
+    $this->kernel->prepareLegacyRequest($request);
     // Force the container to be built from scratch instead of loaded from the
     // disk. This forces us to not accidently load the parent site.
     $container = $this->kernel->rebuildContainer();
-    $this->kernel->prepareLegacyRequest($request);
...

I guess the intention of rebuilding the kernel before preparing it was to avoid building the container repeatedly. However, the result was that the container still was compiled two times (once by prepareLegacyRequest() -> boot() -> initializeContainer() and once by rebuildContainer()). Additionally that sequence of calls somehow did lead to the session-manager having injected an empty request-stack service. Swapping the lines results in a) the container being compiled only once and b) the proper request-stack service injected into the session-manager.

I think it forced the rebuild first to ensure we didn't load the parent site container at all. We may have fixed that since then through other changes.

Addresses #45. Did not include 2 because that would stretch the line over 80 chars, also I like the explicitness of recording the old value vs. assigning the new one. Regarding 4..5, reworded the comment a little bit.

This patch essentially introduces throw-away sessions during execution of test-methods. That makes it impossible to access the proper session name (which is required for SessionTest and SessionHttpsTest) via session_name() from within a test-method.

I think we can simplify the code in WebTestBase to just close the session rather than restarting it.

Maybe we can simplify even more - the only reason we have a session to close is the user login in install_finished() which only really makes sense for interactive installs.

I started to comment on this yesterday but it seems to have gotten lost.

The only thing it _might_ affect is drush but clearly not significantly because Testbot ran fine.

Lets move the UA stuff into another issue. I have strong feelings about adding testing code to the kernel and I don't want to block this.

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -816,6 +826,14 @@ protected function setUp() {
+    // @todo: Introduce a setting such that the session name can be customized
+    // for the child site.

Is this really needed? Otherwise looks rtbc.

Ok let's remove the @todo - it't not properly formatted and really should have an existing issue.

Skimmed again and still looks good.

Looks good to me. Committed to 8.x.