Add boolean-return authorization helper for control-flow decisions by ZephyrYWZhou · Pull Request #5034 · apache/polaris

ZephyrYWZhou · 2026-07-10T22:08:02Z

Summary

This PR introduces isResolvedBasicTableLikeOperationAuthorized() in CatalogHandler—the first service-layer caller of PolarisAuthorizer.authorize(state, request) for boolean authorization decisions—and refactors authorizeLoadTable to use it.

Problem

authorizeLoadTable currently relies on catching ForbiddenException for normal control flow. It first attempts write-delegation authorization and, if that fails, falls back to read-delegation authorization. Using exceptions for expected control flow reduces readability and incurs unnecessary overhead from exception creation and stack trace capture. The code already contains a TODO for this improvement:

// TODO: Refactor to have a boolean-return version of the helpers so we can fallthrough easily.

Changes

1. `CatalogHandler.java`

Added isResolvedBasicTableLikeOperationAuthorized(op, identifier), which uses the decision-native authorize(AuthorizationState, AuthorizationRequest).isAllowed() path instead of relying on exceptions. Like authorizeResolvedBasicTableLikeOperationOrThrow(), this helper assumes resolveBasicTableLikeTargetOrThrow() has already been called.

2. `IcebergCatalogHandler.java`

Refactored authorizeLoadTable to use the new helper, replacing exception-based control flow with a straightforward conditional.

Before

try {
    authorizeResolvedBasicTableLikeOperationOrThrow(
        write, PolarisEntitySubType.ICEBERG_TABLE, tableIdentifier);
    actionsRequested.add(PolarisStorageActions.WRITE);
} catch (ForbiddenException e) {
    authorizeResolvedBasicTableLikeOperationOrThrow(
        read, PolarisEntitySubType.ICEBERG_TABLE, tableIdentifier);
}

After

if (isResolvedBasicTableLikeOperationAuthorized(write, tableIdentifier)) {
    actionsRequested.add(PolarisStorageActions.WRITE);
    initializeCatalog();
} else {
    authorizeResolvedBasicTableLikeOperationOrThrow(
        read, PolarisEntitySubType.ICEBERG_TABLE, tableIdentifier);
}

3. `IcebergCatalogHandlerTest.java`

Updated loadCredentialsFallbackResolvesOnceThenAuthorizesReadDelegation to mock the decision-native authorize() path (returning AuthorizationDecision.deny()) instead of configuring authorizeOrThrow() to throw. Also added a default authorize() mock returning AuthorizationDecision.allow() for the remaining tests.

Scope

This PR only refactors the authorizeLoadTable call site. A similar exception-based pattern exists in authorizeRegisterTable, but addressing that will require additional boolean-returning helpers for namespace-level and overwrite authorization. That work will be handled in a follow-up PR.

Testing

Updated the existing test in IcebergCatalogHandlerTest to verify the new decision-native authorization flow. All tests in IcebergCatalogHandlerTest pass.

Checklist

🛡️ Not a security issue
🔗 Resolves the existing TODO in IcebergCatalogHandler
🧪 Updated existing tests to cover the new authorize() path
💡 Added Javadoc for the new helper
🧾 No CHANGELOG update required (internal refactoring)
📚 No documentation update required

dimas-b

Thanks for your contribution, @ZephyrYWZhou !

+1 to the intent behind this PR 👍

There are some WIP activities to switch to exception-less authZ calls. Perhaps it might be best to wait for that transition to happen across the codebase.

dimas-b · 2026-07-10T22:37:16Z

+      authorizer()
+          .authorizeOrThrow(
+              polarisPrincipal(),
+              resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
+              op,
+              target,
+              null /* secondary */);
+      initializeCatalog();
+      return true;


This may be a bit premature. Ideally we should switch callers to PolarisAuthorizer.authorize(), but I'm not sure the code is ready for that yet.

@sungwy : WDYT?

Thanks @dimas-b ! Good to know there's work already happening here.

Would it make sense to merge this as-is for the immediate improvement and then I can follow up to adopt the authorize(state, request) path once that transition is further along? I can hold off until then to avoid churn as well if that is the preferred direction. Either way works for me

@ZephyrYWZhou : TBH, I do not see much of a change in this PR. The exception is still in the control flow. It just gets caught in a different place 🤔 Did I miss something?

Yeah you're right @dimas-b, I think the end-of-day right way is to use authorize(state, request).isAllowed() directly so no exception is thrown in the first place. What do you think?

Yes, I think Polaris code needs to switch to the authorize(state, request) method.

Revised per your feedback - now uses authorize(state, request).isAllowed() directly, no exceptions in the control flow. Would appreciate another look when you get a chance. Thanks!

flyingImer

Curious why not go through authorize(state, request).isAllowed() instead, PolarisAuthorizer already has that decision-native method and nothing calls it yet. This PR hand-rolls a boolean twin of the throw method instead, and authorizeRegisterTable's flagged as needing the same fix next. Feels like we're growing a second method family per operation instead of just using the one already there. Not blocking, just checking this is the direction you want.

ZephyrYWZhou · 2026-07-13T22:37:32Z

Curious why not go through authorize(state, request).isAllowed() instead, PolarisAuthorizer already has that decision-native method and nothing calls it yet. This PR hand-rolls a boolean twin of the throw method instead, and authorizeRegisterTable's flagged as needing the same fix next. Feels like we're growing a second method family per operation instead of just using the one already there. Not blocking, just checking this is the direction you want.

