Reduce resolver work for external authorizers by dimas-b · Pull Request #5662 · apache/polaris

dimas-b · 2026-09-30T21:24:53Z

Migrate OPA and Ranger to using .resolveSelections()
Use AuthorizationState as the accumulator of Resolvable selectors shared between service endpoint implementations and Authorizers
The only non-trivial case of using selectors outside of Authorizer code is in PolarisAdminService.authorizeBasicTopLevelEntityOperationOrThrow()
Ranger Auth always uses REQUESTED_TOP_LEVEL_ENTITIES due to its reliance on the synthetic root container.
Extract common selection code into NonRBACResolutionSemantics
Update TestPolarisAuthorizer to represent general-purpose non-Polaris (external) behaviours in tests.

Relates to #5476

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)

dimas-b · 2026-09-30T21:27:05Z

FYI: @gracechen09 @sungwy : this is still WIP, but please feel free to post early review comments.

dimas-b · 2026-09-30T23:49:36Z

    when(resolutionManifest.resolveAll()).thenReturn(successStatus);
    when(resolutionManifest.getPrimaryResolverStatusOrThrow()).thenReturn(successStatus);
    when(resolutionManifest.getIsPassthroughFacade()).thenReturn(false);
-    doAnswer(


This mock happens to be unnecessary 🤷

sungwy · 2026-10-01T19:22:15Z

+  public void resolve() {
+    if (selections.isEmpty()) {
+      resolutionManifest.resolveAll();


I find this semantic a bit confusing - can we just have the Authorizer selectAll() before resolving?

The built-in Authorizer does that.

For OPA and Ranger some selections come from the authorizer, others from PolarisAdminService.authorizeBasicTopLevelEntityOperationOrThrow(), but we can (should) only resolve once.

I guess we can lose the "select all" flag if selectAll() is called first, and select(X) later... I'll fix that.

updated... PTAL.

sungwy · 2026-10-01T19:27:22Z

+import org.jspecify.annotations.NonNull;
+
+/**
+ * Utility class for processing {@linke AuthorizationRequest} in context that do not involve


nit: typo

Suggested change

* Utility class for processing {@linke AuthorizationRequest} in context that do not involve

* Utility class for processing {@linke AuthorizationRequest} in contexts that do not involve

thx - fixed

sungwy · 2026-10-01T19:27:40Z

+      if (intent instanceof TargetlessAuthorizationIntent) {
+        authzState.select(REQUESTED_TOP_LEVEL_ENTITIES);
+      } else {
+        intent.visitSecurables((securable) -> mergeSelections(authzState, securable));


I like the consumer based approach 👍

sungwy · 2026-10-01T19:34:25Z

+import org.junit.jupiter.api.BeforeAll;
+
+@QuarkusTest
+@TestProfile(Profiles.RestCatalogFileIntegrationProfile.class)


Would it be worth a variant where the test principal has no Polaris grants at all (skip makeAdmin), so we prove loadTable doesn't depend on RBAC state anywhere?

To my understanding PolarisRestCatalogIntegrationBase creates the principal, principal role and the grants prior to the tests. It would be worthwhile having a variation of this without the principal and grants

Also okay to keep this out of scope

Good point. It increases the test matrix a bit, because I think we still have to test with internal principals too (mixed auth mode), but it's worth having that coverage - will add.

Actually, I looks like ExternalPrincipalKeycloakOpaIT covers that case... WDYT?

From the name it sounds like it should, but I see that it creates a test Principal:

polaris/runtime/service-it/src/intTest/java/org/apache/polaris/service/it/ExternalPrincipalKeycloakOpaIT.java

Line 115 in 292101c

ClientPrincipal principal = super.createTestPrincipal(client, principalName, principalRole);

But this does feel like an enormous scope creep. I feel this would be good to write for what you have planned for step2 - I think having this test that relies on an external Authenticator that avoids creating a Principal will help us identify if there's a gap, or if the forced grant loading in the resolver doesn't result in errors anymore.

Updated this test to not create / use local principals at all. In fact, it did not rely on local principals before, auth type and credential-mode were "external"... but now it does not event create them :)

adutra · 2026-10-02T16:38:34Z

    PolarisResolutionManifest resolutionManifest = newResolutionManifest(referenceCatalogName);
    resolutionManifest.addTopLevelName(topLevelEntityName, entityType, false /* isOptional */);
    AuthorizationState authorizationState = new AuthorizationState(resolutionManifest);
+    if (referenceCatalogName != null) {


Out of curiosity, couldn't we change the authorization intent to include the catalog as a securable? Wouldn't it be resolved automatically in that case?

The use case coming from listCatalogRolesForPrincipalRole() is asymmetric. The authZ check does not need the catalog, but later service code needs it.

To make it symmetric, we'll need to authorize LIST_CATALOG_ROLES_FOR_PRINCIPAL_ROLE on the catalog (as a securable), which will require extra grants in the native RBAC, AFAIK.

I'm open to doing that, if you think it makes sense.

I think it makes sense conceptually to require the catalog to be a securable when authorizing listing catalog roles. However, let's defer this work until after this PR is merged.

adutra · 2026-10-02T16:46:14Z

          AuthorizationIntentResolver.resolve(resolutionManifest, intent, prependRootContainer);
      authorizeRangerOrThrow(
          polarisPrincipal,
          resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),


Since we never resolve CALLER_PRINCIPAL_ROLES or CALLER_CATALOG_ROLES, this is now unused it seems, and will always return empty. Should we just remove this parameter?

(it's only used for logging anyways)

Makes sense... Yet, I'd like to keep the scope of this PR narrow. Let's do that change separately. Would that work?

adutra · 2026-10-02T16:46:54Z

  @Test
-  void resolveAuthorizationInputsResolvesAll() {
+  void resolveAuthorizationInputsResolvesSelections() {
    // resolveAll() is intentionally used for compatibility and is expected


Stale comment

thx - removed

adutra · 2026-10-02T16:48:10Z

@@ -133,14 +131,6 @@ void setUp() throws Exception {
    when(resolutionManifest.resolveAll()).thenReturn(successStatus);


I wonder if we still need this stubbing?

adutra · 2026-10-02T16:49:58Z

+- Minor adjustment to the semantics of `PolarisAuthorizer.resolveAuthorizationInputs()`.
+  Existing implementations that used to call `PolarisResolutionManifest.resolveAll()`
+  are expected to be compatible with the new Polaris code. Still, adjustments are recommended as
+  noted in javadoc.


The note is a bit cryptic imho. It doesn't say what changed, and where the "adjustments" should be found (which javadoc?).

I'll redo the CHANGELOG entry when I open this PR for the next review round.

gracechen09 · 2026-10-04T17:08:12Z

+   * delegate to {@link PolarisResolutionManifest#resolveSelections(Set)}, otherwise it will
+   * delegate to {@link PolarisResolutionManifest#resolveAll()}.
+   */
+  public void resolve() {


resolveSelections(Set) is public and takes its selections as a parameter. If a caller injects
selectors via select(...), an implementation that calls resolveSelections(...) on the
manifest would drop the caller's selectors.
Is going through AuthorizationState.resolve() intended to be required once the PR is merged?

@gracechen09 : I'm not sure where the "drop" can happen. Right now the logic is cumulative... unless I missed something 🤔

PolarisResolutionManifest can (and probably should) only be resolved once.

Also, according to the dev thread about multi-entity changes, I believe we should aim at having at most one PolarisResolutionManifest object per request.

https://lists.apache.org/thread/qm60lz3xtnv15swwfb3w6xc1stmflsv3

dimas-b · 2026-10-07T14:46:45Z

I had to squash to simplify conflict resolution during rebase - PTAL.

The commit message is a mess. Please refer to the PR description for change summary.

Relates to apache#5476 Restore resolveAuthorizationInputs Restore resolveAuthorizationInputs mocks Rename AuthorizationState.resolve() resolveAuthorizationInputsResolvesSelections() Add CHANGELOG typo review: Clarify selectAll / resolve logic review: fold ExampleNonRBACAuthorizer into TestPolarisAuthorizer review: rename BasicResolutionSemantics to NonRBACResolutionSemantics review: authorizer javadoc review: remove irrelevant comment review: Do not create local principals in ExternalPrincipalKeycloakOpaIT add CHANGELOG entry (breaking changes)

github-project-automation Bot added this to Basic Kanban Board Sep 30, 2026

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

dimas-b mentioned this pull request Sep 30, 2026

Opa Auth: Skip Polaris native principal and RBAC resolution in OpaPolarisAuthorizer #5476

Open

dimas-b changed the title ~~WIP: Minimize resolver work for external authorizers~~ WIP: Reduce resolver work for external authorizers Sep 30, 2026

dimas-b force-pushed the ext-authz-resolve branch 4 times, most recently from ca1c1b9 to b27c77e Compare September 30, 2026 23:22

dimas-b commented Sep 30, 2026

View reviewed changes

dimas-b changed the title ~~WIP: Reduce resolver work for external authorizers~~ Reduce resolver work for external authorizers Oct 1, 2026

dimas-b requested review from adutra and sungwy October 1, 2026 01:14

dimas-b force-pushed the ext-authz-resolve branch from 1aa359b to 49d87e4 Compare October 1, 2026 01:26

dimas-b marked this pull request as ready for review October 1, 2026 01:26

sungwy reviewed Oct 1, 2026

View reviewed changes

adutra mentioned this pull request Oct 2, 2026

Switch OPA and Ranger authorizers to resolveSelections #5445

Closed

adutra reviewed Oct 2, 2026

View reviewed changes

gracechen09 reviewed Oct 4, 2026

View reviewed changes

adutra mentioned this pull request Oct 5, 2026

Remove synthetic principals hack in Resolver #5439

Open

dimas-b marked this pull request as draft October 5, 2026 18:40

dimas-b force-pushed the ext-authz-resolve branch 2 times, most recently from c0203de to 90bfa2c Compare October 7, 2026 00:12

dimas-b marked this pull request as ready for review October 7, 2026 00:56

dimas-b requested review from adutra and sungwy October 7, 2026 00:57

dimas-b force-pushed the ext-authz-resolve branch from 90bfa2c to a4f32a7 Compare October 7, 2026 14:43

dimas-b added 2 commits October 7, 2026 14:53

add CHANGELOG

8b042b7

dimas-b force-pushed the ext-authz-resolve branch from a4f32a7 to 8b042b7 Compare October 7, 2026 18:55

	* Utility class for processing {@linke AuthorizationRequest} in context that do not involve
	* Utility class for processing {@linke AuthorizationRequest} in contexts that do not involve

		@@ -133,14 +131,6 @@ void setUp() throws Exception {
		when(resolutionManifest.resolveAll()).thenReturn(successStatus);

Conversation

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

Uh oh!

Checklist

Uh oh!

dimas-b commented Sep 30, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

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!

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!

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

Uh oh!

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dimas-b commented Sep 30, 2026 •

edited

Loading

dimas-b commented Oct 7, 2026 •

edited

Loading