xiangfu0 commented on code in PR #18975:
URL: https://github.com/apache/pinot/pull/18975#discussion_r4244914163
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthPrincipal.java:
##########
@@ -62,6 +74,21 @@ public boolean hasTable(String tableName) {
return isTableIncluded(tableName) && isTableNotExcluded(tableName);
}
+ /// Returns whether this principal is scoped to every table, i.e. it carries
neither an allow-list nor an
+ /// exclude-list. Access control implementations use this to decide requests
that name no table, so the return value
+ /// must satisfy: `true` implies [#hasTable(String)] holds for every table
name. A subclass that narrows table scope
+ /// by any other means — in particular by overriding [#hasTable(String)] —
must override this method to match, or it
+ /// will report unrestricted scope while denying individual tables.
+ ///
+ /// Table lists are the signal rather than [ZkBasicAuthPrincipal]'s
`RoleType`. The static factory has no role, both
+ /// controller factories share this check, and an ADMIN that also carries an
allow-list is still scoped. Granting
+ /// cluster access by role would let that principal reach cluster state
outside the list. The bootstrapped admin
+ /// is bootstrapped with a null `UserConfig#getTables()`, which normalizes
to an empty set here, so
+ /// `RoleType.ADMIN` without a table list still passes.
+ public boolean hasUnrestrictedTableAccess() {
Review Comment:
Retaining the existing subclass contract for compatibility, as explained in
the previous thread:
https://github.com/apache/pinot/pull/18975#discussion_r4244764956
Making both methods final would enforce the table-list policy, but hasTable
was already overridable before this PR; existing custom principal subclasses
overriding it would no longer compile or link. Making only
hasUnrestrictedTableAccess final would leave the mismatch possible while
preventing a subclass from reflecting its custom restriction in the cluster
gate. The method Javadoc and PR migration notes require subclasses narrowing
hasTable to override hasUnrestrictedTableAccess as well. The in-tree
ZkBasicAuthPrincipal uses the base policy. Keeping that documented extension
contract is the chosen tradeoff for this security patch; no further code change
for this optional suggestion.
--
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]
