Skip to content

[Hash] Improve user hash change detection during request by comparing at the end #543

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Aug 25, 2020
Merged

[Hash] Improve user hash change detection during request by comparing at the end #543

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5eefeb5
[Hash] Improve user hash change detection during request by comparing…
andrerom Jun 19, 2020
84e9b51
Adapting for moving target on code style config
andrerom Aug 19, 2020
ec50558
fixup! [Hash] Improve user hash change detection during request by co…
andrerom Aug 25, 2020
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
25 changes: 15 additions & 10 deletions src/EventListener/UserContextListener.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,9 @@ class UserContextListener implements EventSubscriberInterface
private $hasSessionListener;

/**
* @var string
* @var bool
*/
private $hash;
private $wasAnonymous;

/**
* Used to exclude anonymous requests (no authentication nor session) from user hash sanity check.
Expand Down Expand Up @@ -138,12 +138,12 @@ public function onKernelRequest(UserContextRequestEvent $event)

$request = $event->getRequest();
if (!$this->requestMatcher->matches($request)) {
if ($event->getRequest()->headers->has($this->options['user_hash_header'])
&& !$this->isAnonymous($event->getRequest())
) {
$this->hash = $this->hashGenerator->generateHash();
if ($request->headers->has($this->options['user_hash_header'])) {
// Keep track of if user is anonymous when we have user hash header in request
$this->wasAnonymous = $this->isAnonymous($request);
}

// Return early if request is not a hash lookup
return;
}

Expand Down Expand Up @@ -181,8 +181,6 @@ public function onKernelRequest(UserContextRequestEvent $event)
*
* For backward compatibility reasons, true will be returned if no anonymous request matcher was provided.
*
* @param Request $request
*
* @return bool
*/
private function isAnonymous(Request $request)
Expand All @@ -202,11 +200,18 @@ public function onKernelResponse(UserContextResponseEvent $event)

$response = $event->getResponse();
$request = $event->getRequest();

$vary = $response->getVary();

if ($request->headers->has($this->options['user_hash_header'])) {
if (null !== $this->hash && $this->hash !== $request->headers->get($this->options['user_hash_header'])) {
$requestHash = $request->headers->get($this->options['user_hash_header']);

// Generate hash to see if it might have changed during request if user was, or is "logged in" (session)
// But only needed if user was, or is, logged in
if (!$this->wasAnonymous || !$this->isAnonymous($request)) {
$hash = $this->hashGenerator->generateHash();
}

if (isset($hash) && $hash !== $requestHash) {
// hash has changed, session has most certainly changed, prevent setting incorrect cache
$response->setCache([
'max_age' => 0,
Expand Down
51 changes: 47 additions & 4 deletions tests/Unit/EventListener/UserContextListenerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ public function testOnKernelRequest()

$requestMatcher = $this->getRequestMatcher($request, true);
$hashGenerator = \Mockery::mock(HashGenerator::class);
$hashGenerator->shouldReceive('generateHash')->andReturn('hash');
$hashGenerator->shouldReceive('generateHash')->once()->andReturn('hash');
$responseTagger = \Mockery::mock(ResponseTagger::class);
$responseTagger->shouldReceive('addTags')->with(['fos_http_cache_hashlookup-hash']);

Expand Down Expand Up @@ -123,7 +123,7 @@ public function testOnKernelRequestCached()

$requestMatcher = $this->getRequestMatcher($request, true);
$hashGenerator = \Mockery::mock(HashGenerator::class);
$hashGenerator->shouldReceive('generateHash')->andReturn('hash');
$hashGenerator->shouldReceive('generateHash')->once()->andReturn('hash');
$responseTagger = \Mockery::mock(ResponseTagger::class);
$responseTagger->shouldReceive('addTags')->with(['fos_http_cache_hashlookup-hash']);

Expand Down Expand Up @@ -158,7 +158,7 @@ public function testOnKernelRequestNotMatched()

$requestMatcher = $this->getRequestMatcher($request, false);
$hashGenerator = \Mockery::mock(HashGenerator::class);
$hashGenerator->shouldReceive('generateHash')->andReturn('hash');
$hashGenerator->shouldReceive('generateHash')->never();
$responseTagger = \Mockery::mock(ResponseTagger::class);
$responseTagger->shouldReceive('addTags')->never();

Expand All @@ -181,6 +181,41 @@ public function testOnKernelRequestNotMatched()
$this->assertNull($response);
}

public function testOnKernelRequestNotMatchedHasHeader()
{
$request = new Request();
$request->setMethod('HEAD');
$request->headers->set('X-Hash', 'hash');

$requestMatcher = $this->getRequestMatcher($request, false);
$hashGenerator = \Mockery::mock(HashGenerator::class);
$hashGenerator->shouldReceive('generateHash')->never();
$responseTagger = \Mockery::mock(ResponseTagger::class);
$responseTagger->shouldReceive('addTags')->never();

// TODO anonymousRequestMatcher
$anonymousRequestMatcher = \Mockery::mock(RequestMatcherInterface::class);
$anonymousRequestMatcher->shouldReceive('matches')->once()->andReturn(true);

$userContextListener = new UserContextListener(
$requestMatcher,
$hashGenerator,
$anonymousRequestMatcher,
$responseTagger,
[
'user_identifier_headers' => ['X-SessionId'],
'user_hash_header' => 'X-Hash',
]
);
$event = $this->getKernelRequestEvent($request);

$userContextListener->onKernelRequest($event);

$response = $event->getResponse();

$this->assertNull($response);
}

public function testOnKernelResponse()
{
$request = new Request();
Expand All @@ -189,6 +224,7 @@ public function testOnKernelResponse()

$requestMatcher = $this->getRequestMatcher($request, false);
$hashGenerator = \Mockery::mock(HashGenerator::class);
$hashGenerator->shouldReceive('generateHash')->once()->andReturn('hash');
$responseTagger = \Mockery::mock(ResponseTagger::class);
$responseTagger->shouldReceive('addTags')->never();

Expand Down Expand Up @@ -221,6 +257,7 @@ public function testOnKernelResponseSetsNoAutoCacheHeader()
$request->headers->set('X-User-Context-Hash', 'hash');

$hashGenerator = \Mockery::mock(HashGenerator::class);
$hashGenerator->shouldReceive('generateHash')->once()->andReturn('hash');

$userContextListener = new UserContextListener(
$this->getRequestMatcher($request, false),
Expand All @@ -245,6 +282,7 @@ public function testOnKernelResponseDoesNotSetNoAutoCacheHeaderWhenNoSessionList
$request->headers->set('X-User-Context-Hash', 'hash');

$hashGenerator = \Mockery::mock(HashGenerator::class);
$hashGenerator->shouldReceive('generateHash')->once()->andReturn('hash');

$userContextListener = new UserContextListener(
$this->getRequestMatcher($request, false),
Expand Down Expand Up @@ -273,6 +311,7 @@ public function testOnKernelResponseSetsNoAutoCacheHeaderWhenCustomHeader()
$request->headers->set('X-User-Context-Hash', 'hash');

$hashGenerator = \Mockery::mock(HashGenerator::class);
$hashGenerator->shouldReceive('generateHash')->once()->andReturn('hash');

$userContextListener = new UserContextListener(
$this->getRequestMatcher($request, false),
Expand All @@ -296,6 +335,7 @@ public function testOnKernelResponseSetsNoAutoCacheHeaderWhenCustomHeaderAndNoAd
$request->headers->set('X-User-Context-Hash', 'hash');

$hashGenerator = \Mockery::mock(HashGenerator::class);
$hashGenerator->shouldReceive('generateHash')->once()->andReturn('hash');

$userContextListener = new UserContextListener(
$this->getRequestMatcher($request, false),
Expand Down Expand Up @@ -324,6 +364,7 @@ public function testOnKernelResponseDoesNotSetNoAutoCacheHeaderWhenNoCustomHeade
$request->headers->set('X-User-Context-Hash', 'hash');

$hashGenerator = \Mockery::mock(HashGenerator::class);
$hashGenerator->shouldReceive('generateHash')->once()->andReturn('hash');

$userContextListener = new UserContextListener(
$this->getRequestMatcher($request, false),
Expand All @@ -349,6 +390,7 @@ public function testOnKernelResponseNotMaster()

$requestMatcher = $this->getRequestMatcher($request, false);
$hashGenerator = \Mockery::mock(HashGenerator::class);
$hashGenerator->shouldReceive('generateHash')->never();

$userContextListener = new UserContextListener(
$requestMatcher,
Expand Down Expand Up @@ -377,6 +419,7 @@ public function testOnKernelResponseNotCached()

$requestMatcher = $this->getRequestMatcher($request, false);
$hashGenerator = \Mockery::mock(HashGenerator::class);
$hashGenerator->shouldReceive('generateHash')->never();

$userContextListener = new UserContextListener(
$requestMatcher,
Expand Down Expand Up @@ -406,7 +449,7 @@ public function testFullRequestHashOk()

$requestMatcher = $this->getRequestMatcher($request, false);
$hashGenerator = \Mockery::mock(HashGenerator::class);
$hashGenerator->shouldReceive('generateHash')->andReturn('hash');
$hashGenerator->shouldReceive('generateHash')->once()->andReturn('hash');
$responseTagger = \Mockery::mock(ResponseTagger::class);
$responseTagger->shouldReceive('addTags')->with(['fos_http_cache_usercontext-hash']);

Expand Down