eaugene commented on code in PR #13195:
URL: https://github.com/apache/pinot/pull/13195#discussion_r1612966022
##########
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:
> 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`
I don't think this would happen .
**Reason :**
We are making both implementations of hasAccess() default only with this
change . So by that all existing implementations would already have
`hasAccess()` overridden .
Only chance of getting `true` is any new implementation of `AccessControl`
missing to override both `hasAccess()` and `authorize()` ( previously
`verifyAccess()` ) . This chance of miss is again only going to in the
transition phase till we get `hasAccess()` removed and make `authorize()`
non-default hopefully in upcoming major pinot version future releases.
Please correct me if I misunderstood the scenario you are trying to say
**Further thoughts :**
My concern with making them call each other is , any new implementation of
`AccessControl` ( outside of pinot repo & by mistake not having `hasAccess()`
or `authorize()` implemented ) and client calling either of `hasAccess()` or
`authorize()` of that undesirable implementation will cause
`StackOverflowError` .
As its currently marked as `@Deprecated` , the compiler would throw warning
.
Further more if any clients calling implementations of `hasAccess()` would
work normally .
To make it more rigid , instead of returning `true` from `hasAccess()`
default implementation , we can probably throw an `DeprecationException()` /
`NotImplementedException()` [ I prefer the former exception ] . LMK if this
would work @Jackie-Jiang ?
--
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]
