Skip to content

Entity-level filtering: API and feature flag #4831

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gracechen09 wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Entity-level filtering: API and feature flag #4831

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
89570d8
entity-filtering API and feature flag
gracechen09 Jun 19, 2026
cf1e959
Merge branch 'main' into entityfilter
gracechen09 Jun 19, 2026
7674d46
Fix Spotless formatting in PolarisAuthorizer Javadoc
gracechen09 Jun 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,4 +80,19 @@ void authorizeOrThrow(
@NonNull PolarisAuthorizableOperation authzOp,
@Nullable List<PolarisResolvedPathWrapper> targets,
@Nullable List<PolarisResolvedPathWrapper> secondaries);

/**
* Filters a candidate list of securables to only those the principal is authorized to see.
*
* <p>The default implementation returns all candidates unchanged, preserving backward
* compatibility for authorizers that do not implement visibility filtering.
*
* <p>If filtering encounters an error, implementations should throw rather than fall back to
* returning unfiltered results.
*/
@NonNull
default List<PolarisSecurable> filterByVisibility(

@dimas-b dimas-b Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my POV using default SPI methods is rather confusing. Implementations will be tempted to forget implementing this method, when they should 🤔

By contrast, the default authorizeOrThrow() method on line 61 is not an SPI method IMHO, but a convenience method.

@rmannibucau rmannibucau Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any hope the API evolves to

  1. be reactive (CompletionStage)
  2. not filter in memory but enables pushdown upfront of the filtering in a backend (like zanzibar or precomputed permission database)

?

@dimas-b dimas-b Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The callers in Polaris code are all synchronous now... what would be the benefit of making just the Authrorizer reactive?

@rmannibucau rmannibucau Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slowly moving to reactive code to use a remote backend, this has some serious impact on scalability (hundreds vs thousands of concurrent requests)

@dimas-b dimas-b Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are virtual threads a possible alternative?

@dimas-b dimas-b Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but in general, I'd +1 moving towards more concurrent and more scalable code 😉

@rmannibucau rmannibucau Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

virtual threads can be an option even if there are still cases they lead to antipatterns, I always prefer to explicit state by the signature the method is not compute only, CompletionStage enables to make it generic and quarkus independent/more interop (nicer for extensions IMHO) otherwise mutiny works in a quarkus only world

@NonNull AuthorizationState authzState, @NonNull VisibilityFilterRequest request) {

@dimas-b dimas-b Jun 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This conceptually overlaps with the new authorize() method (line 53).

Once can call this method and deduce the authorization decision for the entity from the fact that is it excluding (or included) in the output of this method. At the same time, this should always be aligned with passing the data "operation" and "securable" through the authorize() method, right?

Would it be possible to have one main authorization method that would return an AuthorizationDecision for each element given a list of inputs. It would be called directly for filtering use cases. Simpler operations would call via via convenience utility methods to simplify default with singular parameters.

WDYT?

@gracechen09 gracechen09 Jul 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review! I'll address comments one by one, starting with this suggestion.
Yes, filterByEntity needs to always be aligned with passing the data operation and securable through authorize(), and this creates some redundancy.
Just confirming my understanding, instead of a separate filterByVisibility method on the authorizer, are you proposing something like? Extending authorize() to accept a list of requests and return per-entity auth decisions, and the filtering can be triggered by caller. The caller-site (LIST operations) interprets the auth decisions for each entity, then returns the authorized entities.
I think the batch authorize() interface makes sense! My concern is: the filtering algorithm includes a short-circuit logic (if the caller holds the visibility privilege on the parent, skip per-entity checks via inheritance). This optimization is authorizer-specific, for example, only RBAC authorizer uses privileges inheritance, but an OPA authorizer may use a different model. So the authorizer would need to own some part of batch authorization logic.
Would like hear about your thoughts!

@dimas-b dimas-b Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can update the authorize() method or create a new one and retrofit the old methods into it. Something like authorize(request(list of entities, etc..)) -> list of decisions per entity.

The request will have some information that is shared across all entities in the list, and implementation are free to use it to short-circuit decisions, of course. Mapping the same decision to each entity in the returned value is probably not a lot of overhead after the decision is made.

We may need to change the return type of authorize() to allow more than one output decision.

Does this sound reasonable?

@flyrain flyrain Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed with @dimas-b here. Reusing the existing authorize() avoids concept-level duplication. To achieve that, we also need some changes in AuthorizationDecision so that it can return the decision for a list of securables. Here is the general process:

  1. Authorize the list operation on the container.
  2. Get a list of securables under the container.
  3. Bulk authorize a list of securables under that container, returning the decision on each securable.

@gracechen09 gracechen09 Jul 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this makes sense to me, I agree on using a batch method rather than a separate filterbyVisibility.
Laying out proposed approaches from the discussions:

Approach 1: reusing existing authorize() method

List<AuthorizeDecision> authorize(state, batchRequest)

pros:

  • solves the duplication problem
  • avoids the potential implantation drift between two authorize methods

cons:

  • all single-entity authorization calls will have extra overheads for list creation and unwrapping
  • need to migrate on the caller-sites

Approach 2: creating a new authorize() method and retrofit the old methods into it

List<AuthorizeDecision> authorizeAll(state, batchRequest)

retrofit old method:

default AuthorizeDecision authorize(state, request) {
    return authorizeBatch(state, toBatch(request).get(0);
}

pros: same as approach 1 + no need to migrate the caller-sites immediately
cons: same as approach 1

Approach 3: creating a new authorize() method intended for batch

keep old method
AuthorizationDecision authorize(state, request);

create new batch method
List<AuthorizationDecision> authorize(state, batchRequest);

pros:

  • no impact on existing callers
  • each method has clean semantics

cons:

  • doesn't solve the duplication problem
  • potential implementation drift between the two

I'm a bit leaning toward approach 3 since the existing single-entity callers don't benefit from batch method, but the consistency and deduplication are also very appealing point for approach 1&2.

What do you think about the single-entity callers concern?

@flyrain flyrain Jul 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @gracechen09 for the writeup. Approach 3 makes sense to me. Here are reasons:

  1. It doesn't touch the AND-combine contract in the existing authorize() method. For example, the updateTable case stays a single request with multiple AND-combined intents, exactly as designed. The new "independent" axis lives one level up, which is a list of requests, so the two meanings never collide. updateTable is really the only genuine multi-intent case we have, and it's a compound single question, not a batch; keeping it on the intent list and putting batch on the request list keeps both honest. Sorry I missed the "AND-combine contract" from my previous comment.
  2. It can't drift. The batch method defaults to calling the single one, so there's one grant-walk. An authorizer that wants a bulk OPA query overrides the batch default, which is considered as an optional optimization, not a second implementation to keep in sync. This was the main thing I wanted to avoid with two parallel methods(authorize vs. filter).
  3. The authorizer stays pure. It returns decisions, never securables. The caller already owns the candidate list and does the projection, which aligns with our previous discussions.

Based on the above points. Here is the what it could look like:

// UNCHANGED — intents within a request AND-combine to one verdict. updateTable rides this as-is.
AuthorizationDecision authorize(AuthorizationState state, AuthorizationRequest request);

// NEW — independent axis is a list of REQUESTS, defaulting to mapping the single method.
default List<AuthorizationDecision> authorize(
    AuthorizationState state, List<AuthorizationRequest> requests) {
  return requests.stream().map(r -> authorize(state, r)).toList();  // aligned by index
}

Filtering becomes caller-side glue, not an SPI method:

var requests = candidates.stream().map(c -> req(principal, readOp, c)).toList();
var decisions = authorizer.authorize(state, requests);
var visible = range(0, candidates.size())
    .filter(i -> decisions.get(i).isAllowed())
    .mapToObj(candidates::get).toList();

@dimas-b dimas-b Jul 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re: List<AuthorizationDecision> authorize(state, batchRequest) - that LGTM 👍

Re: AuthorizationDecision authorize(state, request) - this method exists, but it is NOT currently called.

Perhaps it might be best to migrate all authZ callers to use authorize(state, request) first, remove old authorizeOrThrow methods, then add batches, then revisit this PR. WDYT?

CC: @sungwy

Cf. #5034

return request.candidates();
}
}
28 changes: 28 additions & 0 deletions polaris-core/src/main/java/org/apache/polaris/core/auth/VisibilityFilterRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.polaris.core.auth;

