Conversation
Contributor
|
Important Review skippedDraft detected. 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:
📝 WalkthroughWalkthroughIntroduces a new typesafe authentication framework for Ktor server plugins, including typed configuration classes for API key, JWT, and digest auth, core abstractions for typed auth schemes and contexts, route builders with multi-scheme support, optional/anonymous/role-based auth layers, and comprehensive test coverage. Enables context receiver compiler flag across affected modules. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 |
![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: 8
🧹 Nitpick comments (3)
ktor-server/ktor-server-plugins/ktor-server-auth-api-key/common/test/io/ktor/server/auth/apikey/ApiKeyAuthTest.kt (1)
108-111: ThisallErrorsaccess is not exercised in this test path.The test only sends a valid API key, so the challenge lambda won’t run; this line currently adds no coverage signal. Consider removing it here or moving the assertion into an invalid-credentials test that executes the challenge.
Possible cleanup in this test
challenge { call -> - call.authentication.allErrors call.respond(errorStatus) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ktor-server/ktor-server-plugins/ktor-server-auth-api-key/common/test/io/ktor/server/auth/apikey/ApiKeyAuthTest.kt` around lines 108 - 111, The test's challenge block is referencing call.authentication.allErrors but that path isn't exercised by the valid-key test, so remove the unused access or move the assertion into the test that triggers the challenge (the invalid-credentials flow). Locate the challenge lambda in ApiKeyAuthTest.kt (the block containing "challenge { call -> call.authentication.allErrors call.respond(errorStatus) }") and either delete the call.authentication.allErrors line here, or relocate that assertion into the test which sends an invalid API key so the challenge actually runs and validates authentication errors.ktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/typesafe/Scopes.kt (1)
96-98: Makeroles()fail like the principal accessors do.
roles()indexes the attribute bag directly, so a missing value becomes a generic missing-key failure. UsinggetOrNull+checkNotNullhere would keep misuse errors as actionable asprincipal().Possible fix
public fun roles(context: RoutingContext): Set<R> { - return context.call.attributes[rolesKey] + return checkNotNull(context.call.attributes.getOrNull(rolesKey)) { + "Roles not found. This should not happen inside a role-based authenticateWith block." + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/typesafe/Scopes.kt` around lines 96 - 98, The roles(context: RoutingContext) function currently indexes attributes[rolesKey] directly which yields a generic missing-key exception; change it to read the attribute via context.call.attributes.getOrNull(rolesKey) and wrap that result with checkNotNull(... ) providing a clear message (similar to how principal() does) so misuse produces the same actionable missing-principal style failure; reference the rolesKey symbol and the roles(context: RoutingContext) function when applying the change.ktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/typesafe/RouteBuilders.kt (1)
139-172: Keep route-levelonForbiddenhandlers typed toR.
ForbiddenHandlererases the concrete role type toAuthRole, so route-level handlers lose the same type information thatwithRoles(...)preserves at scheme level. Making this alias generic would keep the route API aligned with the typed DSL.Possible direction
- public typealias ForbiddenHandler = suspend (ApplicationCall, Set<AuthRole>) -> Unit + public typealias ForbiddenHandler<R> = suspend (ApplicationCall, Set<R>) -> Unit public fun <P : Any, R : AuthRole> Route.authenticateWith( scheme: RoleBasedAuthScheme<P, R, *>, roles: Set<R>, onUnauthorized: UnauthorizedHandler? = null, - onForbidden: ForbiddenHandler? = null, + onForbidden: ForbiddenHandler<R>? = null, build: AuthenticatedRouteBuilder<RoleBasedContext<P, R>> ): Route {You'd also want the matching
RoleBasedAuthScheme.validateRoles(...)parameter to useForbiddenHandler<R>?.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/typesafe/RouteBuilders.kt` around lines 139 - 172, The route-level ForbiddenHandler currently erases the concrete role type to AuthRole, losing type-safety; make ForbiddenHandler generic (e.g., ForbiddenHandler<R : AuthRole>) so it accepts suspend (ApplicationCall, Set<R>) -> Unit, update the Route.authenticateWith signature to use ForbiddenHandler<R>? for its onForbidden parameter, and change RoleBasedAuthScheme.validateRoles(...) (and any other usages) to accept ForbiddenHandler<R>? so route-level handlers preserve the same typed role parameter produced by withRoles(...).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@ktor-server/ktor-server-plugins/ktor-server-auth/api/ktor-server-auth.klib.api`:
- Line 141: The public buildProvider(kotlin/String) bridge method
(TypedBasicAuthConfig.buildProvider) is leaking wiring internals; change its
visibility to internal or annotate it as opt-in/internal API and do the same for
the other similar methods reported (the entries at lines 161, 180, 194) so the
typed-config → raw-provider assembly plumbing is not part of the stable public
ABI; update the symbol declarations (TypedBasicAuthConfig.buildProvider and the
corresponding Typed*Config.buildProvider methods) to non-public visibility or
add the internal/ExperimentalWiring opt-in annotation so callers still using the
top-level DSL remain unaffected.
- Line 801: The generated public API exposes
Application.registerSchemeIfNeeded(DefaultAuthScheme<*, *>) which freezes
internal lazy-registration plumbing into the public surface; change its
visibility to internal (or annotate with an internal/opt-in marker such as
`@InternalAPI`) so it is not part of the public .api contract, update the function
declaration for
io.ktor.server.application.Application.registerSchemeIfNeeded(io.ktor.server.auth.typesafe.DefaultAuthScheme<*,
*>) accordingly, and adjust any call sites in auth modules to reside in the same
module or use the internal marker/opt-in mechanism so only intended Ktor auth
modules can invoke it.
In
`@ktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/typesafe/AuthScheme.kt`:
- Around line 23-36: The file currently uses `@OptIn`(ExperimentalKtorApi::class)
and leaves UnauthorizedHandler unannotated; update the API exposure by
annotating the top-level typealias UnauthorizedHandler and the interface
AuthScheme with `@ExperimentalKtorApi` so the experimental status is part of the
public API (remove the `@OptIn` usage on the interface declaration and apply the
`@ExperimentalKtorApi` annotation to both UnauthorizedHandler and AuthScheme to
match the rest of the typed-auth public APIs).
In
`@ktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/typesafe/RoleBasedAuthScheme.kt`:
- Around line 49-70: PerKey currently caches roles solely by
extractKey(principal) which lets a roles resolver that depends on
ApplicationCall (e.g., tenant/header/path or revocations) return stale results
across requests; update the design in RolesCacheStrategy.PerKey to either (1)
restrict caching to resolvers that only depend on the principal (document and
enforce that PerKey is only used when resolve is call-independent), or (2) make
the cache key include call-derived scope (e.g., add a call-scoped token) and add
eviction/invalidation (time-based TTL or explicit invalidation hooks) so role
resolution that uses ApplicationCall cannot be incorrectly reused; change
references to RolesCacheStrategy.PerKey, its extractKey, and the role resolution
path (resolve) accordingly and ensure the cache is bounded (size or TTL) to
avoid unbounded growth.
In
`@ktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/typesafe/TypedAuthInterceptors.kt`:
- Around line 148-149: The per-scheme failure aggregation uses
schemeContext.allFailures.firstOrNull(), which can pick an earlier cause instead
of the provider’s final failure; change to use the final failure (e.g.,
schemeContext.allFailures.lastOrNull()) when populating failures[scheme.name],
falling back to AuthenticationFailedCause.NoCredentials, so onUnauthorized
receives the provider's final failure result for that scheme.
In
`@ktor-server/ktor-server-plugins/ktor-server-auth/common/test/io/ktor/tests/auth/typesafe/DiagTest.kt`:
- Around line 39-50: The test currently only prints response statuses for the
two requests to client.get("/nested") (using bearerAuthHeader("valid") and
basicAuthHeader("user")) so add assertions that validate the expected outcomes:
assert that resp.status (the response from the bearer request) equals the
expected HttpStatusCode (e.g., HttpStatusCode.OK) and likewise assert
resp2.status (the response from the basic auth request) equals the expected
HttpStatusCode; update the DiagTest.kt test to replace or accompany the println
lines with assertions referencing resp, resp2, bearerAuthHeader, basicAuthHeader
and ensure HttpStatusCode is imported/used.
In
`@ktor-server/ktor-server-plugins/ktor-server-auth/jvm/src/io/ktor/server/auth/typesafe/TypedDigestAuthConfig.kt`:
- Around line 123-135: The two digestProvider(...) overloads write to different
backing fields (legacyDigestProviderFn and digestProviderFn) so calling both can
silently override the V2 provider in buildProvider; update each setter so it
clears the other field when invoked: in the DigestProviderFunction setter
(public fun digestProvider(digest: DigestProviderFunction)) null out
digestProviderFn before assigning legacyDigestProviderFn, and in the
DigestProviderFunctionV2 setter (public fun digestProvider(digest:
DigestProviderFunctionV2)) null out legacyDigestProviderFn before assigning
digestProviderFn; also apply the same mutual-clear behavior to the analogous
pair referenced at lines 171-172.
In
`@ktor-server/ktor-server-plugins/ktor-server-auth/jvm/test/io/ktor/tests/auth/typesafe/DigestAuthTest.kt`:
- Around line 48-57: The Authorization header in DigestAuthTest.kt uses a Digest
`uri` of "/dir/index.html" while the actual request is sent with
client.get("/"); update the Digest header to use uri="/" (or alternatively
change client.get to request the path used in the header) so the
`client.get("/")` call and the header's uri value align; edit the header block
where HttpHeaders.Authorization is set in the test to change the uri="..." to
the matching request path.
---
Nitpick comments:
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-api-key/common/test/io/ktor/server/auth/apikey/ApiKeyAuthTest.kt`:
- Around line 108-111: The test's challenge block is referencing
call.authentication.allErrors but that path isn't exercised by the valid-key
test, so remove the unused access or move the assertion into the test that
triggers the challenge (the invalid-credentials flow). Locate the challenge
lambda in ApiKeyAuthTest.kt (the block containing "challenge { call ->
call.authentication.allErrors call.respond(errorStatus) }") and either delete
the call.authentication.allErrors line here, or relocate that assertion into the
test which sends an invalid API key so the challenge actually runs and validates
authentication errors.
In
`@ktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/typesafe/RouteBuilders.kt`:
- Around line 139-172: The route-level ForbiddenHandler currently erases the
concrete role type to AuthRole, losing type-safety; make ForbiddenHandler
generic (e.g., ForbiddenHandler<R : AuthRole>) so it accepts suspend
(ApplicationCall, Set<R>) -> Unit, update the Route.authenticateWith signature
to use ForbiddenHandler<R>? for its onForbidden parameter, and change
RoleBasedAuthScheme.validateRoles(...) (and any other usages) to accept
ForbiddenHandler<R>? so route-level handlers preserve the same typed role
parameter produced by withRoles(...).
In
`@ktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/typesafe/Scopes.kt`:
- Around line 96-98: The roles(context: RoutingContext) function currently
indexes attributes[rolesKey] directly which yields a generic missing-key
exception; change it to read the attribute via
context.call.attributes.getOrNull(rolesKey) and wrap that result with
checkNotNull(... ) providing a clear message (similar to how principal() does)
so misuse produces the same actionable missing-principal style failure;
reference the rolesKey symbol and the roles(context: RoutingContext) function
when applying the change.
🪄 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: 65935e0f-9734-405a-a7af-482e06127b7b
📒 Files selected for processing (41)
ktor-server/ktor-server-plugins/ktor-server-auth-api-key/api/ktor-server-auth-api-key.apiktor-server/ktor-server-plugins/ktor-server-auth-api-key/api/ktor-server-auth-api-key.klib.apiktor-server/ktor-server-plugins/ktor-server-auth-api-key/build.gradle.ktsktor-server/ktor-server-plugins/ktor-server-auth-api-key/common/src/io/ktor/server/auth/apikey/ApiKeyAuth.ktktor-server/ktor-server-plugins/ktor-server-auth-api-key/common/src/io/ktor/server/auth/typesafe/ApiKeyTypedProvider.ktktor-server/ktor-server-plugins/ktor-server-auth-api-key/common/src/io/ktor/server/auth/typesafe/TypedApiKeyAuthConfig.ktktor-server/ktor-server-plugins/ktor-server-auth-api-key/common/test/io/ktor/server/auth/apikey/ApiKeyAuthTest.ktktor-server/ktor-server-plugins/ktor-server-auth-api-key/common/test/io/ktor/server/auth/apikey/TypedApiKeyAuthTest.ktktor-server/ktor-server-plugins/ktor-server-auth-jwt/api/ktor-server-auth-jwt.apiktor-server/ktor-server-plugins/ktor-server-auth-jwt/build.gradle.ktsktor-server/ktor-server-plugins/ktor-server-auth-jwt/jvm/src/io/ktor/server/auth/typesafe/JwtTypedProvider.ktktor-server/ktor-server-plugins/ktor-server-auth-jwt/jvm/src/io/ktor/server/auth/typesafe/TypedJwtAuthConfig.ktktor-server/ktor-server-plugins/ktor-server-auth-jwt/jvm/test/io/ktor/server/auth/jwt/TypedJwtAuthTest.ktktor-server/ktor-server-plugins/ktor-server-auth/api/ktor-server-auth.apiktor-server/ktor-server-plugins/ktor-server-auth/api/ktor-server-auth.klib.apiktor-server/ktor-server-plugins/ktor-server-auth/build.gradle.ktsktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/AuthenticationInterceptors.ktktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/typesafe/AuthScheme.ktktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/typesafe/BasicTypedProvider.ktktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/typesafe/OptionalAuthScheme.ktktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/typesafe/RoleBasedAuthScheme.ktktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/typesafe/RouteBuilders.ktktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/typesafe/SchemeRegistration.ktktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/typesafe/Scopes.ktktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/typesafe/TypedAuthInterceptors.ktktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/typesafe/TypedBasicAuthConfig.ktktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/typesafe/TypedBearerAuthConfig.ktktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/typesafe/TypedFormAuthConfig.ktktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/typesafe/TypedSessionAuthConfig.ktktor-server/ktor-server-plugins/ktor-server-auth/common/test/io/ktor/tests/auth/typesafe/BasicSchemesTest.ktktor-server/ktor-server-plugins/ktor-server-auth/common/test/io/ktor/tests/auth/typesafe/DiagTest.ktktor-server/ktor-server-plugins/ktor-server-auth/common/test/io/ktor/tests/auth/typesafe/LegacyApiCoexistenceTest.ktktor-server/ktor-server-plugins/ktor-server-auth/common/test/io/ktor/tests/auth/typesafe/MultipleSchemeTest.ktktor-server/ktor-server-plugins/ktor-server-auth/common/test/io/ktor/tests/auth/typesafe/NestedRoutesTest.ktktor-server/ktor-server-plugins/ktor-server-auth/common/test/io/ktor/tests/auth/typesafe/OptionalAndAnonymousAuthTest.ktktor-server/ktor-server-plugins/ktor-server-auth/common/test/io/ktor/tests/auth/typesafe/RoleBasedAuthTest.ktktor-server/ktor-server-plugins/ktor-server-auth/common/test/io/ktor/tests/auth/typesafe/TestUtils.ktktor-server/ktor-server-plugins/ktor-server-auth/common/test/io/ktor/tests/auth/typesafe/UnauthorizedAndChallengesTest.ktktor-server/ktor-server-plugins/ktor-server-auth/jvm/src/io/ktor/server/auth/typesafe/DigestTypedProvider.ktktor-server/ktor-server-plugins/ktor-server-auth/jvm/src/io/ktor/server/auth/typesafe/TypedDigestAuthConfig.ktktor-server/ktor-server-plugins/ktor-server-auth/jvm/test/io/ktor/tests/auth/typesafe/DigestAuthTest.kt
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
Motivation
(ktorio/ktor-klip#6)
Solution
The PR can be reviewed commit by commit
Deviations from the KLIP
Chose the typed provider DSL direction, but implemented schemes as provider-owning objects created by top-level builders (
basic<P>(),jwt<P>(), etc.).authenticateWithlazily registers the provider when the scheme is used. This avoids name drift, extrainstall(Authentication)wiring, and overload pressure on existing builders.Kept the API in existing modules instead of adding
ktor-server-typesafe-auth: core DSL inktor-server-auth, JWT inktor-server-auth-jwt, API key inktor-server-auth-api-key.Extended
AuthSchemefromAuthScheme<P>toAuthScheme<P, C>, whereCis the authenticated route context. This allows custom typed contexts, not onlyprincipal/roles.Replaced
authenticateWith(vararg schemes)with explicitauthenticateWithAnyOf(...)for multi-scheme auth. This makes “any of these schemes” clearer and avoids overload/type inference ambiguity.Replaced
authenticateWithOptional(...)andorAnonymous(...)withscheme.optional()andscheme.optional { anonymous }, then regularauthenticateWith(...). Optionality is now encoded in the scheme type.onUnauthorizedreceivesAuthenticationFailedCause; multi-scheme auth receives per-scheme failure causes. This lets custom handlers distinguish missing credentials from invalid credentials.Role support includes built-in
PerCall/PerKeycaching and scheme-level or route-levelonForbidden. The cache strategy is sealed for now to keep semantics narrow.Typed configs intentionally expose
onUnauthorizedinstead of provider-levelchallenge, while preserving default provider challenges when no custom handler is configured.Provider coverage differs slightly: this PR adds typed Basic, Bearer, Form, Session, Digest, JWT, and API Key. OAuth is intentionally left for the follow-up
typed-oauth-with-sessionPR.transformPrincipalis not implemented. The KLIP marks its caching/null behavior as TBD, so it is left out of this first PR.