runtime: Auth Refactor - Update `CatalogHandler`s and `PolarisAdminService` to use `resolveAuthorizationInputs` by sungwy · Pull Request #4356 · apache/polaris

sungwy · 2026-05-02T22:22:38Z

Phase 4 of #3779

Use resolveAuthorizationInputs() at handler and admin call sites so authorization-driven entity resolution is owned by the PolarisAuthorizer SPI rather than being hardcoded in service code.

Before this change, CatalogHandlers and PolarisAdminService eagerly performed broad Polaris resolution before authorization, including the target entity and other RBAC-related entities such as the reference catalog, caller principal, principal roles, and catalog roles. That meant resolution scope was effectively fixed by the handlers, even for authorizers that would not need Polaris RBAC state.

After this change, handlers build authorization inputs and pass them to PolarisAuthorizer.resolveAuthorizationInputs(). The authorizer then decides what Polaris entities, if any, need to be resolved in order to authorize the request.

Important call outs:

updateTable() now resolves the reference catalog separately before auth so it can read catalog-scoped config for fine-grained update privileges, then runs normal authorizer-controlled pre-resolution for authorization. This removes the old unconditional full RBAC-style pre-auth resolution, but introduces a less-atomic two-resolution window for catalog config.
Empty-namespace table-like identifiers are no longer tolerated in the migrated paths. Table-like targets are now expected to have a valid namespace. This removes legacy compatibility for invalid admin grant inputs like TableIdentifier.of(Namespace.empty(), ...)

Checklist

🛡️ Don't disclose security issues! (contact security@apache.org)
🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
🧪 Added/updated tests with good coverage, or manually tested (and explained how)
💡 Added comments for complex logic
🧾 Updated CHANGELOG.md (if needed)
📚 Updated documentation in site/content/in-dev/unreleased (if needed)

Copilot

Pull request overview

This PR advances the runtime auth refactor from issue #3779 by moving pre-authorization entity resolution out of handlers/admin call sites and into PolarisAuthorizer.resolveAuthorizationInputs(), so authorizers can decide what Polaris state they need to resolve before authorization.

Changes:

Replaced direct resolutionManifest.resolveAll() calls in catalog/admin handlers with AuthorizationRequest/AuthorizationState-driven pre-resolution.
Added PolarisSecurableMapper to translate handler/admin identifiers into canonical authorization targets.
Updated test scaffolding and mocks to simulate the new pre-resolution flow, and adjusted updateTable() to read catalog-scoped config via a separate catalog-only resolution step.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`runtime/service/src/testFixtures/java/org/apache/polaris/service/TestServices.java`	Updates shared test fixture authorizer mock to resolve inputs through the new SPI path.
`runtime/service/src/test/java/org/apache/polaris/service/admin/PolarisAdminServiceTest.java`	Adapts admin service tests to the new pre-resolution contract and revised table-like grant setup.
`runtime/service/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalogHandler.java`	Migrates policy handler auth pre-resolution to `AuthorizationRequest` inputs.
`runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java`	Refactors `updateTable()` auth setup to resolve catalog config separately before auth.
`runtime/service/src/main/java/org/apache/polaris/service/catalog/common/PolarisSecurableMapper.java`	New helper for mapping catalog/domain identifiers into `PolarisSecurable` paths.
`runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java`	Migrates shared catalog handler auth helpers to authorizer-controlled input resolution.
`runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java`	Migrates admin auth helpers and grant flows to `resolveAuthorizationInputs()`.
`polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/PolarisResolutionManifest.java`	Exposes primary resolver status for callers that now read post-resolution status after authorizer-controlled resolution.
`extensions/auth/ranger/impl/src/main/java/org/apache/polaris/extension/auth/ranger/RangerPolarisAuthorizer.java`	Adds temporary backward-compatible `resolveAuthorizationInputs()` implementation for Ranger.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

sungwy · 2026-05-05T11:40:52Z

