Hi Dennis, > As in, the tests equally pass whether or not we accidentally included > TABLE_WRITE_DATA as a subsumed privilege or whether or not we accidentally > create a backdoor in the `updateTable` method.
I guess, the assumption is that you wouldn't intentionally or accidentally do anything like that. If a new super-privilege is added to SUPER_PRIVILEGES, this is a potentially VERY impactful change, and I would assume that it would be reviewed thoroughly before being merged. I noticed that the tests equally pass before and after the super-privilege addition. But since that was the expected outcome, I considered this situation as normal, and even told myself "good, the test factory is working as expected" :-) > would that make new super-privilege additions automatically appear in the > "missingInsufficientPrivilegeSets" by default and thus intentionally cause > all the test cases for operations allowed by the new privilege to fail until > they're updated? Yes. But anyways, the automatic super-privilege additions seem to cause friction, and I apologize for that. I will prepare a PR to revert that portion of the Authz tests refactoring. Thanks, Alex On Sat, Mar 7, 2026 at 7:16 AM Dennis Huo <[email protected]> wrote: > > +1 to the overall idea, definitely a useful super-privilege to define for > that persona. Responded with some comments on the PR. One thing to point > out is regarding the statement: > > Motivation: > > Currently, granting read-only access to a data analyst across an entire > > catalog requires individually granting TABLE_READ_DATA on every table. > > > It may not be sufficiently documented or obvious, but in general the scope > of the securable "authz targets" applicable to a privilege do *not* > restrict the scope of allowed entities on which the privilege is granted. > > This is easier to convey with concrete examples -- TABLE_READ_PROPERTIES is > a privilege for which the authz target is naturally a TableEntity. However, > grants of TABLE_READ_PROPERTIES do *not* need to be on a table -- they can > be granted on NamespaceEntities or CatalogEntities as well, where the > *container hierarchy* (catalog, namespace, table) defines the > inheritance of grants on the parent container. > > So, currently if someone really wants TABLE_READ_DATA across a whole > catalog, it's already supported by granting TABLE_READ_DATA on the catalog > object itself, and it's not necessary to individually grant TABLE_READ_DATA > on every table. > > However, the motivation is still valid in that there is a convenient > *bundle* of privileges that otherwise need to be granted individually at > whatever level is desired -- granting TABLE_READ_DATA, > VIEW_READ_PROPERTIES, NAMESPACE_READ_PROPERTIES, NAMESPACE_LIST, > TABLE_LIST, etc. (Technically since _READ_PROPERTIES subsumes _LIST, > actually just the _READ_PROPERTIES should be sufficient). > > My suggestion on the PR was that if we indeed only have NAMESPACE and lower > level read privileges in this new bundle, we should name it as a > NAMESPACE-level bundle ("NAMESPACE_READ_DATA") while still allowing it to > be set at either Catalog or Namespace levels. > > The second big thing I wanted to discuss and hear thoughts from Alex was > regarding the auto-addition of subsuming privileges in > `PolarisAuthzTestsFactory` in > https://github.com/apache/polaris/pull/3824/changes#diff-43fc1673a0b67957e44db18a44467065151a67e4b9068180b1750b21e9059f6d > - right now the tests don't really help identify important considerations > when adding a new super-privilege like here, so it's hard to prove to > ourselves that the SUPER_PRIVILEGES were set up correctly without leaking > write privileges. It's somewhat bad that in this new PR, the tests > *technically* have code-coverage of using the new super-privilege, but will > still pass no matter what bundle of privileges we add to the new privilege. > As in, the tests equally pass whether or not we accidentally included > TABLE_WRITE_DATA as a subsumed privilege or whether or not we accidentally > create a backdoor in the `updateTable` method. > > If we kept the auto-generation of "missingInsufficientPrivilegeSets" but > did *not* auto-generate the subsuming ones, would that make new > super-privilege additions automatically appear in the > "missingInsufficientPrivilegeSets" by default and thus intentionally cause > all the test cases for operations allowed by the new privilege to fail > until they're updated? > > What do others think about the balance here of intentionally opting for > more verbosity in the Authz tests in the interest of having the Authz test > automatically function as a safety belt when working with new privilege > sets? > > > On Fri, Mar 6, 2026 at 1:28 PM Yufei Gu <[email protected]> wrote: > > > +1 on the idea. Left some comments on the PR. > > > > Yufei > > > > > > On Fri, Mar 6, 2026 at 10:57 AM Dmitri Bourlatchkov <[email protected]> > > wrote: > > > > > Hi All, > > > > > > The proposed change LGTM too. I approved the PR in GH. > > > > > > I see a positive review from Michael in GH too. > > > > > > Since Michael also asked Dennis to review, I propose waiting a few more > > > days before merging. How about merging on Mar 10? > > > > > > Cheers, > > > Dmitri. > > > > > > On Fri, Mar 6, 2026 at 6:46 AM Alexandre Dutra <[email protected]> > > wrote: > > > > > > > Hi Praneeth, > > > > > > > > The changes make sense to me. I approved your PR. > > > > > > > > Thanks, > > > > Alex > > > > > > > > On Fri, Mar 6, 2026 at 5:47 AM Travis Bowen <[email protected]> > > > > wrote: > > > > > > > > > > Hi Praneeth, > > > > > > > > > > Thanks for sending this out. > > > > > I looked at the PR and think the motivation and PR makes sense. > > > > > > > > > > -Travis > > > > > > > > > > On Thu, Mar 5, 2026 at 11:20 AM vemula praneeth < > > > > > [email protected]> wrote: > > > > > > > > > > > Hi dev, > > > > > > > > > > > > I've submitted PR #3927 ( > > https://github.com/apache/polaris/pull/3927 > > > ) > > > > > > which adds a new catalog-level privilege CATALOG_READ_DATA (code > > > 103). > > > > > > > > > > > > Motivation: > > > > > > Currently, granting read-only access to a data analyst across an > > > entire > > > > > > catalog requires individually granting TABLE_READ_DATA on every > > > table. > > > > > > CATALOG_READ_DATA is a single catalog-level grant that subsumes: > > > > > > - TABLE_READ_DATA, TABLE_LIST, TABLE_READ_PROPERTIES > > > > > > - NAMESPACE_LIST, NAMESPACE_READ_PROPERTIES > > > > > > - VIEW_LIST, VIEW_READ_PROPERTIES > > > > > > > > > > > > It fits naturally between CATALOG_MANAGE_METADATA (no data access) > > > > > > and CATALOG_MANAGE_CONTENT (full access), filling a gap for > > read-only > > > > > > analyst principals. > > > > > > > > > > > > Feedback welcome! > > > > > > > > > > > > Regards, > > > > > > Praneeth > > > > > > > > > > > > > > >
