Add OIDC provider lifecycle#5637
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 adds a complete OpenID Connect (OIDC) server authentication plugin to Ktor. The implementation includes provider registration with validation, initial discovery with configurable retry logic, periodic metadata refresh with event publishing, comprehensive configuration models, and full test coverage. ChangesOIDC Plugin Implementation and Tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 |
|
CodeRabbit (@coderabbitai) review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/OidcConfig.kt`:
- Around line 80-87: Update the OidcConfig.validate() function to also validate
discoveryRefreshFailureDelay by requiring
discoveryRefreshFailureDelay.isFinite() and
discoveryRefreshFailureDelay.isPositive(), and update the existing require error
messages for initialDiscoveryAttempts and initialDiscoveryRetryDelay (and the
new discoveryRefreshFailureDelay check) to include the actual invalid value for
easier debugging; reference the validate() method and the
discoveryRefreshFailureDelay property (and
initialDiscoveryAttempts/initialDiscoveryRetryDelay) so the change prevents
immediate retry loops observed in the while(isActive) loop in Oidc.kt.
🪄 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: f1bbf002-437c-4872-934a-f351e229a7fb
📒 Files selected for processing (11)
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/OidcConfig.ktktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/src/io/ktor/server/auth/openid/OidcDiscovery.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/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/OidcPluginRegistrationTest.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
| internal fun validate() { | ||
| require(initialDiscoveryAttempts >= 1) { | ||
| "initialDiscoveryAttempts must be greater than or equal to 1" | ||
| } | ||
| require(initialDiscoveryRetryDelay.isFinite() && !initialDiscoveryRetryDelay.isNegative()) { | ||
| "initialDiscoveryRetryDelay must be finite and non-negative" | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In kotlinx.coroutines, what is the behavior of delay(Duration.ZERO) and delay(negative Duration), and can this create immediate retry loops in while(isActive) polling code?
💡 Result:
In kotlinx.coroutines, the delay function behaves as follows when provided with non-positive durations: - Behavior: If the duration passed to delay is Duration.ZERO or a negative Duration, the function returns immediately without suspending the coroutine [1]. - Cancellation Check: Because the function returns immediately without entering a suspended state, it does not perform a cancellation check [2]. The prompt cancellation guarantee provided by delay only applies when the coroutine actually suspends [2][1]. - Impact on Polling Loops: This behavior can indeed create immediate retry loops in while(isActive) polling code [2]. If a loop relies on delay(duration) to yield control and check for cancellation, passing a non-positive duration causes the loop to execute continuously without suspension. This prevents the coroutine from yielding to other tasks and, crucially, prevents it from noticing that its job has been cancelled, potentially leading to an infinite loop or excessive resource consumption [2]. To ensure proper cancellation and yielding in such loops, you should explicitly check for cancellation (e.g., by using yield or ensuring the loop condition is robust) or ensure that the delay duration is always positive [2].
Citations:
- 1: https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines/delay.html
- 2:
delay(0.seconds)does not check for cancellation Kotlin/kotlinx.coroutines#3373
🏁 Script executed:
# First, let's examine the OidcConfig.kt file to understand the context
wc -l ktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/src/io/ktor/server/auth/openid/OidcConfig.ktRepository: ktorio/ktor
Length of output: 164
🏁 Script executed:
# Read the OidcConfig.kt file to understand the properties and validate() method
cat -n ktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/src/io/ktor/server/auth/openid/OidcConfig.ktRepository: ktorio/ktor
Length of output: 4665
🏁 Script executed:
# Search for discoveryRefreshFailureDelay usage in the codebase
rg "discoveryRefreshFailureDelay" -B 3 -A 3Repository: ktorio/ktor
Length of output: 4331
🏁 Script executed:
# Let's find and examine the Oidc.kt file to see how discoveryRefreshFailureDelay is used
fd "Oidc.kt" -t f | head -5Repository: ktorio/ktor
Length of output: 154
🏁 Script executed:
# Read the Oidc.kt file to see the retry loop context
cat -n ktor-server/ktor-server-plugins/ktor-server-auth-openid/jvm/src/io/ktor/server/auth/openid/Oidc.kt | grep -A 20 -B 5 "while (isActive)"Repository: ktorio/ktor
Length of output: 1370
Add validation for discoveryRefreshFailureDelay to prevent immediate retry loops.
The validate() method does not validate discoveryRefreshFailureDelay. If set to Duration.ZERO or negative, delay() will return immediately without suspending, causing immediate retries in the while(isActive) loop (Oidc.kt) and flooding discovery requests.
Add a require check for discoveryRefreshFailureDelay with isFinite() and isPositive(), and improve all error messages to include the actual value for better debugging.
Suggested fix
internal fun validate() {
require(initialDiscoveryAttempts >= 1) {
- "initialDiscoveryAttempts must be greater than or equal to 1"
+ "initialDiscoveryAttempts must be greater than or equal to 1, but was $initialDiscoveryAttempts"
+ }
+ require(discoveryRefreshFailureDelay.isFinite() && discoveryRefreshFailureDelay.isPositive()) {
+ "discoveryRefreshFailureDelay must be finite and positive, but was $discoveryRefreshFailureDelay"
}
require(initialDiscoveryRetryDelay.isFinite() && !initialDiscoveryRetryDelay.isNegative()) {
- "initialDiscoveryRetryDelay must be finite and non-negative"
+ "initialDiscoveryRetryDelay must be finite and non-negative, but was $initialDiscoveryRetryDelay"
}
}📝 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.
| internal fun validate() { | |
| require(initialDiscoveryAttempts >= 1) { | |
| "initialDiscoveryAttempts must be greater than or equal to 1" | |
| } | |
| require(initialDiscoveryRetryDelay.isFinite() && !initialDiscoveryRetryDelay.isNegative()) { | |
| "initialDiscoveryRetryDelay must be finite and non-negative" | |
| } | |
| } | |
| internal fun validate() { | |
| require(initialDiscoveryAttempts >= 1) { | |
| "initialDiscoveryAttempts must be greater than or equal to 1, but was $initialDiscoveryAttempts" | |
| } | |
| require(discoveryRefreshFailureDelay.isFinite() && discoveryRefreshFailureDelay.isPositive()) { | |
| "discoveryRefreshFailureDelay must be finite and positive, but was $discoveryRefreshFailureDelay" | |
| } | |
| require(initialDiscoveryRetryDelay.isFinite() && !initialDiscoveryRetryDelay.isNegative()) { | |
| "initialDiscoveryRetryDelay must be finite and non-negative, but was $initialDiscoveryRetryDelay" | |
| } | |
| } |
🤖 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 80 - 87, Update the OidcConfig.validate() function to also validate
discoveryRefreshFailureDelay by requiring
discoveryRefreshFailureDelay.isFinite() and
discoveryRefreshFailureDelay.isPositive(), and update the existing require error
messages for initialDiscoveryAttempts and initialDiscoveryRetryDelay (and the
new discoveryRefreshFailureDelay check) to include the actual invalid value for
easier debugging; reference the validate() method and the
discoveryRefreshFailureDelay property (and
initialDiscoveryAttempts/initialDiscoveryRetryDelay) so the change prevents
immediate retry loops observed in the while(isActive) loop in Oidc.kt.
# Conflicts: # ktor-server/ktor-server-plugins/ktor-server-auth-oidc/api/ktor-server-auth-oidc.api # ktor-server/ktor-server-plugins/ktor-server-auth-oidc/jvm/test/io/ktor/server/auth/oidc/FetchOpenIdProviderMetadataTest.kt
6767cec to
8faf94e
Compare
Subsystem
Server Auth
Motivation
KTOR-5001 Add OpenID Connect (OIDC) support on client and server
Solution
Adds the base OpenID Connect plugin lifecycle, but no authentication flows yet.
This introduces
openIdConnectOidcProvider.currentMetadata()for consumers that need the active provider metadataAuthentication, bearer token validation, OAuth callback handling, UserInfo, PKCE, sessions/logout, introspection, and resource metadata stay in later PRs.