gracechen09 commented on code in PR #4831:
URL: https://github.com/apache/polaris/pull/4831#discussion_r3510206082
##########
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:
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?
--
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]
