You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "LakshSingla (via GitHub)" <gi...@apache.org> on 2023/09/06 04:28:03 UTC
[PR] Allow users with STATE permissions to read and write the state APIs for querying with deep storage (druid)
LakshSingla opened a new pull request, #14944:
URL: https://github.com/apache/druid/pull/14944
### Description
Currently, only the user who has submitted the async query has permission to interact with the status APIs for that async query. However, often we want an administrator to interact with these resources as well.
Druid handles these with the `STATE` resource traditionally, and if the requesting user has necessary permissions on it as well, alternatively, they should be allowed to interact with the status APIs, irrespective of whether they are the submitter of the query. This PR adds the same behavior to these async API resources as well, as documented below:
(Path is relative to `/druid/v2/sql/statements/`)
| Method and Path | Original Behavior | New Behaviour (In conjunction with the original behavior) |
|--------|--------|--------|
| `GET /enabled` | Unguarded | Unguarded |
| `POST /` | Doesn't matter since caller will be the submitter of the query | Doesn't matter since caller will be the submitter of the query |
| `GET /{id}` | Original submitter can access the status | Original Behavior + Users with `READ` permissions on `STATE` |
| `GET /{id}/results` | Original submitter can access the results | Original Behavior + Users with `READ` permissions on `STATE` |
| `DELETE /{id}` | Original submitter can cancel the query | Original Behavior + Users with `WRITE` permissions on `STATE` |
<!--
In each section, please describe design decisions made, including:
- Choice of algorithms
- Behavioral aspects. What configuration values are acceptable? How are corner cases and error conditions handled, such as when there are insufficient resources?
- Class organization and design (how the logic is split between classes, inheritance, composition, design patterns)
- Method organization and design (how the logic is split between methods, parameters and return types)
- Naming (class, method, API, configuration, HTTP endpoint, names of emitted metrics)
-->
<!-- It's good to describe an alternative design (or mention an alternative name) for every design (or naming) decision point and compare the alternatives with the designs that you've implemented (or the names you've chosen) to highlight the advantages of the chosen designs and names. -->
<!-- If there was a discussion of the design of the feature implemented in this PR elsewhere (e. g. a "Proposal" issue, any other issue, or a thread in the development mailing list), link to that discussion from this PR description and explain what have changed in your final design compared to your original proposal or the consensus version in the end of the discussion. If something hasn't changed since the original discussion, you can omit a detailed discussion of those aspects of the design here, perhaps apart from brief mentioning for the sake of readability of this PR description. -->
<!-- Some of the aspects mentioned above may be omitted for simple and small changes. -->
#### Release note
<!-- Give your best effort to summarize your changes in a couple of sentences aimed toward Druid users.
If your change doesn't have end user impact, you can skip this section.
For tips about how to write a good release note, see [Release notes](https://github.com/apache/druid/blob/master/CONTRIBUTING.md#release-notes).
-->
<hr>
##### Key changed/added classes in this PR
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Allow users with STATE permissions to read and write the state APIs for querying with deep storage (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14944:
URL: https://github.com/apache/druid/pull/14944#discussion_r1322420558
##########
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:
We can access the status of the query therefore it is no longer forbidden.
Since it is a running query, requesting the results returns in the BAD_REQUEST response code (as is the case if the user submitting the query requests the results before completion)
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Allow users with STATE permissions to read and write the state APIs for querying with deep storage (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe merged PR #14944:
URL: https://github.com/apache/druid/pull/14944
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Allow users with STATE permissions to read and write the state APIs for querying with deep storage (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Allow users with STATE permissions to read and write the state APIs for querying with deep storage (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Allow users with STATE permissions to read and write the state APIs for querying with deep storage (druid)
Posted by "LakshSingla (via GitHub)" <gi...@apache.org>.
LakshSingla commented on code in PR #14944:
URL: https://github.com/apache/druid/pull/14944#discussion_r1322419232
##########
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:
Good catch
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org