Refactor PolarisPrincipal to contain generic attributes by adutra · Pull Request #5085 · apache/polaris

adutra · 2026-07-17T13:29:28Z

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 refactors PolarisPrincipal to carry a generic attributes map (instead of a string-only properties map and separate token), and updates the runtime auth flow and related tests to construct principals with these attributes (including storing the PrincipalEntity as an attribute).

Changes:

Refactor PolarisPrincipal to expose Map<String, Object> getAttributes() with standard attribute keys (principal entity + optional JWT).
Update authentication flow: Authenticator / DefaultAuthenticator now authenticate from a Quarkus SecurityIdentity and build principal attributes from identity + entity.
Update affected unit/integration tests and extension code to use the new PolarisPrincipal.of(name, attributes, roles) factory and updated error messaging.

Reviewed changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
runtime/service/src/testFixtures/java/org/apache/polaris/service/TestServices.java	Update test fixture principal creation to populate principal attributes.
runtime/service/src/test/java/org/apache/polaris/service/catalog/RootPrincipalAugmentor.java	Update test augmentor to create principals with attributes map.
runtime/service/src/test/java/org/apache/polaris/service/catalog/policy/PolicyCatalogHandlerAuthzTest.java	Update test principals to include `PrincipalEntity` attribute.
runtime/service/src/test/java/org/apache/polaris/service/catalog/policy/AbstractPolicyCatalogTest.java	Update authenticated root principal setup to use attributes.
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerFineGrainedDisabledTest.java	Update test principal construction for new `PolarisPrincipal` API.
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogViewTest.java	Update authenticated root principal setup for new attributes-based principal.
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogHandlerAuthzTest.java	Update multiple test principal constructions to include attributes map + entity attribute.
runtime/service/src/test/java/org/apache/polaris/service/catalog/generic/PolarisGenericTableCatalogHandlerAuthzTest.java	Update test wrapper principal creation for new principal API.
runtime/service/src/test/java/org/apache/polaris/service/catalog/generic/AbstractPolarisGenericTableCatalogTest.java	Update authenticated root principal setup to include principal entity attribute.
runtime/service/src/test/java/org/apache/polaris/service/auth/external/OidcPolarisCredentialAugmentorTest.java	Update OIDC augmentor tests to reflect credential/token handling changes.
runtime/service/src/test/java/org/apache/polaris/service/auth/DefaultAuthenticatorTest.java	Update tests for `SecurityIdentity`-based auth and new principal attributes behavior (incl. JWT attribute).
runtime/service/src/test/java/org/apache/polaris/service/auth/AuthenticatingAugmentorTest.java	Update tests for new authenticator signature and identity augmentation behavior.
runtime/service/src/test/java/org/apache/polaris/service/admin/PolarisAuthzTestBase.java	Update authenticated root principal construction to include attributes.
runtime/service/src/test/java/org/apache/polaris/service/admin/PolarisAdminServiceAuthzTest.java	Update admin service tests to construct principals with attributes.
runtime/service/src/test/java/org/apache/polaris/service/admin/ManagementServiceTest.java	Update test principal setup to include principal entity attribute.
runtime/service/src/main/java/org/apache/polaris/service/auth/PolarisCredential.java	Remove token field from `PolarisCredential` factory/contract.
runtime/service/src/main/java/org/apache/polaris/service/auth/external/OidcPolarisCredentialAugmentor.java	Stop embedding JWT raw token into `PolarisCredential`; credential contains id/name/roles only.
runtime/service/src/main/java/org/apache/polaris/service/auth/DefaultAuthenticator.java	Authenticate using `SecurityIdentity`, build `PolarisPrincipal` with attributes (identity attrs + principal entity + optional JWT).
runtime/service/src/main/java/org/apache/polaris/service/auth/Authenticator.java	Change authenticator contract to accept `SecurityIdentity`.
runtime/service/src/main/java/org/apache/polaris/service/auth/AuthenticatingAugmentor.java	Simplify augmentation: authenticate via identity and add principal attributes onto the `SecurityIdentity`.
polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/BaseResolverTest.java	Update test fixture principal creation to set principal entity attribute.
polaris-core/src/test/java/org/apache/polaris/core/auth/AuthorizationPreConditionsTest.java	Update tests to reflect new rotation-required check path and updated message text.
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisPrincipal.java	Introduce generic attributes map and standard attribute keys.
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationPreConditions.java	Switch rotation-required enforcement to inspect `PrincipalEntity` via principal attributes.
integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisManagementServiceIntegrationTest.java	Update expected error message string for rotation-required enforcement.
extensions/auth/ranger/src/main/java/org/apache/polaris/extension/auth/ranger/utils/RangerUtils.java	Update Ranger user-attribute extraction to use `PrincipalEntity` attribute instead of `getProperties()`.

