You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/09/11 10:49:03 UTC
[GitHub] [druid] clintropolis opened a new pull request #11692: refactor sql authorization to get resource type from schema, resource type to be string
clintropolis opened a new pull request #11692:
URL: https://github.com/apache/druid/pull/11692
### Description
This PR introduces the refactor detailed in proposal #11690, which consists of two main changes.
The first is that `ResourceType` enum is no longer used by `Resource` in favor of using a string. `ResourceType` has been repurposed into a registry of built-in types (and the set of known types can be extended by registering custom types with this registry).
The second is that `NamedSchema` has been expanded to include a new method, `getSchemaResourceType` which returns the newly string resource type that should be used to authorize queries against that schema, and this mapping is pushed down into `SqlResourceCollectorShuttle`, which has been modified to construct the set of `Resource` to authorize by checking for table schemas which have a non-null type instead of explicitly only looking for the 'druid' and 'view' schema. Currently only `NamedDruidSchema` and `NamedViewSchema` implement this method with a non-null return value, so the behavior should be all-around unchanged at this time.
A follow-up PR will add `NamedSystemSchema` to this list so that the system schema tables can be directly authorized (on top of the per row filtering that occurs based on the other permissions of the user), and `NamedLookupSchema` could similarly be added in the future to provide authorization against lookup queries (though additional work would need to be done to fully authorize lookups used via expressions or extraction functions).
<hr>
This PR has:
- [x] been self-reviewed.
- [x] 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.)
- [x] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [x] 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.
- [x] 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
[GitHub] [druid] zachjsh commented on a change in pull request #11692: refactor sql authorization to get resource type from schema, resource type to be string
Posted by GitBox <gi...@apache.org>.
zachjsh commented on a change in pull request #11692:
URL: https://github.com/apache/druid/pull/11692#discussion_r708916714
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/SqlResourceCollectorShuttle.java
##########
@@ -69,10 +68,9 @@ public SqlNode visit(SqlIdentifier id)
// 'schema'.'identifier'
if (qualifiedNameParts.size() == 2) {
final String schema = qualifiedNameParts.get(0);
- if (druidSchemaName.equals(schema)) {
- resources.add(new Resource(qualifiedNameParts.get(1), ResourceType.DATASOURCE));
- } else if (NamedViewSchema.NAME.equals(schema)) {
- resources.add(new Resource(qualifiedNameParts.get(1), ResourceType.VIEW));
+ final String resourceType = plannerContext.getSchemaResourceType(schema);
+ if (resourceType != null) {
Review comment:
Oh I see, the default implementation for the other schema types will return null resource type. nvm, all good.
--
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
[GitHub] [druid] zachjsh commented on a change in pull request #11692: refactor sql authorization to get resource type from schema, resource type to be string
Posted by GitBox <gi...@apache.org>.
zachjsh commented on a change in pull request #11692:
URL: https://github.com/apache/druid/pull/11692#discussion_r708909760
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/schema/InformationSchema.java
##########
@@ -483,18 +479,21 @@ private static TableMacro getView(final SchemaPlus schemaPlus, final String func
final AuthenticationResult authenticationResult
)
{
- if (druidSchemaName.equals(subSchema.getName())) {
- // The "druid" schema's tables represent Druid datasources which require authorization
+ final NamedSchema schema = namedSchemaMap.get(subSchema.getName());
+ if (schema != null && schema.getSchemaResourceType() != null) {
Review comment:
nit: this functions code and getAuthorizedFunctionNamesFromSubSchema look very similar. Could do a slight refactor and reduce code here it seems. Not necessary though.
--
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
[GitHub] [druid] jihoonson commented on a change in pull request #11692: refactor sql authorization to get resource type from schema, resource type to be string
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #11692:
URL: https://github.com/apache/druid/pull/11692#discussion_r709635298
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerContext.java
##########
@@ -179,6 +185,12 @@ public DateTimeZone getTimeZone()
return localNow.getZone();
}
+ @Nullable
+ public String getSchemaResourceType(String schema)
Review comment:
This seems to return the resourceType of the resources in the schema, rather than the resourceType of the schema itself. This behavior is currently correct because all resources should be the same type in the same schema. However, I can imagine that the resources of different types can be in the same schema in the future as what other databases do. In that case, we will want to check the resourceType per resource. How about adding something like `getResourceType(String schema, String resource)` instead?
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/schema/NamedSchema.java
##########
@@ -32,6 +34,12 @@
*/
String getSchemaName();
+ @Nullable
+ default String getSchemaResourceType()
Review comment:
How about something like `getResourceType(String resource)` as I suggested in my other comment? Even though most implementations will ignore the `resource` parameter for now, it will be useful once we allow different resource types in the same schema.
--
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
[GitHub] [druid] clintropolis commented on pull request #11692: refactor sql authorization to get resource type from schema, resource type to be string
Posted by GitBox <gi...@apache.org>.
clintropolis commented on pull request #11692:
URL: https://github.com/apache/druid/pull/11692#issuecomment-921941958
thanks for review @zachjsh and @jihoonson!
--
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
[GitHub] [druid] zachjsh commented on a change in pull request #11692: refactor sql authorization to get resource type from schema, resource type to be string
Posted by GitBox <gi...@apache.org>.
zachjsh commented on a change in pull request #11692:
URL: https://github.com/apache/druid/pull/11692#discussion_r708532764
##########
File path: extensions-core/druid-basic-security/src/test/java/org/apache/druid/security/BasicAuthUtilsTest.java
##########
@@ -92,7 +93,7 @@ public void testPermissionSerdeIsChillAboutUnknownEnumStuffs() throws JsonProces
"resourceAction",
ImmutableMap.of(
"resource",
- ImmutableMap.of("name", "bar", "type", "UNKNOWN"),
+ ImmutableMap.of("name", "bar", "type", "CUSTOM"),
Review comment:
nit: why not use customRead here as you have used fooRead online 89 above?
--
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
[GitHub] [druid] zachjsh commented on a change in pull request #11692: refactor sql authorization to get resource type from schema, resource type to be string
Posted by GitBox <gi...@apache.org>.
zachjsh commented on a change in pull request #11692:
URL: https://github.com/apache/druid/pull/11692#discussion_r708914196
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/schema/NamedSchema.java
##########
@@ -32,6 +34,12 @@
*/
String getSchemaName();
+ @Nullable
+ default String getSchemaResourceType()
Review comment:
Would be good to add javadoc here and describe where this is used and the importance of returning non-null if authorization checks are needed on the schema, if my understanding is correct.
--
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
[GitHub] [druid] clintropolis merged pull request #11692: refactor sql authorization to get resource type from schema, resource type to be string
Posted by GitBox <gi...@apache.org>.
clintropolis merged pull request #11692:
URL: https://github.com/apache/druid/pull/11692
--
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
[GitHub] [druid] zachjsh commented on pull request #11692: refactor sql authorization to get resource type from schema, resource type to be string
Posted by GitBox <gi...@apache.org>.
zachjsh commented on pull request #11692:
URL: https://github.com/apache/druid/pull/11692#issuecomment-919773951
Looks good overall, just a few questions in comments left. Also, does this framework allow authorization for only SQL based queries, or also native queries too? Also will it work with JDBC?
--
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
[GitHub] [druid] zachjsh commented on a change in pull request #11692: refactor sql authorization to get resource type from schema, resource type to be string
Posted by GitBox <gi...@apache.org>.
zachjsh commented on a change in pull request #11692:
URL: https://github.com/apache/druid/pull/11692#discussion_r708882212
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/SqlResourceCollectorShuttle.java
##########
@@ -69,10 +68,9 @@ public SqlNode visit(SqlIdentifier id)
// 'schema'.'identifier'
if (qualifiedNameParts.size() == 2) {
final String schema = qualifiedNameParts.get(0);
- if (druidSchemaName.equals(schema)) {
- resources.add(new Resource(qualifiedNameParts.get(1), ResourceType.DATASOURCE));
- } else if (NamedViewSchema.NAME.equals(schema)) {
- resources.add(new Resource(qualifiedNameParts.get(1), ResourceType.VIEW));
+ final String resourceType = plannerContext.getSchemaResourceType(schema);
+ if (resourceType != null) {
Review comment:
Before it seemed that only DATASOURCE and VIEW being mapped to resources. How do we ensure this old behavior if desired?
--
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
[GitHub] [druid] clintropolis commented on a change in pull request #11692: refactor sql authorization to get resource type from schema, resource type to be string
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #11692:
URL: https://github.com/apache/druid/pull/11692#discussion_r709902430
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerContext.java
##########
@@ -179,6 +185,12 @@ public DateTimeZone getTimeZone()
return localNow.getZone();
}
+ @Nullable
+ public String getSchemaResourceType(String schema)
Review comment:
i like this, changed :+1:
--
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
[GitHub] [druid] clintropolis commented on a change in pull request #11692: refactor sql authorization to get resource type from schema, resource type to be string
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #11692:
URL: https://github.com/apache/druid/pull/11692#discussion_r710323191
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/SqlResourceCollectorShuttle.java
##########
@@ -65,12 +65,13 @@ public SqlNode visit(SqlIdentifier id)
// this should not probably be null if the namespace was not null,
if (validatorTable != null) {
List<String> qualifiedNameParts = validatorTable.getQualifiedName();
- // 'schema'.'identifier'
+ // 'schema'.'identifier' d
Review comment:
oof i bet i started typing something before switching active windows, will fix heh
--
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
[GitHub] [druid] jihoonson commented on a change in pull request #11692: refactor sql authorization to get resource type from schema, resource type to be string
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #11692:
URL: https://github.com/apache/druid/pull/11692#discussion_r710312777
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/SqlResourceCollectorShuttle.java
##########
@@ -65,12 +65,13 @@ public SqlNode visit(SqlIdentifier id)
// this should not probably be null if the namespace was not null,
if (validatorTable != null) {
List<String> qualifiedNameParts = validatorTable.getQualifiedName();
- // 'schema'.'identifier'
+ // 'schema'.'identifier' d
Review comment:
typo?
--
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
[GitHub] [druid] zachjsh commented on a change in pull request #11692: refactor sql authorization to get resource type from schema, resource type to be string
Posted by GitBox <gi...@apache.org>.
zachjsh commented on a change in pull request #11692:
URL: https://github.com/apache/druid/pull/11692#discussion_r708540274
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerContext.java
##########
@@ -179,6 +185,12 @@ public DateTimeZone getTimeZone()
return localNow.getZone();
}
+ @Nullable
+ public String getSchemaResourceType(String schema)
Review comment:
think there would be a use case to have multiple resource types for a given schema?
--
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
[GitHub] [druid] clintropolis commented on pull request #11692: refactor sql authorization to get resource type from schema, resource type to be string
Posted by GitBox <gi...@apache.org>.
clintropolis commented on pull request #11692:
URL: https://github.com/apache/druid/pull/11692#issuecomment-920704924
>Also, does this framework allow authorization for only SQL based queries, or also native queries too? Also will it work with JDBC?
Yeah, this only impacts how the set of `Resource` for SQL queries are computed (including JDBC), native queries still compute this from the `DataSource`.
It doesn't actually change anything about the Druid authorization model though, so SQL and JSON queries should still require the same set of permissions for the same query, though SQL has some additional things like system tables and views that don't have a native equivalent (yet at least).
Any additional authorization we add in the future will need to be sure to be hooked into both SQL and native queries however, since they compute their `Resource` sets to check differently - `SqlLifecycle.validateAndAuthorize` vs `QueryLifecycle.authorize` (`SqlLifecycle` actually has one or more `QueryLifecycle` to run the native queries that the SQL planned to, but since they have already been authorized when validating the SQL, they use a pre-authorized path through `QueryLifecycle`)
--
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
[GitHub] [druid] clintropolis commented on a change in pull request #11692: refactor sql authorization to get resource type from schema, resource type to be string
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #11692:
URL: https://github.com/apache/druid/pull/11692#discussion_r708605582
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerContext.java
##########
@@ -179,6 +185,12 @@ public DateTimeZone getTimeZone()
return localNow.getZone();
}
+ @Nullable
+ public String getSchemaResourceType(String schema)
Review comment:
Hmm, I can't really think of a use case off the top of my head, but I guess it would be easy enough to support it. The change would go here https://github.com/apache/druid/pull/11692/files#diff-efcf39975403e19080cc5cd35af9a573608177c5ead5420ba3390c2268de0da4R38 to make it return a set instead, and then Map<String, Set<String>> through to https://github.com/apache/druid/pull/11692/files#diff-630eb92856c1b8dd3034980b486124e31952c8eef0a9195bbacb9bbe310a3023R71 which would just add all of the items in the set. https://github.com/apache/druid/pull/11692/files#diff-902bbc6a46aa1001ed51739a48b9847682c9450d7c122ce2d917370c98d4e3f9R482 would also need updated.
--
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
[GitHub] [druid] zachjsh commented on a change in pull request #11692: refactor sql authorization to get resource type from schema, resource type to be string
Posted by GitBox <gi...@apache.org>.
zachjsh commented on a change in pull request #11692:
URL: https://github.com/apache/druid/pull/11692#discussion_r708873102
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerContext.java
##########
@@ -179,6 +185,12 @@ public DateTimeZone getTimeZone()
return localNow.getZone();
}
+ @Nullable
+ public String getSchemaResourceType(String schema)
Review comment:
if no use case, then no worries, I couldnt think of a case either. Think ok for now, up to you if you want to add support for that here now.
--
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