-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Server Auth. Make non-optional principal. #5332
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdds a value-class wrapper Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@ktor-server/ktor-server-plugins/ktor-server-auth/common/test/io/ktor/tests/auth/AuthenticatedRouteTest.kt`:
- Around line 181-183: The two assertions use assertEquals with arguments in the
wrong order; swap the parameters so expected comes first: change
assertEquals(response.status, HttpStatusCode.Unauthorized) to
assertEquals(HttpStatusCode.Unauthorized, response.status) (leave
assertEquals("", response.bodyAsText()) as-is), updating the assertions in the
AuthenticatedRouteTest test where the local variable response is used.
🧹 Nitpick comments (2)
ktor-server/ktor-server-plugins/ktor-server-auth/common/src/io/ktor/server/auth/AuthenticatedRoute.kt (1)
7-11: Consider using star imports forio.ktor.*packages.Per the coding guidelines, star imports should be used for
io.ktor.*packages.Suggested imports
-import io.ktor.http.* -import io.ktor.server.request.* -import io.ktor.server.response.respond -import io.ktor.server.routing.* +import io.ktor.http.* +import io.ktor.server.request.* +import io.ktor.server.response.* +import io.ktor.server.routing.*ktor-server/ktor-server-plugins/ktor-server-auth/common/test/io/ktor/tests/auth/AuthenticatedRouteTest.kt (1)
7-23: Consider using star imports forio.ktor.*packages.Per the coding guidelines, star imports should be used for
io.ktor.*packages.Suggested imports
-import io.ktor.client.request.* -import io.ktor.client.statement.* -import io.ktor.http.HttpStatusCode -import io.ktor.server.auth.Authentication -import io.ktor.server.auth.authenticate -import io.ktor.server.auth.basic -import io.ktor.server.auth.delete -import io.ktor.server.auth.get -import io.ktor.server.auth.head -import io.ktor.server.auth.options -import io.ktor.server.auth.patch -import io.ktor.server.auth.post -import io.ktor.server.auth.put -import io.ktor.server.auth.route -import io.ktor.server.response.* -import io.ktor.server.testing.* +import io.ktor.client.request.* +import io.ktor.client.statement.* +import io.ktor.http.* +import io.ktor.server.auth.* +import io.ktor.server.response.* +import io.ktor.server.testing.*
...tor-server-plugins/ktor-server-auth/common/test/io/ktor/tests/auth/AuthenticatedRouteTest.kt
Show resolved
Hide resolved
bjhham
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could use a little discussion for the design here. Maybe not a full KLIP but a thread in #ktor-team. Introducing a new set of route DSL functions with new lambda arguments seems a little problematic.
| public final class io/ktor/server/routing/openapi/OpenApiDocSource$Companion { | ||
| public final fun readOpenApiSource (Lio/ktor/server/application/Application;Lio/ktor/server/routing/openapi/OpenApiDocSource;Lio/ktor/openapi/OpenApiDoc;)Lio/ktor/server/routing/openapi/OpenApiDocSource$OpenApiDocText; | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You got some outdated ABI, looks like you need a clean
Subsystem
Server Auth
Motivation
KTOR-8594 Auth: Non-optional principal is of nullable type
Solution
Added dsl
Will automatically return
HttpStatusCode.Unauthorizedif no principal is provided.