+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
> > > > >
> > >
> >
>
