cryptoe commented on code in PR #14944:
URL: https://github.com/apache/druid/pull/14944#discussion_r1327960139
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/resources/SqlStatementResource.java:
##########
@@ -610,7 +629,21 @@ private Optional<SqlStatementResult>
getStatementStatus(String queryId, String c
}
- private MSQControllerTask getMSQControllerTaskOrThrow(String queryId, String
currentUser)
+ /**
+ * This method contacts the overlord for the controller task and returns it
if the requested user has the necessary
+ * permissions to do so. A user has the necessary permissions if they
satisfy one of the following criterias:
+ * 1. They are the one who has submitted the query
+ * 2. They belong to a role containing the READ or WRITE permissions over
the STATE resource. For endpoints like GET,
+ * the user should have READ permission for the STATE resource, while for
endpoints like POST and DELETE, the user
+ * should have WRITE permission for the STATE resource (note: POST is only
required when the user submits the query
+ * so the currentUser should be equal to the queryUser, and we won't need
to check the permissions for STATE resource
+ * for them anyway)
+ */
+ private MSQControllerTask getMSQControllerTaskAndCheckPermission(
Review Comment:
```suggestion
* This method contacts the overlord for the controller task and checks if
the requested user has the
* necessary permissions. A user has the necessary permissions if one of
the following criteria is satisfied:
* 1. The user is the one who submitted the query
* 2. The user belongs to a role containing the READ or WRITE permissions
over the STATE resource. For endpoints like GET,
* the user should have READ permission for the STATE resource, while
for endpoints like DELETE, the user should have WRITE permission for the STATE
resource. ( Note: POST API does not need to check the state permissions since
the currentUser always equal to the queryUser.)
*/
private MSQControllerTask getMSQControllerTaskAndCheckPermission(
```
##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/sql/resources/SqlStatementResourceTest.java:
##########
@@ -935,14 +934,45 @@ public void testForbiddenRequest()
).getStatus()
);
Assert.assertEquals(
- Response.Status.FORBIDDEN.getStatusCode(),
+ Response.Status.ACCEPTED.getStatusCode(),
resource.deleteQuery(
RUNNING_SELECT_MSQ_QUERY,
makeExpectedReq(CalciteTests.SUPER_USER_AUTH_RESULT)
).getStatus()
);
}
+ @Test
+ public void testAPIBehaviourWithForbiddenUser()
Review Comment:
We should add the following test cases:
And one test case where the user has state READ perms ONLY
And one test case where the user has state WRITE perms only
And one test case where the user has both READ and WRITE.
In all cases the query is submitted by a different user.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
