Hi Grace Thanks for the ping. As I'm back on Polaris, I will take a look on the PR.
Regards JB On Tue, Jun 30, 2026 at 3:00 AM Grace Chen <[email protected]> wrote: > Hi everyone, > > I'd like to request reviews for the following PR, which implements the > first phase of the entity-level visibility filtering proposal: > https://github.com/apache/polaris/pull/4831. > > This PR introduces the API surface for entity-level visibility > filtering on LIST operations. Specifically, it adds: > 1. A new request object and filterByVisibility method with a > backward-compatible default implementation > 2. A feature flag to control the new functionality > > Scope: this is Phase 1 of a multi-phase implementation. Follow-up PRs > will wire in the actual filtering logic for the RBAC and OPA > authorizers, and integrate the new method at the call sites. Keeping > this first PR focused on the API to makes it easier to review. > > Happy to discuss any aspect of the design here or on the PR itself. > Thanks in advance for taking a look! > > Thanks, > Grace > > On Thu, Jun 18, 2026 at 9:55 AM Zhiyang Chen <[email protected]> wrote: > > > > Hi everyone, > > > > Apologies for the delay in response, I've revised the proposal to > > align with the new authorization SPI changes. Here's the summary of > > the major changes: > > > > 1. Use the new authorize() SPI instead of authorizeOrThrow > > The original proposal used the authorizeOrThrow with exception-driven > > control flow to check per-entity visibility. The updated version uses > > the new authorize(AuthorizationState, AuthorizationRequest) SPI, which > > returns an AuthorizationDecision without throwing. This avoids using > > exceptions for expected control flow. > > > > 2. Refined the new entity filtering API on the PolarisAuthorizer > interface > > After the authorization SPI refactoring, privilege hierarchy semantics > > moves into the RBAC authorizer implementation. The original proposal's > > filtering API leaks the RBAC assumptions into the interfaces. The > > updated proposal moves all filtering decisions (including > > short-circuit) inside each authorizer. This lets RBAC authorizer use > > privilege inheritance, OPA uses its own policy checks, decoupling RBAC > > semantics from authorizer implementation. > > > > 3. Defined error handling > > When the entity-filtering feature is on, if the entity-filtering logic > > throws an exception (e.g., OPA server unreachable, DB query timeout), > > the LIST request will fail with a 500 error. The response does not > > fall back to unfiltered results because it would silently turn off the > > feature. > > > > > > Sung and Dmitri, thanks for sharing the thoughts on the feature flag, > > I agree on checking the feature flag before performing any filtering. > > From the caller site logic below, we could check the feature flag > > before step 3 to decide whether the filtering should be performed. > > What do you think? > > > > ``` > > // Step 1 existing code: checks user has TABLE_LIST on the namespace > > > > authorizeBasicNamespaceOperationOrThrow(LIST_TABLES, namespace); > > > > // Step 2 catalog backend queries the DB and returns table names > > > > List<TableIdentifier> candidates = baseCatalog.listTables(namespace, > > pageRequest); > > > > // Step 3 construct container securable from the namespace > > > > PolarisSecurable containerSecurable = PolarisSecurable.of( > > new PathSegment(PolarisEntityType.CATALOG, catalogName()), > > new PathSegment(PolarisEntityType.NAMESPACE, namespace.level(0))); > > > > // Step 4 per-entity filter: each authorizer decides visibility in its > own way > > > > List<PolarisSecurable> visible = authorizer.filterByVisibility( > > authzState, new > > VisibilityFilterRequest(LIST_TABLES,containerSecurable, > > toSecurables(candidates))); > > ``` > > > > I'm working on the draft PR and will send it out once it's ready. In > > the meantime, feel free to share your thoughts on the proposal > > anytime! The link to the proposal is > > > https://docs.google.com/document/d/1PyIISgK2FVK0m8t4104KZpKAWb4d1Wy0UhZQzut4pIc/edit?usp=sharing > > > > Thanks, > > Grace > > > > On Wed, Jun 17, 2026 at 5:32 PM Dmitri Bourlatchkov <[email protected]> > wrote: > > > > > > Hi Sung, > > > > > > I'm not sure I understand what you mean by "switching off the boolean > > > authorization based on the existing operations", but we can probably > > > resolve that when we have a specific PR :) > > > > > > Cheers, > > > Dmitri. > > > > > > On Tue, Jun 16, 2026 at 10:30 PM Sung Yun <[email protected]> wrote: > > > > > > > Hi Dmitri, > > > > > > > > I actually meant completely switching off the boolean authorization > based > > > > on the existing operations. But I think I could sway either way. > > > > > > > > I had a small bias towards just using the filter based approach > without > > > > the first check if the feature flag is on, because that'll make it > easier > > > > for system operators to reason about setting up their policies in > their > > > > respective PAP. If we introduce two-layered checks, there will need > to be > > > > two sets of policies for the same operation. > > > > > > > > Sung > > > > > > > > On 2026/06/16 02:52:19 Dmitri Bourlatchkov wrote: > > > > > Hi Sung, > > > > > > > > > > Good point about exceptions. I agree that it is preferable to > avoid them > > > > > (as denial indicators) on the filtering call paths. > > > > > > > > > > Re: feature flags, I suppose we cloud model this as a two-layered > check: > > > > 1) > > > > > per-batch permissions (same as now) and 2) per-element > include/exclude > > > > > decisions. The first check is always "on", the second can be > controlled > > > > by > > > > > a feature flag the the AuthZ layer. Callers (REST services) always > get > > > > the > > > > > filtering flags, but if the feature is disabled, the flag are > always > > > > > "include". > > > > > > > > > > This way the code in REST services can be uniform across all use > cases, > > > > > which will simplify testing. > > > > > > > > > > I'm not sure you meant the same approach in your message, but I > hope it > > > > > makes sense :) > > > > > > > > > > WDYT? > > > > > > > > > > Cheers, > > > > > Dmitri. > > > > > > > > > > > > > > > > > > > > On Mon, Jun 15, 2026 at 10:08 PM Sung Yun <[email protected]> > wrote: > > > > > > > > > > > Hi Grace and Venkateshwaran, > > > > > > > > > > > > Thanks for the discussion. I agree with the general direction of > > > > > > introducing authorizer-layer filtering and returning callers a > list > > > > scoped > > > > > > to what they can access. > > > > > > > > > > > > One thing I wanted to float: I think it would help to make the > > > > > > authorization input for list operations deterministic, rather > than > > > > relying > > > > > > on a fallback path. By fallback I mean the handler running the > normal > > > > > > LIST_* check and, on failure, dropping into a filtered path. That > > > > works, > > > > > > but it leans on exception-driven branching that I'd argue is > harder to > > > > > > reason about and test. > > > > > > > > > > > > The alternative would be to treat this as a mode, enabled by a > feature > > > > > > flag for filtering on list operations. When it's off, behavior is > > > > exactly > > > > > > what we have today. When it's on, every caller goes through the > same > > > > > > listing/filtering path, and whether you see everything > underneath just > > > > > > falls out of that check (admins included, via the container > > > > short-circuit) > > > > > > instead of being a separate branch keyed on who the caller is. > > > > > > > > > > > > That being said I think we are generally in agreement, and we > could > > > > iron > > > > > > out these mechanisms on PR reviews as well. > > > > > > > > > > > > Cheers > > > > > > Sung > > > > > > > > > > > > On 2026/06/05 23:23:30 Yufei Gu wrote: > > > > > > > The proposed behavior makes sense. Returning a list of > catalogs only > > > > a > > > > > > > caller can access feels more aligned with least privilege and > with > > > > how > > > > > > most > > > > > > > users would expect catalog discovery to work. > > > > > > > > > > > > > > Implementation does not seem particularly difficult. The > existing > > > > admin > > > > > > > behavior could remain unchanged, while non-admin callers > receive a > > > > > > filtered > > > > > > > list based on catalog level authorization checks similar to > those > > > > already > > > > > > > performed by getCatalog(). There are minor perf concerns, but > I > > > > think it > > > > > > > should be acceptable in general. To me, the bigger question is > > > > agreeing > > > > > > on > > > > > > > the API semantics rather than the implementation itself. > > > > > > > > > > > > > > Overall, I'm supportive of the change. > > > > > > > > > > > > > > Yufei > > > > > > > > > > > > > > > > > > > > > On Thu, Jun 4, 2026 at 5:05 PM Dmitri Bourlatchkov < > [email protected] > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > It looks like this reply got disconnected from the original > thread. > > > > > > > > > > > > > > > > Posting a link to it for reference: > > > > > > > > > https://lists.apache.org/thread/w58zrjt6jpq33mv9lvqzndqc6no5l7xb > > > > > > > > > > > > > > > > Cheers, > > > > > > > > Dmitri. > > > > > > > > > > > > > > > > On Sun, May 31, 2026 at 12:15 PM venkateshwaran cholan < > > > > > > > > [email protected]> wrote: > > > > > > > > > > > > > > > > > Hi all, > > > > > > > > > > > > > > > > > > Thanks Grace for putting together the proposal, and thanks > > > > everyone > > > > > > for > > > > > > > > the > > > > > > > > > detailed feedback and discussion so far. > > > > > > > > > > > > > > > > > > Dimas pointed me to this discussion from #4573, and I > wanted to > > > > > > share an > > > > > > > > > observation after tracing the current implementation on > main. > > > > > > > > > > > > > > > > > > The discoverability gap there (catalog-role access works > for > > > > > > getCatalog > > > > > > > > and > > > > > > > > > Iceberg operations, but listCatalogs requires root > CATALOG_LIST) > > > > > > looks > > > > > > > > like > > > > > > > > > one concrete case of the broader visibility model > discussed here, > > > > > > > > > particularly if CATALOG_READ_PROPERTIES ends up being the > > > > selected > > > > > > > > > visibility privilege for catalogs. > > > > > > > > > > > > > > > > > > listCatalogs() still gates on > > > > > > > > > authorizeBasicRootOperationOrThrow(LIST_CATALOGS) against > the > > > > root > > > > > > > > > container, while getCatalog() authorizes per catalog. That > > > > matches > > > > > > the > > > > > > > > > asymmetry described in the issue. > > > > > > > > > > > > > > > > > > I agree that authorizer-layer filtering is preferable to a > > > > > > > > > listCatalogs-only special case so default RBAC, OPA, and > other > > > > > > > > authorizers > > > > > > > > > stay consistent. > > > > > > > > > > > > > > > > > > One thing I am still unclear on for LIST_CATALOGS: the > proposal's > > > > > > scope > > > > > > > > > short-circuit does not apply because there is no scope > container > > > > > > (unlike > > > > > > > > > LIST_NAMESPACES / LIST_TABLES). > > > > > > > > > > > > > > > > > > For #4573-style discoverability, it seems we would want > callers > > > > > > without > > > > > > > > > root CATALOG_LIST to get a filtered list instead of 403. > > > > > > > > > > > > > > > > > > Separately, when entity filtering is enabled, should > callers with > > > > > > root > > > > > > > > > CATALOG_LIST still get a filtered list, or is root > CATALOG_LIST > > > > > > intended > > > > > > > > to > > > > > > > > > mean "see all catalogs" with filtering only handling the > > > > > > discoverability > > > > > > > > > case? > > > > > > > > > > > > > > > > > > Happy to help with regression coverage either way. For > example, > > > > an > > > > > > authz > > > > > > > > > test where a principal has catalog-scoped > CATALOG_READ_PROPERTIES > > > > > > via a > > > > > > > > > catalog role but no root CATALOG_LIST: getCatalog succeeds > and > > > > > > > > listCatalogs > > > > > > > > > fails today, documenting the gap until filtering lands. > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > Venkateshwaran > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
