Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add auto_regenerate option to CacheSessionPersistence #51

Merged
merged 7 commits into from
May 7, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions docs/book/v1/manual.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,10 @@ The following details the constructor of the `Mezzio\Session\Cache\CacheSessionP
* @param string $cookieSameSite The same-site rule to apply to the persisted
* cookie. Options include "Lax", "Strict", and "None".
* Available since 1.4.0
*
* @param bool $autoRegenerate Whether or not the session ID should be
* regenerated on session data changes
* Available since 1.13.0
*
* @todo reorder these arguments so they make more sense and are in an
* order of importance
*/
Expand All @@ -51,7 +54,8 @@ public function __construct(
string $cookieDomain = null,
bool $cookieSecure = false,
bool $cookieHttpOnly = false,
string $cookieSameSite = 'Lax'
string $cookieSameSite = 'Lax',
bool $autoRegenerate = true,
) {
```

Expand Down
3 changes: 3 additions & 0 deletions psalm-baseline.xml
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
<code>$cookieSecure</code>
<code>$lastModified</code>
<code>$persistent</code>
<code>$autoRegenerate</code>
</MixedArgument>
<MixedArrayAccess>
<code><![CDATA[$config['cache_expire']]]></code>
Expand All @@ -43,6 +44,7 @@
<code><![CDATA[$config['last_modified']]]></code>
<code><![CDATA[$config['mezzio-session-cache']]]></code>
<code><![CDATA[$config['persistent']]]></code>
<code><![CDATA[$config['auto_regenerate']]]></code>
</MixedArrayAccess>
<MixedAssignment>
<code>$cacheExpire</code>
Expand All @@ -58,6 +60,7 @@
<code>$cookieSecure</code>
<code>$lastModified</code>
<code>$persistent</code>
<code>$autoRegenerate</code>
</MixedAssignment>
</file>
<file src="test/Asset/TestHandler.php">
Expand Down
12 changes: 9 additions & 3 deletions src/CacheSessionPersistence.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ class CacheSessionPersistence implements SessionPersistenceInterface
private CacheItemPoolInterface $cache;

private bool $persistent;
private bool $autoRegenerate;

/**
* Prepare session cache and default HTTP caching headers.
Expand Down Expand Up @@ -67,6 +68,8 @@ class CacheSessionPersistence implements SessionPersistenceInterface
* be accessed by client-side apis.
* @param string $cookieSameSite The same-site rule to apply to the persisted
* cookie. Options include "Lax", "Strict", and "None".
* @param bool $autoRegenerate Whether or not the session ID should be
* regenerated on session data changes
* @todo reorder the constructor arguments
*/
public function __construct(
Expand All @@ -80,7 +83,8 @@ public function __construct(
?string $cookieDomain = null,
bool $cookieSecure = false,
bool $cookieHttpOnly = false,
string $cookieSameSite = 'Lax'
string $cookieSameSite = 'Lax',
bool $autoRegenerate = true
) {
$this->cache = $cache;

Expand Down Expand Up @@ -112,6 +116,8 @@ public function __construct(
: $this->getLastModified();

$this->persistent = $persistent;

$this->autoRegenerate = $autoRegenerate;
}

public function initializeSessionFromRequest(ServerRequestInterface $request): SessionInterface
Expand Down Expand Up @@ -139,8 +145,8 @@ public function persistSession(SessionInterface $session, ResponseInterface $res
// Regenerate the session if:
// - we have no session identifier
// - the session is marked as regenerated
// - the session has changed (data is different)
if ('' === $id || $session->isRegenerated() || $session->hasChanged()) {
// - the session has changed (data is different) and autoRegenerate is turned on (default) in the configuration
if ('' === $id || $session->isRegenerated() || ($this->autoRegenerate && $session->hasChanged())) {
$id = $this->regenerateSession($id);
}

Expand Down
4 changes: 3 additions & 1 deletion src/CacheSessionPersistenceFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ public function __invoke(ContainerInterface $container)
$cacheExpire = $config['cache_expire'] ?? 10800;
$lastModified = $config['last_modified'] ?? null;
$persistent = $config['persistent'] ?? false;
$autoRegenerate = $config['auto_regenerate'] ?? true;

return new CacheSessionPersistence(
$container->get($cacheService),
Expand All @@ -46,7 +47,8 @@ public function __invoke(ContainerInterface $container)
$cookieDomain,
$cookieSecure,
$cookieHttpOnly,
$cookieSameSite
$cookieSameSite,
$autoRegenerate
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd cast this to boolean here (and do the same for $cookieSameSite while you're at it):

(bool) $cookieSameSite,
(bool) $autoRegenerate,

This will remove one more baseline item, and prevent some common errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've introduced type casting for all boolean arguments (as suggested) - $cookieSameSite is expected to be a string so I left it as is for now

);
}
}
3 changes: 3 additions & 0 deletions test/CacheSessionPersistenceFactoryTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ public function testFactoryUsesSaneDefaultsForConstructorArguments(): void
$this->assertAttributeSame(10800, 'cacheExpire', $persistence);
$this->assertAttributeNotEmpty('lastModified', $persistence);
$this->assertAttributeSame(false, 'persistent', $persistence);
$this->assertAttributeSame(true, 'autoRegenerate', $persistence);
}

public function testFactoryAllowsConfiguringAllConstructorArguments(): void
Expand All @@ -95,6 +96,7 @@ public function testFactoryAllowsConfiguringAllConstructorArguments(): void
'cache_expire' => 300,
'last_modified' => $lastModified,
'persistent' => true,
'auto_regenerate' => false,
],
]);

Expand All @@ -115,6 +117,7 @@ public function testFactoryAllowsConfiguringAllConstructorArguments(): void
$persistence
);
$this->assertAttributeSame(true, 'persistent', $persistence);
$this->assertAttributeSame(false, 'autoRegenerate', $persistence);
}

public function testFactoryAllowsConfiguringCacheAdapterServiceName(): void
Expand Down
36 changes: 36 additions & 0 deletions test/CacheSessionPersistenceIntegrationTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -111,4 +111,40 @@ public function testThatAChangedSessionWillCauseRegenerationAndASetCookieHeader(
$value = $item->get();
self::assertNull($value, 'The previous session data should have been deleted');
}

public function testThatAChangedSessionWillNotCauseRegenerationWhenAutoRegenerateIsFalse(): void
{
$this->storage = new CacheSessionPersistence(
$this->cache,
cookieName: 'Session',
autoRegenerate: false,
);

$item = $this->cache->getItem('foo');
$item->set(['foo' => 'bar']);
$this->cache->save($item);

$request = (new ServerRequest())->withHeader('Cookie', 'Session=foo;');
$middleware = new SessionMiddleware($this->storage);
$handler = new TestHandler();
$handler->setSessionVariable('something', 'groovy');
$response = $middleware->process($request, $handler);

$setCookie = SetCookies::fromResponse($response);
$cookie = $setCookie->get('Session');
self::assertNotNull($cookie);

$id = $cookie->getValue();
self::assertNotNull($id);

self::assertSame('foo', $id);
self::assertNotSame($handler->response, $response);

$item = $this->cache->getItem('foo');
$value = $item->get();
self::assertSame([
'foo' => 'bar',
'something' => 'groovy',
], $value, 'The session data should have been updated');
}
}