Skip to content
This repository was archived by the owner on Jan 29, 2020. It is now read-only.

Regenerate required to initiate a session #19

Closed
rtek opened this issue May 12, 2018 · 3 comments
Closed

Regenerate required to initiate a session #19

rtek opened this issue May 12, 2018 · 3 comments

Comments

@rtek
Copy link

rtek commented May 12, 2018

I am unable to get the session cookie to attach to the response from an initial client request without a regeneration. I would expect the first response to contain a Set-Cookie since there is not yet a session to regenerate.

   public function testCookiesNotSetWithoutRegenerate(): void
   {
        $persistence = new PhpSessionPersistence();
        $request = new ServerRequest();
        $session = $persistence->initializeSessionFromRequest($request);

        //remove to work around
        //$session = $session->regenerate();

        $response = new Response();
        $response = $persistence->persistSession($session, $response);

        $this->assertNotEmpty($response->getHeaderLine('Set-Cookie'));
   }
$ vendor/bin/phpunit test/BugTest.php
PHPUnit 7.0.2 by Sebastian Bergmann and contributors.

F                                                                   1 / 1 (100%)

Time: 93 ms, Memory: 4.00MB

There was 1 failure:

1) ZendTest\Expressive\Session\Ext\BugTest::testCookiesNotSetOnFreshRequest
Failed asserting that a string is not empty.

C:\server\root\oss\zend-expressive-session-ext\test\BugTest.php:33
@rtek
Copy link
Author

rtek commented May 12, 2018

Proposed fix resulted in lots of failed tests. Reviewing the tests:

  • If Session COOKIE is present, persistSession() method must return Response with Set-Cookie header
  • If Session COOKIE is not present, persistSession() method must return the original Response

These seem like they are reversed. Wouldn't a session cookie imply existing session, so no need to Set-Cookie unless there is a regeneration?

@geerteltink
Copy link
Member

If there isn't any data set, it doesn't need to generate a new session id or add the cookie to the response. So your test is wrong. The assert should be like this:
$this->assertEmpty($response->getHeaderLine('Set-Cookie'));

However it also fails this test:

    public function testCookiesSetWithoutRegenerate(): void
    {
        $persistence = new PhpSessionPersistence();
        $request = new ServerRequest();
        $session = $persistence->initializeSessionFromRequest($request);

        $session->set('foo', 'bar');

        $response = new Response();
        $response = $persistence->persistSession($session, $response);

        $this->assertNotEmpty($response->getHeaderLine('Set-Cookie'));
    }

In case you have a new session and add data to it, it should be persisted. The problem is that nowhere a check is performed for changed data. This is solved in #21:

    public function persistSession(SessionInterface $session, ResponseInterface $response) : ResponseInterface
    {
        // Regenerate if the session is marked as regenerated
        // Regenerate if there is no cookie id set but the session has changed (new session with data)
        if ($session->isRegenerated()
            || ($session->hasChanged() && ! $this->cookie)) {
            $this->regenerateSession();
        }

        // ...
    }

@rtek
Copy link
Author

rtek commented May 12, 2018

Yes this works. I misunderstood the underlying issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants