Skip to content

Commit c2c802e

Browse files
andreromandrerom
authored
[Hash] Improve user hash change detection during request by comparing at the end (#543)
1 parent b4285c5 commit c2c802e

File tree

2 files changed

+62
-14
lines changed

2 files changed

+62
-14
lines changed

‎src/EventListener/UserContextListener.php

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -78,9 +78,9 @@ class UserContextListener implements EventSubscriberInterface
7878
private $hasSessionListener;
7979

8080
/**
81-
* @var string
81+
* @var bool
8282
*/
83-
private $hash;
83+
private $wasAnonymous;
8484

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

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

146+
// Return early if request is not a hash lookup
147147
return;
148148
}
149149

@@ -181,8 +181,6 @@ public function onKernelRequest(UserContextRequestEvent $event)
181181
*
182182
* For backward compatibility reasons, true will be returned if no anonymous request matcher was provided.
183183
*
184-
* @param Request $request
185-
*
186184
* @return bool
187185
*/
188186
private function isAnonymous(Request $request)
@@ -202,11 +200,18 @@ public function onKernelResponse(UserContextResponseEvent $event)
202200

203201
$response = $event->getResponse();
204202
$request = $event->getRequest();
205-
206203
$vary = $response->getVary();
207204

208205
if ($request->headers->has($this->options['user_hash_header'])) {
209-
if (null !== $this->hash && $this->hash !== $request->headers->get($this->options['user_hash_header'])) {
206+
$requestHash = $request->headers->get($this->options['user_hash_header']);
207+
208+
// Generate hash to see if it might have changed during request if user was, or is "logged in" (session)
209+
// But only needed if user was, or is, logged in
210+
if (!$this->wasAnonymous || !$this->isAnonymous($request)) {
211+
$hash = $this->hashGenerator->generateHash();
212+
}
213+
214+
if (isset($hash) && $hash !== $requestHash) {
210215
// hash has changed, session has most certainly changed, prevent setting incorrect cache
211216
$response->setCache([
212217
'max_age' => 0,

‎tests/Unit/EventListener/UserContextListenerTest.php

Lines changed: 47 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ public function testOnKernelRequest()
6161

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

@@ -123,7 +123,7 @@ public function testOnKernelRequestCached()
123123

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

@@ -158,7 +158,7 @@ public function testOnKernelRequestNotMatched()
158158

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

@@ -181,6 +181,41 @@ public function testOnKernelRequestNotMatched()
181181
$this->assertNull($response);
182182
}
183183

184+
public function testOnKernelRequestNotMatchedHasHeader()
185+
{
186+
$request = new Request();
187+
$request->setMethod('HEAD');
188+
$request->headers->set('X-Hash', 'hash');
189+
190+
$requestMatcher = $this->getRequestMatcher($request, false);
191+
$hashGenerator = \Mockery::mock(HashGenerator::class);
192+
$hashGenerator->shouldReceive('generateHash')->never();
193+
$responseTagger = \Mockery::mock(ResponseTagger::class);
194+
$responseTagger->shouldReceive('addTags')->never();
195+
196+
// TODO anonymousRequestMatcher
197+
$anonymousRequestMatcher = \Mockery::mock(RequestMatcherInterface::class);
198+
$anonymousRequestMatcher->shouldReceive('matches')->once()->andReturn(true);
199+
200+
$userContextListener = new UserContextListener(
201+
$requestMatcher,
202+
$hashGenerator,
203+
$anonymousRequestMatcher,
204+
$responseTagger,
205+
[
206+
'user_identifier_headers' => ['X-SessionId'],
207+
'user_hash_header' => 'X-Hash',
208+
]
209+
);
210+
$event = $this->getKernelRequestEvent($request);
211+
212+
$userContextListener->onKernelRequest($event);
213+
214+
$response = $event->getResponse();
215+
216+
$this->assertNull($response);
217+
}
218+
184219
public function testOnKernelResponse()
185220
{
186221
$request = new Request();
@@ -189,6 +224,7 @@ public function testOnKernelResponse()
189224

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

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

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

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

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

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

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

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

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

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

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

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

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

353395
$userContextListener = new UserContextListener(
354396
$requestMatcher,
@@ -377,6 +419,7 @@ public function testOnKernelResponseNotCached()
377419

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

381424
$userContextListener = new UserContextListener(
382425
$requestMatcher,
@@ -406,7 +449,7 @@ public function testFullRequestHashOk()
406449

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

0 commit comments

Comments
 (0)