Skip to content

Add boolean-return authorization helper for control-flow decisions #5034

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ZephyrYWZhou wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add boolean-return authorization helper for control-flow decisions #5034

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,38 @@
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.
*
* <p>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

Check warning on line 408 in runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java

View workflow job for this annotation

GitHub Actions / CI/PR / Gradle Build Checks 

reference not found: #notFoundExceptionForTableLikeEntity
* 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();

@dimas-b dimas-b Jul 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about using authorize() just in one narrow sub-case. This is prone to correctness deviations on different authorizer impl. call paths.

I'd prefer to switch all callers from authorizeOrThrow() to authorize()and remove the old method.

@ZephyrYWZhou ZephyrYWZhou Jul 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense to me, it looks like there are 22 authorizeOrThrow call sites across CatalogHandler (9), PolicyCatalogHandler (3), and PolarisAdminService (10). I can take the full migration task there.

@ZephyrYWZhou ZephyrYWZhou Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dimas-b Quick clarification on the scope: when you say "switch all callers from authorizeOrThrow() to authorize() and remove the old method," do you mean removing only the legacy overloads (those taking principal, activatedEntities, op, target, secondary) while keeping authorizeOrThrow(AuthorizationState, AuthorizationRequest)? Or do you mean removing all authorizeOrThrow() variants and having every call site handle AuthorizationDecision directly?

My preference is to keep authorizeOrThrow(state, request) since it's a thin convenience wrapper over authorize().isAllowed() and most call sites simply want to throw on denial. The main benefit, in my view, is removing the legacy overloads that bypass the decision-native path.

CC: @sungwy for opinion as well

@ZephyrYWZhou ZephyrYWZhou Jul 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update on the broader migration - I tried switching all authorizeOrThrow call sites to authorize(state, request) and ran into a fundamental issue.

The legacy path operates on already-resolved entities, while authorize(state, request) re-resolves them via getResolvedSecurable(). Many existing call sites don't populate the resolution manifest in a way that's compatible with this re-resolution, resulting in IllegalStateException: never_registered_top_level_name_and_type_for_resolved_entity.

The loadTable change in this PR works because its manifest is already compatible. Other call sites will require manifest changes in addition to the authorization API switch, which seems to align with the broader migration.

@dimas-b dimas-b Jul 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the authorize() method is meant to be the main method. So the work to migrate all callers to using it needs to be done at some point.

@ZephyrYWZhou : If you feel like taking that task, your efforts will be greatly appreciated. Yet, it's your choice, of course. It may be possible to make a new smaller preparatory PRs before using authorize() at all call sites.

@ZephyrYWZhou ZephyrYWZhou Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good @dimas-b, I would be willing and beyond thrilled to take that task. I will make smaller preparatory PRs as they will be needed before full migration. Thanks

}

protected void resolveAndAuthorizeBasicTableLikeOperationOrThrow(
PolarisAuthorizableOperation op, PolarisEntitySubType subType, TableIdentifier identifier) {
resolveBasicTableLikeTargetOrThrow(op, identifier);
Expand Down
10 changes: 5 additions & 5 deletions ...rvice/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1022,12 +1022,12 @@ private Set<PolarisStorageActions> authorizeLoadTable(

Set<PolarisStorageActions> 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);
}
Expand Down
30 changes: 14 additions & 16 deletions ...e/src/test/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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"})
Expand Down Expand Up @@ -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<AuthorizationRequest> requestCaptor =
ArgumentCaptor.forClass(AuthorizationRequest.class);
ArgumentCaptor<AuthorizationState> stateCaptor =
ArgumentCaptor.forClass(AuthorizationState.class);
ArgumentCaptor<PolarisAuthorizableOperation> 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
Expand Down
6 changes: 6 additions & 0 deletions runtime/service/src/testFixtures/java/org/apache/polaris/service/TestServices.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down