Wrong cached page is being served after re-opening the browser

Seems that needs some tests :)

1. +++ b/src/PageCache/RequestPolicy/PendingPersistentLogin.php
   @@ -0,0 +1,60 @@
   + * a persistent login one. This should happen only when the user reopened the
   + * browser where he was logged with a persistent login session. In the first
   + * kernel request event, the cookie will be validated and the user will be
   

   Let's make this sentence gender neutral, not all visitors are male.

   Also "This should happen only" sounds quite strong, maybe just say "A common situation"?

2. +++ b/src/PageCache/RequestPolicy/PendingPersistentLogin.php
   @@ -0,0 +1,60 @@
   + * present again and this policy won't apply anymore. If the cookie is invalid,
   + * it will be removed during the response event, again making this policy not
   + * applicable on next page loads.
   

   It would sound better to say "..not applicable on _subsequent_ page loads."

3. +++ b/src/PageCache/RequestPolicy/PendingPersistentLogin.php
   @@ -0,0 +1,60 @@
   +  /**
   +   * Instantiate a new PendingPersistentLogin object.
   +   *
   

   Use third person verb.

4. +++ b/src/PageCache/RequestPolicy/PendingPersistentLogin.php
   @@ -0,0 +1,60 @@
   +   * @param \Drupal\persistent_login\TokenHelper $token_helper
   +   *   The token helper service.
   

   When injecting services, it's good practice to provide an interface instead of a concrete implementation.

5. +++ b/src/PageCache/RequestPolicy/PendingPersistentLogin.php
   @@ -0,0 +1,60 @@
   +  public function check(Request $request) {
   +    if (!$this->sessionConfiguration->hasSession($request) && $this->tokenHelper->hasCookie($request)) {
   +      return static::DENY;
   +    }
   +  }
   

   This is the actual bugfix. Let's add some documentation here why we are returning DENY if our cookie is present but there is no session.

6. +++ b/src/TokenHelper.php
   @@ -0,0 +1,59 @@
   +/**
   + * Token helper service.
   + */
   +class TokenHelper {
   

   Good idea to put these helper methods in a separate service. Let's add an interface for it so the methods can be swapped out with a custom implementation when needed.

7. +++ b/src/TokenHelper.php
   @@ -0,0 +1,59 @@
   +  /**
   +   * Instantiate a new TokenHelper instance.
   +   *
   

   Use a third person verb to start the summary of a class, interface, or method: "Instantiates a new...".

8. +++ b/src/TokenHelper.php
   @@ -0,0 +1,59 @@
   +  /**
   +   * Get the name of the persistent login cookie.
   +   *
   

   Use third person verb.

9. +++ b/src/TokenHelper.php
   @@ -0,0 +1,59 @@
   +  /**
   +   * Check if a request contains a persistent login cookie.
   +   *
   

   Use third person verb.

Bumping this to major, since the main functionality of the module (staying logged in) is broken on websites that use the page cache, which I presume is the most common situation for sites that are in production.

Thank you @sardara for the detailed report and patch, and @pfrenssen for the review.

The name TokenHelper didn't indicate to me the need for a new class or its role before reading the code. Given its scope, I think CookieHelper may be more appropriate.

I generally agree with, but am less strict about having an interface for each service.

CookieHelper sounds like a really good name for the service.

Assigning to me, I'll address the remarks, and port the Behat test that is linked in the issue summary to BrowserTestBase. I very much like the way the Behat test is set up since it also covers the base functionality of the module.

Addressed the remarks. Going to start on the test coverage now.

Here is the test + some renames that I missed in the previous patch.

Full credit for this work should go to @sardara, I shamelessly ripped off the code + general approach from the Behat test posted in the issue summary.

@gapple, can you enable the test coverage in the Automated Tests tab on the project homepage so we can see the test results?

Awaiting automated testing to be enabled for the projects, here are the test results from a local test run. The first test run passes with the patch applied, and the second test run is done on the 8.x-1.x branch without the patch, and this test fails. This indicates that the patch fixes the problem correctly.

[pieter@thinkarch web]$ cd modules/contrib/persistent_login/
[pieter@thinkarch persistent_login (2853553)]$ ../../../../vendor/bin/phpunit -c ../../../phpunit.xml tests/
PHPUnit 4.8.35 by Sebastian Bergmann and contributors.

..

Time: 23.44 seconds, Memory: 4.00MB

OK (2 tests, 10 assertions)
[pieter@thinkarch persistent_login (2853553)]$ git checkout 8.x-1.x 
Switched to branch '8.x-1.x'
Your branch is up-to-date with 'origin/8.x-1.x'.
[pieter@thinkarch persistent_login (8.x-1.x=)]$ git apply --index /tmp/2853553-10-test_only.patch
[pieter@thinkarch persistent_login (8.x-1.x +=)]$ ../../../../vendor/bin/phpunit -c ../../../phpunit.xml tests/
PHPUnit 4.8.35 by Sebastian Bergmann and contributors.

.F

Time: 23.14 seconds, Memory: 4.00MB

There was 1 failure:

1) Drupal\Tests\persistent_login\Functional\PersistentLoginTest::testPersistentLogin with data set #1 (true, 0, SplObjectStorage Object ())
Failed asserting that false matches expected true.

/home/pieter/v/joinup/web/core/tests/Drupal/Tests/BrowserTestBase.php:1343
/home/pieter/v/joinup/web/modules/contrib/persistent_login/tests/src/Functional/PersistentLoginTest.php:71

FAILURES!
Tests: 2, Assertions: 10, Failures: 1.

Thanks @sardara, the cookie life time is critical for the functioning of the module. This is very useful to have added this to the test setup.

1. +++ b/tests/src/Functional/PersistentLoginTest.php
   @@ -132,11 +140,13 @@
   +        'The user should not be logged in after starting a new session.'
   

   Missing comma at the end.

2. +++ b/tests/src/Functional/PersistentLoginTest.php
   @@ -132,11 +140,13 @@
   +        'The user should be logged in after starting a new session.'
   

   Missing comma at the end.

Thanks! Looks good now.

I have written the test in this patch, but this has been approved by @sardara in comment #13. I have reviewed the original fix. So seeing that all work has been reviewed properly I think it's fine to RTBC this :)

There was a related security report submitted.

If after a cache rebuild a user returns to the site with a persistent login token, the response is cached including their updated persistent login cookie. The next visitor to that url will also receive the cookie and may be authenticated as another user.

Since this patch turns off page cache behaviour for the entire request, it also excludes page cache from storing the response, resolving the issue. The only other change I've introduced is to mark the response as private if the persistent login cookie is set, updated, or cleared to ensure that no other intermediary caches attempt to store the response.

Since the module is still in alpha and did not support the page cache prior to this patch there won't be a security announcement, but I will tag the next alpha release as containing a security fix.