Vamsi-klu commented on code in PR #18975:
URL: https://github.com/apache/pinot/pull/18975#discussion_r3817440696
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/access/BaseBasicAuthAccessControl.java:
##########
@@ -36,8 +37,14 @@ public final boolean protectAnnotatedOnly() {
}
@Override
- public final boolean hasAccess(String tableName, AccessType accessType,
HttpHeaders httpHeaders,
+ public final boolean hasAccess(@Nullable String tableName, AccessType
accessType, HttpHeaders httpHeaders,
String endpointUrl) {
+ // A null table name means the request named no table, which makes it
cluster-wide however the caller reached this
+ // overload. Route it to the cluster check so the scope rule cannot be
sidestepped by omitting the table: callers
+ // include AccessControl's own default cluster overload and the
/auth/verify probe, whose table name is optional.
+ if (tableName == null) {
+ return hasAccess(accessType, httpHeaders, endpointUrl);
+ }
Review Comment:
Agreed, and it belongs in that list. v1 GET /auth/verify with no tableName
now hits the cluster check, so a table-scoped principal gets false where it
used to get true. UI login stays on v2, so that gate is fine. Third-party
probes still on v1 will read it as a credential failure. Called that out in the
description and in the helm comment.
--
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]
