Include user-defined attributes when building PolarisPrincipal from PrincipalEntity#5032
Open
iprithv wants to merge 1 commit into
Open
Include user-defined attributes when building PolarisPrincipal from PrincipalEntity#5032iprithv wants to merge 1 commit into
iprithv wants to merge 1 commit into
Conversation
github-project-automation
Bot
moved this from PRs In Progress
to Done
in Basic Kanban Board
iprithv
changed the title
github-project-automation
Bot
moved this from Done
to PRs In Progress
in Basic Kanban Board
iprithv
force-pushed
the
fix/polaris-principal-user-defined-properties
branch
from
July 10, 2026 21:51
ee0c065 to
d729092
Compare
github-project-automation
Bot
moved this from PRs In Progress
to Done
in Basic Kanban Board
github-project-automation
Bot
moved this from Done
to PRs In Progress
in Basic Kanban Board
iprithv
marked this pull request as draft
July 10, 2026 23:12
github-project-automation
Bot
moved this from PRs In Progress
to Done
in Basic Kanban Board
github-project-automation
Bot
moved this from Done
to PRs In Progress
in Basic Kanban Board
iprithv
force-pushed
the
fix/polaris-principal-user-defined-properties
branch
from
July 16, 2026 21:42
d729092 to
ff0aba7
Compare
iprithv
changed the title
iprithv
marked this pull request as ready for review
July 16, 2026 22:33
Contributor
|
@iprithv : Please rebase to get compilation fixes from |
dimas-b
previously approved these changes
Jul 16, 2026
|
|
||
| ### Fixes | ||
| - Fixed native catalog credential vending paths (`loadCredentials` and `loadTable` with delegation) to re-validate locations against the *current* catalog `allowedLocations`. Previously these paths trusted persisted table entity locations, allowing stale credentials after an admin tightened allowed locations on a native catalog. (The federated path had a partial check.) | ||
| - Fixed `PolarisPrincipal` construction from a `PrincipalEntity` to expose the principal's user-defined properties (alongside internal properties) so external authorizers such as OPA and Ranger can use them for policy evaluation. Internal properties win on key collision so that system-managed values such as `client_id` cannot be shadowed by user input. |
Contributor
There was a problem hiding this comment.
nit: I'd put this under "changes". Old behaviour was not a "bug" 😉
github-project-automation
Bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
Contributor
iprithv
force-pushed
the
fix/polaris-principal-user-defined-properties
branch
from
July 16, 2026 23:08
ff0aba7 to
a245fbb
Compare
dimas-b
previously approved these changes
Jul 16, 2026
Contributor
|
#5085 proposes a more fundamental refactoring, which should support this use case too. |
6 tasks
Contributor
dimas-b
dismissed
their stale review
July 17, 2026 14:50
removing approval based on feedback from @adutra
flyrain
reviewed
Jul 17, 2026
| * collision the internal value wins, so that system-managed properties (for example {@code | ||
| * client_id}) cannot be shadowed by user-supplied properties. | ||
| */ | ||
| private static Map<String, String> mergeEntityProperties(PrincipalEntity principalEntity) { |
Contributor
There was a problem hiding this comment.
This is the PrincipalEntity -> PolarisPrincipal property merge that the dev list moved away from (PR #4405 thread, Jul 7-11). The agreed direction is for the auth layer to forward user info to PolarisPrincipal as optional attributes, keeping PolarisPrincipal decoupled from PrincipalEntity (federated/detached principals may have no entity). It also only fires on the DefaultAuthenticator path -- a pluggable authenticator with no PrincipalEntity won't forward these. Is this still the intended mechanism, or superseded by the rework you described on the list?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 readPolarisPrincipal.getProperties()never see user attributes likeregion=northamericaordepartment=finance, and policies written against them never match.