From 9cf1cbca54d91d335e83ee3586b6f6bbfbe420e6 Mon Sep 17 00:00:00 2001 From: Brent Shaffer Date: Wed, 17 Jan 2024 13:59:54 -0800 Subject: [PATCH 1/5] fix: ensure ratelimit expiry is set every time --- src/CachedKeySet.php | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/CachedKeySet.php b/src/CachedKeySet.php index 01e27132..bbfe6504 100644 --- a/src/CachedKeySet.php +++ b/src/CachedKeySet.php @@ -212,15 +212,24 @@ private function rateLimitExceeded(): bool } $cacheItem = $this->cache->getItem($this->rateLimitCacheKey); +<<<<<<< Updated upstream if (!$cacheItem->isHit()) { $cacheItem->expiresAfter(60); // # of calls are cached each minute +======= + if (!$cacheItem->isHit() || !is_array($cacheItemData = $cacheItem->get())) { + $cacheItemData = [ + 'callsPerMinute' => 0, + 'expiry' => new \DateTime('+60 seconds', new \DateTimeZone('UTC')), + ]; +>>>>>>> Stashed changes } - $callsPerMinute = (int) $cacheItem->get(); - if (++$callsPerMinute > $this->maxCallsPerMinute) { + if (++$cacheItemData['callsPerMinute'] > $this->maxCallsPerMinute) { return true; } - $cacheItem->set($callsPerMinute); + + $cacheItem->set($cacheItemData); + $cacheItem->expiresAt($cacheItemData['expiry']); $this->cache->save($cacheItem); return false; } From 4e4f327f6bd56a8d8b78c0ba2f14c02b0547100c Mon Sep 17 00:00:00 2001 From: Brent Shaffer Date: Wed, 17 Jan 2024 14:04:11 -0800 Subject: [PATCH 2/5] resolve merge --- src/CachedKeySet.php | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/CachedKeySet.php b/src/CachedKeySet.php index bbfe6504..76ef2ea9 100644 --- a/src/CachedKeySet.php +++ b/src/CachedKeySet.php @@ -212,16 +212,12 @@ private function rateLimitExceeded(): bool } $cacheItem = $this->cache->getItem($this->rateLimitCacheKey); -<<<<<<< Updated upstream - if (!$cacheItem->isHit()) { - $cacheItem->expiresAfter(60); // # of calls are cached each minute -======= + if (!$cacheItem->isHit() || !is_array($cacheItemData = $cacheItem->get())) { $cacheItemData = [ 'callsPerMinute' => 0, 'expiry' => new \DateTime('+60 seconds', new \DateTimeZone('UTC')), ]; ->>>>>>> Stashed changes } if (++$cacheItemData['callsPerMinute'] > $this->maxCallsPerMinute) { From 5063644640119b17fb12ad4607a39a73310b4871 Mon Sep 17 00:00:00 2001 From: Brent Shaffer Date: Wed, 17 Jan 2024 14:05:17 -0800 Subject: [PATCH 3/5] cs fix --- src/CachedKeySet.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/CachedKeySet.php b/src/CachedKeySet.php index 76ef2ea9..6f867880 100644 --- a/src/CachedKeySet.php +++ b/src/CachedKeySet.php @@ -213,7 +213,7 @@ private function rateLimitExceeded(): bool $cacheItem = $this->cache->getItem($this->rateLimitCacheKey); - if (!$cacheItem->isHit() || !is_array($cacheItemData = $cacheItem->get())) { + if (!$cacheItem->isHit() || !\is_array($cacheItemData = $cacheItem->get())) { $cacheItemData = [ 'callsPerMinute' => 0, 'expiry' => new \DateTime('+60 seconds', new \DateTimeZone('UTC')), From ec55a4ae8495dcad361eaca4e7c36660366eaa83 Mon Sep 17 00:00:00 2001 From: Brent Shaffer Date: Sat, 18 May 2024 10:39:05 -0700 Subject: [PATCH 4/5] review comment, add test --- src/CachedKeySet.php | 17 +++++++------- tests/CachedKeySetTest.php | 47 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 8 deletions(-) diff --git a/src/CachedKeySet.php b/src/CachedKeySet.php index 6f867880..65bab74f 100644 --- a/src/CachedKeySet.php +++ b/src/CachedKeySet.php @@ -213,19 +213,20 @@ private function rateLimitExceeded(): bool $cacheItem = $this->cache->getItem($this->rateLimitCacheKey); - if (!$cacheItem->isHit() || !\is_array($cacheItemData = $cacheItem->get())) { - $cacheItemData = [ - 'callsPerMinute' => 0, - 'expiry' => new \DateTime('+60 seconds', new \DateTimeZone('UTC')), - ]; + $cacheItemData = []; + if ($cacheItem->isHit() && \is_array($data = $cacheItem->get())) { + $cacheItemData = $data; } - if (++$cacheItemData['callsPerMinute'] > $this->maxCallsPerMinute) { + $callsPerMinute = $cacheItemData['callsPerMinute'] ?? 0; + $expiry = $cacheItemData['expiry'] ?? new \DateTime('+60 seconds', new \DateTimeZone('UTC')); + + if (++$callsPerMinute > $this->maxCallsPerMinute) { return true; } - $cacheItem->set($cacheItemData); - $cacheItem->expiresAt($cacheItemData['expiry']); + $cacheItem->set(['expiry' => $expiry, 'callsPerMinute' => $callsPerMinute]); + $cacheItem->expiresAt($expiry); $this->cache->save($cacheItem); return false; } diff --git a/tests/CachedKeySetTest.php b/tests/CachedKeySetTest.php index e5d3aa86..9b8ddcbc 100644 --- a/tests/CachedKeySetTest.php +++ b/tests/CachedKeySetTest.php @@ -358,6 +358,53 @@ public function testRateLimit() $this->assertFalse(isset($cachedKeySet[$invalidKid])); } + public function testRateLimitWithExpiresAfter() + { + // We request the key 17 times, HTTP should only be called 15 times + $shouldBeCalledTimes = 10; + $cachedTimes = 2; + $afterExpirationTimes = 5; + + $totalHttpTimes = $shouldBeCalledTimes + $afterExpirationTimes; + + $cachePool = new TestMemoryCacheItemPool(); + + // Instantiate the cached key set + $cachedKeySet = new CachedKeySet( + $this->testJwksUri, + $this->getMockHttpClient($this->testJwks1, $totalHttpTimes), + $factory = $this->getMockHttpFactory($totalHttpTimes), + $cachePool, + 10, // expires after seconds + true // enable rate limiting + ); + + // Set the rate limit cache to expire after 1 second + $cacheItem = $cachePool->getItem('jwksratelimitjwkshttpsjwk.uri'); + $cacheItemData = $cacheItem->get(); + $cacheItemData['expiry'] = new \DateTime('+1 seconds', new \DateTimeZone('UTC')); + $cacheItem->set($cacheItemData); + $cacheItem->expiresAfter(1); + $cachePool->save($cacheItem); + + $invalidKid = 'invalidkey'; + for ($i = 0; $i < $shouldBeCalledTimes; $i++) { + $this->assertFalse(isset($cachedKeySet[$invalidKid])); + } + + // The next calls do not call HTTP + for ($i = 0; $i < $cachedTimes; $i++) { + $this->assertFalse(isset($cachedKeySet[$invalidKid])); + } + + sleep(1); // wait for cache to expire + + // These calls DO call HTTP because the cache has expired + for ($i = 0; $i < $afterExpirationTimes; $i++) { + $this->assertFalse(isset($cachedKeySet[$invalidKid])); + } + } + /** * @dataProvider provideFullIntegration */ From 6d43c991ee95db6ae30e10822e23cc94857bf3d0 Mon Sep 17 00:00:00 2001 From: Brent Shaffer Date: Sat, 18 May 2024 11:00:55 -0700 Subject: [PATCH 5/5] improve tests --- tests/CachedKeySetTest.php | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/tests/CachedKeySetTest.php b/tests/CachedKeySetTest.php index 9b8ddcbc..39bbc919 100644 --- a/tests/CachedKeySetTest.php +++ b/tests/CachedKeySetTest.php @@ -344,7 +344,7 @@ public function testRateLimit() $cachedKeySet = new CachedKeySet( $this->testJwksUri, $this->getMockHttpClient($this->testJwks1, $shouldBeCalledTimes), - $factory = $this->getMockHttpFactory($shouldBeCalledTimes), + $this->getMockHttpFactory($shouldBeCalledTimes), new TestMemoryCacheItemPool(), 10, // expires after seconds true // enable rate limiting @@ -373,7 +373,7 @@ public function testRateLimitWithExpiresAfter() $cachedKeySet = new CachedKeySet( $this->testJwksUri, $this->getMockHttpClient($this->testJwks1, $totalHttpTimes), - $factory = $this->getMockHttpFactory($totalHttpTimes), + $this->getMockHttpFactory($totalHttpTimes), $cachePool, 10, // expires after seconds true // enable rate limiting @@ -381,9 +381,10 @@ public function testRateLimitWithExpiresAfter() // Set the rate limit cache to expire after 1 second $cacheItem = $cachePool->getItem('jwksratelimitjwkshttpsjwk.uri'); - $cacheItemData = $cacheItem->get(); - $cacheItemData['expiry'] = new \DateTime('+1 seconds', new \DateTimeZone('UTC')); - $cacheItem->set($cacheItemData); + $cacheItem->set([ + 'expiry' => new \DateTime('+1 second', new \DateTimeZone('UTC')), + 'callsPerMinute' => 0, + ]); $cacheItem->expiresAfter(1); $cachePool->save($cacheItem); @@ -513,7 +514,10 @@ final class TestMemoryCacheItemPool implements CacheItemPoolInterface public function getItem($key): CacheItemInterface { - return current($this->getItems([$key])); + $item = current($this->getItems([$key])); + $item->expiresAt(null); // mimic symfony cache behavior + + return $item; } public function getItems(array $keys = []): iterable