From e7fda2d489cff799ca1521f77a0763e9bf83a206 Mon Sep 17 00:00:00 2001 From: Yuewei Zhou Date: Fri, 17 Jul 2026 14:55:25 -0700 Subject: [PATCH] Use authorize(state, request).isAllowed() for exception-less auth check Introduce isResolvedBasicTableLikeOperationAuthorized() in CatalogHandler that uses the decision-native authorize(AuthorizationState, AuthorizationRequest) method to check authorization without throwing exceptions. This is the first service-layer caller of PolarisAuthorizer.authorize() for boolean decisions. Refactor authorizeLoadTable in IcebergCatalogHandler to use the new helper, eliminating the ForbiddenException-based control flow entirely. No exception is thrown or caught for the write-delegation probe -- the authorization decision is obtained directly from the authorizer. The helper guards against an unresolved target (e.g. a missing table) by throwing notFoundExceptionForTableLikeEntity before delegating to the authorizer, mirroring authorizeResolvedBasicTableLikeOperationOrThrow. Without this guard the real authorizer's getResolvedSecurable() throws IllegalStateException (surfacing as a 500) for missing entities instead of the expected NoSuchTableException (404). Also stub authorize(state, request) in the TestServices mock authorizer to return an "allow" decision, mirroring its no-op authorizeOrThrow behavior, so decision-native callers do not NPE on a null AuthorizationDecision. This resolves the TODO at IcebergCatalogHandler: 'Refactor to have a boolean-return version of the helpers so we can fallthrough easily.' --- .../catalog/common/CatalogHandler.java | 32 +++++++++++++++++++ .../iceberg/IcebergCatalogHandler.java | 10 +++--- .../iceberg/IcebergCatalogHandlerTest.java | 30 ++++++++--------- .../apache/polaris/service/TestServices.java | 6 ++++ 4 files changed, 57 insertions(+), 21 deletions(-) diff --git a/runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java b/runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java index 2995bdba7ec..50b84a8b11a 100644 --- a/runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java +++ b/runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java @@ -396,6 +396,38 @@ protected void authorizeResolvedBasicTableLikeOperationOrThrow( initializeCatalog(); } + /** + * Returns {@code true} if the current principal is authorized for the given operation on the + * specified table-like entity that has already been resolved, {@code false} otherwise. Unlike + * {@link #authorizeResolvedBasicTableLikeOperationOrThrow}, this method does not throw on + * authorization failure, making it suitable for control-flow decisions where a denied operation + * should fall through to an alternative check rather than immediately failing the request. + * + *

