eaugene commented on code in PR #13195:
URL: https://github.com/apache/pinot/pull/13195#discussion_r1611104017
##########
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 thought a bit about this before arriving at an option to have the default
set for both `hasAccess()` & `verifyAccess()` ( now `authorize()` ).
Now the RequestHandlers are calling `verifyAccess()`, and the default
implementation of `verifyAccess()` now calls `hasAccess()` - This is a safe
case for all existing implementations of AccessControl to work ( added a UT
for this specific scenario ).
about having default for `hasAccess()` - The goal was to move away from
`hasAccess()` to `VerifyAccess()` for all implementations . So as not to cause
any friction during this intermediary phase, I added a default implementation.
I'll proceed to mark `hasAccess()` as `@Deprecated`. LMK your thoughts?
--
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]