dimas-b · 2026-07-17T14:17:26Z

This change probably overshadows #5032 🤔

adutra · 2026-07-17T14:39:39Z

This change probably overshadows #5032 🤔

Yes, that's the intent 😄

dimas-b · 2026-07-17T14:23:35Z


+  /** Returns the principal attributes. */
+  @Value.Redacted
+  Map<String, Object> getAttributes();


nit: maybe Map<String, ?>?

Returning Map<String, ?> would make it hard to use the returned value, e.g. this code in AuthenticatingAugmentor wouldn't compile:

return QuarkusSecurityIdentity.builder(identity) .setAnonymous(false) .setPrincipal(polarisPrincipal) .addRoles(polarisPrincipal.getRoles()) .addAttributes(polarisPrincipal.getAttributes()) // doesn't compile .build();

Correction: Map<String, ? super Object> should work, I hope 😅

Side question: do we really need PolarisPrincipal attributes as top-level attributes in QuarkusSecurityIdentity? Quarkus probably does not need them and Polaris code should probably go through PolarisPrincipal 🤔

dimas-b · 2026-07-17T15:24:39Z

-  static PolarisPrincipal of(String name, Map<String, String> properties, Set<String> roles) {
-    return of(name, properties, roles, Optional.empty());
-  }
+  String JWT_ATTRIBUTE_KEY = "org.apache.polaris.core.auth.JWT";


FYI: @cccs-cat001

@adutra : This change may be worth mentioning in the CHANGELOG for downstream project visibility, WDYT?

I was reluctant because there is no user-facing changes, only downstream-facing changes, but OK.

I'm fine either way... but other contributors tend to mention java changes in CHANGELOG 🤔

dimas-b · 2026-07-17T15:46:36Z

+    Map<String, String> properties =
+        principal
+            .getAttribute(PolarisPrincipal.PRINCIPAL_ENTITY_ATTRIBUTE_KEY, PrincipalEntity.class)
+            .map(PrincipalEntity::getInternalPropertiesAsMap)


I guess we still need #5032 to forward user-level principal properties to authorizers... It will be a follow-up change. WDYT? CC: @iprithv

I don't think we'd need #5032, authorizers can get the user-facing properties as follows :

principal .getAttribute(PolarisPrincipal.PRINCIPAL_ENTITY_ATTRIBUTE_KEY, PrincipalEntity.class) .map(PrincipalEntity::getPropertiesAsMap) .orElse(Map.of());

Right, I mean that #5032 actually forwards user-level principal properties to OPA and Ranger (IIRC), so some extra change will be required to keep that behaviour after this PR.

flyrain · 2026-07-17T23:12:12Z

    }
  }