+  public static PolarisSecurable namespace(String catalogName, Namespace namespace) {
+    ImmutablePolarisSecurable.Builder builder =
+        ImmutablePolarisSecurable.builder()
+            .addPathSegment(new PathSegment(PolarisEntityType.CATALOG, catalogName));
+    Arrays.stream(namespace.levels())
+        .map(level -> new PathSegment(PolarisEntityType.NAMESPACE, level))
+        .forEach(builder::addPathSegment);
+    return builder.build();


Thanks Co-pilot, let me take a look into this

updated PolarisSecurableMapper methods to explicitly validate the inputs: .namespace() expects non-empty Namespace

dimas-b · 2026-05-13T18:03:53Z

Should we hold this until after 1.5.0 is branched ?

dimas-b · 2026-05-13T18:09:19Z

      TableIdentifier identifier) {
    ensureResolutionManifestForTable(identifier);
+    if (resolutionManifest.getPrimaryResolverStatus() == null) {
+      if (!ops.isEmpty()) {


Should it be a runtime error is ops is empty? 🤔

dimas-b · 2026-05-13T18:10:39Z

      PolarisEntitySubType subType,
      TableIdentifier identifier) {
    ensureResolutionManifestForTable(identifier);
+    if (resolutionManifest.getPrimaryResolverStatus() == null) {


How can we have a resolutionManifest here that has already been resolved?.. Just for my education 😅

This is a good call out. I broke down resolution and authorization into two separate methods so we can have the caller ensure that resolution is being called once

dimas-b · 2026-06-02T17:19:51Z

@sungwy : Is this ready for another review round, I assume?

…hase4

sungwy · 2026-06-03T20:29:36Z

  }

-  protected void authorizeBasicTableLikeOperationOrThrow(
+  protected void resolveBasicTableLikeTargetOrThrow(


note to reviewer: we have a few cases where we resolve only once, and then authorize using the same manifest in IcebergCatalogHandler like in updateTable and loadTable.

Having two separate methods for resolution and authorization ensures that we are resolving just once, without relying on a logic that checks if we have yet to resolve - like if (resolutionManifest.getPrimaryResolverStatus() == null) - which is more errorprone.

dimas-b

LGTM 👍 Thanks, @sungwy !

sungwy · 2026-06-05T22:12:25Z

LGTM 👍 Thanks, @sungwy !

Thanks @dimas-b !

My apologies, but looking at the CI, I think there's some additional changes I'd need to make in order to update the existing test suite to conform to the new pattern.

My hunch is that a CatalogHander instance is being reused across requests in the test suite, which doesn't mirror how we do things in production today.

…zing twice

# Conflicts: # runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java # runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogTest.java

flyrain

Thanks @sungwy for working on it. Left some comments.
Also empty-namespace table-like identifiers (TableIdentifier.of(Namespace.empty(), ...)) are now rejected on migrated admin-grant paths. It's user-observable, worth a CHANGELOG.md line even though the bulk is an internal refactor.

flyrain · 2026-06-17T17:09:59Z

+            List.of(
+                new SingleTargetAuthorizationIntent(
+                    op, PolarisSecurable.of(new PathSegment(entityType, topLevelEntityName))))));
+    ResolverStatus status = resolutionManifest.getPrimaryResolverStatus();


getPrimaryResolverStatus() is @Nullable, should we check it before using?

That's a good call out. I've changed the public method to getPrimaryResolverStatusOrThrow() which throws if the PolarisResolutionManifest had not yet resolved.

flyrain · 2026-06-17T17:09:59Z

+    return builder.build();
+  }
+
+  public static PolarisSecurable tableLike(String catalogName, TableIdentifier identifier) {


namespace() throws on an empty namespace, but tableLike() here silently maps an empty namespace to catalog/<name>. Since this PR explicitly drops empty-namespace table-like support, add the same identifier.namespace().isEmpty() guard here rather than relying on the resolver to reject it downstream.

Thanks @flyrain - this is a good callout. When I first ran into this, I decided that it would be safe to make this change because we don't support empty namespaces in the IcebergCatalogHandler today in Polaris.

But I agree that it's good to document any behavioral changes in the CHANGELOG

flyrain · 2026-06-17T17:59:32Z


  public abstract PolarisAuthorizer authorizer();

+  public abstract AuthorizationState authorizationState();


AuthorizationState looks like a per-call value, not request-scoped shared state.

Tracing how it's used: the handler already holds the manifest in its own resolutionManifest field (CatalogHandler.java:96). Each authorize method does resolutionManifest = newResolutionManifest(), then authorizationState().setResolutionManifest(resolutionManifest), then reads results back from its own field, never from authorizationState().getResolutionManifest(). So AuthorizationState's only job is to be the parameter type carrying the manifest into resolveAuthorizationInputs(...) so the authorizer can read it back out.

That leaves two holders of the same reference with contradictory semantics: the handler field gets freely reassigned, but AuthorizationState.setResolutionManifest is write-once (compareAndSet(null, ...) that throws on a second set). The write-once guard only makes sense because the instance is @RequestScoped and shared, and it's effectively dead defensive code, since every call site just wants to wrap a freshly-built manifest and pass it along.

Since every setResolutionManifest is really "replace with the new manifest I just built," I think this should just be a fresh object per authorize call:

authorizer().resolveAuthorizationInputs(new AuthorizationState(resolutionManifest), request);

That would let us drop the @Produces @RequestScoped producer, the abstract authorizationState() accessor + injected factory fields, and the AtomicReference/compareAndSet guard (just a final field). This simplify the plumbing quite a bit. It also removes a latent landmine: any future flow that runs two authorize passes in one request would hit "resolution manifest already set" on the shared write-once instance.

If the intent was for AuthorizationState to accumulate state across auth phases, then the write-once guard contradicts that and we should design it as a real accumulator instead. Either way the current shape feels in-between. WDYT?

Hi @flyrain thanks for the detailed review.

I see why it reads that way, but I think the intended contract is different.

AuthorizationState is meant to be request-scoped state that the PolarisAuthorizer can manipulate throughout the request. This is basically a renamed version of the “auth call context” idea we discussed in the Polaris Community Sync 1/27/2026, which allows us to avoid having to do a large refactoring of the Persistence/Resolution layer to reach the same outcome today of skipping RBAC resolution for external authorizers.

The write-once behavior per request is intentional to me. For a given request, we should know the full path of the resource we want to resolve up front. If we try to swap the manifest halfway through the request, I’d rather fail loudly than silently authorize/execute against a different resolved view.

I agree the current duplicate storage is awkward: the handler field and AuthorizationState point to the same thing. But I think that becomes a supporting case for a refactoring of the Resolver/Persistence layer, rather than making AuthorizationState a disposable per-call wrapper.

Thanks for all the context, @sungwy . Do we expect adding more attributes to the AuthorizationState? Are they expected to be request scoped? It'd be nice to share more forward looking thoughts if you don't mind.

Hi @flyrain I don’t currently have a concrete second attribute in mind for AuthorizationState. The main state I care about here is the PolarisResolutionManifest, because it represents the resolved view we use across authz and execution.

The forward-looking part is that this was meant to preserve today’s resolution semantics while moving the decision of what to resolve into the PolarisAuthorizer. Longer term, I still think a larger Persistence/Resolution refactor is probably the cleaner direction, so handlers and authorizers are less coupled to a callsite-managed PolarisResolutionManifest directly. Keeping AuthorizationState request-scoped felt like a small step in that direction, but I agree it is not the full design.

Given that the larger refactor has not been designed yet, I’m okay relaxing this for now and treating AuthorizationState as a per-resolution wrapper around the manifest. That keeps this PR focused on moving resolution choice into the authorizer, while leaving request-scoped persistence/resolution state for a more concrete follow-up design.

The tradeoff is that we lose the request-lifetime guard and keep responsibility for resolved-view consistency at the call sites, which is basically where it already lives today.

@flyrain , @dimas-b - does that sound like a good way to move this PR forward?

@sungwy : that works for me 👍

Works for me, thanks @sungwy !

# Conflicts: # runtime/service/src/test/java/org/apache/polaris/service/catalog/generic/PolarisGenericTableCatalogHandlerAuthzTest.java

dimas-b · 2026-06-23T15:44:46Z


 ### Breaking changes
 - The `MaintenanceService.performMaintenance()` signature now requires an explicit `OptionalLong overrideRunId` argument to supersede the latest unfinished maintenance run.
+- Admin grant APIs now reject table-like privilege targets with an empty namespace. A table-like target without a namespace is considered invalid input.


nit: I'm not sure this is actually a breaking change from the end user perspective, because IRC catalogs do not allow creating tables/views without a namespace, AFAIK... at least not in the Iceberg java client 🤔

flyingImer

Thanks for this. Read against the #3779 arc, the shape is right. The request side now says what to authorize as plain intent, which is what lets an authorizer participate without inheriting Polaris RBAC. That was #3760's stated aim, and this PR takes it live at the call sites.

The piece I'd settle before the decision path cuts over: the request became implementation-neutral, but the state object didn't. AuthorizationState still hands every authorizer a PolarisResolutionManifest, and each one just calls resolveAll() on it, so an authorizer that doesn't want Polaris-native resolution still has to accept and drive it to satisfy the contract. The intent half lets an authorizer bring its own machinery; the state half still requires the native one. Since the point of the series is to let the authorizer decide whether native resolution happens at all, I'd make that state opaque or optional before phases 5 and 6 lock the contract in.

Two smaller things in the same spirit:

updateTable resolves with a placeholder operation to read catalog config, then authorizes a different set. It's fine today, but it quietly assumes resolution is operation-independent, which your own comment flags. That assumption is part of the contract now, so it belongs in the resolveAuthorizationInputs javadoc rather than a TODO. It's exactly what a selective authorizer would break.
The empty-namespace rejection turns a 404 into a 400 on public catalog paths (commitTransaction, renameTable source), not just admin grants, and the new mapper applies the guard unevenly (policy() is missing it). The 400 is the right call; the CHANGELOG just scopes it too narrowly.

Direction's good. The contract is easiest to get neutral now, while the new decision API is still dormant at the call sites.

sungwy · 2026-07-07T14:12:10Z

Thanks for this. Read against the #3779 arc, the shape is right. The request side now says what to authorize as plain intent, which is what lets an authorizer participate without inheriting Polaris RBAC. That was #3760's stated aim, and this PR takes it live at the call sites.

The piece I'd settle before the decision path cuts over: the request became implementation-neutral, but the state object didn't. AuthorizationState still hands every authorizer a PolarisResolutionManifest, and each one just calls resolveAll() on it, so an authorizer that doesn't want Polaris-native resolution still has to accept and drive it to satisfy the contract. The intent half lets an authorizer bring its own machinery; the state half still requires the native one. Since the point of the series is to let the authorizer decide whether native resolution happens at all, I'd make that state opaque or optional before phases 5 and 6 lock the contract in.

Two smaller things in the same spirit:

updateTable resolves with a placeholder operation to read catalog config, then authorizes a different set. It's fine today, but it quietly assumes resolution is operation-independent, which your own comment flags. That assumption is part of the contract now, so it belongs in the resolveAuthorizationInputs javadoc rather than a TODO. It's exactly what a selective authorizer would break.

The empty-namespace rejection turns a 404 into a 400 on public catalog paths (commitTransaction, renameTable source), not just admin grants, and the new mapper applies the guard unevenly (policy() is missing it). The 400 is the right call; the CHANGELOG just scopes it too narrowly.

Direction's good. The contract is easiest to get neutral now, while the new decision API is still dormant at the call sites.

Hi @flyingImer - thanks for the review. Are these blocking concerns in your opinion? I'm looking to do one more pass to incorporate the consensus I've been able to reach with @dimas-b @flyrain on the AuthorizationState. If these aren't blocking concerns, I'd like to move this PR forward

# Conflicts: # runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogTest.java

flyrain

+1 Thanks @sungwy for working on it.

# Conflicts: # runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogViewTest.java

sungwy · 2026-07-08T14:46:33Z

Thanks for the reviews @dimas-b , @flyrain and @flyingImer !

sungwy added 3 commits April 30, 2026 09:10

use resolveAuthorizationInputs

02a6cdf

clean up

43b52d1

use PolarisSecurable.of directly

bc2ae04

github-project-automation Bot added this to Basic Kanban Board May 2, 2026

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

sungwy added 3 commits May 2, 2026 22:45

implement resolveAuthorizationInputs in RangerPolarisAuthorizer

e9538ea

lint

9b6218d

fix tests

0a15a77

sungwy requested a review from Copilot May 4, 2026 11:29

Copilot started reviewing on behalf of sungwy May 4, 2026 11:30 View session

Copilot AI reviewed May 4, 2026

View reviewed changes

sungwy marked this pull request as ready for review May 5, 2026 11:41

adopt review feedback

e1300d0

dimas-b reviewed May 13, 2026

View reviewed changes

sungwy added 2 commits June 2, 2026 13:14

merge main

b809f59

merge main and resolve conflict

2caec94

sungwy added 3 commits June 2, 2026 15:49

use constructor

4771a9b

fix

4082191

Merge branch 'main' of github.com:sungwy/polaris into auth-refactor-p…

b7cfc0d

…hase4

sungwy commented Jun 3, 2026

View reviewed changes

Use request-scoped CDI AuthorizationState instead of local instantiation

4c0dbee

dimas-b previously approved these changes Jun 3, 2026

View reviewed changes

github-project-automation Bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Jun 3, 2026

dimas-b requested review from dennishuo and flyrain June 3, 2026 21:37

sungwy added 2 commits June 8, 2026 22:32

make fallbackToFullLoadTable reuse resolved entity instead of authori…

3003978

…zing twice

update test suite to refresh used request-scoped context

dde2e46

sungwy dismissed dimas-b’s stale review via dde2e46 June 9, 2026 02:32

sungwy added 3 commits June 9, 2026 06:31

merge main

f2ce9c2

update test suite to refresh used request-scoped context

1d4507a

dimas-b previously approved these changes Jun 16, 2026

View reviewed changes

flyrain reviewed Jun 17, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into auth-refactor-phase4

32389bb

# Conflicts: # runtime/service/src/test/java/org/apache/polaris/service/catalog/generic/PolarisGenericTableCatalogHandlerAuthzTest.java

sungwy dismissed dimas-b’s stale review via 32389bb June 22, 2026 14:52

adopt feedback - thanks @flyrain

a2f62fd

dimas-b previously approved these changes Jun 23, 2026

View reviewed changes

flyingImer reviewed Jun 24, 2026

View reviewed changes

sungwy added 2 commits July 7, 2026 11:12

AuthorizationState no longer request-scoped

2691b60

Merge remote-tracking branch 'origin/main' into auth-refactor-phase4

30fffc8

# Conflicts: # runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogTest.java

sungwy dismissed dimas-b’s stale review via 30fffc8 July 7, 2026 15:27

dimas-b previously approved these changes Jul 7, 2026

View reviewed changes

flyrain previously approved these changes Jul 7, 2026

View reviewed changes

only allow resolveAll to succeed once

59abcf1

sungwy dismissed stale reviews from flyrain and dimas-b via 59abcf1 July 7, 2026 23:14

sungwy requested a review from dimas-b July 8, 2026 01:30

dimas-b previously approved these changes Jul 8, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into auth-refactor-phase4

9dcb6d6

# Conflicts: # runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogViewTest.java

sungwy dismissed dimas-b’s stale review via 9dcb6d6 July 8, 2026 13:58

dimas-b approved these changes Jul 8, 2026

View reviewed changes

dimas-b mentioned this pull request Jul 8, 2026

Make admin tool bootstrap idempotent for already-bootstrapped realms #4996

Merged

6 tasks

sungwy merged commit 896841e into apache:main Jul 8, 2026
25 checks passed

github-project-automation Bot moved this from Ready to merge to Done in Basic Kanban Board Jul 8, 2026


		public abstract PolarisAuthorizer authorizer();

		public abstract AuthorizationState authorizationState();

Uh oh!

Conversation

sungwy commented May 2, 2026

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dimas-b commented May 13, 2026

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!

Uh oh!

dimas-b commented Jun 2, 2026

Uh oh!

sungwy Jun 3, 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 left a comment

Choose a reason for hiding this comment

Uh oh!

sungwy commented Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flyrain 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!

flyrain Jun 18, 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!

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!

sungwy commented Jul 7, 2026

Uh oh!

flyrain left a comment

Choose a reason for hiding this comment

Uh oh!

sungwy commented Jul 8, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

sungwy Jun 3, 2026 •

edited

Loading

sungwy commented Jun 5, 2026 •

edited

Loading

flyrain Jun 18, 2026 •

edited

Loading