Add boolean-return authorization helper for control-flow decisions

[ZephyrYWZhou]

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]

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?

[ZephyrYWZhou]

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

[dimas-b]

@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?

[ZephyrYWZhou]

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?

[dimas-b]

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

[ZephyrYWZhou]

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]

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]

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]

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]

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]

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]

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]

@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]

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.