Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.8k
[Security] Set OIDC JWKS cache TTL from provider headers#62369
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[Security] Set OIDC JWKS cache TTL from provider headers#62369
Conversation
src/Symfony/Component/Security/Http/AccessToken/Oidc/OidcTokenHandler.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Security/Http/AccessToken/Oidc/OidcTokenHandler.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Security/Http/AccessToken/Oidc/OidcTokenHandler.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Security/Http/AccessToken/Oidc/OidcTokenHandler.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Security/Http/AccessToken/Oidc/OidcTokenHandler.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
38cd272 to981d813Compare
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd consider this as a bugfix for 7.4
src/Symfony/Component/Security/Http/AccessToken/Oidc/OidcTokenHandler.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
981d813 tocf49d44Comparecf49d44 to8898652Compare8898652 to61acbf1Comparenicolas-grekas commentedNov 14, 2025
Thank you@Ali-HENDA. |
42a8072 intosymfony:7.4Uh oh!
There was an error while loading.Please reload this page.
| * The cache entry lifetime is automatically adjusted based on the lowest TTL | ||
| * advertised by the providers (via "Cache-Control: max-age" or "Expires" headers). | ||
| * | ||
| * @internal this method is public to enable async offline cache population |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Where is this method called publicly ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Because per PR#30572, async recomputation via Messenger only works with callables of the form [$service, 'publicMethod'], so the callback must be public.
| } | ||
| $jwkSetResponses = []; | ||
| foreach ($client->stream($configResponses)as$response =>$chunk) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is broken IMO:
$clientmight be undefined (if the loop is empty)- if the clients are different, there is no guarantee that calling
streamwill work. Responses must be streamed with the client that created them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
After checking, this issue was already present before my patch. I didn't notice it when working on the cache handling. You're absolutely right to point it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@Ali-HENDA up to send a PR fixing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sure, I’ll open a PR for this.
| $this->assertSame('user-cache-control',$handler->getUserBadgeFrom($token)->getUserIdentifier()); | ||
| $this->assertSame(2,$requestCount); | ||
| $this->assertSame('user-cache-control',$handler->getUserBadgeFrom($token)->getUserIdentifier()); | ||
| $this->assertSame(2,$requestCount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why are these lines duplicated?
And does this test actually test that the cache-control header is used as cache TTL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The lines are duplicated because the first call fills the cache (2 requests) and the second call verifies that the cached JWKS is reused (request count stays at 2). The TTL is tested implicitly: since the second call happens
within the max-age window, it must be a cache hit.
| $this->assertSame('user-expires',$handler->getUserBadgeFrom($token)->getUserIdentifier()); | ||
| $this->assertSame(2,$requestCount); | ||
| $this->assertSame('user-expires',$handler->getUserBadgeFrom($token)->getUserIdentifier()); | ||
| $this->assertSame(2,$requestCount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same reason as above
Uh oh!
There was an error while loading.Please reload this page.
This PR aligns the OIDC JWKS discovery cache with OpenID Connect best practices by making it dynamic and respecting provider cache headers.
Before
After
Cache-Control: max-ageExpires