Add typed OAuth with session support#5567
Conversation
|
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:
📝 WalkthroughWalkthroughTyped session auth now uses ChangesOAuth auth and typesafe session refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 |
|
Could you pull the latest main? Failing tests seem to be fixed in the main branch. |
Pantus Oleh (zibet27)
force-pushed
the
typed-oauth-with-session
branch
from
686588c to
8a267ac
Compare
| * @return a compound scheme for [oauthCallback] and [authenticateWith]. | ||
| */ | ||
| @ExperimentalKtorApi | ||
| public inline fun <reified S : Any, reified P : Any> oauthWithSession( |
There was a problem hiding this comment.
the seprate method looks adhoc for me, could we make it more unified and have session support inside the regular one?
Simon Vergauwen (@nomisRev), what do you think?
There was a problem hiding this comment.
We can
I didn't include the typed oauth scheme in the #5565
oauthWithSession is named like this to make clear that session integration is provided by default
Pantus Oleh (zibet27)
force-pushed
the
typed-auth-dsl
branch
from
1eac0ef to
afdc036
Compare
| cookie(scheme = it) | ||
| } | ||
| sessionCreator = { token -> | ||
| val oauth2 = token as OAuthAccessTokenResponse.OAuth2 |
There was a problem hiding this comment.
Why is this cast needed here? We should have typed access the accessToken and the other information from the successfully logged in principal, right?
There was a problem hiding this comment.
Yeah, but the type of token is OAuthAccessTokenResponse, not oauth2
There was a problem hiding this comment.
Actually, this is a bug
I've reduced the token type to OAuthAccessTokenResponse.OAuth2, but the session creator gets raw OAuthAccessTokenResponse (I forgot it or lost it at some point)
Good catch!
|
Leonid Stashevsky (@e5l) I am wondering if we might need to rethink some of these APIs a little bit because it still won't be straightforward for a developer to allow secure access from a server, there are a lot of different moving parts here. There was a secondary goal besides introducing "typed authentication" which was to simplify setting up secure OAuth with session. Even though the PR is small it's kind-of tricky to wrap my head around but user needs to separately setup:
And/or is the goal to tackle this in ktorio/ktor-klip#7? In that case this can still be considered low-level, but we might want to deviate more from the current (old) design. Perhaps we should consider skipping this all together, and jumping straight to KLIP 7 to provide a high-level typed DSL? 🤔 WDYT Pantus Oleh (@zibet27)? |
Also, not 100% sure about this one
Yeah, this PR is a low-level API for easier migration from the existing API and a base for OIDC
Personally, I am open to more changes. This is a new API anyway
So you mean, not to add Typesafe OAuth DSL and only keep OIDC? |
| * @return the route that contains the OAuth callback. | ||
| */ | ||
| @ExperimentalKtorApi | ||
| public fun Route.oauthCallback( |
There was a problem hiding this comment.
Maybe this could be slightly nicer signature?
context(route: Route)
fun OAuth2Flow.oauthCallback(path: String, onSuccess: ...): Route
|
Okay, I gave this some more thought and it might make sense to have this intermediate APIs. Perhaps for people that have a more complex setup with the current low-level APIs that will not be able to easily migrate to the higher-level OIDC support. It might indeed feel inconsistent, and to be honest I'm personally open to removing or deprecating the "old" code although that'd be a huge breaking change. Maybe deprecate in Ktor 4 and in 10 years remove in Ktor 5.0 😅 I think what bothers me a little bit here is that the routes are still "implicitly" linked, and without them it doesn't work at all. So you have the OAuth2 configured but it's incomplete. So maybe we just need to:
This unifies the OAuth setup a little bit better in my opinion but I think it breaks the current |
# Conflicts: # ktor-server/ktor-server-plugins/ktor-server-auth/api/ktor-server-auth.api # ktor-server/ktor-server-plugins/ktor-server-auth/api/ktor-server-auth.klib.api
Pantus Oleh (zibet27)
force-pushed
the
typed-oauth-with-session
branch
from
18c4b71 to
a801fbf
Compare
|
CodeRabbit (@coderabbitai) review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/common/src/io/ktor/server/auth/OAuth2.kt`:
- Around line 79-95: The callback flow in OAuth2.requestToken currently
evaluates extraTokenParametersProvider before state validation, so move state
verification ahead of the dynamic token-parameter hook. Update the request
handling in OAuth2.kt so verifyState is called first using the existing
stateVerifier/settings.verifyState path, then compute extraTokenParameters and
pass them into oauth2RequestAccessToken only after the state has been accepted.
In
`@ktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/typesafe/OAuthFlow.kt`:
- Line 430: The callback routes in OAuthFlow are currently registered with GET
only, so form_post OAuth callbacks are missed even though the flow supports
them. Update the typed callback handlers in OAuthFlow to use a method-agnostic
route(path) with handle { ... } for flow.callback.path, and apply the same
change to the session-backed callback route, while leaving the optional login
route as GET-only.
- Around line 103-110: The validate(flowName) helpers in OAuthFlow are currently
part of the stable public ABI even though they are only used by builder/factory
internals. Change these validation hooks to internal or `@PublishedApi` internal,
and if they must remain callable from outside, annotate them with `@InternalAPI`
instead. Apply the same visibility treatment to the other validate(flowName)
methods mentioned in this diff so the public surface stays intentional.
- Around line 463-467: The OAuth callback flow is writing the session before
principal resolution, which can leave an OAuth session cookie behind even when
principal creation fails. In OAuthFlow’s callback handling, move the
call.sessions.set(...) invocation to after flow.principalResolver(this, session)
succeeds, and keep the failure path in the same block so a null principal
returns via callback.failureHandler without persisting the session.
🪄 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: f45444fd-e1ba-4a81-988b-2b1f4f6b272d
📒 Files selected for processing (15)
ktor-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/common/src/io/ktor/server/auth/AuthenticationInterceptors.ktktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/OAuth2.ktktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/OAuthCommon.ktktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/OAuthProcedure.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/OAuthFlow.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/SessionAuthScheme.ktktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/typesafe/SessionTransport.ktktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/typesafe/SessionsConfigTypesafeExtensions.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/OAuth2Test.ktktor-server/ktor-server-plugins/ktor-server-auth/common/test/io/ktor/tests/auth/typesafe/OAuthFlowTest.kt
4 participants
Subsystem
Server Auth
Motivation
https://github.com/ktorio/ktor-klip/blob/zibet27/auth-3.5/proposals/0006-auth-3.5.md
Why
oauthWithSessionThe existing OAuth provider is useful for completing the OAuth callback and obtaining an
OAuthAccessTokenResponse, but it is easy to read it as a provider that can protect application routes directly.That is misleading for typical web login flows: OAuth authenticates only the callback request. It does not persist the login state, create an application principal, or protect later requests. Applications still need to map the token response to their own session/principal and then use session authentication for protected routes.
oauthWithSessionmakes this pattern explicit:This keeps the OAuth provider focused on OAuth, while giving users a typed, end-to-end login flow that matches how OAuth is commonly used in server-side applications.
This is still a question if we want to just provide typed DSL for
oauthoroauthWithSession, as shown in this PR