FrankChen021 commented on code in PR #15645: URL: https://github.com/apache/druid/pull/15645#discussion_r1493912377
########## processing/src/main/java/org/apache/druid/math/expr/LookupNameExtractionShuttle.java: ########## @@ -0,0 +1,43 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.math.expr; + + +public class LookupNameExtractionShuttle implements Expr.Shuttle +{ + private String lookupName; + + public String getLookupName() + { + return lookupName; + } + + @Override + public Expr visit(Expr expr) + { + if (expr instanceof StringExpr) { + this.lookupName = expr.stringify(); + if (lookupName.startsWith("'") && lookupName.endsWith("'")) { + this.lookupName = lookupName.substring(1, lookupName.length() - 1); Review Comment: Not sure if an empty string is acceptable to run here. If it's, then the substring function call throws exception ########## server/src/main/java/org/apache/druid/server/QueryLifecycle.java: ########## @@ -242,6 +267,70 @@ public Access authorize(HttpServletRequest req) ); } + private String getLookUpName(DimensionSpec dimension) + { + if (dimension instanceof LookupDimensionSpec) { + return ((LookupDimensionSpec) dimension).getName(); + } else if (dimension instanceof ExtractionDimensionSpec) { + ExtractionFn extractionFn = ((ExtractionDimensionSpec) dimension).getExtractionFn(); + if (extractionFn instanceof RegisteredLookupExtractionFn) { + return ((RegisteredLookupExtractionFn) extractionFn).getLookupName(); + } + } + return null; + } + + private Set<String> collectLookupNames() + { + if (authConfig.isEnableAuthorizeLookupDirectly()) { + Set<String> lookupNames = new HashSet<>(); + if (baseQuery instanceof GroupByQuery) { + GroupByQuery groupByQuery = (GroupByQuery) baseQuery; + lookupNames.addAll(getLookupNameFromDimension(groupByQuery)); + lookupNames.addAll(getLookUpNameFromVirtualColumn()); + } else if (baseQuery instanceof TopNQuery) { + TopNQuery topNQuery = (TopNQuery) baseQuery; + DimensionSpec dimension = topNQuery.getDimensionSpec(); + lookupNames.addAll(getLookUpNameFromVirtualColumn()); + String lookupName = getLookUpName(dimension); + if (!Strings.isNullOrEmpty(lookupName)) { + lookupNames.add(lookupName); + } + } else if (baseQuery instanceof ScanQuery) { + lookupNames.addAll(getLookUpNameFromVirtualColumn()); + } Review Comment: there're some other queries like `TimeseriesQuery`, don't need to perform authorization on these queries? ########## sql/src/main/java/org/apache/druid/sql/calcite/planner/SqlResourceCollectorShuttle.java: ########## @@ -69,6 +69,16 @@ public SqlNode visit(SqlCall call) )); } + if (call.getOperator().getName().equals("LOOKUP")) { + String lookupName = call.getOperandList().get(1).toString(); + if (lookupName.startsWith("'") && lookupName.endsWith("'")) { + lookupName = lookupName.substring(1, lookupName.length() - 1); Review Comment: the question, can the lookupName here be an empty string? ########## server/src/test/java/org/apache/druid/server/QueryLifecycleTest.java: ########## @@ -181,6 +252,122 @@ public void testRunSimpleUnauthorized() lifecycle.runSimple(query, authenticationResult, new Access(false)); } + @Test + public void testScanQuery_authorized() Review Comment: Do we have tests for `enableAuthorizeLookupDirectly = 0`? And where is this field set to zero in these test cases? ########## integration-tests/src/test/java/org/apache/druid/tests/security/ITBasicAuthConfigurationTest.java: ########## @@ -62,13 +61,9 @@ public class ITBasicAuthConfigurationTest extends AbstractAuthConfigurationTest @BeforeClass public void before() throws Exception { - // ensure that auth_test segments are loaded completely, we use them for testing system schema tables - ITRetryUtil.retryUntilTrue( - () -> coordinatorClient.areSegmentsLoaded("auth_test"), "auth_test segment load" - ); - Review Comment: why is this removed? ########## server/src/main/java/org/apache/druid/server/QueryLifecycle.java: ########## @@ -242,6 +267,70 @@ public Access authorize(HttpServletRequest req) ); } + private String getLookUpName(DimensionSpec dimension) Review Comment: from the review perspective, I'm suggesting to move this method after the following `collectLookupNames` because it's more related to other two getLookupNameXXX methods below, and more easier to understand the code ########## server/src/test/java/org/apache/druid/server/QueryLifecycleTest.java: ########## @@ -181,6 +252,122 @@ public void testRunSimpleUnauthorized() lifecycle.runSimple(query, authenticationResult, new Access(false)); } + @Test + public void testScanQuery_authorized() + { + EasyMock.expect(queryConfig.getContext()).andReturn(ImmutableMap.of()).anyTimes(); + EasyMock.expect(authenticationResult.getIdentity()).andReturn(IDENTITY).anyTimes(); + EasyMock.expect(authenticationResult.getAuthorizerName()).andReturn(AUTHORIZER).anyTimes(); + EasyMock.expect(authorizer.authorize(authenticationResult, new Resource(DATASOURCE, ResourceType.DATASOURCE), Action.READ)) + .andReturn(Access.OK).times(2); Review Comment: Can add a comment to state why the times should be 2 -- 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]
