cryptoe commented on code in PR #14944:
URL: https://github.com/apache/druid/pull/14944#discussion_r1322413379
##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/sql/resources/SqlStatementResourceTest.java:
##########
@@ -916,17 +917,17 @@ public void testMSQInsertRunningQuery()
}
@Test
- public void testForbiddenRequest()
Review Comment:
Can you also add a test case for forbidden users.
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/resources/SqlStatementResource.java:
##########
@@ -620,15 +653,30 @@ private MSQControllerTask
getMSQControllerTaskOrThrow(String queryId, String cur
.getQuery()
.getContext()
.get(MSQTaskQueryMaker.USER_KEY));
- if (currentUser == null || !currentUser.equals(queryUser)) {
- throw new ForbiddenException(StringUtils.format(
- "The current user[%s] cannot view query id[%s] since the query is
owned by user[%s]",
- currentUser,
- queryId,
- queryUser
- ));
+
+ String currentUser = authenticationResult.getIdentity();
+
+
+ if (currentUser != null && currentUser.equals(queryUser)) {
+ return msqControllerTask;
+ }
+
+ Access access = AuthorizationUtils.authorizeAllResourceActions(
+ authenticationResult,
+ Collections.singletonList(new ResourceAction(Resource.STATE_RESOURCE,
forAction)),
+ authorizerMapper
+ );
+
+ if (access.isAllowed()) {
+ return msqControllerTask;
}
- return msqControllerTask;
+
+ throw new ForbiddenException(StringUtils.format(
+ "The current user[%s] cannot view query id[%s] since the query is
owned by user[%s]",
+ currentUser,
+ queryId,
+ queryUser
+ ));
Review Comment:
```suggestion
throw new ForbiddenException(StringUtils.format(
"The current user[%s] cannot view query id[%s] since the query is
owned by another user",
currentUser,
queryId
));
```
We should probably not expose the query user to the current user
##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/sql/resources/SqlStatementResourceTest.java:
##########
@@ -916,17 +917,17 @@ public void testMSQInsertRunningQuery()
}
@Test
- public void testForbiddenRequest()
+ public void testAPIBehaviourWithSuperUsers()
{
Assert.assertEquals(
- Response.Status.FORBIDDEN.getStatusCode(),
+ Response.Status.OK.getStatusCode(),
resource.doGetStatus(
RUNNING_SELECT_MSQ_QUERY,
makeExpectedReq(CalciteTests.SUPER_USER_AUTH_RESULT)
).getStatus()
);
Assert.assertEquals(
- Response.Status.FORBIDDEN.getStatusCode(),
+ Response.Status.BAD_REQUEST.getStatusCode(),
Review Comment:
Why has this changed from forbidden to bad request ?
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/resources/SqlStatementResource.java:
##########
@@ -620,15 +653,30 @@ private MSQControllerTask
getMSQControllerTaskOrThrow(String queryId, String cur
.getQuery()
.getContext()
.get(MSQTaskQueryMaker.USER_KEY));
- if (currentUser == null || !currentUser.equals(queryUser)) {
- throw new ForbiddenException(StringUtils.format(
- "The current user[%s] cannot view query id[%s] since the query is
owned by user[%s]",
- currentUser,
- queryId,
- queryUser
- ));
+
+ String currentUser = authenticationResult.getIdentity();
+
Review Comment:
Nit: extra spaces.
--
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]
