server: Use max-age when refreshing JWKs if must-revalidate is present

PR-URL: https://github.com/hasura/graphql-engine-mono/pull/3979
GitOrigin-RevId: 5f5ebfb25ff9729e34397084d86b0fe968099b25

.circleci/test-server.sh

@@ -1035,6 +1035,16 @@ jwk-url)
 	kill_hge_servers
 	unset HASURA_GRAPHQL_JWT_SECRET
 
+	echo "Test: Cache-Control with must-revalidate, max-age=3"
+	export HASURA_GRAPHQL_JWT_SECRET='{"jwk_url": "http://localhost:5001/jwk-cache-control?must-revalidate=true&must-revalidate=true"}'
+	run_hge_with_args serve
+	wait_for_port 8080
+
+	pytest -n 1 --hge-urls "$HGE_URL" --pg-urls "$HASURA_GRAPHQL_DATABASE_URL" --hge-key="$HASURA_GRAPHQL_ADMIN_SECRET" --test-jwk-url -k 'test_cache_control_header_max_age'
+
+	kill_hge_servers
+	unset HASURA_GRAPHQL_JWT_SECRET
+
 	echo "Test: Cache-Control with must-revalidate"
 	export HASURA_GRAPHQL_JWT_SECRET='{"jwk_url": "http://localhost:5001/jwk-cache-control?must-revalidate=true"}'
 	run_hge_with_args serve

CHANGELOG.md

@@ -98,7 +98,7 @@ function:
 
 ### Bug fixes and improvements
 
-- server: add jsonb to string cast support - postgres 
+- server: add jsonb to string cast support - postgres
 - server: improve performance of fetching postgres catalog metadata for tables and functions
 - server: Queries present in query collections, such as allow-list, and rest-endpoints are now validated (against the schema)
 - server: Redesigns internal implementation of webhook transforms.
