Jackie-Jiang commented on code in PR #13195:
URL: https://github.com/apache/pinot/pull/13195#discussion_r1610627153
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/AccessControl.java:
##########
@@ -42,21 +46,58 @@ default boolean hasAccess(RequesterIdentity
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);
+
Review Comment:
(format) Remove the empty line
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/AccessControl.java:
##########
@@ -42,21 +46,58 @@ default boolean hasAccess(RequesterIdentity
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);
+
+ default boolean hasAccess(RequesterIdentity requesterIdentity, BrokerRequest
brokerRequest) {
+ return true;
+ }
+
+ /**
+ * 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 verifyAccess(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);
+
Review Comment:
(format) Remove the empty line
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/AccessControl.java:
##########
@@ -42,21 +46,58 @@ default boolean hasAccess(RequesterIdentity
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);
+
+ default boolean hasAccess(RequesterIdentity requesterIdentity, BrokerRequest
brokerRequest) {
+ return true;
+ }
+
+ /**
+ * 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 verifyAccess(RequesterIdentity
requesterIdentity, BrokerRequest brokerRequest) {
Review Comment:
(minor) Since the result is called `AuthorizationResult`, consider renaming
the method to be `authorize()`?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/AccessControl.java:
##########
@@ -42,21 +46,58 @@ default boolean hasAccess(RequesterIdentity
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);
+
+ default boolean hasAccess(RequesterIdentity requesterIdentity, BrokerRequest
brokerRequest) {
+ return true;
Review Comment:
The high level idea is to only override one of `hasAccess` and
`verifyAccess`, and both of them can be used. So I think we should `return
verifyAccess(requesterIdentity, brokerRequest).hasAccess()`
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