The caller must have already invoked {@link #resolveBasicTableLikeTargetOrThrow} to resolve + * the entity before calling this method. If the entity did not resolve (for example, the table + * does not exist), this method throws {@link #notFoundExceptionForTableLikeEntity} rather than + * delegating to the authorizer, mirroring {@link + * #authorizeResolvedBasicTableLikeOperationOrThrow} so that a missing entity surfaces as a + * not-found (404) response instead of a server error. + */ + protected boolean isResolvedBasicTableLikeOperationAuthorized( + PolarisAuthorizableOperation op, PolarisEntitySubType subType, TableIdentifier identifier) { + PolarisResolvedPathWrapper target = + resolutionManifest.getResolvedPath(ResolvedPathKey.ofTableLike(identifier), subType, true); + if (target == null) { + throw notFoundExceptionForTableLikeEntity(identifier, subType); + } + + AuthorizationState authorizationState = new AuthorizationState(resolutionManifest); + AuthorizationRequest request = + new AuthorizationRequest( + polarisPrincipal(), + List.of( + new SingleTargetAuthorizationIntent( + op, PolarisSecurableMapper.tableLike(catalogName(), identifier)))); + return authorizer().authorize(authorizationState, request).isAllowed(); + } + protected void resolveAndAuthorizeBasicTableLikeOperationOrThrow( PolarisAuthorizableOperation op, PolarisEntitySubType subType, TableIdentifier identifier) { resolveBasicTableLikeTargetOrThrow(op, identifier); diff --git a/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java b/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java index e6ad4bc0a1c..d3b7f361145 100644 --- a/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java +++ b/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java @@ -1022,12 +1022,12 @@ private Set authorizeLoadTable( Set actionsRequested = new HashSet<>(Set.of(PolarisStorageActions.READ, PolarisStorageActions.LIST)); - try { - authorizeResolvedBasicTableLikeOperationOrThrow( - write, PolarisEntitySubType.ICEBERG_TABLE, tableIdentifier); + if (isResolvedBasicTableLikeOperationAuthorized( + write, PolarisEntitySubType.ICEBERG_TABLE, tableIdentifier)) { actionsRequested.add(PolarisStorageActions.WRITE); - } catch (ForbiddenException e) { - // Reuse the already-resolved table view for the read-delegation fallback. + initializeCatalog(); + } else { + // Fall back to read-delegation authorization. Reuse the already-resolved table view. authorizeResolvedBasicTableLikeOperationOrThrow( read, PolarisEntitySubType.ICEBERG_TABLE, tableIdentifier); } diff --git a/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerTest.java b/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerTest.java index 761b4e590a7..3fc94dcaced 100644 --- a/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerTest.java +++ b/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerTest.java @@ -53,6 +53,7 @@ import org.apache.iceberg.rest.responses.ImmutableLoadCredentialsResponse; import org.apache.iceberg.rest.responses.LoadTableResponse; import org.apache.polaris.core.PolarisDiagnostics; +import org.apache.polaris.core.auth.AuthorizationDecision; import org.apache.polaris.core.auth.AuthorizationRequest; import org.apache.polaris.core.auth.AuthorizationState; import org.apache.polaris.core.auth.PolarisAuthorizableOperation; @@ -120,6 +121,9 @@ void setUp() { .build(); when(storageAccessConfigProvider.getStorageAccessConfig(any(), any(), any(), any(), any())) .thenReturn(storageAccessConfig); + // Default: authorize() returns allow. Tests that need a deny override this. + when(authorizer.authorize(any(AuthorizationState.class), any(AuthorizationRequest.class))) + .thenReturn(AuthorizationDecision.allow()); } @SuppressWarnings({"unchecked"}) @@ -319,42 +323,36 @@ void loadCredentialsFallbackResolvesOnceThenAuthorizesReadDelegation() { when(catalog.loadTable(TABLE2)).thenReturn(table); when(accessDelegationModeResolver.resolve(any(), any())) .thenReturn(Optional.of(VENDED_CREDENTIALS)); - doThrow(new ForbiddenException("write delegation denied")) - .when(authorizer) - .authorizeOrThrow( - any(), - any(), - eq(PolarisAuthorizableOperation.LOAD_TABLE_WITH_WRITE_DELEGATION), - nullable(PolarisResolvedPathWrapper.class), - nullable(PolarisResolvedPathWrapper.class)); + // The write-delegation probe now uses authorize(state, request).isAllowed() which does not + // throw. Mock it to return a deny decision so the fallback to read-delegation is triggered. + when(authorizer.authorize(any(AuthorizationState.class), any(AuthorizationRequest.class))) + .thenReturn(AuthorizationDecision.deny("write delegation denied")); @SuppressWarnings("unchecked") ArgumentCaptor requestCaptor = ArgumentCaptor.forClass(AuthorizationRequest.class); ArgumentCaptor stateCaptor = ArgumentCaptor.forClass(AuthorizationState.class); - ArgumentCaptor operationCaptor = - ArgumentCaptor.forClass(PolarisAuthorizableOperation.class); @SuppressWarnings("resource") IcebergCatalogHandler handler = newHandler(); handler.loadCredentials(TABLE2, Optional.empty()); + // Verify resolve is called once for the write-delegation operation. verify(authorizer).resolveAuthorizationInputs(stateCaptor.capture(), requestCaptor.capture()); assertThat(stateCaptor.getValue().getResolutionManifest()).isSameAs(resolutionManifest); assertThat(requestCaptor.getValue().intents().getFirst().getOperation()) .isEqualTo(PolarisAuthorizableOperation.LOAD_TABLE_WITH_WRITE_DELEGATION); - verify(authorizer, org.mockito.Mockito.times(2)) + // Verify authorize() was called for the write-delegation boolean check. + verify(authorizer).authorize(any(AuthorizationState.class), any(AuthorizationRequest.class)); + // Verify authorizeOrThrow is called once for the read-delegation fallback. + verify(authorizer) .authorizeOrThrow( any(), any(), - operationCaptor.capture(), + eq(PolarisAuthorizableOperation.LOAD_TABLE_WITH_READ_DELEGATION), nullable(PolarisResolvedPathWrapper.class), nullable(PolarisResolvedPathWrapper.class)); - assertThat(operationCaptor.getAllValues()) - .containsExactly( - PolarisAuthorizableOperation.LOAD_TABLE_WITH_WRITE_DELEGATION, - PolarisAuthorizableOperation.LOAD_TABLE_WITH_READ_DELEGATION); } @Test diff --git a/runtime/service/src/testFixtures/java/org/apache/polaris/service/TestServices.java b/runtime/service/src/testFixtures/java/org/apache/polaris/service/TestServices.java index 53375902f82..79206a8e1b9 100644 --- a/runtime/service/src/testFixtures/java/org/apache/polaris/service/TestServices.java +++ b/runtime/service/src/testFixtures/java/org/apache/polaris/service/TestServices.java @@ -41,6 +41,7 @@ import org.apache.polaris.core.PolarisCallContext; import org.apache.polaris.core.PolarisDefaultDiagServiceImpl; import org.apache.polaris.core.PolarisDiagnostics; +import org.apache.polaris.core.auth.AuthorizationDecision; import org.apache.polaris.core.auth.AuthorizationState; import org.apache.polaris.core.auth.PolarisAuthorizer; import org.apache.polaris.core.auth.PolarisPrincipal; @@ -255,6 +256,11 @@ public TestServices build() { }) .when(authorizer) .resolveAuthorizationInputs(any(), any()); + // The mock authorizer authorizes everything: authorizeOrThrow(...) is a no-op (void), and + // authorize(state, request) must mirror that by returning an "allow" decision. Without this + // stub the mock returns null and decision-native callers (e.g. loadTable's write-delegation + // probe) would NPE on AuthorizationDecision.isAllowed(). + Mockito.when(authorizer.authorize(any(), any())).thenReturn(AuthorizationDecision.allow()); // Application level StorageCredentialCacheConfig storageCredentialCacheConfig = () -> 10_000;