Add OIDC OAuth callback and UserInfo#5641
Open
Pantus Oleh (zibet27) wants to merge 1 commit into
Open
Conversation
Pantus Oleh (zibet27)
requested review from
Bruce Hamilton (bjhham),
Leonid Stashevsky (e5l) and
Simon Vergauwen (nomisRev)
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 extends Ktor's OpenID Connect plugin with OAuth 2.0 login flow support, enabling authentication via OAuth callbacks alongside existing Bearer token validation. New public configuration APIs define OAuth credentials and callbacks; internal transaction storage manages authorization state. The implementation includes OAuth flow creation, login/callback routing, enhanced token verification with optional UserInfo fetching, and comprehensive test coverage. ChangesOIDC OAuth 2.0 Login Flow Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: 5
🤖 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/OidcAuthorizationTransactionStore.kt`:
- Around line 29-41: entries is currently keyed only by authorizationSessionId
causing parallel logins to clobber each other; modify the store to key by both
session and state (e.g., use a composite key string
"${authorizationSessionId}:$state" or change entries to Map<String, Map<String,
Entry>>) and update all related methods (put, lookup/get, remove/pruneExpired)
to use the new keying strategy so each OidcAuthorizationTransaction is preserved
per (authorizationSessionId, state) pair; ensure Entry,
put(authorizationSessionId, state, transaction), pruneExpired(), and any
retrieval/remove logic refer to the composite/nested structure consistently.
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/src/io/ktor/server/auth/openid/OidcConfig.kt`:
- Around line 451-457: The validate() function currently only checks that
clientId and clientSecret are initialized, allowing empty strings; update
validate() (in OidcConfig) to first require(::clientId.isInitialized) and
::clientSecret.isInitialized as it does, then additionally require that
clientId.isNotBlank() and clientSecret.isNotBlank(), e.g. add
require(clientId.isNotBlank()) { "clientId must be configured and non-blank" }
and require(clientSecret.isNotBlank()) { "clientSecret must be configured and
non-blank" } so blank values fail early during configuration.
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/src/io/ktor/server/auth/openid/OidcOAuth.kt`:
- Around line 58-74: The try block currently wraps the entire authentication
flow including oauthConfig.onSuccess, causing exceptions from the success
handler to be treated as auth failures; move the call to
oauthConfig.onSuccess(this, typedPrincipal) out of the try/catch so that only
the provider/validation/creation steps
(provider.validateAuthorizationResponseIssuer,
provider.consumeAuthorizationTransaction, provider.buildOAuthPrincipal,
provider.transformPrincipal) are guarded; keep the catch handlers to call
oauthConfig.onFailure only for exceptions from those guarded steps, and allow
exceptions thrown by oauthConfig.onSuccess to propagate (or handle them
separately) to avoid converting real handler errors into 401s or causing a
second response write.
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/src/io/ktor/server/auth/openid/OidcTokens.kt`:
- Around line 173-180: The call to fetchUserInfo currently passes
idTokenAudience as the audience used to validate a signed UserInfo JWT, which is
wrong for UserInfo responses; update the call sites (including the other
occurrences around the 193–216 block) to pass oauthConfig.clientId as the
expected audience instead of idTokenAudience (i.e., replace expectedAudience =
idTokenAudience with expectedAudience = oauthConfig.clientId), keeping the other
parameters (userInfoEndpoint, accessToken, idTokenSubject, metadata,
jwkProvider) unchanged so UserInfo JWT audience is validated against the OAuth
client ID.
- Around line 83-88: The access-token-only branch currently skips validating the
token_type; update the branch that returns
requireNotNull(verifyAccessToken(response.accessToken)) to first assert
response.tokenType (or response.token_type) equals "Bearer" (same check used in
the id_token path) and fail with a clear message if not, and only then call
verifyAccessToken; reference config.accessTokenConfig,
response.tokenType/response.token_type, verifyAccessToken, and
OidcPrincipal.AccessToken so the check is placed where the access-token-only
flow returns the AccessToken principal.
🪄 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: c3410495-35e0-450b-a66c-e7968230bd2e
📒 Files selected for processing (19)
ktor-server/ktor-server-plugins/ktor-server-auth-openid/api/ktor-server-auth-openid.apiktor-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/OidcAuthorizationTransactionStore.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/OidcOAuth.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/OidcAuthorizationTransactionStoreTest.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/OidcEnvironmentConfigTest.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/OidcOAuthAccessTokenFallbackTest.ktktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/test/io/ktor/server/auth/openid/OidcOAuthCallbackSecurityTest.ktktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/test/io/ktor/server/auth/openid/OidcOAuthCallbackTest.ktktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/test/io/ktor/server/auth/openid/OidcUserInfoTest.ktktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/test/io/ktor/server/auth/openid/utils/OidcTestOAuthFlow.ktktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/test/io/ktor/server/auth/openid/utils/OidcTestProviders.ktktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/test/io/ktor/server/auth/openid/utils/OidcTestUtils.kt
Comment on lines
+451
to
+457
| internal fun validate(accessTokenAllowed: Boolean) { | ||
| require(::clientId.isInitialized) { | ||
| "clientId must be configured" | ||
| } | ||
| require(::clientSecret.isInitialized) { | ||
| "clientSecret must be configured" | ||
| } |
Contributor
There was a problem hiding this comment.
Reject blank OAuth client credentials.
validate() only checks initialization here, so clientId = "" or clientSecret = "" still passes and then fails later during the redirect/token exchange. Please require both values to be non-blank at config time.
Suggested fix
internal fun validate(accessTokenAllowed: Boolean) {
- require(::clientId.isInitialized) {
- "clientId must be configured"
+ require(::clientId.isInitialized && clientId.isNotBlank()) {
+ "clientId must be configured and non-blank for provider $providerName"
}
- require(::clientSecret.isInitialized) {
- "clientSecret must be configured"
+ require(::clientSecret.isInitialized && clientSecret.isNotBlank()) {
+ "clientSecret must be configured and non-blank for provider $providerName"
}🤖 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/OidcConfig.kt`
around lines 451 - 457, The validate() function currently only checks that
clientId and clientSecret are initialized, allowing empty strings; update
validate() (in OidcConfig) to first require(::clientId.isInitialized) and
::clientSecret.isInitialized as it does, then additionally require that
clientId.isNotBlank() and clientSecret.isNotBlank(), e.g. add
require(clientId.isNotBlank()) { "clientId must be configured and non-blank" }
and require(clientSecret.isNotBlank()) { "clientSecret must be configured and
non-blank" } so blank values fail early during configuration.
Comment on lines
+173
to
+180
| fetchUserInfo( | ||
| endpoint = userInfoEndpoint, | ||
| accessToken = accessToken, | ||
| expectedSubject = idTokenSubject, | ||
| expectedAudience = expectedAudience, | ||
| metadata = metadata, | ||
| jwkProvider = jwkProvider, | ||
| ) |
Contributor
There was a problem hiding this comment.
Use clientId for signed UserInfo audience validation.
idTokenAudience is documented as ID-token-specific, but the same value is passed into fetchUserInfo() and then used to verify the signed UserInfo JWT audience. With fetchUserInfo = true, overriding idTokenAudience can reject an otherwise valid UserInfo response addressed to the OAuth client. Validate UserInfo against oauthConfig.clientId instead.
Also applies to: 193-216
🤖 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/OidcTokens.kt`
around lines 173 - 180, The call to fetchUserInfo currently passes
idTokenAudience as the audience used to validate a signed UserInfo JWT, which is
wrong for UserInfo responses; update the call sites (including the other
occurrences around the 193–216 block) to pass oauthConfig.clientId as the
expected audience instead of idTokenAudience (i.e., replace expectedAudience =
idTokenAudience with expectedAudience = oauthConfig.clientId), keeping the other
parameters (userInfoEndpoint, accessToken, idTokenSubject, metadata,
jwkProvider) unchanged so UserInfo JWT audience is validated against the OAuth
client ID.
Pantus Oleh (zibet27)
force-pushed
the
zibet27/oidc-token-validation-bearer
branch
2 times, most recently
from
3763385 to
6e1c5f4
Compare
Pantus Oleh (zibet27)
force-pushed
the
zibet27/oidc-oauth-callback-userinfo
branch
2 times, most recently
from
efcebf4 to
54e5f6f
Compare
Co-authored-by: Cursor <cursoragent@cursor.com>
Pantus Oleh (zibet27)
force-pushed
the
zibet27/oidc-oauth-callback-userinfo
branch
from
54e5f6f to
dc16069
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 OIDC
Motivation
KTOR-5001 Add OpenID Connect (OIDC) support on client and server
Solution
Adds the OIDC browser login/callback flow and UserInfo support on top of provider lifecycle and token validation.
This introduces
UserInfofetching, and signed UserInfo validationPKCE, persistent sessions/logout, token introspection, resource indicators, and protected resource metadata stay in later PRs.