Add OIDC token validation and Bearer auth#5639
Open
Pantus Oleh (zibet27) wants to merge 1 commit into
Open
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces strongly-typed OpenID Connect authentication to Ktor by adding generic principal support, bearer JWT verification with JWK management, and comprehensive configuration for token validation and caching. ChangesTyped OIDC Principal and Bearer Auth
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Collaborator
Author
|
CodeRabbit (@coderabbitai) review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/src/io/ktor/server/auth/openid/OidcProvider.kt (1)
60-92: 💤 Low valueConsider a more descriptive error message.
Line 88's error message
"Invalid principal type."doesn't include the expected vs actual type, which would help debugging.Suggested improvement
check(config.principalType.isInstance(principal)) { - "Invalid principal type." + "Principal type ${principal::class.simpleName} is not assignable to ${config.principalType.simpleName}" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/src/io/ktor/server/auth/openid/OidcProvider.kt` around lines 60 - 92, Update the error message in transformPrincipal (in OidcProvider.transformPrincipal) to include the expected principal type from config.principalType and the actual runtime type of principal when throwing; replace the current check(config.principalType.isInstance(principal)) { "Invalid principal type." } with a message that interpolates config.principalType (or its name) and principal::class (or principal.javaClass) so the thrown IllegalStateException shows both expected and actual types to aid debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/src/io/ktor/server/auth/openid/OidcBearer.kt`:
- Around line 40-42: The trace log inside the onFailure block in OidcBearer.kt
currently only logs the provider name; update the logger.trace call in the
onFailure { ... } handler to include the caught exception (it) so the exception
message/stacktrace is recorded (use the logger.trace overload that accepts a
Throwable or include the throwable in the log arguments) for better debugging of
token validation failures.
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/test/io/ktor/server/auth/openid/OidcJwkProviderConfigTest.kt`:
- Around line 169-171: Replace the TOCTOU port allocation that uses
ServerSocket(0) and then binds embeddedServer to that port; instead start
embeddedServer with port = 0 and after starting call server.start() (or use
server.start(false) as appropriate) then read the actual bound port via
server.engine.resolvedConnectors().first().port; update both places that create
the test server (the blocks around the ServerSocket usage in
OidcJwkProviderConfigTest where embeddedServer(...) is constructed) to use port
= 0 and to retrieve the bound port from
server.engine.resolvedConnectors().first().port after server start.
---
Nitpick comments:
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/src/io/ktor/server/auth/openid/OidcProvider.kt`:
- Around line 60-92: Update the error message in transformPrincipal (in
OidcProvider.transformPrincipal) to include the expected principal type from
config.principalType and the actual runtime type of principal when throwing;
replace the current check(config.principalType.isInstance(principal)) { "Invalid
principal type." } with a message that interpolates config.principalType (or its
name) and principal::class (or principal.javaClass) so the thrown
IllegalStateException shows both expected and actual types to aid debugging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 578ef392-02a9-4b0f-bacb-90e4d70811f7
📒 Files selected for processing (18)
ktor-server/ktor-server-plugins/ktor-server-auth-openid/api/ktor-server-auth-openid.apiktor-server/ktor-server-plugins/ktor-server-auth-openid/build.gradle.ktsktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/src/io/ktor/server/auth/openid/Oidc.ktktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/src/io/ktor/server/auth/openid/OidcAuthenticatedContext.ktktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/src/io/ktor/server/auth/openid/OidcBearer.ktktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/src/io/ktor/server/auth/openid/OidcConfig.ktktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/src/io/ktor/server/auth/openid/OidcPrincipal.ktktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/src/io/ktor/server/auth/openid/OidcProvider.ktktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/src/io/ktor/server/auth/openid/OidcTokens.ktktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/test/io/ktor/server/auth/openid/OidcBearerJwtTest.ktktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/test/io/ktor/server/auth/openid/OidcConfigValidationTest.ktktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/test/io/ktor/server/auth/openid/OidcDiscoveryTest.ktktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/test/io/ktor/server/auth/openid/OidcEnvironmentConfigTest.ktktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/test/io/ktor/server/auth/openid/OidcJwkProviderConfigTest.ktktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/test/io/ktor/server/auth/openid/OidcJwtKeyAndAlgorithmTest.ktktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/test/io/ktor/server/auth/openid/OidcPluginRegistrationTest.ktktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/test/io/ktor/server/auth/openid/utils/OidcTestKeys.ktktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/test/io/ktor/server/auth/openid/utils/OidcTestLogging.kt
Comment on lines
+40
to
+42
| }.onFailure { | ||
| logger.trace("OpenID access token authentication failed for provider {}", name) | ||
| }.getOrNull() |
Contributor
There was a problem hiding this comment.
Include the exception in trace log for debugging.
The onFailure block has access to the exception (it) but it's not logged. Without the exception details, debugging token validation failures (expired token, wrong issuer, invalid signature, etc.) becomes difficult.
🔧 Proposed fix
}.onFailure {
- logger.trace("OpenID access token authentication failed for provider {}", name)
+ logger.trace("OpenID access token authentication failed for provider {}", name, it)
}.getOrNull()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| }.onFailure { | |
| logger.trace("OpenID access token authentication failed for provider {}", name) | |
| }.getOrNull() | |
| }.onFailure { | |
| logger.trace("OpenID access token authentication failed for provider {}", name, it) | |
| }.getOrNull() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/src/io/ktor/server/auth/openid/OidcBearer.kt`
around lines 40 - 42, The trace log inside the onFailure block in OidcBearer.kt
currently only logs the provider name; update the logger.trace call in the
onFailure { ... } handler to include the caught exception (it) so the exception
message/stacktrace is recorded (use the logger.trace overload that accepts a
Throwable or include the throwable in the log arguments) for better debugging of
token validation failures.
Comment on lines
+169
to
+171
| val port = ServerSocket(0).use { it.localPort } | ||
| val server = embeddedServer(CIO, port = port) { | ||
| routing { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify current TOCTOU pattern and discover established connector-access API usage in repo.
rg -n --type=kt -C2 'ServerSocket\(0\)\.use \{ it\.localPort \}'
rg -n --type=kt -C2 'resolvedConnectors'
rg -n --type=kt -C2 'embeddedServer\([^)]*port\s*=\s*0'Repository: ktorio/ktor
Length of output: 82
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the test file to verify the current code at lines 169-171 and 184
echo "=== Current test file content (lines 160-200) ==="
sed -n '160,200p' ktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/test/io/ktor/server/auth/openid/OidcJwkProviderConfigTest.kt
echo ""
echo "=== Searching for resolvedConnectors usage ==="
rg -n 'resolvedConnectors' --glob='*.kt' -A2 -B2
echo ""
echo "=== Searching for ServerSocket(0) pattern ==="
rg -n 'ServerSocket\(0\)' --glob='*.kt' -A2 -B2
echo ""
echo "=== Searching for embeddedServer with port = 0 pattern ==="
rg -n 'embeddedServer.*port\s*=\s*0' --glob='*.kt' -A5 -B2Repository: ktorio/ktor
Length of output: 34956
Fix TOCTOU port allocation in test server setup.
Using ServerSocket(0) to allocate a port, then binding embeddedServer to that port, creates a race condition where another process can claim the port between release and bind, causing intermittent test failures. Instead, bind embeddedServer directly to port = 0, then retrieve the actual bound port from server.engine.resolvedConnectors().first().port after the server starts.
Suggested change
- val port = ServerSocket(0).use { it.localPort }
- val server = embeddedServer(CIO, port = port) {
+ val server = embeddedServer(CIO, port = 0) {
routing {
get("/jwks") {
fetchCount.incrementAndGet()
call.respondText(
text = keys.jwksJson(),
status = HttpStatusCode.OK,
contentType = ContentType.Application.Json,
)
}
}
}.start(wait = false)
+ val port = server.engine.resolvedConnectors().first().portApply to both occurrences at lines 169–171 and 184.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| val port = ServerSocket(0).use { it.localPort } | |
| val server = embeddedServer(CIO, port = port) { | |
| routing { | |
| val server = embeddedServer(CIO, port = 0) { | |
| routing { | |
| get("/jwks") { | |
| fetchCount.incrementAndGet() | |
| call.respondText( | |
| text = keys.jwksJson(), | |
| status = HttpStatusCode.OK, | |
| contentType = ContentType.Application.Json, | |
| ) | |
| } | |
| } | |
| }.start(wait = false) | |
| val port = server.engine.resolvedConnectors().first().port |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/test/io/ktor/server/auth/openid/OidcJwkProviderConfigTest.kt`
around lines 169 - 171, Replace the TOCTOU port allocation that uses
ServerSocket(0) and then binds embeddedServer to that port; instead start
embeddedServer with port = 0 and after starting call server.start() (or use
server.start(false) as appropriate) then read the actual bound port via
server.engine.resolvedConnectors().first().port; update both places that create
the test server (the blocks around the ServerSocket usage in
OidcJwkProviderConfigTest where embeddedServer(...) is constructed) to use port
= 0 and to retrieve the bound port from
server.engine.resolvedConnectors().first().port after server start.
Pantus Oleh (zibet27)
force-pushed
the
zibet27/oidc-provider-lifecycle
branch
from
6767cec to
8faf94e
Compare
Pantus Oleh (zibet27)
force-pushed
the
zibet27/oidc-token-validation-bearer
branch
from
cdfd0df to
3763385
Compare
Pantus Oleh (zibet27)
force-pushed
the
zibet27/oidc-token-validation-bearer
branch
from
3763385 to
6e1c5f4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Subsystem
Server Auth
Motivation
KTOR-5001 Add OpenID Connect (OIDC) support on client and server
Solution
Adds JWT-based OIDC access-token validation and typed Bearer authentication on top of the provider lifecycle.
This introduces
OAuth login/callback handling, UserInfo, PKCE, sessions/logout, introspection, resource indicators, and protected resource metadata stay in later PRs.