OAuth2 auto-configuration #5341
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
WalkthroughThe PR extends OpenID Connect support in Ktor's authentication system by adding new properties to OpenIdConfiguration, introducing a new OpenIdOAuthConfig class, and providing new OAuth configuration overloads that accept OpenID configuration for streamlined OpenID-based OAuth flows. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@ktor-server/ktor-server-plugins/ktor-server-auth/api/ktor-server-auth.api`:
- Around line 488-493: The two 4-arg Kotlin overloads
oauth(AuthenticationConfig, String, OpenIdConfiguration, Function1) and
oauth(AuthenticationConfig, String, String, Function1) cause Java-call ambiguity
when the third parameter is null; annotate the Kotlin overloads with distinct
JVM names (e.g., add `@JvmName`("oauthWithOpenId") to the OpenIdConfiguration
variant and `@JvmName`("oauthWithClientId") to the String-variant) so the
generated methods have unique JVM signatures and Java callers can unambiguously
invoke the desired overload; apply the annotations to the corresponding oauth
functions (and their default dispatchers if necessary) so the compiled API
exposes disambiguated method names.
In
`@ktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/OAuthProcedure.kt`:
- Around line 193-209: The oauth wrapper leaves
OAuthAuthenticationProvider.Config.client uninitialized when
OpenIdOAuthConfig.client is null, causing UninitializedPropertyAccessException;
fix by ensuring a default HttpClient is provided: in AuthenticationConfig.oauth
(the function in OAuthProcedure.kt) assign this.client = openIdConfig.client ?:
HttpClient() (or alternatively make OpenIdOAuthConfig initialize client to
HttpClient() by default) so the provider's lateinit client is always set before
build(); reference symbols: AuthenticationConfig.oauth, OpenIdOAuthConfig,
OAuthAuthenticationProvider.Config.client.
In
`@ktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/OpenIdOAuthConfig.kt`:
- Around line 63-72: The KDoc for OpenIdOAuthConfig.defaultScopes says it should
default to the intersection of ["openid","profile","email"] with
OpenIdConfiguration.scopesSupported, but the code assigns
openIdConfiguration.scopesSupported directly; update the implementation where
`scopes` is computed (the `scopes = defaultScopes ?:
openIdConfiguration.scopesSupported ?: listOf("openid")` logic) to instead
compute `defaultScopes ?:
(openIdConfiguration.scopesSupported?.intersect(listOf("openid","profile","email"))?.toList())
?: listOf("openid")` so the effective scopes match the documented intersection
with `OpenIdConfiguration.scopesSupported`; alternatively, if you prefer to keep
current behavior, update the KDoc for `defaultScopes` to describe that it uses
`scopesSupported` directly and falls back to ["openid"].
🧹 Nitpick comments (3)
ktor-server/ktor-server-plugins/ktor-server-auth/jvm/test/io/ktor/tests/auth/FetchOpenIdConfigurationTest.kt (1)
121-138: HTTP error handling test is adequate.The test verifies that a 404 response results in a
DiscoveryException. Consider adding a test for network failures or malformed JSON responses for more comprehensive coverage.ktor-server/ktor-server-plugins/ktor-server-auth/common/test/io/ktor/tests/auth/OpenIdOAuthTest.kt (1)
184-205: Validation tests for required configuration fields.These tests ensure that
IllegalArgumentExceptionis thrown when required fields (clientId,clientSecret,urlProvider) are missing. The error messages clearly indicate which field is missing.Note: The
assertFailsWithmessage parameter (e.g.,"clientId must be specified") is a test failure message, not a matcher for the exception message. If you intend to verify the exception message content, useassertTrue(exception.message!!.contains(...))pattern as inFetchOpenIdConfigurationTest.📝 Alternative: Verify exception message content
`@Test` fun testOAuthWithOpenIdConfigurationMissingClientId() { val openIdConfig = OpenIdConfiguration(...) val exception = assertFailsWith<IllegalArgumentException> { testApplication { install(Authentication) { oauth("test-openid", openIdConfig) { client = this@testApplication.client clientSecret = "test-client-secret" urlProvider = { "http://localhost/callback" } } } } } assertTrue(exception.message!!.contains("clientId")) }Also applies to: 207-241
ktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/OpenIdConfiguration.kt (1)
108-130: Validation logic ensures required endpoints are present.The validation checks for blank values rather than just null, which is appropriate since the JSON might contain empty strings. Consider reordering validations to match the property declaration order for consistency (authorizationEndpoint → tokenEndpoint → jwksUri).
♻️ Optional: Reorder validations to match property order
- if (config.jwksUri.isBlank()) { - throw DiscoveryException("OpenID configuration from $issuer is missing jwks_uri") - } if (config.authorizationEndpoint.isBlank()) { throw DiscoveryException("OpenID configuration from $issuer is missing authorization_endpoint") } if (config.tokenEndpoint.isBlank()) { throw DiscoveryException("OpenID configuration from $issuer is missing token_endpoint") } + if (config.jwksUri.isBlank()) { + throw DiscoveryException("OpenID configuration from $issuer is missing jwks_uri") + } return config
| public static final fun oauth (Lio/ktor/server/auth/AuthenticationConfig;Ljava/lang/String;Lio/ktor/server/auth/OpenIdConfiguration;Lkotlin/jvm/functions/Function1;)V | ||
| public static final fun oauth (Lio/ktor/server/auth/AuthenticationConfig;Ljava/lang/String;Ljava/lang/String;Lio/ktor/server/auth/OpenIdConfiguration;Lkotlin/jvm/functions/Function1;)V | ||
| public static final fun oauth (Lio/ktor/server/auth/AuthenticationConfig;Ljava/lang/String;Ljava/lang/String;Lkotlin/jvm/functions/Function1;)V | ||
| public static final fun oauth (Lio/ktor/server/auth/AuthenticationConfig;Ljava/lang/String;Lkotlin/jvm/functions/Function1;)V | ||
| public static synthetic fun oauth$default (Lio/ktor/server/auth/AuthenticationConfig;Ljava/lang/String;Lio/ktor/server/auth/OpenIdConfiguration;Lkotlin/jvm/functions/Function1;ILjava/lang/Object;)V | ||
| public static synthetic fun oauth$default (Lio/ktor/server/auth/AuthenticationConfig;Ljava/lang/String;Ljava/lang/String;Lio/ktor/server/auth/OpenIdConfiguration;Lkotlin/jvm/functions/Function1;ILjava/lang/Object;)V |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the OAuth-related source files
fd -e kt -t f oauth | grep -E "(auth|oauth)" | head -20Repository: ktorio/ktor
Length of output: 1041
🏁 Script executed:
# Search for oauth function definitions
rg "fun.*oauth" --type kt -A 5 | head -100Repository: ktorio/ktor
Length of output: 81
🏁 Script executed:
# Look for the main oauth implementation in ktor-server-auth
find . -path "*/ktor-server-auth/*" -name "*.kt" -type f | head -20Repository: ktorio/ktor
Length of output: 2083
🏁 Script executed:
cat ./ktor-server/ktor-server-plugins/ktor-server-auth/jvm/src/io/ktor/server/auth/OAuth.ktRepository: ktorio/ktor
Length of output: 2486
🏁 Script executed:
cat ./ktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/OpenIdConfiguration.ktRepository: ktorio/ktor
Length of output: 5695
🏁 Script executed:
# Search for oauth function definitions - extension functions
rg "fun.*oauth\(" --type-add 'kotlin:*.kt' -t kotlin | head -50Repository: ktorio/ktor
Length of output: 602
🏁 Script executed:
# Check OAuth2.kt which likely contains the oauth extension functions
cat ./ktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/OAuth2.kt | head -200Repository: ktorio/ktor
Length of output: 6610
🏁 Script executed:
cat ./ktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/OAuthProcedure.ktRepository: ktorio/ktor
Length of output: 11535
🏁 Script executed:
# Check if there are any `@JvmName` annotations on the oauth functions
rg "@JvmName" ./ktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/OAuthProcedure.ktRepository: ktorio/ktor
Length of output: 37
🏁 Script executed:
# Look for Java test files that might use the oauth functions
find ./ktor-server/ktor-server-plugins/ktor-server-auth -name "*.java" -type fRepository: ktorio/ktor
Length of output: 37
🏁 Script executed:
# Check if there's an actual overload ambiguity by creating a test case
cat > /tmp/test_ambiguity.kt << 'EOF'
// Simulating the overloads from the actual code
class TestAmbiguity {
interface Config
interface OpenIdOAuthConfig
data class OpenIdConfiguration(val issuer: String)
fun oauth(
config: Any,
name: String? = null,
configure: Config.() -> Unit
) {}
fun oauth(
config: Any,
name: String? = null,
description: String? = null,
configure: Config.() -> Unit
) {}
fun oauth(
config: Any,
name: String? = null,
openIdConfiguration: OpenIdConfiguration,
configure: OpenIdOAuthConfig.() -> Unit
) {}
fun oauth(
config: Any,
name: String? = null,
description: String? = null,
openIdConfiguration: OpenIdConfiguration,
configure: OpenIdOAuthConfig.() -> Unit
) {}
fun test() {
// Test calls
oauth(Any(), "name") {} // Should match first overload
oauth(Any(), "name", null) {} // POTENTIALLY AMBIGUOUS - String or OpenIdConfiguration?
oauth(Any(), "name", null as String?) {} // Should match second overload
oauth(Any(), "name", OpenIdConfiguration("issuer")) {} // Should match third overload
}
}
EOF
cat /tmp/test_ambiguity.ktRepository: ktorio/ktor
Length of output: 1283
🏁 Script executed:
# Let's check the .api file to see what synthetic defaults were generated
grep -A 20 "oauth\$default" ./ktor-server/ktor-server-plugins/ktor-server-auth/api/ktor-server-auth.api | head -30Repository: ktorio/ktor
Length of output: 2122
🏁 Script executed:
# Let's look at the actual parameter types more carefully - check if OpenIdConfiguration is nullable
cat ./ktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/OAuthProcedure.kt | grep -A 10 "fun AuthenticationConfig.oauth"Repository: ktorio/ktor
Length of output: 1700
🏁 Script executed:
# Also check the `@api` file more carefully around the oauth definitions to see full signatures
grep -B 5 -A 10 "fun oauth" ./ktor-server/ktor-server-plugins/ktor-server-auth/api/ktor-server-auth.api | head -50Repository: ktorio/ktor
Length of output: 2176
🏁 Script executed:
# Let's see the exact lines around 488-493 in the api file
sed -n '480,500p' ./ktor-server/ktor-server-plugins/ktor-server-auth/api/ktor-server-auth.apiRepository: ktorio/ktor
Length of output: 2130
🏁 Script executed:
# Let's create a test to see if Kotlin actually allows calling with null and which overload gets picked
cat > /tmp/test_kotlin_overload.kt << 'EOF'
data class OpenIdConfiguration(val issuer: String)
fun test(
name: String? = null,
openIdConfiguration: OpenIdConfiguration,
configure: () -> Unit
) {
println("OpenIdConfiguration overload called")
}
fun test(
name: String? = null,
description: String? = null,
configure: () -> Unit
) {
println("String overload called")
}
fun main() {
// This should be unambiguous because the 3rd parameter type is different
test("name", OpenIdConfiguration("issuer")) {}
// Can we call with null?
// test("name", null) {} // This would be ambiguous!
}
EOF
cat /tmp/test_kotlin_overload.ktRepository: ktorio/ktor
Length of output: 648
🏁 Script executed:
# Check if there are any existing tests for the oauth functions
find ./ktor-server/ktor-server-plugins/ktor-server-auth -name "*Test.kt" -o -name "*Test.java" | xargs grep -l "oauth" 2>/dev/null | head -5Repository: ktorio/ktor
Length of output: 345
🏁 Script executed:
# Look at one of the test files to see how oauth is actually called
cat ./ktor-server/ktor-server-plugins/ktor-server-auth/common/test/io/ktor/tests/auth/OpenIdOAuthTest.kt | head -100Repository: ktorio/ktor
Length of output: 3800
🏁 Script executed:
# Check if the oauth functions in OAuthProcedure.kt have any `@JvmName` annotations
grep -B 2 "fun AuthenticationConfig.oauth" ./ktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/OAuthProcedure.kt | grep -E "@JvmName|fun AuthenticationConfig"Repository: ktorio/ktor
Length of output: 210
🏁 Script executed:
# Let's verify if Java can actually call these functions with null - check for Java test files
find ./ktor-server/ktor-server-plugins/ktor-server-auth -name "*Test.java" -type fRepository: ktorio/ktor
Length of output: 37
🏁 Script executed:
# Check the exact signature by looking at what parameters are required
sed -n '56,170p' ./ktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/OAuthProcedure.ktRepository: ktorio/ktor
Length of output: 5233
🏁 Script executed:
# Let's verify the actual overload ambiguity by checking what Java sees
# Look at the complete function signatures with their parameter counts
cat ./ktor-server/ktor-server-plugins/ktor-server-auth/api/ktor-server-auth.api | grep -A 2 "OAuthProcedureKt"Repository: ktorio/ktor
Length of output: 338
🏁 Script executed:
# Test if Kotlin compiler would accept a call with null for the 3rd parameter
cat > /tmp/test_null_ambiguity.kt << 'EOF'
package test
data class OpenIdConfiguration(val issuer: String)
interface AuthenticationConfig
fun AuthenticationConfig.oauth(
name: String? = null,
openIdConfiguration: OpenIdConfiguration,
configure: (Any) -> Unit
) {
println("OpenId overload")
}
fun AuthenticationConfig.oauth(
name: String? = null,
description: String? = null,
configure: (Any) -> Unit
) {
println("String overload")
}
fun main() {
val config: AuthenticationConfig = object : AuthenticationConfig {}
// Test 1: These should be unambiguous
config.oauth("name", OpenIdConfiguration("issuer")) {} // Clear - OpenId overload
config.oauth("name", "desc") {} // Clear - String overload
// Test 2: What about null?
// config.oauth("name", null) {} // Would this compile?
}
EOF
python3 << 'PYEOF'
# Since we can't actually compile Kotlin, let's reason through the JVM signatures
# and what Java would see
api_lines = [
"oauth (Lio/ktor/server/auth/AuthenticationConfig;Ljava/lang/String;Lio/ktor/server/auth/OpenIdConfiguration;Lkotlin/jvm/functions/Function1;)V",
"oauth (Lio/ktor/server/auth/AuthenticationConfig;Ljava/lang/String;Ljava/lang/String;Lio/ktor/server/auth/OpenIdConfiguration;Lkotlin/jvm/functions/Function1;)V",
"oauth (Lio/ktor/server/auth/AuthenticationConfig;Ljava/lang/String;Ljava/lang/String;Lkotlin/jvm/functions/Function1;)V",
"oauth (Lio/ktor/server/auth/AuthenticationConfig;Ljava/lang/String;Lkotlin/jvm/functions/Function1;)V",
]
print("JVM Signatures for oauth:")
print("=" * 80)
for i, sig in enumerate(api_lines, 1):
# Parse parameter count
params = sig.count("L")
print(f"{i}. Parameters: {params}")
print(f" {sig}")
print()
print("\nAnalysis:")
print("-" * 80)
print("In Java, a call oauth(config, \"name\", null, function) has 4 arguments")
print("\nCan match:")
print(" - Signature 1 (4 params): oauth(..., String, OpenIdConfiguration, Function1)")
print(" - Signature 3 (4 params): oauth(..., String, String, Function1)")
print("\nBoth accept null as 3rd parameter:")
print(" - OpenIdConfiguration (nullable)")
print(" - String (nullable)")
print("\n⚠️ This IS ambiguous in Java!")
PYEOFRepository: ktorio/ktor
Length of output: 1215
Java overload ambiguity with 4-argument oauth calls.
The 4-parameter overloads oauth(AuthenticationConfig, String, OpenIdConfiguration, Function1) and oauth(AuthenticationConfig, String, String, Function1) create ambiguity when called from Java with a null third argument—the compiler cannot determine which overload to invoke since both accept null. Consider adding @JvmName annotations to disambiguate, e.g., @JvmName("oauthWithOpenId") on the OpenIdConfiguration overload.
🤖 Prompt for AI Agents
In `@ktor-server/ktor-server-plugins/ktor-server-auth/api/ktor-server-auth.api`
around lines 488 - 493, The two 4-arg Kotlin overloads
oauth(AuthenticationConfig, String, OpenIdConfiguration, Function1) and
oauth(AuthenticationConfig, String, String, Function1) cause Java-call ambiguity
when the third parameter is null; annotate the Kotlin overloads with distinct
JVM names (e.g., add `@JvmName`("oauthWithOpenId") to the OpenIdConfiguration
variant and `@JvmName`("oauthWithClientId") to the String-variant) so the
generated methods have unique JVM signatures and Java callers can unambiguously
invoke the desired overload; apply the annotations to the corresponding oauth
functions (and their default dispatchers if necessary) so the compiled API
exposes disambiguated method names.
...server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/OAuthProcedure.kt
Show resolved
Hide resolved
...ver/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/OpenIdOAuthConfig.kt
Show resolved
Hide resolved
bjhham
left a comment
There was a problem hiding this comment.
The API approach looks good. I think the rabbit had some good points, and there are a couple breaking changes in the API that can probably be mitigated.
| } | ||
|
|
||
| final class io.ktor.server.auth/OpenIdConfiguration { // io.ktor.server.auth/OpenIdConfiguration|null[0] | ||
| constructor <init>(kotlin/String, kotlin/String) // io.ktor.server.auth/OpenIdConfiguration.<init>|<init>(kotlin.String;kotlin.String){}[0] |
There was a problem hiding this comment.
Appears to be a breaking change in the constructor args.
There was a problem hiding this comment.
This API has not been released yet.
Maybe it's better to make it a DSL-like non-data class, so it's easier to add extra fields in the future without breaking the API?
There was a problem hiding this comment.
Oh, my mistake I thought it was. I'd remove the data keyword... unless if it's used in serialization.
There was a problem hiding this comment.
It is used for deserialization.
| public field clientId Ljava/lang/String; | ||
| public field clientSecret Ljava/lang/String; | ||
| public field urlProvider Lkotlin/jvm/functions/Function3; |
There was a problem hiding this comment.
This appears to be breaking, though I'm not sure what happened. Maybe there's a JVM annotation to retain them as fields.
There was a problem hiding this comment.
OpenIdOAuthConfig was introduced in this pull request.
The issue here is that I was switching branches (git switch instead of git checkout), and the OpenIdOAuthConfig definition appeared there.
jwt-auto-discover has now been updated with a valid ABI.
ktor-server/ktor-server-plugins/ktor-server-auth/api/ktor-server-auth.klib.api
Outdated
Show resolved
Hide resolved
...ver/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/OpenIdOAuthConfig.kt
Show resolved
Hide resolved
7a9392b to
2c8dc17
Compare
99a1c49 to
18c517e
Compare
Subsystem
Server Auth
Motivation
KTOR-8596 OpenID Connect (OAuth2) auto-discover & configuration
Also see KTOR-8595 Auth JWK Support (auto-discover)
#5339
Solution
New DSL