Good question. I looked into it - as far as I can tell, authorize(state, request) isn't actually called anywhere in the service layer today. All existing call sites go through resolveAuthorizationInputs + authorizeOrThrow with the old-style individual params. So using authorize(state, request).isAllowed() here would be the first call site to adopt that path.

I'm up for being that first caller though if that's the direction folks want.

flyrain · 2026-07-14T20:57:30Z

Good question. I looked into it - as far as I can tell, authorize(state, request) isn't actually called anywhere in the service layer today. All existing call sites go through resolveAuthorizationInputs + authorizeOrThrow with the old-style individual params. So using authorize(state, request).isAllowed() here would be the first call site to adopt that path.

I'm up for being that first caller though if that's the direction folks want.

Yes, that's the direction the community want to go. We are in the middle of the transition from authorizeOrThrow to authorize(state, request). cc @sungwy, who was leading that effort.

ZephyrYWZhou · 2026-07-15T04:21:59Z

Good question. I looked into it - as far as I can tell, authorize(state, request) isn't actually called anywhere in the service layer today. All existing call sites go through resolveAuthorizationInputs + authorizeOrThrow with the old-style individual params. So using authorize(state, request).isAllowed() here would be the first call site to adopt that path.
I'm up for being that first caller though if that's the direction folks want.

Yes, that's the direction the community want to go. We are in the middle of the transition from authorizeOrThrow to authorize(state, request). cc @sungwy, who was leading that effort.

Sounds good thanks @flyrain, let me refactor my PR to reflect upon that direction. Feel free to let me know if there is any related tasks that need to be pick up as well.

CC: @sungwy

ZephyrYWZhou · 2026-07-17T22:08:41Z

Revised - the implementation now uses authorize(state, request).isAllowed() directly via the new isResolvedBasicTableLikeOperationAuthorized() helper. The write-delegation probe no longer relies on throwing or catching exceptions.

The corresponding test has also been updated to mock the decision-native authorization path.

Since this is the first service-layer caller of PolarisAuthorizer.authorize() that performs boolean authorization decisions, my implementation was finalized after aligning with the community's feedback and consensus.

dimas-b · 2026-07-18T19:28:41Z

+            List.of(
+                new SingleTargetAuthorizationIntent(
+                    op, PolarisSecurableMapper.tableLike(catalogName(), identifier))));
+    return authorizer().authorize(authorizationState, request).isAllowed();


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.

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.

@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

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.

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.

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

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.'

ZephyrYWZhou · 2026-07-21T07:13:16Z

Re-pushed with the CI fix (null-resolved-path guard + test-fixture stub); checks should be green now. @flyrain would appreciate you retriggering the workflow and your review when you have a chance.

ZephyrYWZhou · 2026-07-28T02:20:30Z

I have created the issue here to track the full migration progress - #5171 within which I put down the planning for the PRs to perform the migration incrementally.

BTW here is the first preparatory PR - #5170 - will be appreicated if I can get a reivew on this PR as the first PR of this series @dimas-b.

github-project-automation Bot added this to Basic Kanban Board Jul 10, 2026

github-project-automation Bot moved this to PRs In Progress in Basic Kanban Board Jul 10, 2026

dimas-b reviewed Jul 10, 2026

View reviewed changes

dimas-b mentioned this pull request Jul 10, 2026

Entity-level filtering: API and feature flag #4831

Open

6 tasks

flyingImer reviewed Jul 11, 2026

View reviewed changes

ZephyrYWZhou requested review from dimas-b and flyingImer July 13, 2026 22:37

ZephyrYWZhou force-pushed the refactor-boolean-auth-helper branch 2 times, most recently from c27a018 to 117cdc8 Compare July 17, 2026 22:01

dimas-b reviewed Jul 18, 2026

View reviewed changes

ZephyrYWZhou force-pushed the refactor-boolean-auth-helper branch from 117cdc8 to e7fda2d Compare July 21, 2026 06:58

ZephyrYWZhou requested a review from dimas-b July 21, 2026 07:10

This was referenced Jul 28, 2026

Route table-like authorization through authorize(state, request) #5170

Open

Migrate all authorizeOrThrow(principal, ...) callers to authorize(state, request) and remove the legacy overloads #5171

Open

Uh oh!

Conversation

ZephyrYWZhou commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problem

Changes

1. CatalogHandler.java

2. IcebergCatalogHandler.java

3. IcebergCatalogHandlerTest.java

Scope

Testing

Checklist

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flyingImer left a comment

Choose a reason for hiding this comment

Uh oh!

ZephyrYWZhou commented Jul 13, 2026

Uh oh!

flyrain commented Jul 14, 2026

Uh oh!

ZephyrYWZhou commented Jul 15, 2026

Uh oh!

ZephyrYWZhou commented Jul 17, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZephyrYWZhou Jul 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZephyrYWZhou Jul 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZephyrYWZhou commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZephyrYWZhou commented Jul 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ZephyrYWZhou commented Jul 10, 2026 •

edited

Loading

1. `CatalogHandler.java`

2. `IcebergCatalogHandler.java`

3. `IcebergCatalogHandlerTest.java`

ZephyrYWZhou Jul 19, 2026 •

edited

Loading

ZephyrYWZhou Jul 19, 2026 •

edited

Loading

dimas-b Jul 23, 2026 •

edited

Loading

ZephyrYWZhou commented Jul 21, 2026 •

edited

Loading