@@ -107,6 +107,7 @@ function:
 - server: add support for customising the GraphQL schema descriptions of table columns in metadata
 - server: implement column presets for SQL Server (#8221)
 - server: fix caching bug with session variables in remote joins
+- server: fix regression where JWKs are refreshed once per second when both must-revalidate and max-age are specified in the Cache-Control header (#8299)
 - console: fixed an issue where editing both a column's name and its GraphQL field name at the same time caused an error
 - console: enable searching tables within a schema
 - cli: fix inherited roles metadata not being updated when dropping all roles (#7872)

docs/graphql/core/auth/authentication/jwt.rst

@@ -198,12 +198,10 @@ Following is the behaviour in detail:
    3. third, check if ``Expires`` header is present (if ``Cache-Control`` is not present), and try
       to parse the value as a timestamp.
 
-2. If ``no-cache``, ``no-store`` or ``must-revalidate`` are parsed successfully, then it will refresh
-   the JWKs once a second. If it is able to parse ``max-age``, ``s-maxage`` or ``Expires`` successfully,
-   then it will use that parsed time to refresh/refetch the JWKs again. If it is unable to parse,
-   then it will not refresh the JWKs (it assumes that if the above headers are not present, the provider
-   doesn't rotate their JWKs). If the parsed time is less than a second, the JWKs will at most be 
-   fetched once per second regardless.
+2. If it is able to parse ``max-age``, ``s-maxage`` or ``Expires`` successfully, then it will use that parsed time to refresh/refetch the JWKs again. 
+  If it is unable to parse, then it will not refresh the JWKs (it assumes that if the above headers are not present, the provider doesn't rotate their JWKs). If the parsed time is less than a second, the JWKs will be fetched once per second regardless.
+  If ``must-revalidate`` and ``max-age`` are present, then it will refresh the JWK again after the time period specified in ``max-age`` has passed. 
+  However, if ``max-age`` is not specified or if ``no-cache`` or ``no-store`` are present, then it will refresh the JWKs once a second. 
 
 **While running**:
 
@@ -381,7 +379,7 @@ Examples:
 .. note::
 
    A JWT configuration without an issuer will match any issuer field present in
-   an incoming JWT. 
+   an incoming JWT.
 
 .. note::
 
@@ -624,7 +622,7 @@ the JWT config only needs to have the public key.
       "
     }
 
-.. _running_with_jwt: 
+.. _running_with_jwt:
 
 Running with JWT
 ^^^^^^^^^^^^^^^^

server/src-lib/Hasura/Server/Auth/JWT.hs

@@ -404,9 +404,14 @@ determineJwkExpiryLifetime getCurrentTime' (Logger logger) responseHeaders =
     timeFromCacheControl = do
       header <- afold $ bsToTxt <$> lookup "Cache-Control" responseHeaders
       cacheControl <- parseCacheControl header `onLeft` \err -> logAndThrowInfo $ parseCacheControlErr $ T.pack err
-      if noCacheExists cacheControl || noStoreExists cacheControl || mustRevalidateExists cacheControl
-        then pure 0 -- In these cases we want don't want to cache the JWK, so we use an immediate expiry time
-        else fromInteger <$> MaybeT (findMaxAge cacheControl `onLeft` \err -> logAndThrowInfo $ parseCacheControlErr $ T.pack err)
+      maxAgeMaybe <- fmap fromInteger <$> findMaxAge cacheControl `onLeft` \err -> logAndThrowInfo $ parseCacheControlErr $ T.pack err
+      if
+          -- If a max-age is specified with a must-revalidate we use it, but if not we use an immediate expiry time
+          | mustRevalidateExists cacheControl -> pure $ fromMaybe 0 maxAgeMaybe
+          -- In these cases we want don't want to cache the JWK, so we use an immediate expiry time
+          | noCacheExists cacheControl || noStoreExists cacheControl -> pure 0
+          -- Use max-age, if it exists
+          | otherwise -> hoistMaybe maxAgeMaybe
 
     timeFromExpires :: MaybeT m NominalDiffTime
     timeFromExpires = do

server/src-test/Hasura/Server/Auth/JWTSpec.hs

@@ -23,11 +23,17 @@ determineJwkExpiryLifetimeTests = describe "determineJwkExpiryLifetime" $ do
     result <- determineJwkExpiryLifetime' [expires, cacheControl]
     result `shouldBe` (Right . Just $ secondsToNominalDiffTime 0)
 
-  it "must-revalidate in Cache-Control means an immediate expiry" $ do
+  it "must-revalidate without max-age in Cache-Control means an immediate expiry" $ do
+    let expires = expiresHeader (addUTCTime (secondsToNominalDiffTime 60) currentTimeForTest)
+    let cacheControl = cacheControlHeader "must-revalidate"
+    result <- determineJwkExpiryLifetime' [expires, cacheControl]
+    result `shouldBe` (Right . Just $ secondsToNominalDiffTime 0)
+
+  it "must-revalidate with max-age in Cache-Control uses max-age for token expiry" $ do
     let expires = expiresHeader (addUTCTime (secondsToNominalDiffTime 60) currentTimeForTest)
     let cacheControl = cacheControlHeader "max-age=10, must-revalidate"
     result <- determineJwkExpiryLifetime' [expires, cacheControl]
-    result `shouldBe` (Right . Just $ secondsToNominalDiffTime 0)
+    result `shouldBe` (Right . Just $ secondsToNominalDiffTime 10)
 
   it "no-store in Cache-Control means an immediate expiry" $ do
     let expires = expiresHeader (addUTCTime (secondsToNominalDiffTime 60) currentTimeForTest)
@@ -35,13 +41,13 @@ determineJwkExpiryLifetimeTests = describe "determineJwkExpiryLifetime" $ do
     result <- determineJwkExpiryLifetime' [expires, cacheControl]
     result `shouldBe` (Right . Just $ secondsToNominalDiffTime 0)
 
-  it "max-age in Cache-Control without no-cache, must-revalidate, no-store is used for token expiry" $ do
+  it "max-age in Cache-Control without no-cache, no-store is used for token expiry" $ do
     let expires = expiresHeader (addUTCTime (secondsToNominalDiffTime 60) currentTimeForTest)
     let cacheControl = cacheControlHeader "public, max-age=10"
     result <- determineJwkExpiryLifetime' [expires, cacheControl]
     result `shouldBe` (Right . Just $ secondsToNominalDiffTime 10)
 
-  it "s-maxage in Cache-Control without no-cache, must-revalidate, no-store is used for token expiry" $ do
+  it "s-maxage in Cache-Control without no-cache, no-store is used for token expiry" $ do
     let expires = expiresHeader (addUTCTime (secondsToNominalDiffTime 60) currentTimeForTest)
     let cacheControl = cacheControlHeader "public, s-maxage=10"
     result <- determineJwkExpiryLifetime' [expires, cacheControl]