Skip to content

Commit 3db9325

Browse files
committed
feat(authorization): Fix the status code for failed authorization
If a request fails because of missing permissions, return the correct status code 403. Add a new `AuthorizationException` that is thrown in this case and can be mapped to the corresponding status code. Signed-off-by: Oliver Heger <[email protected]>
1 parent cf0d465 commit 3db9325

File tree

3 files changed

+151
-18
lines changed

3 files changed

+151
-18
lines changed

components/authorization/backend/build.gradle.kts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ dependencies {
4444

4545
testImplementation(ktorLibs.client.contentNegotiation)
4646
testImplementation(ktorLibs.client.core)
47+
testImplementation(ktorLibs.server.statusPages)
4748
testImplementation(ktorLibs.server.testHost)
4849
testImplementation(ktorLibs.utils)
4950
testImplementation(libs.kotestAssertionsCore)

components/authorization/backend/src/main/kotlin/routes/AuthorizedRoutes.kt

Lines changed: 36 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ import io.github.smiley4.ktoropenapi.post
2929
import io.github.smiley4.ktoropenapi.put
3030

3131
import io.ktor.server.application.ApplicationCall
32+
import io.ktor.server.auth.principal
3233
import io.ktor.server.routing.Route
3334
import io.ktor.server.routing.RouteSelector
3435
import io.ktor.server.routing.RouteSelectorEvaluation
@@ -51,17 +52,13 @@ suspend fun ApplicationCall.createAuthorizedPrincipal(
5152
(this as? RoutingPipelineCall)?.let { routingCall ->
5253
val checker = routingCall.route.findAuthorizationChecker()
5354

54-
val effectiveRole = if (checker != null) {
55-
checker.loadEffectiveRole(
56-
service = authorizationService,
57-
userId = payload.getClaim("preferred_username").asString(),
58-
call = this
59-
).takeIf { checker.checkAuthorization(it) }
60-
} else {
61-
EffectiveRole.EMPTY
62-
}
55+
val effectiveRole = checker?.loadEffectiveRole(
56+
service = authorizationService,
57+
userId = payload.getClaim("preferred_username").asString(),
58+
call = this
59+
) ?: EffectiveRole.EMPTY
6360

64-
effectiveRole?.let { OrtServerPrincipal.create(payload, it) }
61+
OrtServerPrincipal.create(payload, effectiveRole)
6562
}
6663

6764
/**
@@ -71,7 +68,7 @@ fun Route.get(
7168
builder: RouteConfig.() -> Unit,
7269
checker: AuthorizationChecker,
7370
body: suspend RoutingContext.() -> Unit
74-
): Route = documentedAuthorized(checker) { get(builder, body) }
71+
): Route = documentedAuthorized(checker, body) { get(builder, it) }
7572

7673
/**
7774
* Create a new [Route] for HTTP POST requests that performs an automatic authorization check using the given [checker].
@@ -80,7 +77,7 @@ fun Route.post(
8077
builder: RouteConfig.() -> Unit,
8178
checker: AuthorizationChecker,
8279
body: suspend RoutingContext.() -> Unit
83-
): Route = documentedAuthorized(checker) { post(builder, body) }
80+
): Route = documentedAuthorized(checker, body) { post(builder, it) }
8481

8582
/**
8683
* Create a new [Route] for HTTP PATCH requests that performs an automatic authorization check using the given
@@ -90,7 +87,7 @@ fun Route.patch(
9087
builder: RouteConfig.() -> Unit,
9188
checker: AuthorizationChecker,
9289
body: suspend RoutingContext.() -> Unit
93-
): Route = documentedAuthorized(checker) { patch(builder, body) }
90+
): Route = documentedAuthorized(checker, body) { patch(builder, it) }
9491

9592
/**
9693
* Create a new [Route] for HTTP PUT requests that performs an automatic authorization check using the given
@@ -100,7 +97,7 @@ fun Route.put(
10097
builder: RouteConfig.() -> Unit,
10198
checker: AuthorizationChecker,
10299
body: suspend RoutingContext.() -> Unit
103-
): Route = documentedAuthorized(checker) { put(builder, body) }
100+
): Route = documentedAuthorized(checker, body) { put(builder, it) }
104101

105102
/**
106103
* Create a new [Route] for HTTP DELETE requests that performs an automatic authorization check using the given
@@ -110,16 +107,31 @@ fun Route.delete(
110107
builder: RouteConfig.() -> Unit,
111108
checker: AuthorizationChecker,
112109
body: suspend RoutingContext.() -> Unit
113-
): Route = documentedAuthorized(checker) { delete(builder, body) }
110+
): Route = documentedAuthorized(checker, body) { delete(builder, it) }
114111

115112
/**
116113
* Generic function to create a new [Route] that performs an automatic authorization check using the given [checker].
117-
* The content of the route is defined by the given [build] function.
114+
* The content of the route is defined by the given original [body] and the [build] function.
118115
*/
119-
private fun Route.documentedAuthorized(checker: AuthorizationChecker, build: Route.() -> Unit): Route {
116+
private fun Route.documentedAuthorized(
117+
checker: AuthorizationChecker,
118+
body: suspend RoutingContext.() -> Unit,
119+
build: Route.(suspend RoutingContext.() -> Unit) -> Unit
120+
): Route {
120121
val authorizedRoute = createChild(authorizedRouteSelector(checker.toString()))
121122
authorizedRoute.attributes.put(AuthorizationCheckerKey, checker)
122-
authorizedRoute.build()
123+
124+
val authorizedBody: suspend RoutingContext.() -> Unit = {
125+
val principal = call.principal<OrtServerPrincipal>() ?: throw AuthorizationException()
126+
127+
if (!checker.checkAuthorization(principal.effectiveRole)) {
128+
throw AuthorizationException()
129+
}
130+
131+
body()
132+
}
133+
134+
authorizedRoute.build(authorizedBody)
123135
return authorizedRoute
124136
}
125137

@@ -160,3 +172,9 @@ object AuthenticationProviders {
160172
*/
161173
const val TOKEN_PROVIDER = "token"
162174
}
175+
176+
/**
177+
* An exception class to indicate a failed authorization check. Such exceptions are thrown by the route functions when
178+
* the current user does not have the required permissions. They are mapped to HTTP 403 Forbidden responses.
179+
*/
180+
class AuthorizationException : RuntimeException()

components/authorization/backend/src/test/kotlin/routes/AuthorizedRoutesTest.kt

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ import io.ktor.server.auth.Authentication
4444
import io.ktor.server.auth.authenticate
4545
import io.ktor.server.auth.jwt.jwt
4646
import io.ktor.server.auth.principal
47+
import io.ktor.server.plugins.statuspages.StatusPages
4748
import io.ktor.server.response.respond
4849
import io.ktor.server.routing.Route
4950
import io.ktor.server.routing.route
@@ -93,6 +94,12 @@ class AuthorizedRoutesTest : WordSpec() {
9394
}
9495
}
9596

97+
install(StatusPages) {
98+
exception<AuthorizationException> { call, _ ->
99+
call.respond(HttpStatusCode.Forbidden)
100+
}
101+
}
102+
96103
routing {
97104
authenticate(AuthenticationProviders.TOKEN_PROVIDER, build = routeBuilder)
98105
}
@@ -264,6 +271,113 @@ class AuthorizedRoutesTest : WordSpec() {
264271
}
265272
}
266273
}
274+
275+
"failed authorization checks" should {
276+
"return a 403 response for GET with insufficient organization permission" {
277+
val effectiveRole = mockk<EffectiveRole> {
278+
every { hasOrganizationPermission(any()) } returns false
279+
}
280+
val service = createServiceForOrganizationRole(effectiveRole)
281+
282+
runAuthorizationTest(
283+
service,
284+
routeBuilder = {
285+
route("test/{organizationId}") {
286+
get(testDocs, requirePermission(OrganizationPermission.READ)) {
287+
call.respond(HttpStatusCode.OK)
288+
}
289+
}
290+
}
291+
) { client ->
292+
val response = client.get("test/$ID_PARAMETER")
293+
response.status shouldBe HttpStatusCode.Forbidden
294+
}
295+
}
296+
297+
"return a 403 response for POST with insufficient organization permission" {
298+
val effectiveRole = mockk<EffectiveRole> {
299+
every { hasOrganizationPermission(any()) } returns false
300+
}
301+
val service = createServiceForOrganizationRole(effectiveRole)
302+
303+
runAuthorizationTest(
304+
service,
305+
routeBuilder = {
306+
route("test/{organizationId}") {
307+
post(testDocs, requirePermission(OrganizationPermission.CREATE_PRODUCT)) {
308+
call.respond(HttpStatusCode.OK)
309+
}
310+
}
311+
}
312+
) { client ->
313+
val response = client.post("test/$ID_PARAMETER")
314+
response.status shouldBe HttpStatusCode.Forbidden
315+
}
316+
}
317+
318+
"return a 403 response for PATCH with insufficient organization permission" {
319+
val effectiveRole = mockk<EffectiveRole> {
320+
every { hasOrganizationPermission(any()) } returns false
321+
}
322+
val service = createServiceForOrganizationRole(effectiveRole)
323+
324+
runAuthorizationTest(
325+
service,
326+
routeBuilder = {
327+
route("test/{organizationId}") {
328+
patch(testDocs, requirePermission(OrganizationPermission.MANAGE_GROUPS)) {
329+
call.respond(HttpStatusCode.OK)
330+
}
331+
}
332+
}
333+
) { client ->
334+
val response = client.patch("test/$ID_PARAMETER")
335+
response.status shouldBe HttpStatusCode.Forbidden
336+
}
337+
}
338+
339+
"return a 403 response for PUT with insufficient organization permission" {
340+
val effectiveRole = mockk<EffectiveRole> {
341+
every { hasOrganizationPermission(any()) } returns false
342+
}
343+
val service = createServiceForOrganizationRole(effectiveRole)
344+
345+
runAuthorizationTest(
346+
service,
347+
routeBuilder = {
348+
route("test/{organizationId}") {
349+
put(testDocs, requirePermission(OrganizationPermission.WRITE_SECRETS)) {
350+
call.respond(HttpStatusCode.OK)
351+
}
352+
}
353+
}
354+
) { client ->
355+
val response = client.put("test/$ID_PARAMETER")
356+
response.status shouldBe HttpStatusCode.Forbidden
357+
}
358+
}
359+
360+
"return a 403 response for DELETE with insufficient organization permission" {
361+
val effectiveRole = mockk<EffectiveRole> {
362+
every { hasOrganizationPermission(any()) } returns false
363+
}
364+
val service = createServiceForOrganizationRole(effectiveRole)
365+
366+
runAuthorizationTest(
367+
service,
368+
routeBuilder = {
369+
route("test/{organizationId}") {
370+
delete(testDocs, requirePermission(OrganizationPermission.DELETE)) {
371+
call.respond(HttpStatusCode.OK)
372+
}
373+
}
374+
}
375+
) { client ->
376+
val response = client.delete("test/$ID_PARAMETER")
377+
response.status shouldBe HttpStatusCode.Forbidden
378+
}
379+
}
380+
}
267381
}
268382
}
269383

0 commit comments

Comments
 (0)