gracechen09 commented on code in PR #4831:
URL: https://github.com/apache/polaris/pull/4831#discussion_r3503019404


##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizer.java:
##########
@@ -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(
+      @NonNull AuthorizationState authzState, @NonNull VisibilityFilterRequest 
request) {

Review Comment:
   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!



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to