Hi all, Here is the PR : https://github.com/apache/polaris/pull/3970
Thanks, Alex On Tue, Mar 10, 2026 at 4:40 PM Alexandre Dutra <[email protected]> wrote: > > 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 > > > > > > > > > > > > > > > > > > >
