gianm commented on code in PR #17864:
URL: https://github.com/apache/druid/pull/17864#discussion_r2033797678
##########
processing/src/main/java/org/apache/druid/query/metadata/metadata/SegmentMetadataQuery.java:
##########
@@ -107,7 +109,19 @@ public SegmentMetadataQuery(
this.toInclude = toInclude == null ? new AllColumnIncluderator() :
toInclude;
this.merge = merge == null ? false : merge;
this.analysisTypes = analysisTypes;
- if (!(dataSource instanceof TableDataSource || dataSource instanceof
UnionDataSource)) {
+
+ if (dataSource.getClass().equals(RestrictedDataSource.class)) {
+ // Only no-restriction policy is allowed for segment metadata queries.
+ validateWithNoRestrictionPolicy((RestrictedDataSource) dataSource);
+ } else if (dataSource.getClass().equals(UnionDataSource.class)) {
+ // Verify that all children of the union data source are either table or
restricted data sources with no-restriction policy.
+ dataSource.getChildren()
Review Comment:
This check is too broad, since Union can have stuff other than Table as a
child. I think the old code had the same problem, so you didn't introduce this
problem, but you might as well fix it since you're reworking this part anyway.
##########
processing/src/main/java/org/apache/druid/query/metadata/metadata/SegmentMetadataQuery.java:
##########
@@ -245,6 +261,17 @@ public List<Interval> getIntervals()
return this.getQuerySegmentSpec().getIntervals();
}
+ private void validateWithNoRestrictionPolicy(RestrictedDataSource restricted)
+ {
+ if (!(restricted.getPolicy() instanceof NoRestrictionPolicy)) {
+ throw InvalidInput.exception(
Review Comment:
This should be category `FORBIDDEN` not `INVALID_INPUT`. Also, the persona
for this error should be User, so the message should be written as such. Couple
things about that:
- `NoRestrictionPolicy` and `SegmentMetadataQuery` are names of Java
classes, so those are more appropriate for the Developer persona rather than
the User. The User persona should see user-oriented things like
"segmentMetadata query type"
- The User does not have the ability to change their own policies, so
suggesting a policy change is not a helpful message for the user. Really, they
have no recourse here, it should just be a simple "you can't do that".
- We don't necessarily want to echo policies back to the user, they should
ideally be transparent. So I wouldn't include the `getPolicy` interpolated into
the message.
A suggested message:
> You do not have permission to run a segmentMetadata query on table[%s].
With `restricted.getBase()` interpolated into the `%s`.
--
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]
