Skip to content

Commit 817f454

Browse files
slissonslisson
committed
fix(authorization): allow only resource owners/admins to manage permissions
The previous behavior of allowing to grant your own permission to others might be too risky. It's hardcoded to
1 parent 11c6d46 commit 817f454

20 files changed

+175
-78
lines changed

‎authorization/src/main/kotlin/org/modelix/authorization/AuthorizationPlugin.kt‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,8 @@ import io.ktor.server.routing.get
3232
import io.ktor.server.routing.routing
3333
import io.ktor.util.AttributeKey
3434
import org.modelix.authorization.permissions.PermissionEvaluator
35+
import org.modelix.authorization.permissions.PermissionInstanceReference
36+
import org.modelix.authorization.permissions.PermissionParser
3537
import org.modelix.authorization.permissions.PermissionParts
3638
import org.modelix.authorization.permissions.SchemaInstance
3739
import java.nio.charset.StandardCharsets
@@ -169,11 +171,15 @@ class ModelixAuthorizationPluginInstance(val config: ModelixAuthorizationConfig)
169171
private val deniedPermissionRequests: MutableSet<DeniedPermissionRequest> = Collections.synchronizedSet(LinkedHashSet())
170172
private val permissionCache = CacheBuilder.newBuilder()
171173
.expireAfterWrite(5, TimeUnit.SECONDS)
172-
.build<Pair<AccessTokenPrincipal, PermissionParts>, Boolean>()
174+
.build<Pair<AccessTokenPrincipal, PermissionInstanceReference>, Boolean>()
173175

174176
fun getDeniedPermissions(): Set<DeniedPermissionRequest> = deniedPermissionRequests.toSet()
175177

