Jackie-Jiang commented on code in PR #13195:
URL: https://github.com/apache/pinot/pull/13195#discussion_r1617736311
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/AccessControl.java:
##########
@@ -31,32 +35,88 @@ public interface AccessControl extends
FineGrainedAccessControl {
/**
* First-step access control when processing broker requests. Decides
whether request is allowed to acquire resources
* for further processing. Request may still be rejected at table-level
later on.
- *
+ * The default implementation is kept to have backward compatibility with
the existing implementations
* @param requesterIdentity requester identity
*
* @return {@code true} if authorized, {@code false} otherwise
*/
+ @Deprecated
default boolean hasAccess(RequesterIdentity requesterIdentity) {
return true;
}
/**
- * Fine-grained access control on parsed broker request. May check table,
column, permissions, etc.
+ * First-step access control when processing broker requests. Decides
whether request is allowed to acquire resources
+ * for further processing. Request may still be rejected at table-level
later on.
+ * The default implementation returns a {@link BasicAuthorizationResultImpl}
with the result of the hasAccess() of
+ * the implementation
*
* @param requesterIdentity requester identity
+ *
+ * @return {@code AuthorizationResult} with the result of the access control
check
+ */
+ default AuthorizationResult authorize(RequesterIdentity requesterIdentity) {
+ return new BasicAuthorizationResultImpl(hasAccess(requesterIdentity));
+ }
+
+ /**
+ * Fine-grained access control on parsed broker request. May check table,
column, permissions, etc.
+ * The default implementation is kept to have backward compatibility with
the existing implementations
+ * @param requesterIdentity requester identity
* @param brokerRequest broker request (incl query)
*
* @return {@code true} if authorized, {@code false} otherwise
*/
- boolean hasAccess(RequesterIdentity requesterIdentity, BrokerRequest
brokerRequest);
+ @Deprecated
+ default boolean hasAccess(RequesterIdentity requesterIdentity, BrokerRequest
brokerRequest) {
+ throw new UnsupportedOperationException(
+ "Both hasAccess() and authorize() are not implemented . Do implement
authorize() method for new "
+ + "implementations.");
+ }
+
+ /**
+ * Verify access control on parsed broker request. May check table, column,
permissions, etc.
+ * The default implementation returns a {@link BasicAuthorizationResultImpl}
with the result of the hasAccess() of
+ * the implementation
+ *
+ * @param requesterIdentity requester identity
+ * @param brokerRequest broker request (incl query)
+ *
+ * @return {@code AuthorizationResult} with the result of the access control
check
+ */
+ default AuthorizationResult authorize(RequesterIdentity requesterIdentity,
BrokerRequest brokerRequest) {
+ return new BasicAuthorizationResultImpl(hasAccess(requesterIdentity,
brokerRequest));
+ }
/**
* Fine-grained access control on pinot tables.
+ * The default implementation is kept to have backward compatibility with
the existing implementations
*
* @param requesterIdentity requester identity
* @param tables Set of pinot tables used in the query. Table name can be
with or without tableType.
*
* @return {@code true} if authorized, {@code false} otherwise
*/
- boolean hasAccess(RequesterIdentity requesterIdentity, Set<String> tables);
+ @Deprecated
+ default boolean hasAccess(RequesterIdentity requesterIdentity, Set<String>
tables) {
+ throw new UnsupportedOperationException(
+ "Both hasAccess() and authorize() are not implemented . Do implement
authorize() method for new "
+ + "implementations.");
+ }
+
+ /**
+ * Verify access control on pinot tables.
+ * The default implementation returns a {@link TableAuthorizationResult}
with the result of the hasAccess() of the
+ * implementation
+ *
+ * @param requesterIdentity requester identity
+ * @param tables Set of pinot tables used in the query. Table name can be
with or without tableType.
+ *
+ * @return {@code TableAuthorizationResult} with the result of the access
control check
+ */
+ default TableAuthorizationResult authorize(RequesterIdentity
requesterIdentity, Set<String> tables) {
+ // Taking all tables when hasAccess Failed , to not break existing
implementations
+ // It will say all tables names failed AuthZ even only some failed AuthZ -
which is same as just boolean output
+ return new TableAuthorizationResult(hasAccess(requesterIdentity, tables) ?
new HashSet<>() : tables);
Review Comment:
```suggestion
return hasAccess(requesterIdentity, tables) ?
TableAuthorizationResult.success() : new TableAuthorizationResult(tables);
```
##########
pinot-spi/src/main/java/org/apache/pinot/spi/auth/BasicAuthorizationResultImpl.java:
##########
@@ -0,0 +1,118 @@
+/**
+ * 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 boolean _hasAccess;
+ private String _failureMessage;
Review Comment:
Suggest making both `final` and only allow setting them in constructor. Also
add a singleton for SUCCESS
##########
pinot-spi/src/main/java/org/apache/pinot/spi/auth/TableAuthorizationResult.java:
##########
@@ -0,0 +1,93 @@
+/**
+ * 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 Set<String> _failedTables;
Review Comment:
Suggest making this `final` and always set it via the constructor. Remove
`setFailedTables()` and `addFailedTable()`
##########
pinot-spi/src/main/java/org/apache/pinot/spi/auth/TableAuthorizationResult.java:
##########
@@ -0,0 +1,93 @@
+/**
+ * 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 Set<String> _failedTables;
+
+ public TableAuthorizationResult() {
Review Comment:
Suggest not exposing this API, and creating a `SUCCESS` singleton with
`_failedTables = Set.of()` (an immutable empty set), and always return it in
`success()` to reduce garbage
##########
pinot-spi/src/main/java/org/apache/pinot/spi/auth/TableAuthorizationResult.java:
##########
@@ -0,0 +1,93 @@
+/**
+ * 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 Set<String> _failedTables;
+
+ public TableAuthorizationResult() {
+ _failedTables = new HashSet<>();
+ }
+
+ public TableAuthorizationResult(Set<String> failedTables) {
+ setFailedTables(failedTables);
+ }
+
+ /**
+ * Creates a TableAuthorizationResult with no failed tables.
+ *
+ * @return a TableAuthorizationResult with no failed tables.
+ */
+ public static TableAuthorizationResult noFailureResult() {
+ return new TableAuthorizationResult();
+ }
+
+ @Override
+ public boolean hasAccess() {
+ return _failedTables.isEmpty();
+ }
+
+ public Set<String> getFailedTables() {
+ return _failedTables;
+ }
+
+ public void setFailedTables(Set<String> failedTables) {
+ _failedTables = new HashSet<>(failedTables);
+ ;
+ }
+
+ public void addFailedTable(String failedTable) {
+ _failedTables.add(failedTable);
+ }
+
+ /**
+ * Provides the failure message indicating which tables failed authorization.
+ *
+ * @return a string containing the failure message if there are failed
tables, otherwise an empty string.
+ */
+ @Override
+ public String getFailureMessage() {
+ if (hasAccess()) {
+ return StringUtils.EMPTY;
+ }
+ StringBuilder sb = new StringBuilder();
+ sb.append("Authorization Failed for tables: ");
+
+ List<String> failedTablesList = new ArrayList<>(_failedTables);
+ Collections.sort(failedTablesList); // Sort to make output deterministic
+
+ for (String table : failedTablesList) {
+ sb.append(table);
+ sb.append(", ");
+ }
+ return sb.toString().trim();
Review Comment:
```suggestion
List<String> failedTablesList = new ArrayList<>(_failedTables);
Collections.sort(failedTablesList); // Sort to make output deterministic
return "Authorization Failed for tables: " + failedTablesList;
```
##########
pinot-spi/src/main/java/org/apache/pinot/spi/auth/TableAuthorizationResult.java:
##########
@@ -0,0 +1,93 @@
+/**
+ * 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 Set<String> _failedTables;
+
+ public TableAuthorizationResult() {
+ _failedTables = new HashSet<>();
+ }
+
+ public TableAuthorizationResult(Set<String> failedTables) {
+ setFailedTables(failedTables);
+ }
+
+ /**
+ * Creates a TableAuthorizationResult with no failed tables.
+ *
+ * @return a TableAuthorizationResult with no failed tables.
+ */
+ public static TableAuthorizationResult noFailureResult() {
Review Comment:
Consider renaming it to `success()` and return a singleton instance
--
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]
