Jackie-Jiang commented on code in PR #13195: URL: https://github.com/apache/pinot/pull/13195#discussion_r1619397017
########## pinot-spi/src/main/java/org/apache/pinot/spi/auth/TableAuthorizationResult.java: ########## @@ -0,0 +1,79 @@ +/** + * 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.pinot.spi.auth; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import org.apache.commons.lang.StringUtils; + + +/** + * Implementation of the AuthorizationResult interface that provides authorization results + * at the table level, including which tables failed authorization. + */ +public class TableAuthorizationResult implements AuthorizationResult { + + private static final TableAuthorizationResult SUCCESS = new TableAuthorizationResult(Set.of()); + private final Set<String> _failedTables; + + public TableAuthorizationResult() { + _failedTables = new HashSet<>(); + } + + public TableAuthorizationResult(Set<String> failedTables) { + _failedTables = new HashSet<>(failedTables); Review Comment: I don't think we need to clone a set ```suggestion _failedTables = failedTables; ``` ########## pinot-spi/src/main/java/org/apache/pinot/spi/auth/TableAuthorizationResult.java: ########## @@ -0,0 +1,79 @@ +/** + * 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.pinot.spi.auth; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import org.apache.commons.lang.StringUtils; + + +/** + * Implementation of the AuthorizationResult interface that provides authorization results + * at the table level, including which tables failed authorization. + */ +public class TableAuthorizationResult implements AuthorizationResult { + + private static final TableAuthorizationResult SUCCESS = new TableAuthorizationResult(Set.of()); + private final Set<String> _failedTables; + + public TableAuthorizationResult() { Review Comment: We can remove this constructor ########## pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java: ########## @@ -366,18 +368,22 @@ protected BrokerResponse handleRequest(long requestId, String query, @Nullable S BrokerRequest brokerRequest = CalciteSqlCompiler.convertToBrokerRequest(pinotQuery); BrokerRequest serverBrokerRequest = serverPinotQuery == pinotQuery ? brokerRequest : CalciteSqlCompiler.convertToBrokerRequest(serverPinotQuery); - boolean hasTableAccess = - accessControl.hasAccess(requesterIdentity, serverBrokerRequest) && accessControl.hasAccess(httpHeaders, - TargetType.TABLE, tableName, Actions.Table.QUERY); + AuthorizationResult authorizationResult = accessControl.authorize(requesterIdentity, serverBrokerRequest); + if (authorizationResult.hasAccess()) { + authorizationResult = BasicAuthorizationResultImpl.joinResults(authorizationResult, + accessControl.authorize(httpHeaders, TargetType.TABLE, tableName, Actions.Table.QUERY)); + } _brokerMetrics.addPhaseTiming(rawTableName, BrokerQueryPhase.AUTHORIZATION, System.nanoTime() - compilationEndTimeNs); - if (!hasTableAccess) { + if (!authorizationResult.hasAccess()) { _brokerMetrics.addMeteredTableValue(tableName, BrokerMeter.REQUEST_DROPPED_DUE_TO_ACCESS_ERROR, 1); - LOGGER.info("Access denied for request {}: {}, table: {}", requestId, query, tableName); + LOGGER.info("Access denied for request {}: {}, table: {}, reason :{}", requestId, query, tableName, + authorizationResult.getFailureMessage()); requestContext.setErrorCode(QueryException.ACCESS_DENIED_ERROR_CODE); - throw new WebApplicationException("Permission denied", Response.Status.FORBIDDEN); + throw new WebApplicationException("Permission denied . Reason : " + authorizationResult.getFailureMessage(), Review Comment: ```suggestion throw new WebApplicationException("Permission denied. Reason: " + authorizationResult.getFailureMessage(), ``` ########## pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java: ########## @@ -136,8 +138,12 @@ protected BrokerResponse handleRequest(long requestId, String query, @Nullable S queryPlanResult = queryEnvironment.explainQuery(query, sqlNodeAndOptions, requestId); String plan = queryPlanResult.getExplainPlan(); Set<String> tableNames = queryPlanResult.getTableNames(); - if (!hasTableAccess(requesterIdentity, tableNames, requestContext, httpHeaders)) { - throw new WebApplicationException("Permission denied", Response.Status.FORBIDDEN); + TableAuthorizationResult tableAuthorizationResult = + hasTableAccess(requesterIdentity, tableNames, requestContext, httpHeaders); + if (!tableAuthorizationResult.hasAccess()) { + throw new WebApplicationException( + "Permission denied . Message : " + tableAuthorizationResult.getFailureMessage(), Review Comment: ```suggestion "Permission denied. Reason: " + tableAuthorizationResult.getFailureMessage(), ``` ########## pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java: ########## @@ -366,18 +368,22 @@ protected BrokerResponse handleRequest(long requestId, String query, @Nullable S BrokerRequest brokerRequest = CalciteSqlCompiler.convertToBrokerRequest(pinotQuery); BrokerRequest serverBrokerRequest = serverPinotQuery == pinotQuery ? brokerRequest : CalciteSqlCompiler.convertToBrokerRequest(serverPinotQuery); - boolean hasTableAccess = - accessControl.hasAccess(requesterIdentity, serverBrokerRequest) && accessControl.hasAccess(httpHeaders, - TargetType.TABLE, tableName, Actions.Table.QUERY); + AuthorizationResult authorizationResult = accessControl.authorize(requesterIdentity, serverBrokerRequest); + if (authorizationResult.hasAccess()) { + authorizationResult = BasicAuthorizationResultImpl.joinResults(authorizationResult, Review Comment: We don't need to join results since the first auth already pass. Joining multiple auth result seems an uncommon operation ########## pinot-broker/src/main/java/org/apache/pinot/broker/broker/AuthenticationFilter.java: ########## @@ -82,9 +83,12 @@ public void filter(ContainerRequestContext requestContext) HttpRequesterIdentity httpRequestIdentity = HttpRequesterIdentity.fromRequest(request); - // default authorization handling - if (!accessControl.hasAccess(httpRequestIdentity)) { - throw new WebApplicationException("Failed access check for " + httpRequestIdentity.getEndpointUrl(), + AuthorizationResult authorizationResult = accessControl.authorize(httpRequestIdentity); + + if (!authorizationResult.hasAccess()) { + throw new WebApplicationException( + "Failed access check for " + httpRequestIdentity.getEndpointUrl() + " ,with reason: " Review Comment: ```suggestion "Failed access check for " + httpRequestIdentity.getEndpointUrl() + " with reason: " ``` ########## pinot-spi/src/main/java/org/apache/pinot/spi/auth/BasicAuthorizationResultImpl.java: ########## @@ -0,0 +1,103 @@ +/** + * 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.pinot.spi.auth; + +import org.apache.commons.lang3.StringUtils; + + +/** + * Implementation of the AuthorizationResult interface that provides basic + * authorization results including access status and failure messages. + */ +public class BasicAuthorizationResultImpl implements AuthorizationResult { + + private static final BasicAuthorizationResultImpl SUCCESS = new BasicAuthorizationResultImpl(true); + private final boolean _hasAccess; + private final String _failureMessage; + + /** + * Constructs a BasicAuthorizationResultImpl with the specified access status and failure message. + * + * @param hasAccess true if access is granted, false otherwise. + * @param failureMessage the failure message if access is denied. + */ + public BasicAuthorizationResultImpl(boolean hasAccess, String failureMessage) { + _hasAccess = hasAccess; + _failureMessage = failureMessage; + } + + /** + * Constructs a BasicAuthorizationResultImpl with the specified access status and an empty failure message. + * + * @param hasAccess true if access is granted, false otherwise. + */ + public BasicAuthorizationResultImpl(boolean hasAccess) { + _hasAccess = hasAccess; + _failureMessage = StringUtils.EMPTY; + } + + /** + * Creates a BasicAuthorizationResultImpl with access granted and no failure message. + * + * @return a BasicAuthorizationResultImpl with access granted and an empty failure message. + */ + public static BasicAuthorizationResultImpl success() { + return SUCCESS; + } + + /** + * Combines the results of two AuthorizationResult instances. + * If both results grant access, a new result with access granted and no failure message is returned. + * If either result denies access, a new result with access denied and a combined failure message is returned. + * + * @param result1 the first AuthorizationResult. + * @param result2 the second AuthorizationResult. + * @return a combined BasicAuthorizationResultImpl based on the two provided results. + */ + public static BasicAuthorizationResultImpl joinResults(AuthorizationResult result1, AuthorizationResult result2) { Review Comment: Suggest removing this API. When performing auth, we should stop after the first failure. I do not see the need of doing join ########## pinot-broker/src/main/java/org/apache/pinot/broker/broker/AuthenticationFilter.java: ########## @@ -82,9 +83,12 @@ public void filter(ContainerRequestContext requestContext) HttpRequesterIdentity httpRequestIdentity = HttpRequesterIdentity.fromRequest(request); - // default authorization handling - if (!accessControl.hasAccess(httpRequestIdentity)) { - throw new WebApplicationException("Failed access check for " + httpRequestIdentity.getEndpointUrl(), + AuthorizationResult authorizationResult = accessControl.authorize(httpRequestIdentity); + + if (!authorizationResult.hasAccess()) { + throw new WebApplicationException( + "Failed access check for " + httpRequestIdentity.getEndpointUrl() + " ,with reason: " Review Comment: Also suggest checking if failure message is empty first, then decide whether to append the reason part. Same for other places -- 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]