176178
fun hasPermission(call: ApplicationCall, permissionToCheck: PermissionParts): Boolean {
179+
return hasPermission(call, PermissionParser(config.permissionSchema).parse(permissionToCheck))
180+
}
181+
182+
fun hasPermission(call: ApplicationCall, permissionToCheck: PermissionInstanceReference): Boolean {
177183
if (!config.permissionCheckingEnabled()) return true
178184

179185
val principal = call.principal<AccessTokenPrincipal>() ?: throw NotLoggedInException()
@@ -184,7 +190,7 @@ class ModelixAuthorizationPluginInstance(val config: ModelixAuthorizationConfig)
184190
if (userId != null) {
185191
synchronized(deniedPermissionRequests) {
186192
deniedPermissionRequests += DeniedPermissionRequest(
187-
permissionId = permissionToCheck,
193+
permissionRef = permissionToCheck,
188194
userId = userId,
189195
jwtPayload = principal.jwt.payload,
190196
)
@@ -222,7 +228,7 @@ class ModelixAuthorizationPluginInstance(val config: ModelixAuthorizationConfig)
222228
}
223229

224230
data class DeniedPermissionRequest(
225-
val permissionId: PermissionParts,
231+
val permissionRef: PermissionInstanceReference,
226232
val userId: String,
227233
val jwtPayload: String,
228234
) {

‎authorization/src/main/kotlin/org/modelix/authorization/KtorAuthUtils.kt‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import io.ktor.server.request.header
1717
import io.ktor.server.routing.Route
1818
import io.ktor.util.pipeline.PipelineContext
1919
import org.modelix.authorization.permissions.PermissionEvaluator
20+
import org.modelix.authorization.permissions.PermissionInstanceReference
2021
import org.modelix.authorization.permissions.PermissionParts
2122

2223
internal const val MODELIX_JWT_AUTH = "modelixJwtAuth"
@@ -49,6 +50,10 @@ fun ApplicationCall.hasPermission(permissionToCheck: PermissionParts): Boolean {
4950
return application.plugin(ModelixAuthorization).hasPermission(this, permissionToCheck)
5051
}
5152

53+
fun ApplicationCall.hasPermission(permissionToCheck: PermissionInstanceReference): Boolean {
54+
return application.plugin(ModelixAuthorization).hasPermission(this, permissionToCheck)
55+
}
56+
5257
fun ApplicationCall.getPermissionEvaluator(): PermissionEvaluator {
5358
return application.plugin(ModelixAuthorization).getPermissionEvaluator(this)
5459
}

‎authorization/src/main/kotlin/org/modelix/authorization/PermissionManagementPage.kt‎

Lines changed: 52 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import io.ktor.server.application.ApplicationCall
44
import io.ktor.server.application.application
55
import io.ktor.server.application.call
66
import io.ktor.server.application.plugin
7+
import io.ktor.server.auth.principal
78
import io.ktor.server.html.respondHtml
89
import io.ktor.server.request.receiveParameters
910
import io.ktor.server.response.respond
@@ -26,8 +27,8 @@ import kotlinx.html.td
2627
import kotlinx.html.textInput
2728
import kotlinx.html.th
2829
import kotlinx.html.tr
29-
import org.modelix.authorization.permissions.PermissionParts
30-
import org.modelix.authorization.permissions.PermissionSchemaBase
30+
import org.modelix.authorization.permissions.PermissionInstanceReference
31+
import org.modelix.authorization.permissions.PermissionParser
3132

3233
fun Route.installPermissionManagementHandlers() {
3334
route("permissions") {
@@ -42,9 +43,7 @@ fun Route.installPermissionManagementHandlers() {
4243
val roleId = formParameters["roleId"]
4344
require(userId != null || roleId != null) { "userId or roleId required" }
4445
val permissionId = requireNotNull(formParameters["permissionId"]) { "permissionId not specified" }
45-
46-
// a user can grant his own permission to other users
47-
checkPermission(PermissionParts.fromString(permissionId))
46+
call.checkCanGranPermission(permissionId)
4847

4948
if (userId != null) {
5049
application.plugin(ModelixAuthorization).config.accessControlPersistence.update {
@@ -59,12 +58,12 @@ fun Route.installPermissionManagementHandlers() {
5958
call.respond("Granted $permissionId to ${userId ?: roleId}")
6059
}
6160
post("remove-grant") {
62-
call.checkPermission(PermissionSchemaBase.permissionData.write)
6361
val formParameters = call.receiveParameters()
6462
val userId = formParameters["userId"]
6563
val roleId = formParameters["roleId"]
6664
require(userId != null || roleId != null) { "userId or roleId required" }
6765
val permissionId = requireNotNull(formParameters["permissionId"]) { "permissionId not specified" }
66+
call.checkCanGranPermission(permissionId)
6867
if (userId != null) {
6968
application.plugin(ModelixAuthorization).config.accessControlPersistence.update {
7069
it.withoutGrantToUser(userId, permissionId)
@@ -138,7 +137,7 @@ fun HTML.buildPermissionManagementPage(call: ApplicationCall, pluginInstance: Mo
138137
th { +"Permission" }
139138
}
140139
for ((userId, permission) in pluginInstance.config.accessControlPersistence.read().grantsToUsers.flatMap { entry -> entry.value.map { entry.key to it } }) {
141-
if (!call.hasPermission(PermissionParts.fromString(permission))) continue
140+
if (!call.canGrantPermission(permission)) continue
142141

143142
tr {
144143
td {
@@ -174,7 +173,7 @@ fun HTML.buildPermissionManagementPage(call: ApplicationCall, pluginInstance: Mo
174173
th { +"Permission" }
175174
}
176175
for ((roleId, permission) in pluginInstance.config.accessControlPersistence.read().grantsToRoles.flatMap { entry -> entry.value.map { entry.key to it } }) {
177-
if (!call.hasPermission(PermissionParts.fromString(permission))) continue
176+
if (!call.canGrantPermission(permission)) continue
178177

179178
tr {
180179
td {
@@ -213,32 +212,30 @@ fun HTML.buildPermissionManagementPage(call: ApplicationCall, pluginInstance: Mo
213212
th { +"Grant" }
214213
}
215214
for (deniedPermission in pluginInstance.getDeniedPermissions()) {
216-
if (!call.hasPermission(deniedPermission.permissionId)) continue
215+
if (!call.canGrantPermission(deniedPermission.permissionRef)) continue
217216

218217
val userId = deniedPermission.userId
219218
tr {
220219
td {
221-
+userId.orEmpty()
220+
+userId
222221
}
223222
td {
224-
+deniedPermission.permissionId.fullId
223+
+deniedPermission.permissionRef.toPermissionParts().fullId
225224
}
226225
td {
227-
if (userId != null) {
228-
val evaluator = pluginInstance.createPermissionEvaluator()
229-
val permissionInstance = evaluator.instantiatePermission(deniedPermission.permissionId)
230-
val candidates = (setOf(permissionInstance) + permissionInstance.transitiveIncludedIn())
231-
postForm(action = "grant") {
232-
hiddenInput {
233-
name = "userId"
234-
value = userId
235-
}
236-
for (candidate in candidates) {
237-
div {
238-
submitInput {
239-
name = "permissionId"
240-
value = candidate.ref.toString()
241-
}
226+
val evaluator = pluginInstance.createPermissionEvaluator()
227+
val permissionInstance = evaluator.instantiatePermission(deniedPermission.permissionRef)
228+
val candidates = (setOf(permissionInstance) + permissionInstance.transitiveIncludedIn())
229+
postForm(action = "grant") {
230+
hiddenInput {
231+
name = "userId"
232+
value = userId
233+
}
234+
for (candidate in candidates) {
235+
div {
236+
submitInput {
237+
name = "permissionId"
238+
value = candidate.ref.toString()
242239
}
243240
}
244241
}
@@ -249,3 +246,32 @@ fun HTML.buildPermissionManagementPage(call: ApplicationCall, pluginInstance: Mo
249246
}
250247
}
251248
}
249+
250+
fun ApplicationCall.canGrantPermission(permissionId: String): Boolean {
251+
return canGrantPermission(parsePermission(permissionId))
252+
}
253+
254+
fun ApplicationCall.canGrantPermission(permissionRef: PermissionInstanceReference): Boolean {
255+
val plugin = application.plugin(ModelixAuthorization)
256+
val schema = plugin.config.permissionSchema
257+
val resources = generateSequence(permissionRef.resource) { it.parent }
258+
return resources.any {
259+
// hardcoded admin/owner to keep it simple and not having to introduce a permission schema for permissions
260+
val managers = listOf(
261+
PermissionInstanceReference("admin", it),
262+
PermissionInstanceReference("owner", it),
263+
)
264+
managers.any { it.isValid(schema) && plugin.hasPermission(this, it) }
265+
}
266+
}
267+
268+
fun ApplicationCall.checkCanGranPermission(id: String) {
269+
if (!canGrantPermission(id)) {
270+
val principal = principal<AccessTokenPrincipal>()
271+
throw NoPermissionException(principal, null, null, "${principal?.getUserName()} has no permission '$id'")
272+
}
273+
}
274+
275+
fun ApplicationCall.parsePermission(id: String): PermissionInstanceReference {
276+
return application.plugin(ModelixAuthorization).config.permissionSchema.let { PermissionParser(it) }.parse(id)
277+
}

‎authorization/src/main/kotlin/org/modelix/authorization/permissions/PermissionEvaluator.kt‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,10 @@ class PermissionEvaluator(val schemaInstance: SchemaInstance) {
3838
}
3939

4040
fun instantiatePermission(permissionId: PermissionParts): SchemaInstance.ResourceInstance.PermissionInstance {
41-
val permissionRef = parser.parse(permissionId)
41+
return instantiatePermission(parser.parse(permissionId))
42+
}
43+
44+
fun instantiatePermission(permissionRef: PermissionInstanceReference): SchemaInstance.ResourceInstance.PermissionInstance {
4245
val instance = schemaInstance.instantiatePermission(permissionRef)
4346
hasPermission(permissionRef) // permissions are instantiated during the check
4447
return instance

‎authorization/src/main/kotlin/org/modelix/authorization/permissions/SchemaInstance.kt‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
package org.modelix.authorization.permissions
22

3+
import org.modelix.authorization.UnknownPermissionException
4+
35
/**
46
* Instantiates the abstract schema with data from an actual system.
57
*/
@@ -69,9 +71,8 @@ class SchemaInstance(val schema: Schema) {
6971

7072
fun getOrCreatePermissionInstance(name: String): PermissionInstance {
7173
return permissions.getOrPut(name) {
72-
val permissionSchema = requireNotNull(resourceSchema.permissions[name]) {
73-
"Permission '$name' not found in $reference"
74-
}
74+
val permissionSchema = resourceSchema.permissions[name]
75+
if (permissionSchema == null) throw UnknownPermissionException("", unknownElement = name)
7576
PermissionInstance(permissionSchema, PermissionInstanceReference(name, reference))
7677
}
7778
}
@@ -158,4 +159,12 @@ data class PermissionInstanceReference(val permissionName: String, val resource:
158159
override fun toString(): String {
159160
return toPermissionParts().toString()
160161
}
162+
fun isValid(schema: Schema): Boolean {
163+
return try {
164+
PermissionParser(schema).parse(toPermissionParts())
165+
true
166+
} catch (ex: UnknownPermissionException) {
167+
false
168+
}
169+
}
161170
}

‎authorization/src/test/kotlin/org/modelix/authorization/PermissionManagementTest.kt‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
package org.modelix.authorization
2+
3+
import io.ktor.client.request.bearerAuth
4+
import io.ktor.client.request.forms.submitForm
5+
import io.ktor.http.HttpStatusCode
6+
import io.ktor.http.parameters
7+
import io.ktor.server.application.install
8+
import io.ktor.server.testing.ApplicationTestBuilder
9+
import io.ktor.server.testing.testApplication
10+
import org.modelix.authorization.permissions.buildPermissionSchema
11+
import kotlin.test.Test
12+
import kotlin.test.assertEquals
13+
14+
class PermissionManagementTest {
15+
private val schema = buildPermissionSchema {
16+
resource("server") {
17+
permission("admin")
18+
resource("repository") {
19+
parameter("name")
20+
permission("owner") {
21+
permission("write") {
22+
permission("read")
23+
}
24+
}
25+
}
26+
}
27+
}
28+
private val hmacKey = "unit-test"
29+
30+
private fun runTest(block: suspend ApplicationTestBuilder.() -> Unit) = testApplication {
31+
application {
32+
install(ModelixAuthorization) {
33+
permissionSchema = schema
34+
hmac512Key = hmacKey
35+
permissionManagementEnabled = true
36+
installStatusPages = true
37+
}
38+
}
39+
block()
40+
}
41+
42+
private fun createToken(user: String, vararg permissions: String): String {
43+
return ModelixJWTUtil().also { it.setHmac512Key(hmacKey) }.createAccessToken(user, permissions.toList())
44+
}
45+
46+
@Test
47+
fun `direct resource owner can grant permission`() = runTest {
48+
val response = client.submitForm(
49+
url = "http://localhost/permissions/grant",
50+
formParameters = parameters {
51+
append("userId", "userB")
52+
append("permissionId", "server/repository/my-repo/read")
53+
},
54+
block = {
55+
bearerAuth(createToken("userA", "server/repository/my-repo/owner"))
56+
},
57+
)
58+
assertEquals(HttpStatusCode.OK, response.status)
59+
}
60+
61+
@Test
62+
fun `indirect resource owner can grant permission`() = runTest {
63+
val response = client.submitForm(
64+
url = "http://localhost/permissions/grant",
65+
formParameters = parameters {
66+
append("userId", "userB")
67+
append("permissionId", "server/repository/my-repo/read")
68+
},
69+
block = {
70+
bearerAuth(createToken("userA", "server/admin"))
71+
},
72+
)
73+
assertEquals(HttpStatusCode.OK, response.status)
74+
}
75+
76+
@Test
77+
fun `non-owners cannot grant permissions`() = runTest {
78+
val response = client.submitForm(
79+
url = "http://localhost/permissions/grant",
80+
formParameters = parameters {
81+
append("userId", "userB")
82+
append("permissionId", "server/repository/my-repo/read")
83+
},
84+
block = {
85+
bearerAuth(createToken("userA", "server/repository/my-repo/write"))
86+
},
87+
)
88+
assertEquals(HttpStatusCode.Forbidden, response.status)
89+
}
90+
}

‎model-server/src/main/kotlin/org/modelix/model/server/ModelServerPermissionSchema.kt‎

Lines changed: 2 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ import org.modelix.model.lazy.RepositoryId
99
object ModelServerPermissionSchema {
1010
private const val MODEL_SERVER = "model-server"
1111
private const val ADMIN = "admin"
12-
private const val PERMISSION_SCHEMA = "permission-schema"
1312
private const val WRITE = "write"
1413
private const val READ = "read"
1514
private const val LEGACY_USER_DEFINED_ENTRIES = "legacy-user-defined-entries"
@@ -35,28 +34,17 @@ object ModelServerPermissionSchema {
3534
}
3635
}
3736

38-
resource(PERMISSION_SCHEMA) {
39-
permission(WRITE) {
40-
includedIn(MODEL_SERVER, ADMIN)
41-
permission(READ)
42-
}
43-
}
44-
4537
resource(LEGACY_USER_DEFINED_ENTRIES) {
46-
permission(READ) {
47-
includedIn(MODEL_SERVER, ADMIN)
48-
}
4938
permission(WRITE) {
5039
includedIn(MODEL_SERVER, ADMIN)
40+
permission(READ)
5141
}
5242
}
5343

5444
resource(LEGACY_GLOBAL_OBJECTS) {
55-
permission(READ) {
56-
includedIn(MODEL_SERVER, ADMIN)
57-
}
5845
permission(ADD) {
5946
includedIn(MODEL_SERVER, ADMIN)
47+
permission(READ)
6048
}
6149
}
6250

‎model-server/src/test/kotlin/org/modelix/model/server/LazyLoadingTest.kt‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ package org.modelix.model.server
22

33
import io.ktor.server.testing.ApplicationTestBuilder
44
import io.ktor.server.testing.testApplication
5-
import org.modelix.authorization.installAuthentication
65
import org.modelix.model.api.INode
76
import org.modelix.model.api.NullChildLink
87
import org.modelix.model.api.PBranch
@@ -39,7 +38,6 @@ class LazyLoadingTest {
3938

4039
private fun runTest(block: suspend ApplicationTestBuilder.() -> Unit) = testApplication {
4140
application {
42-
installAuthentication(unitTestMode = true)
4341
installDefaultServerPlugins()
4442
val store = InMemoryStoreClient()
4543
val repoManager = RepositoriesManager(store)

0 commit comments

Comments
 (0)