Add OpenID Connect discovery metadata#5632
Conversation
|
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 OpenID Connect discovery support to Ktor by adding a new ChangesOpenID Connect Discovery Module
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
|
CodeRabbit (@coderabbitai) review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/test/io/ktor/server/auth/openid/FetchOpenIdProviderMetadataTest.kt (1)
49-229: ⚡ Quick winUse descriptive backticked test names for readability.
Please rename these tests to backticked descriptions to match repository test style.
As per coding guidelines:
**/*Test.{kt,kts}: Prefer descriptive test names in backticks:describe what is being tested.🤖 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/FetchOpenIdProviderMetadataTest.kt` around lines 49 - 229, The test functions (e.g., testFetchOpenIdConfiguration, testFetchOpenIdConfigurationNewSpecFields, testFetchOpenIdConfigurationAllOptionalFieldsNull, testFetchOpenIdConfigurationMissingJwksUri, testFetchOpenIdConfigurationMissingAuthorizationEndpoint, testFetchOpenIdConfigurationMissingTokenEndpoint, testFetchOpenIdConfigurationHttpError) should be renamed to descriptive backticked test names to match project style; update each function declaration from fun testXxx() to fun `describe what is being tested`() while leaving the `@Test` annotation, testApplication block, and the test bodies unchanged so only the identifier becomes a human-readable backticked string (for example: fun `fetches OpenID configuration and parses required fields`() = testApplication { ... }).
🤖 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/OpenIdProviderMetadata.kt`:
- Around line 218-223: In fetchOpenIdMetadata, validate the issuer argument
up-front using require(!issuer.isBlank()) to fail fast before any URL building
or network calls; add an actionable error message that includes the problematic
issuer value (e.g., "issuer must not be blank: '$issuer'") so callers get clear
context; perform this check at the start of the suspend function before using
url.takeFrom or calling get.
- Around line 227-231: Replace the broad "catch (e: Throwable)" in the
OpenIdProviderMetadata.kt discovery code with "catch (e: Exception)" so fatal
JVM errors (e.g., OutOfMemoryError, StackOverflowError) are not accidentally
swallowed; keep the existing separate "catch (e: CancellationException) { throw
e }" branch and continue to wrap non-fatal exceptions in the DiscoveryException
thrown when fetching OpenID configuration from the issuer.
---
Nitpick comments:
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/test/io/ktor/server/auth/openid/FetchOpenIdProviderMetadataTest.kt`:
- Around line 49-229: The test functions (e.g., testFetchOpenIdConfiguration,
testFetchOpenIdConfigurationNewSpecFields,
testFetchOpenIdConfigurationAllOptionalFieldsNull,
testFetchOpenIdConfigurationMissingJwksUri,
testFetchOpenIdConfigurationMissingAuthorizationEndpoint,
testFetchOpenIdConfigurationMissingTokenEndpoint,
testFetchOpenIdConfigurationHttpError) should be renamed to descriptive
backticked test names to match project style; update each function declaration
from fun testXxx() to fun `describe what is being tested`() while leaving the
`@Test` annotation, testApplication block, and the test bodies unchanged so only
the identifier becomes a human-readable backticked string (for example: fun
`fetches OpenID configuration and parses required fields`() = testApplication {
... }).
🪄 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: 8cd7fd3a-66b1-4486-84e0-5fdd58fda305
📒 Files selected for processing (6)
gradle/artifacts/publishJvmAndCommonPublications.txtktor-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/OpenIdProviderMetadata.ktktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/test/io/ktor/server/auth/openid/FetchOpenIdProviderMetadataTest.ktsettings.gradle.kts
Bruce Hamilton (bjhham)
left a comment
Bruce Hamilton (bjhham)
left a comment
There was a problem hiding this comment.
Looks good 👍
Added a few trivial comments.
Co-authored-by: Cursor <cursoragent@cursor.com>
Pantus Oleh (zibet27)
force-pushed
the
zibet27/oidc-discovery
branch
from
2144cda to
45a4e80
Compare
| }.body<OpenIdProviderMetadata>() | ||
| } catch (e: CancellationException) { | ||
| throw e | ||
| } catch (e: Exception) { |
There was a problem hiding this comment.
I'm a bit confused why we wrap exceptions like this?
There was a problem hiding this comment.
To unify exceptions
Afaik, it's a common pattern on JVM, though I'm not a huge fan of it
Bruce Hamilton (@bjhham), Osip Fatkullin (@osipxd) wdyt?
There was a problem hiding this comment.
Afaik, it's a common pattern on JVM
It's also consider a controversial pattern by many, I am personally also not a fan of it.
Here is find it especially confusing since we're turning our own exceptions from Ktor HttpClient into different errors, and the actual cause will get buried in a long stack trace.
4 participants
Subsystem
Server Auth
Motivation
KTOR-5001 Add OpenID Connect (OIDC) support on client and server
Solution
Adds the initial ktor-server-auth-openid module with OpenID Connect Discovery support.
OpenIdProviderMetadatafor decoding/.well-known/openid-configurationHttpClient.fetchOpenIdMetadata(issuer)helperDiscoveryExceptionfor fetch/validation failures