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 baseQueryHandler is calling verifyAccess(), and the default
implementation of verifyAccess() now calls hasAccess() - This is a safe case of
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]
