Copilot commented on code in PR #4409:
URL: https://github.com/apache/polaris/pull/4409#discussion_r3234197233
##########
extensions/auth/opa/impl/src/main/java/org/apache/polaris/extension/auth/opa/OpaPolarisAuthorizer.java:
##########
@@ -109,25 +110,55 @@ public OpaPolarisAuthorizer(
*/
@Override
public void resolveAuthorizationInputs(
- @Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest
request) {
+ @Nonnull AuthorizationState authzState,
+ @Nonnull PolarisPrincipal polarisPrincipal,
+ @Nonnull AuthorizationRequest request) {
+ authzState.getResolutionManifest().resolveAll();
+ }
+
+ @Override
+ public void resolveAuthorizationInputs(
+ @Nonnull AuthorizationState authzState,
+ @Nonnull PolarisPrincipal polarisPrincipal,
+ @Nonnull List<AuthorizationRequest> requests) {
+ Preconditions.checkArgument(
+ !requests.isEmpty(), "Authorization request batch must contain at
least one request");
authzState.getResolutionManifest().resolveAll();
}
@Override
@Nonnull
public AuthorizationDecision authorize(
- @Nonnull AuthorizationState authzState, @Nonnull AuthorizationRequest
request) {
+ @Nonnull AuthorizationState authzState,
+ @Nonnull PolarisPrincipal polarisPrincipal,
+ @Nonnull AuthorizationRequest request) {
boolean allowed =
queryOpa(
buildOpaAuthorizationInput(
- request.getPrincipal(),
+ polarisPrincipal,
request.getOperation(),
toResourceEntitiesFromSecurables(request.getTargets()),
toResourceEntitiesFromSecurables(request.getSecondaries())));
return allowed
? AuthorizationDecision.allow()
: AuthorizationDecision.deny(
- "OPA denied authorization for " +
request.formatForAuthorizationMessage());
+ "OPA denied authorization for principal="
+ + polarisPrincipal.getName()
+ + " operation="
+ + request.getOperation());
Review Comment:
The deny message here drops request context (targets/secondaries) that was
previously included, which will make authorization failures significantly
harder to debug. Since the method still has access to
request.getTargets()/getSecondaries(), consider including their formatted
securables (or a stable request summary) in the message while keeping the
principal passed explicitly.
##########
extensions/auth/opa/impl/src/test/java/org/apache/polaris/extension/auth/opa/OpaPolarisAuthorizerTest.java:
##########
@@ -934,14 +930,136 @@ <T> T httpClientExecute(
.isEqualTo(expectedDestination);
}
+ @Test
+ void authorizeSingleOperationBatchEvaluatesSequentially() throws Exception {
+ final List<String> capturedRequestBodies = new ArrayList<>();
+ HttpEntity mockEntity =
HttpEntities.create("{\"result\":{\"allow\":true}}");
+ @SuppressWarnings("resource")
+ ClassicHttpResponse mockResponse = new BasicClassicHttpResponse(200);
+ mockResponse.setEntity(mockEntity);
+
+ PolarisResolutionManifest resolutionManifest =
mock(PolarisResolutionManifest.class);
+ AuthorizationState authzState = new AuthorizationState();
+ authzState.setResolutionManifest(resolutionManifest);
+ PolarisPrincipal principal = PolarisPrincipal.of("alice", Map.of(),
Set.of("role-1"));
+
+ OpaPolarisAuthorizer authorizer =
+ new OpaPolarisAuthorizer(
+ URI.create("http://opa.example.com:8181/v1/data/polaris/allow";),
+ mock(CloseableHttpClient.class),
+ JsonMapper.builder().build(),
+ null) {
+ @Override
+ <T> T httpClientExecute(
+ ClassicHttpRequest request, HttpClientResponseHandler<? extends
T> responseHandler)
+ throws HttpException, IOException {
+ capturedRequestBodies.add(
+ new String(
+ request.getEntity().getContent().readAllBytes(),
StandardCharsets.UTF_8));
+ return responseHandler.handleResponse(mockResponse);
+ }
+ };
+
+ AuthorizationDecision decision =
+ authorizer.authorize(
+ authzState,
+ principal,
+ List.of(
+ AuthorizationRequest.of(
+ PolarisAuthorizableOperation.GET_CATALOG,
+ PolarisSecurable.of(new
PathSegment(PolarisEntityType.CATALOG, "catalog-1"))),
+ AuthorizationRequest.of(
+ PolarisAuthorizableOperation.GET_CATALOG,
+ PolarisSecurable.of(new
PathSegment(PolarisEntityType.CATALOG, "catalog-2")))));
+
+ ObjectMapper mapper = JsonMapper.builder().build();
+ JsonNode firstRoot = mapper.readTree(capturedRequestBodies.get(0));
+ JsonNode secondRoot = mapper.readTree(capturedRequestBodies.get(1));
+
+ assertThat(decision.isAllowed()).isTrue();
+ assertThat(capturedRequestBodies).hasSize(2);
Review Comment:
This test reads capturedRequestBodies.get(0)/(1) before asserting that two
requests were captured, so a regression in batch execution would fail with
IndexOutOfBoundsException instead of a clear assertion. Assert
capturedRequestBodies has size 2 before indexing into it (or use
containsExactly/first/last assertions).
--
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]
