-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Server Auth. JWK auto-configuration #5339
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdds OpenID Connect discovery, a JWK-based JWT configuration path, a new JwkConfig type with caching/rate-limit options, fetchOpenIdConfiguration helper and DiscoveryException, updates JWT verifier wiring and validation flow, extensive JWK tests, build updates, and an IDE dictionary entry. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-jwt/jvm/src/io/ktor/server/auth/jwt/JwkConfig.kt`:
- Around line 117-119: The rateLimit() setter fails to re-enable rate limiting
because it only assigns rateLimitConfig and never sets rateLimitEnabled to true;
update the rateLimit(bucketSize: Long, refillDuration: Duration) function to set
rateLimitConfig = RateLimitConfig(...) and also set rateLimitEnabled = true so
calling disableRateLimit() followed by rateLimit(...) correctly re-enables rate
limiting (similar to how cache() sets cacheEnabled = true).
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-jwt/jvm/src/io/ktor/server/auth/jwt/JWTAuth.kt`:
- Around line 391-422: Update the KDoc for the public jwk() API
(JWTAuthenticationProvider.Config.jwk) to include a `@return` tag (even if Unit)
and `@throws` entries; explicitly document that the function returns Unit and list
possible exceptions such as IllegalArgumentException when required configuration
is missing (e.g., missing validate block or openIdConfig), and any other
exceptions the implementation can throw; reference the jwk() configurator and
JwkConfig in the comments so callers know which symbols the exceptions relate
to.
- Around line 423-428: In jwk(configure: JwkConfig.() -> Unit) replace the
unsafe force-unwrapping of config.openIdConfig (the "!!" on issuer assignment)
with an explicit null check using requireNotNull or similar to throw a clear
error; e.g., call requireNotNull(config.openIdConfig) { "OpenID configuration is
required for jwk() in JwkConfig" } and then read .issuer, ensuring the
verifier(jwkProvider, issuer, config.jwtConfigure) call only runs when
openIdConfig is present.
In
`@ktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/OpenIdConfiguration.kt`:
- Around line 15-77: Update the KDoc fqname links from io.ktor.server.auth.jwt.*
to io.ktor.server.auth.* for OpenIdConfiguration, fetchOpenIdConfiguration, and
DiscoveryException; add a `@return` description to the fetchOpenIdConfiguration
KDoc stating it returns an OpenIdConfiguration; add `@param` tags describing the
issuer parameter in fetchOpenIdConfiguration; and add `@param` message, `@param`
cause, and `@throws` (DiscoveryException) tags to the DiscoveryException KDoc so
all public APIs (OpenIdConfiguration, fetchOpenIdConfiguration,
DiscoveryException) are fully documented per guidelines.
- Around line 7-13: The decodeFromString call in fetchOpenIdConfiguration can
throw a SerializationException that currently escapes despite the KDoc stating
`@throws` DiscoveryException; wrap the call to
discoveryJson.decodeFromString<OpenIdConfiguration>(body) in a try-catch that
catches kotlinx.serialization.SerializationException (and optionally other
parsing-related exceptions) and rethrow a io.ktor.server.auth.DiscoveryException
with a clear message and the caught exception as the cause so callers receive
the documented exception type from fetchOpenIdConfiguration.
🧹 Nitpick comments (3)
ktor-server/ktor-server-plugins/ktor-server-auth-jwt/jvm/test/io/ktor/server/auth/jwt/JwkDiscoveryTest.kt (3)
22-22: Remove unused import.
io.mockk.verifyis imported but never used in this test file.-import io.mockk.verify
105-124: Consider asserting the specific exception type.Using
assertFailsvalidates that an exception is thrown but doesn't verify it's the expectedDiscoveryException. This could mask unexpected failures.Proposed improvement
- assertFails { + assertFailsWith<DiscoveryException> { client.fetchOpenIdConfiguration("https://issuer.example") } - assertFails { + assertFailsWith<DiscoveryException> { client.fetchOpenIdConfiguration("https://issuer.example") }
171-182: Minor: Potential port race condition.The port is released when
ServerSocket(0)is closed, leaving a small window where another process could bind to it before the CIO server starts. This is a common pattern but can occasionally cause flaky tests.Consider using the server's actual port after it starts, if the API supports it.
...server/ktor-server-plugins/ktor-server-auth-jwt/jvm/src/io/ktor/server/auth/jwt/JwkConfig.kt
Show resolved
Hide resolved
ktor-server/ktor-server-plugins/ktor-server-auth-jwt/jvm/src/io/ktor/server/auth/jwt/JWTAuth.kt
Show resolved
Hide resolved
ktor-server/ktor-server-plugins/ktor-server-auth-jwt/jvm/src/io/ktor/server/auth/jwt/JWTAuth.kt
Show resolved
Hide resolved
...r/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/OpenIdConfiguration.kt
Show resolved
Hide resolved
...r/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/OpenIdConfiguration.kt
Show resolved
Hide resolved
| @Serializable | ||
| internal data class JwksKeys(val keys: List<JwksKey>) | ||
|
|
||
| class JwkDiscoveryTest { |
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.
Could you include a test with it working in conjunction with JWT validation?
| * | ||
| * This function will: | ||
| * 1. Set up a JWT verifier with the fetched keys. Keys are cached and rate-limited by default, see [JwkProviderBuilder]. | ||
| * 2. Validate issuer and audience claims automatically. You still need to specify `validate` function before `jwk`. |
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.
You still need to specify
validatefunction beforejwk.
Since this doesn't depend on the result of validate, I think we can mitigate the temporal coupling here by introducing a new field like validateCheck: (JWTCredential) -> String? that can be called before the validate.
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.
Wouldn't it be confusing to have both validateCheck and validate?
Or you mean it to be inside of jwk { }? In that case, I guess it should work
| val issuerMatches = credential.issuer == issuer | ||
| val audienceMatches = config.audience?.let { credential.audience.contains(it) } ?: true | ||
| if (!issuerMatches || !audienceMatches) return@validate null |
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.
Would be nice to include a log message when one of these doesn't match, specifying which field (or both) doesn't match.
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.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-jwt/jvm/src/io/ktor/server/auth/jwt/JWTAuth.kt`:
- Around line 432-434: The error message in JWTAuth.kt referencing
authenticationFunction is misleading—update the exception text thrown in the
require(...) inside the jwk-related logic so it directs users to use the jwk
block for validation (e.g., change "Use jwt { validate { ... } } to fix." to
"Use jwk { validate { ... } } to fix."); locate the require call around
authenticationFunction in the JWTAuth.kt jwk handling and replace only the
message string to reference jwk { validate { ... } }.
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-jwt/jvm/test/io/ktor/server/auth/jwt/JwkDiscoveryTest.kt`:
- Around line 165-176: The current withServer function races on port allocation
because ServerSocket(0).use { it.localPort } closes the socket before
embeddedServer binds; instead reserve the port by creating ServerSocket(0) and
keeping it open until after the server has started, e.g. create val serverSocket
= ServerSocket(0) (store serverSocket.localPort), then start embeddedServer(...)
and only close serverSocket after server.start() (or call serverSocket.close()
in the finally after server.stopSuspend), or replace with Ktor's testApplication
to let the framework manage ports; update withServer to reference ServerSocket,
embeddedServer, server.start and server.stopSuspend accordingly.
🧹 Nitpick comments (1)
ktor-server/ktor-server-plugins/ktor-server-auth-jwt/jvm/src/io/ktor/server/auth/jwt/JWTAuth.kt (1)
391-426: KDoc is improved but could specify@returnand@throwsexplicitly.The documentation is comprehensive with a good usage example. For full compliance with coding guidelines, consider adding explicit
@return Unitand@throws IllegalArgumentExceptionannotations listing the specific conditions (missingvalidate, missingopenIdConfig, or conflictingverifier/validateconfiguration).As per coding guidelines: Document all public APIs including parameters, return types, and exceptions.
Subsystem
Server Auth
Motivation
KTOR-8595 Auth JWK Support (auto-discover)
Connected proposal: KTOR-8596 OpenID Connect (OAuth2) auto-discover & configuration
Solution
Added extension on
HttpClient,jwkdsl (maybe better name?)Open question: json deserialization