Skip to content

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

Open
iprithv wants to merge 1 commit into
apache:mainfrom
iprithv:fix/polaris-principal-user-defined-properties
Open

Include user-defined attributes when building PolarisPrincipal from PrincipalEntity#4405
iprithv wants to merge 1 commit into
apache:mainfrom
iprithv:fix/polaris-principal-user-defined-properties

Conversation

@iprithv
Copy link
Copy Markdown

@iprithv iprithv commented May 11, 2026

Description

Fixes #4291

PolarisPrincipal.of(PrincipalEntity, …) forwards only the entity's internal properties (e.g. client_id) and silently drops the user-defined properties supplied at principal creation. As a result, downstream consumers that read PolarisPrincipal.getProperties() never see user attributes like region=northamerica or department=finance, and policies written against them never match.

Affected paths:

  • DefaultAuthenticator - constructs the PolarisPrincipal during authentication.
  • AuthenticatingAugmentor - copies the principal's properties into QuarkusSecurityIdentity attributes.
  • External authorizers - OPA (OpaPolarisAuthorizer) and Ranger (RangerUtils.getUserAttributes) consume these as user attributes for ABAC policy evaluation. The existing OPA test suite already builds principals like PolarisPrincipal.of("eve", Map.of("department","finance"), Set.of("auditor")), demonstrating the intended contract that the production path can't honor today.

Reproduction confirmed both ways:

On main (fix reverted):

PolarisPrincipalTest > ofPrincipalEntityExposesUserDefinedProperties()                          FAILED
PolarisPrincipalTest > ofPrincipalEntityWithTokenExposesUserDefinedProperties()                 FAILED
DefaultAuthenticatorTest > testUserDefinedPropertiesArePreservedOnAuthenticatedPrincipal()      FAILED

On this branch:

./gradlew :polaris-core:check                                                BUILD SUCCESSFUL
./gradlew :polaris-runtime-service:test --tests "...service.auth.*"          BUILD SUCCESSFUL
./gradlew :polaris-runtime-service:spotlessCheck :checkstyleMain :checkstyleTest   BUILD SUCCESSFUL

Checklist

  • 🛡️ Don't disclose security issues! (contact security@apache.org)
  • 🔗 Clearly explained why the changes are needed, or linked related issues: Fixes PolarisPrincipal missing user-defined attributes supplied during principal creation #4291
  • 🧪 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) — N/A, internal behavior fix; no user-facing config or doc surface affected.

@github-project-automation github-project-automation Bot moved this to PRs In Progress in Basic Kanban Board May 11, 2026
@iprithv iprithv changed the title Include user-defined attributes when building PolarisPrincipal from P… Include user-defined attributes when building PolarisPrincipal from PrincipalEntity May 11, 2026
@dimas-b
Copy link
Copy Markdown
Contributor

dimas-b commented May 12, 2026

Thanks for your contribution, @iprithv !

Please start a discussion for this change on the dev ML. Since it affect authentication and authorization, a wide community review and consensus is advisable.

Copy link
Copy Markdown
Contributor

@dimas-b dimas-b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM 👍 pending a discussion on dev.

* <p>The created principal will have the same ID and name as the {@link PrincipalEntity}, and its
* properties will be derived from the internal properties of the entity.
* <p>The created principal will have the same name as the {@link PrincipalEntity}, and its
* properties will be the merger of the entity's user-defined properties and its internal
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the merger is the actor who merges or the process of merging, but it's not the result, right? 🤔

PrincipalEntity principalEntity, Set<String> roles, Optional<String> token) {
return of(
principalEntity.getName(), principalEntity.getInternalPropertiesAsMap(), roles, token);
return of(principalEntity.getName(), mergeEntityProperties(principalEntity), roles, token);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks fine in isolation. However, from #4291 I guess that the real intent is to ensure the Authenticator implementations forward user-settable principal properties to PolarisPrincipal

Note: Authenticators are pluggable, so whether that happens in all environments is not 100% certain.

This is not a blocker, just a notice for awareness.

Copy link
Copy Markdown
Contributor

@flyrain flyrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PrincipalEntity principalEntity, Set<String> roles, Optional<String> token) {
return of(
principalEntity.getName(), principalEntity.getInternalPropertiesAsMap(), roles, token);
return of(principalEntity.getName(), mergeEntityProperties(principalEntity), roles, token);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

heads up: 2026-04-26 dev-list thread on PolarisPrincipal's user attributes, @adutra raised that PolarisPrincipal was intentionally decoupled from PrincipalEntity (PR #2307) and suggested exposing the persisted entity via a SecurityIdentity attribute rather than widening getProperties(). This PR goes the opposite direction.

@flyrain flyrain requested a review from adutra May 13, 2026 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PolarisPrincipal missing user-defined attributes supplied during principal creation

3 participants