import java.util.List;
import org.jspecify.annotations.NonNull;

/** Carries the context used to filter LIST-operation candidates by visibility. */
public record VisibilityFilterRequest(
@NonNull PolarisAuthorizableOperation listOperation,

@flyrain flyrain Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

container is @NonNull PolarisSecurable, but PolarisSecurable.validate() forbids ROOT segments (PolarisSecurable.java:81) and requires a top-level start (:77) , so there's no way to represent the root container that LIST_CATALOGS needs, even though the flag description names listCatalogs. I think this is another reason to reuse the existing method authorize() and expand on it.

@NonNull PolarisSecurable container,
@NonNull List<PolarisSecurable> candidates) {}
12 changes: 12 additions & 0 deletions polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,18 @@ public static void enforceFeatureEnabledOrThrow(
.defaultValue(false)
.buildFeatureConfiguration();

public static final FeatureConfiguration<Boolean> ENTITY_VISIBILITY_FILTERING_ENABLED =

@dimas-b dimas-b Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This flag is not used in this PR, let's defer introducing it until it's actually required.

PolarisConfiguration.<Boolean>builder()
.key("ENTITY_VISIBILITY_FILTERING_ENABLED")
.description(
"If set to true, entity-level visibility filtering is applied to LIST operations "
+ "(listTables, listViews, listNamespaces, listCatalogs). "
+ "When enabled, each authorizer filters the candidate result set so that only "
+ "entities the principal is authorized to see are returned. "
+ "Requires LIST_PAGINATION_ENABLED to be true.")
.defaultValue(false)
.buildFeatureConfiguration();

public static final FeatureConfiguration<Boolean> ENABLE_GENERIC_TABLES =
PolarisConfiguration.<Boolean>builder()
.key("ENABLE_GENERIC_TABLES")
Expand Down
129 changes: 129 additions & 0 deletions polaris-core/src/test/java/org/apache/polaris/core/auth/VisibilityFilterRequestTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.polaris.core.auth;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.Mockito.mock;

import java.util.List;
import java.util.Set;
import org.apache.polaris.core.entity.PolarisBaseEntity;
import org.apache.polaris.core.entity.PolarisEntityType;
import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper;
import org.jspecify.annotations.Nullable;
import org.junit.jupiter.api.Test;

public class VisibilityFilterRequestTest {

/**
* Minimal concrete authorizer that does not override {@code filterByVisibility}, so the default
* no-op implementation on the interface is exercised.
*/
private static final class NoOpAuthorizer implements PolarisAuthorizer {
@Override
public void resolveAuthorizationInputs(
AuthorizationState authzState, AuthorizationRequest request) {}

@Override
public AuthorizationDecision authorize(
AuthorizationState authzState, AuthorizationRequest request) {
return AuthorizationDecision.allow();
}

@Override
public void authorizeOrThrow(
PolarisPrincipal polarisPrincipal,
Set<PolarisBaseEntity> activatedEntities,
PolarisAuthorizableOperation authzOp,
@Nullable PolarisResolvedPathWrapper target,
@Nullable PolarisResolvedPathWrapper secondary) {}

@Override
public void authorizeOrThrow(
PolarisPrincipal polarisPrincipal,
Set<PolarisBaseEntity> activatedEntities,
PolarisAuthorizableOperation authzOp,
@Nullable List<PolarisResolvedPathWrapper> targets,
@Nullable List<PolarisResolvedPathWrapper> secondaries) {}
}

private static final PolarisSecurable CATALOG_SECURABLE =
PolarisSecurable.of(new PathSegment(PolarisEntityType.CATALOG, "myCatalog"));

private static final PolarisSecurable NS_SECURABLE =
PolarisSecurable.of(
new PathSegment(PolarisEntityType.CATALOG, "myCatalog"),
new PathSegment(PolarisEntityType.NAMESPACE, "ns1"));

private static final PolarisSecurable TABLE_SECURABLE_1 =
PolarisSecurable.of(
new PathSegment(PolarisEntityType.CATALOG, "myCatalog"),
new PathSegment(PolarisEntityType.NAMESPACE, "ns1"),
new PathSegment(PolarisEntityType.TABLE_LIKE, "table1"));

private static final PolarisSecurable TABLE_SECURABLE_2 =
PolarisSecurable.of(
new PathSegment(PolarisEntityType.CATALOG, "myCatalog"),
new PathSegment(PolarisEntityType.NAMESPACE, "ns1"),
new PathSegment(PolarisEntityType.TABLE_LIKE, "table2"));

@Test
void recordExposesAllFields() {
List<PolarisSecurable> candidates = List.of(TABLE_SECURABLE_1, TABLE_SECURABLE_2);

VisibilityFilterRequest request =
new VisibilityFilterRequest(
PolarisAuthorizableOperation.LIST_TABLES, NS_SECURABLE, candidates);

assertThat(request.listOperation()).isEqualTo(PolarisAuthorizableOperation.LIST_TABLES);
assertThat(request.container()).isSameAs(NS_SECURABLE);

@dimas-b dimas-b Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are we testing here?... that the second constructor parameter is the container? 🤔 Does it actually help? I imagine future callers of new VisibilityFilterRequest() can still assign wrong parameters by mistake.

assertThat(request.candidates()).isSameAs(candidates);
}

@Test
void defaultFilterByVisibilityReturnsAllCandidatesUnchanged() {
// The default no-op implementation on PolarisAuthorizer must return all candidates
// unchanged, preserving backward compatibility for authorizers that do not override it.
PolarisAuthorizer authorizer = new NoOpAuthorizer();
List<PolarisSecurable> candidates = List.of(TABLE_SECURABLE_1, TABLE_SECURABLE_2);

VisibilityFilterRequest request =
new VisibilityFilterRequest(
PolarisAuthorizableOperation.LIST_TABLES, NS_SECURABLE, candidates);

List<PolarisSecurable> result =
authorizer.filterByVisibility(mock(AuthorizationState.class), request);

assertThat(result).isSameAs(candidates);
}

@Test
void defaultFilterByVisibilityWithEmptyCandidatesReturnsEmpty() {
PolarisAuthorizer authorizer = new NoOpAuthorizer();

VisibilityFilterRequest request =
new VisibilityFilterRequest(
PolarisAuthorizableOperation.LIST_NAMESPACES, CATALOG_SECURABLE, List.of());

List<PolarisSecurable> result =
authorizer.filterByVisibility(mock(AuthorizationState.class), request);

assertThat(result).isEmpty();
}
}
Loading