Thanks for sending the PR! Repeating from my PR comment for posterity: I agree there's a balance between reducing verbosity vs more visible/explicit test parameters at the individual test-case level. I think in this case it's worth the extra characters in the tests both to provider an easier way to see at a glance the stack of privileges that enables each AuthorizableOperation as well as to help enable test-driven-development of any additional or modified super-privileges.
On Tue, Mar 10, 2026 at 10:58 AM Alexandre Dutra <[email protected]> wrote: > 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 > > > > > > > > > > > > > > > > > > > > > > > >