+
+  private static boolean mustRotateCredentials(PolarisPrincipal principal) {


PolarisPrincipal's own javadoc says callers must never assume this attribute is present. But the new logic here mandate the property PRINCIPAL_ENTITY_ATTRIBUTE_KEY, otherwise, rotation will skip. I'm not sure the new behavior is correct. Maybe we should make PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_STATE the first level property instead of embedded in PRINCIPAL_ENTITY_ATTRIBUTE_KEY. This is also related to my another comments. Directly embedding the whole Principal entity may expose too much internal state.

But the new logic here mandate the property PRINCIPAL_ENTITY_ATTRIBUTE_KEY, otherwise, rotation will skip. I'm not sure the new behavior is correct.

I think that's correct. If the principal entity does not exist, it's because the principal is fully external. In that case, Polaris cannot know if the credentials need rotation. It's the external IDP's responsibility to deny the token request from such a user.

Yes. Polaris should not interfere with token generation for external principals.

flyrain · 2026-07-17T23:12:12Z

-          .log("Unable to authenticate user with token");
-      throw new ServiceFailureException("Unable to fetch principal entity");
+          .log("Unable to resolve principal entity from credentials");
+      throw new InternalServerErrorException("Unable to fetch principal entity");


This used to throw Iceberg's ServiceFailureException, which IcebergExceptionMapper maps to 503 (SERVICE_UNAVAILABLE). InternalServerErrorException is 500. So a transient metastore hiccup during auth flips from a retryable 503 to a non-retryable 500 for clients. Is the 500 intended? If so, it's worth a CHANGELOG line next to the 401 note.

Good catch! No, I intended to keep using 503 but used the wrong exception class. I will amend.

flyrain · 2026-07-18T00:03:09Z


+  protected Map<String, Object> resolvePrincipalAttributes(
+      SecurityIdentity identity, PrincipalEntity principalEntity) {
+    ImmutableMap.Builder<String, Object> principalAttributes =


One thing I'm wondering about is exposing the entire PrincipalEntity as an attribute. Do we really want authorizers to depend on our internal persistence model? The tighter coupling between the authorization logic and the metastore schema is concerning. It also makes it easier for authorizers to rely on fields that weren't intended to be part of the authorization contract.

I wouldn't say that authorizers are now depending on our internal persistence model. They have internal access to the PrincipalEntity, but they don't have to send the whole entity to OPA or Ranger.

IMHO there is no tight coupling between the metastore schema and the authorizer here, but rather a lose one. Each authorizer is free to convert the entity to whatever representation fits best for their use cases.

Do we really want authorizers to depend on our internal persistence model

PrincipalEntity is part of the core entity model. I suppose "core" should be fine as a dependency in any Polaris extension.

I do not think PrincipalEntity is tied to persistence implementations.

Metastore schema coupling is one thing. Besides, are we conformable to expose the whole PrincipalEntity to authorizer? For example, it will automatically expose all internal properties to authorizer. Is that a expected behavior? What if some internal properties are not supposed to be exposed?

We were already exposing the entity's internal properties to the authorizers. This isn't new.

Thanks for the context. I agree this is not necessarily a blocker for this PR, especially if internal properties were already exposed.

I still think the broader security question is valid. Passing the entire PrincipalEntity means any future internal property automatically becomes accessible to authorizers, and potentially to downstream systems such as OPA or Ranger. That makes it easier to expose sensitive or implementation specific fields unintentionally, and it also allows policies to depend on fields that are not part of a stable authorization contract.

It may be safer to define an explicit, allowlisted authorization view of the principal.

Authorizer implementations are capable of loading anything from Polaris Persistence. If this code is not to be trusted, it will require a substantial overhaul of the whole Authorizer framework, I think.

IMHO, Authorizer implementations can be trusted. They are "activated" by a deployment-time decision. Runtime actions cannot replace an Authorizer or alter how it processes data (assuming the impl. is correct by itself).

Explicit filtering of PrincipalEntity properties does make sense, though. However, I believe it is an Authorizer implementation concern. I'd propose to keep PrincipalEntity as a PolarisPrincipal attribute in this PR, but move actual processing of its attributes on the AuthZ side to another PR... possibly to #5032 and have a thorough review of that code there. WDYT?

Sounds good. Let's discuss this separately. I don't think it is purely an Authorizer implementation concern though. I think we should have a unified filter, instead of each authorizer does it.

dimas-b · 2026-07-20T19:19:56Z

@adutra : lots of merge conflicts popped up 😅

adutra · 2026-07-20T19:43:48Z

@adutra : lots of merge conflicts popped up 😅

Yup, I had to squash and rebase to fix all the conflicts.

FYI there is a new attribute now: org.apache.polaris.core.auth.PRINCIPAL_ROLE:ALL.

PTAL!

dimas-b · 2026-07-20T21:01:22Z

-  /** Returns how principal roles should be selected during role resolution. */
-  RoleSelection getRoleSelection();
+  /** Returns the attribute value associated with the given key, if any. */
+  default <T> Optional<T> getAttribute(String key, Class<T> type) {


Late comment, but it might be worth defining a key class (or interface) with specific Class<X> value attributes for each standard key.... then getAttribute(PolarisPrincipalAttribute.PRINCIPAL_ROLE_ALL_ATTRIBUTE_KEY) could immediately be scoped down to Boolean without a per-call type parameter.

Extensibility can be achieved by PrincipalAttribute defining type() and key() methods, which could be customized downstream (if required). I guess it could be an "immutable" interface with utility methods.

WDYT?

Yes, I thought about that and in fact, we already use this pattern for event attributes.

The reason why I went for a simpler, type-unsafe idiom is because this idiom is already used in SecurityIdentity, cf. getPrincipal(Class) and getCredential(Class). So I thought it would look familiar.

A tricky problem to solve though is how to capture the type information. org.apache.polaris.service.events.AttributeKey uses Guava's TypeToken for that. We cannot do that here since this is a public component in polaris-core, so It would be bad to leak a Guava type in the API.

How about we tackle this in a follow-up work? I think this PR is already quite big.

adutra · 2026-07-21T08:34:48Z

In the spirit of moving forward with follow-up tasks, I'm going to merge this PR now. Hope that's OK.

Copilot AI review requested due to automatic review settings July 17, 2026 13:29

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

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

adutra changed the title ~~Auth refactor2~~ Refactor PolarisPrincipal to contain generic attributes Jul 17, 2026

Copilot started reviewing on behalf of adutra July 17, 2026 13:29 View session

Copilot AI reviewed Jul 17, 2026

View reviewed changes

dimas-b mentioned this pull request Jul 17, 2026

Include user-defined attributes when building PolarisPrincipal from PrincipalEntity #5032

Open

dimas-b previously approved these changes Jul 17, 2026

View reviewed changes

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

adutra dismissed dimas-b’s stale review via ceb78ab July 17, 2026 16:17

dimas-b previously approved these changes Jul 17, 2026

View reviewed changes

flyrain reviewed Jul 18, 2026

View reviewed changes

dimas-b mentioned this pull request Jul 20, 2026

Clarify principal role selection semantics #5112

Merged

adutra dismissed dimas-b’s stale review via aca2080 July 20, 2026 17:50

adutra force-pushed the auth-refactor2 branch from aca2080 to bb0df0d Compare July 20, 2026 19:42

Refactor PolarisPrincipal to contain generic attributes

4c64c1e

adutra force-pushed the auth-refactor2 branch from bb0df0d to 4c64c1e Compare July 20, 2026 19:51

changelog

daae953

dimas-b approved these changes Jul 20, 2026

View reviewed changes

adutra merged commit 43ab107 into apache:main Jul 21, 2026
25 checks passed

adutra deleted the auth-refactor2 branch July 21, 2026 08:35

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

Uh oh!

Conversation

adutra commented Jul 17, 2026

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dimas-b commented Jul 17, 2026

Uh oh!

adutra commented Jul 17, 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!

adutra Jul 17, 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!

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 Jul 20, 2026

Uh oh!

adutra commented Jul 20, 2026

Uh oh!

dimas-b Jul 20, 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!

adutra Jul 17, 2026 •

edited

Loading

dimas-b Jul 20, 2026 •

edited

Loading