Jackie-Jiang commented on code in PR #13195:
URL: https://github.com/apache/pinot/pull/13195#discussion_r1612395993
##########
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:
+1 on marking it `@Deprecated`
Making it return `true` by default won't cause problem within Pinot, but my
concern is that if there are client code (outside of Pinot repo) still calling
`hasAccess()`, that one will break because all the existing implementation will
return `true`. That is why it is safer to make them call each other so that as
long as one of them is overridden, both of them can work properly
--
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]
