Entity-level filtering: API and feature flag

[gracechen09]

This PR introduces the API for entity-level visibility filtering on LIST operations, which is the first part of the implementation of proposal, and has been discussed in dev mailing list thread1 and thread2.

Changes:

• added a new request object and a new entity-filtering method with a backward-compatible default implementation
• added feature flag

Follow-up PRs will be:

1. implementation of filterByVisibility for RBAC authorizer
2. implementation of filterByVisibility for OPA authorizer
3. caller integration with filterByVisibility

Checklist

• 🛡️ Don't disclose security issues! (contact security@apache.org)
• 🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
• 🧪 Added/updated tests with good coverage, or manually tested (and explained how)
• 💡 Added comments for complex logic
• 🧾 Updated CHANGELOG.md (if needed)
• 📚 Updated documentation in site/content/in-dev/unreleased (if needed)

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[flyingImer]

Thanks for driving this direction!

I read the linked doc and I'm not sure I fully follow the problem statement. I think it merges two authorization axes that Polaris keeps separate today: discoverability and metadata access.

Discoverability under a parent is the LIST privilege. TABLE_LIST is granted on the namespace, and listTables checks exactly that before returning the children. Metadata access is a separate per-entity grant (*_READ_PROPERTIES). The two don't gate each other: not holding TABLE_LIST on a namespace doesn't stop you from loading a table you were granted directly, and holding TABLE_LIST is what lets you see the child names under that namespace.

On the two problems in the doc:

• Discoverability: "I can access a table but can't list it without namespace LIST" looks like the model working as intended, not a gap. The grant gives you access to the entity, and enumerating names under the parent is what LIST grants, separately.
• Visibility of unauthorized entities: I'd keep "unauthorized" (no metadata or data access) apart from "shows up in a list". A name appearing under a parent you hold LIST on is the LIST privilege doing its job, even if you can't read that entity's contents. Suppressing names from a LIST-privileged caller is a different requirement (anti-enumeration), not the current model misbehaving.

Can we re-pin the problem statements?

[gracechen09]

Thanks for driving this direction!

I read the linked doc and I'm not sure I fully follow the problem statement. I think it merges two authorization axes that Polaris keeps separate today: discoverability and metadata access.

Discoverability under a parent is the LIST privilege. TABLE_LIST is granted on the namespace, and listTables checks exactly that before returning the children. Metadata access is a separate per-entity grant (*_READ_PROPERTIES). The two don't gate each other: not holding TABLE_LIST on a namespace doesn't stop you from loading a table you were granted directly, and holding TABLE_LIST is what lets you see the child names under that namespace.

On the two problems in the doc:

• Discoverability: "I can access a table but can't list it without namespace LIST" looks like the model working as intended, not a gap. The grant gives you access to the entity, and enumerating names under the parent is what LIST grants, separately.
• Visibility of unauthorized entities: I'd keep "unauthorized" (no metadata or data access) apart from "shows up in a list". A name appearing under a parent you hold LIST on is the LIST privilege doing its job, even if you can't read that entity's contents. Suppressing names from a LIST-privileged caller is a different requirement (anti-enumeration), not the current model misbehaving.

Can we re-pin the problem statements?

Thank you for reviewing the proposal, this is a good point! The original problem statement conflates two distinct authorization concerns, therefore I updated the proposal to remove the discoverability from the problem statement. The motivation for this proposal is now aligned with the proposed solution, which is for user who holds LIST_* privilege on a parent should only receive child entities they have access to. The other case where a user holds per-entity grants but no parent-level LIST privileges is moved to Future Work section and requires more discussion in the future.

[dimas-b]

Glad to see a contribution from you, @gracechen09 ! I hope you do not mind working through this PR over a few review / update cycles.

[dimas-b]

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

[dimas-b]

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.

[dimas-b]

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]

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]

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]

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]

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]

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]

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

[dimas-b]

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.

[rmannibucau]

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]

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

[rmannibucau]

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]

Are virtual threads a possible alternative?

[dimas-b]

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

[rmannibucau]

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

[flyrain]

Thanks @gracechen09 for working on it. Left some comments

[flyrain]

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.

[flyrain]

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.