Switch principal attributes to AttributeMap - #5140
Conversation
adutra
changed the title
| * | ||
| * <p>Note: callers must never assume that this attribute is present. | ||
| */ | ||
| String PRINCIPAL_ENTITY_ATTRIBUTE_KEY = "org.apache.polaris.core.auth.PRINCIPAL_ENTITY"; |
There was a problem hiding this comment.
Moved to a separate PolarisPrincipalAttributes class.
|
This PR is ready for review. |
There was a problem hiding this comment.
Pull request overview
Updates the PolarisPrincipal attribute model from a raw Map<String, Object> to a type-safe AttributeMap, introducing shared typed keys and propagating the change through core auth/persistence logic, runtime service authentication, and extension authorizer tests.
Changes:
- Switch
PolarisPrincipalto carry anAttributeMapand introducePolarisPrincipalAttributestyped keys for principal entity / role-all / JWT attributes. - Update runtime/service authentication and core resolver/auth preconditions to use typed attribute access.
- Migrate affected tests and extension integrations (OPA, Ranger, Kafka events) to the new attribute model and add coverage for defensive copying.
Reviewed changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| runtime/service/src/testFixtures/java/org/apache/polaris/service/TestServices.java | Update test fixture principal construction to use ImmutableAttributeMap. |
| runtime/service/src/test/java/org/apache/polaris/service/events/listeners/PolarisPersistenceEventListenerTest.java | Use AttributeMap.EMPTY when building principals in event listener tests. |
| runtime/service/src/test/java/org/apache/polaris/service/events/listeners/opentelemetry/OpenTelemetryEventListenerTest.java | Use AttributeMap.EMPTY for metadata user principals. |
| runtime/service/src/test/java/org/apache/polaris/service/events/jsonEventListener/aws/cloudwatch/AwsCloudWatchEventListenerTest.java | Replace empty Map principal attributes with AttributeMap.EMPTY. |
| runtime/service/src/test/java/org/apache/polaris/service/catalog/RootPrincipalAugmentor.java | Build root principal attributes using PolarisPrincipalAttributes + ImmutableAttributeMap. |
| runtime/service/src/test/java/org/apache/polaris/service/catalog/policy/PolicyCatalogHandlerAuthzTest.java | Migrate principal attributes to typed AttributeMap in policy authz tests. |
| runtime/service/src/test/java/org/apache/polaris/service/catalog/policy/AbstractPolicyCatalogTest.java | Migrate authenticated root principal attributes to ImmutableAttributeMap. |
| runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalogTest.java | Use AttributeMap.EMPTY for test principals. |
| runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerTest.java | Use AttributeMap.EMPTY for handler principal. |
| runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerFineGrainedDisabledTest.java | Use PolarisPrincipalAttributes keys in authz-disabled handler tests. |
| runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogViewTest.java | Migrate authenticated root principal to typed attributes. |
| runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogHandlerAuthzTest.java | Update multiple handler principal constructions to typed attribute maps. |
| runtime/service/src/test/java/org/apache/polaris/service/catalog/generic/PolarisGenericTableCatalogHandlerAuthzTest.java | Update generic handler authz tests to typed attribute maps. |
| runtime/service/src/test/java/org/apache/polaris/service/catalog/generic/AbstractPolarisGenericTableCatalogTest.java | Update generic catalog test base to typed principal attributes. |
| runtime/service/src/test/java/org/apache/polaris/service/auth/DefaultAuthenticatorTest.java | Update assertions and principal creation for AttributeMap + typed keys. |
| runtime/service/src/test/java/org/apache/polaris/service/auth/AuthenticatingAugmentorTest.java | Adjust augmentor test to reflect principal attributes being an AttributeMap. |
| runtime/service/src/test/java/org/apache/polaris/service/admin/PolarisAuthzTestBase.java | Update authenticated root principal to typed attributes. |
| runtime/service/src/test/java/org/apache/polaris/service/admin/PolarisAdminServiceAuthzTest.java | Update admin service authz tests to typed principal attributes. |
| runtime/service/src/test/java/org/apache/polaris/service/admin/ManagementServiceTest.java | Update management service setup principal to typed attribute map. |
| runtime/service/src/main/java/org/apache/polaris/service/auth/DefaultAuthenticator.java | Switch resolved principal attributes from Map to AttributeMap and use typed keys. |
| runtime/service/src/main/java/org/apache/polaris/service/auth/AuthenticatingAugmentor.java | Convert principal AttributeMap back into Quarkus SecurityIdentity attributes. |
| polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/BaseResolverTest.java | Update resolver test fixture principal creation to typed attributes. |
| polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegrationTest.java | Use AttributeMap.EMPTY for test principal. |
| polaris-core/src/test/java/org/apache/polaris/core/persistence/ResolverTest.java | Replace empty Map principal attributes with AttributeMap.EMPTY. |
| polaris-core/src/test/java/org/apache/polaris/core/persistence/PolarisResolutionManifestLookupKeyTest.java | Replace empty Map principal attributes with AttributeMap.EMPTY. |
| polaris-core/src/test/java/org/apache/polaris/core/auth/PolarisPrincipalTest.java | Add test coverage for defensive copy/immutability of principal attributes. |
| polaris-core/src/test/java/org/apache/polaris/core/auth/PolarisAuthorizerImplTest.java | Replace empty Map principal attributes with AttributeMap.EMPTY. |
| polaris-core/src/test/java/org/apache/polaris/core/auth/AuthorizationRequestTest.java | Replace empty Map principal attributes with AttributeMap.EMPTY. |
| polaris-core/src/test/java/org/apache/polaris/core/auth/AuthorizationPreConditionsTest.java | Update principal construction and empty-attrs usage for new attribute model. |
| polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java | Update role-all lookup to use typed AttributeMap accessors. |
| polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisPrincipalAttributes.java | Introduce typed attribute keys for principal-related attributes. |
| polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisPrincipal.java | Change principal attributes to AttributeMap, remove string-key constants and typed getAttribute helper. |
| polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationPreConditions.java | Update credential-rotation precondition checks to use typed principal attributes. |
| extensions/events/kafka/src/test/java/org/apache/polaris/extensions/events/kafka/KafkaEventListenerTest.java | Replace empty Map principal attributes with AttributeMap.EMPTY. |
| extensions/auth/ranger/src/test/java/org/apache/polaris/extension/auth/ranger/RangerPolarisAuthorizerTest.java | Update Ranger principal deserialization test to use AttributeMap.EMPTY. |
| extensions/auth/ranger/src/main/java/org/apache/polaris/extension/auth/ranger/utils/RangerUtils.java | Update principal-entity lookup to use typed attribute keys. |
| extensions/auth/opa/src/test/java/org/apache/polaris/extension/auth/opa/OpaPolarisAuthorizerTest.java | Migrate OPA tests to typed attribute maps (including custom user attributes). |
| extensions/auth/opa/src/test/java/org/apache/polaris/extension/auth/opa/OpaPolarisAuthorizerFactoryTest.java | Migrate OPA factory tests to AttributeMap-backed principals. |
github-project-automation
Bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
flyingImer
left a comment
flyingImer
left a comment
There was a problem hiding this comment.
I guess I'm a bit late to this topic. I wonder is the intent supporting ABAC authz? do we have any pointer to the design doc that can better navigate me throughout prs?
The intent is not to support ABAC, but just to introduce a type-safe mechanism to retrieve principal attributes. Principal attributes predate this PR. Granted, one could leverage principal attributes to implement some form of ABAC, but this PR does not make any visible step in that direction. |
Thanks Alex for the quick turnaround. In that case, could you please point me or share on why we need this type-safe mechanism change? I noticed #5139, but not quite caught up on why |
Simply to avoid unsafe type casts when retrieving attributes. |
flyingImer
left a comment
flyingImer
left a comment
There was a problem hiding this comment.
IIUC, IMHO this PR, and the AttributeMap shape from #5139 it's built on, solves the wrong problem for principal attributes.
There are two different kinds of data on the principal today. The first kind: 3 values Polaris itself produces, where Polaris always knows the type ahead of time. That's the resolved PrincipalEntity, the PRINCIPAL_ROLE_ALL boolean, and an optional JWT string. The second kind: an open set of raw attributes copied from the identity provider, where Polaris does not control the keys or the types. AttributeMap treats both kinds the same way, and it doesn't serve either one well.Take the first kind, the 3 Polaris-owned values. PolarisPrincipal is @PolarisImmutable, with exactly one generated implementation, ImmutablePolarisPrincipal. Adding a named method for each of these 3 values costs one interface change plus a rebuild. Nothing else implements this interface, so nothing else needs to change. A named method gives a real compile-time guarantee: get the name wrong, and the build fails. AttributeKey doesn't give that guarantee. It's record AttributeKey(String key), so T plays no part in equals/hashCode. The javadoc says so directly: the type guarantee only holds "as long as callers never create two instances of AttributeKey having the same string key, but different types." That's a rule callers have to remember, not something the compiler checks. Dressing it up as a typed key makes it look safer than a plain Map<String, Object>, when it isn't.
Take the second kind, the raw attributes from the identity provider. L170 in DefaultAuthenticator does new AttributeKey<>(key), and value's static type there is Object, straight from identity.getAttributes(): Map<String, Object>. So every raw attribute gets typed as AttributeKey. That's exactly as safe as a plain string key, since there's no narrower type underneath it to protect.
I also don't think the raw attribute passthrough does anything today. Based on my search, nothing reads an attribute other than the 3 reserved ones off PolarisPrincipal.getAttributes(). So the raw attribute passthrough on PolarisPrincipal looks speculative right now, on top of not doing what it's meant to do.
What I'd suggest instead:
- Give PolarisPrincipal a named accessor for each of the 3 reserved values, instead of AttributeKey constants. @PolarisImmutable makes this cheap here.
- If a future caller genuinely needs the raw identity-provider attributes off PolarisPrincipal, carry them as a plain Map<String, Object> field, not AttributeKey entries. The typed key buys nothing when the value type is always Object.
- If neither set needs a shared string-keyed map, AttributeMap isn't needed for principal attributes at all. That also removes the "who owns this key prefix" question I'd otherwise raise about PRINCIPAL_ROLE_ALL_ATTRIBUTE_KEY, since there'd be no shared namespace left to collide in.
Let me know if missing anything here
| ImmutableAttributeMap.Builder principalAttributes = ImmutableAttributeMap.builder(); | ||
| identity | ||
| .getAttributes() | ||
| .forEach((key, value) -> principalAttributes.put(new AttributeKey<>(key), value)); |
There was a problem hiding this comment.
IIUC, new AttributeKey<>(key) infers AttributeKey here, since value's static type from identity.getAttributes() is Object. This entry never gets a narrower type than a raw string key would have had. See the general comment for why I think the raw attributes passthrough through AttributeMap isn't earning its keep right now.
| /** | ||
| * Attribute key for the principal entity attribute, of type {@link PrincipalEntity}. | ||
| * | ||
| * <p>Note: callers must never assume that this attribute is present. | ||
| */ | ||
| public static final AttributeKey<PrincipalEntity> PRINCIPAL_ENTITY_ATTRIBUTE_KEY = | ||
| new AttributeKey<>("org.apache.polaris.core.auth.PRINCIPAL_ENTITY"); | ||
|
|
||
| /** | ||
| * Attribute key, of type {@link Boolean}, used to determine how authorizers should resolve | ||
| * principal roles. | ||
| * | ||
| * <p>When absent or false, authorizers should resolve only the roles returned by {@link | ||
| * PolarisPrincipal#getRoles()}; if an empty set is returned, then no principal roles should be | ||
| * resolved. | ||
| * | ||
| * <p>When present and true, authorizers should resolve all roles granted to the principal at the | ||
| * time of check, ignoring roles returned by {@link PolarisPrincipal#getRoles()} (even if an empty | ||
| * set is returned). | ||
| * | ||
| * <p>This attribute is generally present and true when the principal presented a credential | ||
| * including the pseudo-role {@code PRINCIPAL_ROLE:ALL}. | ||
| * | ||
| * <p>Note: Callers must not assume that this attribute is always present. | ||
| */ | ||
| public static final AttributeKey<Boolean> PRINCIPAL_ROLE_ALL_ATTRIBUTE_KEY = | ||
| new AttributeKey<>("org.apache.polaris.core.auth.PRINCIPAL_ROLE:ALL"); | ||
|
|
||
| /** | ||
| * Attribute key used to store or retrieve a JSON Web Token (JWT) associated with the {@link | ||
| * PolarisPrincipal}, of type {@link String}. | ||
| * | ||
| * <p>The associated value is expected to represent the JWT provided during authentication and may | ||
| * be used for further validation or for deriving additional claims. | ||
| * | ||
| * <p>Note: callers must never assume that this attribute is present. | ||
| */ | ||
| public static final AttributeKey<String> JWT_ATTRIBUTE_KEY = | ||
| new AttributeKey<>("org.apache.polaris.core.auth.JWT"); |
There was a problem hiding this comment.
Since PolarisPrincipal only has one implementation (@PolarisImmutable), these 3 constants could be named accessors on the interface instead of AttributeKey entries. That gives a real compile-time type guarantee instead of the string-key convention callers of this class currently have to hold to by hand IMO.
|
I agree with the type-safety concern: I do not agree that all three reserved values should therefore become named I think the contract should instead separate stable, deliberately exposed authentication facts from an open attribute set. For the latter, provenance/namespace and an explicit forwarding allowlist matter more than nominal generic typing: an authorizer must be able to distinguish trusted Polaris-derived facts from IdP claims or other supplied attributes. So I would keep |
No. Polaris does not "always know" these attributes. In fact, they are very specific to the internal authentication mechanism. If/when external principals are enacted (see #5119) both the resolved PrincipalEntity and the PRINCIPAL_ROLE_ALL boolean attribute become an implementation detail of the internal auth path. Similarly, I know of Polaris deployments today that do not use OAuth2; for those deployments, the JWT attribute simply does not make any sense. I am strongly opposed to promoting these attributes to the
Again, not entirely true. The So yes, we are copying attributes from If that's the main point of concern, I'm happy to remove the code that copies attributes from |
That's by design: if keys compared by string value and type, that would be very awkward. So the contract is that consumers of the map know in advance the keys and their types. I think that's a net improvement over untyped maps. The attribute map pattern is widely used by Quarkus itself, Netty, AWS SDK, Spark, etc. etc. I really don't understand why it's so unthinkable to have the same pattern in Polaris.
Fair enough: let's introduce a |
ccb066e to
e6bcff6
Compare
|
FYI I rebased to fix the merge conflicts, PTAL again 🙏 |
|
FYI, attributes are not copied anymore between |
|
With the raw passthrough removed, I don't think the broader principal-contract redesign needs to block this PR. The remaining blocker for me is the 1.7.0 breaking-change note: it still documents the generic Map attributes bag, constants on PolarisPrincipal, and getAttribute(key, type). Could we update it to match the AttributeMap/PolarisPrincipalAttributes API? @adutra Thanks for removing the raw SecurityIdentity passthrough; that resolves that part of my review. I agree the three values are not universal, but presence is separate from API shape. Optional/default semantics work for external principals and non-OAuth deployments too. Role selection has stable in-tree semantics, while JWT has the token-propagation use case from #3236, so I would model them independently rather than treat all three as one class of attributes.
@snazy I agree with that boundary. My original all-three-accessors suggestion was too broad, but the current public AttributeKey still exposes the full persistence entity through PolarisPrincipal; changing the lookup shape does not keep it behind the implementation. A projected authorization-facts type seems like the cleaner contract. |
We cannot change the changelog of a released version, but you are right that we should inform users of the change. I will add a new note under "breaking changes" for the next release.
Can we tackle that as a follow-up? Especially given that a design doc has been proposed to rationalize principal attributes, I think not only the attributes may change where they are declared, but the keys themselves may change. |
0e311b0 to
dcdfeb9
Compare
|
@snazy @flyingImer can I have a second review here please? 🙏 |
Yes, I’m okay handling the role-selection/JWT shape in a follow-up. One caveat: the linked provenance design explicitly makes redesigning PolarisPrincipal and removing PRINCIPAL_ENTITY_ATTRIBUTE_KEY non-goals, so it does not actually own that follow-up; please track it separately. The raw SecurityIdentity passthrough is gone and the changelog now covers this migration, so those earlier points no longer block this PR. |
flyingImer
left a comment
flyingImer
left a comment
There was a problem hiding this comment.
Request changes for one remaining blocker: PolarisPrincipal must snapshot attributes across every construction path, including builder().from(...), with a regression test that mutates the source AttributeMap after construction. Once that holds, I’m okay merging this PR; the role-selection/JWT contract can move to a separately tracked follow-up.
| return ImmutablePolarisPrincipal.builder() | ||
| .name(name) | ||
| .attributes(attributes) | ||
| .attributes(AttributeMap.copyOf(attributes)) |
There was a problem hiding this comment.
The defensive copy in PolarisPrincipal.of(...) only covers that factory path.
IIUC, immutables treats AttributeMap as a scalar, so ImmutablePolarisPrincipal.builder().from(polarisPrincipal).build(), which TaskExecutorImpl already uses, retains a MutableAttributeMap returned by a custom Authenticator: mutating that source later also changes the clone’s authorization attributes.
Could we make every ImmutablePolarisPrincipal construction snapshot its attributes and cover builder/from with a source-map mutation test? One option is to normalize through AttributeMap.copyOf in a @Value.Check.
There was a problem hiding this comment.
Good point. There is actually a much simpler solution: make getAttributes() return ImmutableAttributeMap.
cee9d59 to
aab04c3
Compare
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)