From fb70f6061d2ce611526a5e21166e3266e9c8f357 Mon Sep 17 00:00:00 2001 From: Brent Shaffer Date: Fri, 8 Sep 2023 13:55:57 -0700 Subject: [PATCH 1/4] feat: respect cache control for access token certs --- src/AccessToken.php | 22 ++++++++++---- tests/AccessTokenTest.php | 63 ++++++++++++++++++++++++++++++++++++--- 2 files changed, 76 insertions(+), 9 deletions(-) diff --git a/src/AccessToken.php b/src/AccessToken.php index e1f92ee7e..e8f78c630 100644 --- a/src/AccessToken.php +++ b/src/AccessToken.php @@ -33,6 +33,7 @@ use phpseclib3\Crypt\PublicKeyLoader; use phpseclib3\Math\BigInteger as BigInteger3; use Psr\Cache\CacheItemPoolInterface; +use Psr\Http\Message\ResponseInterface; use RuntimeException; use SimpleJWT\InvalidTokenException; use SimpleJWT\JWT as SimpleJWT; @@ -312,8 +313,19 @@ private function getCerts($location, $cacheKey, array $options = []) $certs = $cacheItem ? $cacheItem->get() : null; $gotNewCerts = false; + $expireTime = '+1 hour'; if (!$certs) { - $certs = $this->retrieveCertsFromLocation($location, $options); + $response = $this->retrieveCertsFromLocation($location, $options); + $certs = json_decode((string) $response->getBody(), true); + + if ($cacheControl = $response->getHeaderLine('Cache-Control')) { + array_map(function($value) use (&$expireTime) { + list($key, $value) = explode('=', $value) + [null, null]; + if ($key == 'max-age') { + $expireTime = '+' . $value . ' seconds'; + } + }, explode(', ', $cacheControl)); + } $gotNewCerts = true; } @@ -332,7 +344,7 @@ private function getCerts($location, $cacheKey, array $options = []) // Push caching off until after verifying certs are in a valid format. // Don't want to cache bad data. if ($gotNewCerts) { - $cacheItem->expiresAt(new DateTime('+1 hour')); + $cacheItem->expiresAt(new DateTime($expireTime)); $cacheItem->set($certs); $this->cache->save($cacheItem); } @@ -345,11 +357,11 @@ private function getCerts($location, $cacheKey, array $options = []) * * @param string $url location * @param array $options [optional] Configuration options. - * @return array certificates + * @return ResponseInterface * @throws InvalidArgumentException If certs could not be retrieved from a local file. * @throws RuntimeException If certs could not be retrieved from a remote location. */ - private function retrieveCertsFromLocation($url, array $options = []) + private function retrieveCertsFromLocation($url, array $options = []): ResponseInterface { // If we're retrieving a local file, just grab it. if (strpos($url, 'http') !== 0) { @@ -367,7 +379,7 @@ private function retrieveCertsFromLocation($url, array $options = []) $response = $httpHandler(new Request('GET', $url), $options); if ($response->getStatusCode() == 200) { - return json_decode((string) $response->getBody(), true); + return $response; } throw new RuntimeException(sprintf( diff --git a/tests/AccessTokenTest.php b/tests/AccessTokenTest.php index f2a6f3c81..3b0a283df 100644 --- a/tests/AccessTokenTest.php +++ b/tests/AccessTokenTest.php @@ -264,7 +264,10 @@ public function testEsVerifyEndToEnd() $this->assertEquals('https://cloud.google.com/iap', $payload['iss']); } - public function testGetCertsForIap() + /** + * @dataProvider provideCertsFromUrl + */ + public function testGetCertsFromUrl($certUrl) { $token = new AccessToken(); $reflector = new \ReflectionObject($token); @@ -272,14 +275,22 @@ public function testGetCertsForIap() $cacheKeyMethod->setAccessible(true); $getCertsMethod = $reflector->getMethod('getCerts'); $getCertsMethod->setAccessible(true); - $cacheKey = $cacheKeyMethod->invoke($token, AccessToken::IAP_CERT_URL); + $cacheKey = $cacheKeyMethod->invoke($token, $certUrl); $certs = $getCertsMethod->invoke( $token, - AccessToken::IAP_CERT_URL, + $certUrl, $cacheKey ); $this->assertTrue(is_array($certs)); - $this->assertEquals(5, count($certs)); + $this->assertGreaterThanOrEqual(2, count($certs)); + } + + public function provideCertsFromUrl() + { + return [ + [AccessToken::IAP_CERT_URL], + [AccessToken::FEDERATED_SIGNON_CERT_URL], + ]; } public function testRetrieveCertsFromLocationLocalFile() @@ -398,6 +409,50 @@ public function testRetrieveCertsFromLocationLocalFileInvalidFileData() ]); } + public function testRetrieveCertsFromLocationRespectsCacheControl() + { + $certsLocation = __DIR__ . '/fixtures/federated-certs.json'; + $certsJson = file_get_contents($certsLocation); + $certsData = json_decode($certsJson, true); + + $httpHandler = function (RequestInterface $request) use ($certsJson) { + return new Response(200, [ + 'cache-control' => 'public, max-age=1000', + ], $certsJson); + }; + + $phpunit = $this; + + $item = $this->prophesize('Psr\Cache\CacheItemInterface'); + $item->get() + ->shouldBeCalledTimes(1) + ->willReturn(null); + $item->set($certsData) + ->shouldBeCalledTimes(1) + ->willReturn($item->reveal()); + + // Assert date-time is set with difference of 1000 (the max-age in the Cache-Control header) + $item->expiresAt(Argument::type('\DateTime')) + ->shouldBeCalledTimes(1) + ->will(function ($value) use ($phpunit) { + $phpunit->assertEqualsWithDelta(1000, $value[0]->getTimestamp() - time(), 1); + }); + + $this->cache->getItem('google_auth_certs_cache|federated_signon_certs_v3') + ->shouldBeCalledTimes(1) + ->willReturn($item->reveal()); + + $this->cache->save(Argument::type('Psr\Cache\CacheItemInterface')) + ->shouldBeCalledTimes(1); + + $token = new AccessTokenStub( + $httpHandler, + $this->cache->reveal() + ); + + $token->verify($this->token); + } + public function testRetrieveCertsFromLocationRemote() { $certsLocation = __DIR__ . '/fixtures/federated-certs.json'; From 4817cdaaa1cc6e998aae585df15d0e13a5ba2a4a Mon Sep 17 00:00:00 2001 From: Brent Shaffer Date: Fri, 8 Sep 2023 14:03:20 -0700 Subject: [PATCH 2/4] fix tests --- src/AccessToken.php | 43 ++++++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/src/AccessToken.php b/src/AccessToken.php index e8f78c630..1f4fe40da 100644 --- a/src/AccessToken.php +++ b/src/AccessToken.php @@ -33,7 +33,6 @@ use phpseclib3\Crypt\PublicKeyLoader; use phpseclib3\Math\BigInteger as BigInteger3; use Psr\Cache\CacheItemPoolInterface; -use Psr\Http\Message\ResponseInterface; use RuntimeException; use SimpleJWT\InvalidTokenException; use SimpleJWT\JWT as SimpleJWT; @@ -312,22 +311,9 @@ private function getCerts($location, $cacheKey, array $options = []) $cacheItem = $this->cache->getItem($cacheKey); $certs = $cacheItem ? $cacheItem->get() : null; - $gotNewCerts = false; - $expireTime = '+1 hour'; + $expireTime = null; if (!$certs) { - $response = $this->retrieveCertsFromLocation($location, $options); - $certs = json_decode((string) $response->getBody(), true); - - if ($cacheControl = $response->getHeaderLine('Cache-Control')) { - array_map(function($value) use (&$expireTime) { - list($key, $value) = explode('=', $value) + [null, null]; - if ($key == 'max-age') { - $expireTime = '+' . $value . ' seconds'; - } - }, explode(', ', $cacheControl)); - } - - $gotNewCerts = true; + list($certs, $expireTime) = $this->retrieveCertsFromLocation($location, $options); } if (!isset($certs['keys'])) { @@ -343,7 +329,7 @@ private function getCerts($location, $cacheKey, array $options = []) // Push caching off until after verifying certs are in a valid format. // Don't want to cache bad data. - if ($gotNewCerts) { + if ($expireTime) { $cacheItem->expiresAt(new DateTime($expireTime)); $cacheItem->set($certs); $this->cache->save($cacheItem); @@ -357,13 +343,14 @@ private function getCerts($location, $cacheKey, array $options = []) * * @param string $url location * @param array $options [optional] Configuration options. - * @return ResponseInterface + * @return array{array, string} * @throws InvalidArgumentException If certs could not be retrieved from a local file. * @throws RuntimeException If certs could not be retrieved from a remote location. */ - private function retrieveCertsFromLocation($url, array $options = []): ResponseInterface + private function retrieveCertsFromLocation($url, array $options = []) { // If we're retrieving a local file, just grab it. + $expireTime = '+1 hour'; if (strpos($url, 'http') !== 0) { if (!file_exists($url)) { throw new InvalidArgumentException(sprintf( @@ -372,14 +359,28 @@ private function retrieveCertsFromLocation($url, array $options = []): ResponseI )); } - return json_decode((string) file_get_contents($url), true); + return [ + json_decode((string) file_get_contents($url), true), + $expireTime + ]; } $httpHandler = $this->httpHandler; $response = $httpHandler(new Request('GET', $url), $options); if ($response->getStatusCode() == 200) { - return $response; + if ($cacheControl = $response->getHeaderLine('Cache-Control')) { + array_map(function ($value) use (&$expireTime) { + list($key, $value) = explode('=', $value) + [null, null]; + if ($key == 'max-age') { + $expireTime = '+' . $value . ' seconds'; + } + }, explode(', ', $cacheControl)); + } + return [ + json_decode((string) $response->getBody(), true), + $expireTime + ]; } throw new RuntimeException(sprintf( From 89e54af99bea64df2d61cb955340a0341b586ff3 Mon Sep 17 00:00:00 2001 From: Brent Shaffer Date: Fri, 8 Sep 2023 14:06:33 -0700 Subject: [PATCH 3/4] fix one more test --- tests/AccessTokenTest.php | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/AccessTokenTest.php b/tests/AccessTokenTest.php index 3b0a283df..de51474b2 100644 --- a/tests/AccessTokenTest.php +++ b/tests/AccessTokenTest.php @@ -436,6 +436,7 @@ public function testRetrieveCertsFromLocationRespectsCacheControl() ->shouldBeCalledTimes(1) ->will(function ($value) use ($phpunit) { $phpunit->assertEqualsWithDelta(1000, $value[0]->getTimestamp() - time(), 1); + return $this; }); $this->cache->getItem('google_auth_certs_cache|federated_signon_certs_v3') From eb3aceb6e6049deda6d12fc14b68af2a77c13b31 Mon Sep 17 00:00:00 2001 From: Brent Shaffer Date: Fri, 15 Sep 2023 09:11:01 -0700 Subject: [PATCH 4/4] make whitespace optional --- src/AccessToken.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/AccessToken.php b/src/AccessToken.php index 1f4fe40da..0afc4ca1e 100644 --- a/src/AccessToken.php +++ b/src/AccessToken.php @@ -372,10 +372,10 @@ private function retrieveCertsFromLocation($url, array $options = []) if ($cacheControl = $response->getHeaderLine('Cache-Control')) { array_map(function ($value) use (&$expireTime) { list($key, $value) = explode('=', $value) + [null, null]; - if ($key == 'max-age') { + if (trim($key) == 'max-age') { $expireTime = '+' . $value . ' seconds'; } - }, explode(', ', $cacheControl)); + }, explode(',', $cacheControl)); } return [ json_decode((string) $response->getBody(), true),