Jackie-Jiang commented on code in PR #13195:
URL: https://github.com/apache/pinot/pull/13195#discussion_r1614054911
##########
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:
I just realized that `BasicAuthAccessControlFactory` and
`ZkBasicAuthAccessControlFactory` override both the methods, thus the problem I
described won't happen.
I like the idea of making the default `hasAccess()` throw
`UnsupportedOperationException` (we usually use this to represent unimplemented
method) which can force caller to switch to the new API if there are any. In
that case we might also want to remove the override in
`BasicAuthAccessControlFactory` and `ZkBasicAuthAccessControlFactory`
--
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]
