Hi Prithvi, I cannot reopen [4405] either as you apparently have another PR using the same branch, i.e. [5032] :)
Since [5032] is fresh, I suggest closing it and restoring the branch to the PolarisPrincipal commits. Then the PR should be possible to re-open, I hope. [4405] https://github.com/apache/polaris/pull/4405 [5032] https://github.com/apache/polaris/pull/5032 Cheers, Dmitri. On Fri, Jul 10, 2026 at 6:54 PM Prithvi S <[email protected]> wrote: > Hi Dmitri, > > Thanks for checking this. That makes sense, I will change it to make > the auth layer forward to PolarisPrincipal properties. > Can you help reopen the PR? I don't seem to have access to reopen. > > Thanks, > Prithvi S > > On Fri, Jul 10, 2026 at 6:25 PM Dmitri Bourlatchkov <[email protected]> > wrote: > > > Hi Prithvi, Alex, > > > > Re: SecurityIdentity, it is a Quarkus class and it is certainly fine to > use > > it in infrastructure code that deals with technical authentication > aspects > > (e.g. in SecurityIdentityAugmentor). > > > > However, I'd like to avoid depending on SecurityIdentity in proper > Polaris > > code like the OPA Authorizer. > > > > I tend to think that the authentication layer should forward whatever > user > > information is necessary to PolarisPrincipal as optional attributes. OPA > > and other authorizers will then have the opportunity to consider those > > attributes. > > > > This will decouple Polaris authorizers both from Quarkus-specific code > and > > from PrincipalEntity. So the authorizers should be usable with any IdP > > (internal or external) and not be affected by Quarkus upgrades. > > > > WDYT? > > > > Thanks, > > Dmitri. > > > > > > On Tue, Jul 7, 2026 at 5:15 AM Prithvi S <[email protected]> > > wrote: > > > > > Hi Alex, > > > > > > Thank you, I understand now that the decoupling in PR #2307 was > > intentional > > > and that `PolarisPrincipal` should not assume a backing > `PrincipalEntity` > > > exists. > > > > > > I'll **close PR #4405** and work on the `SecurityIdentity` attribute > > > approach instead. > > > > > > My understanding of the approach: > > > 1. In `AuthenticatingAugmentor`, when a `PrincipalEntity` is available, > > add > > > it as an optional attribute to `QuarkusSecurityIdentity` (e.g., under a > > key > > > like `"org.apache.polaris.principal_entity"`) > > > 2. Update `OpaPolarisAuthorizer` and `RangerUtils.getUserAttributes` to > > > check for this attribute in the `SecurityIdentity` and extract > > user-defined > > > properties from the `PrincipalEntity` when present > > > 3. `PolarisPrincipal` remains unchanged, no property merging > > > > > > Also can you help with below: > > > 1. Is the attribute key naming convention above acceptable, or is there > > an > > > existing pattern I should follow for `SecurityIdentity` attribute keys > in > > > Polaris? > > > 2. For `DefaultAuthenticator` when it constructs `PolarisPrincipal` > from > > > `PrincipalEntity`, should it also add the `PrincipalEntity` to > > > `SecurityIdentity` attributes at that point, or should this only happen > > in > > > `AuthenticatingAugmentor`? > > > 3. For the OPA test suite that currently builds principals with > > > user-defined properties should I update those tests to instead set the > > > `PrincipalEntity` attribute on `SecurityIdentity` and verify that > > > `OpaPolarisAuthorizer` picks up the properties from there? > > > 4. Should I add a helper method somewhere (e.g., > > > `PolarisPrincipal.getPrincipalEntityFromIdentity(SecurityIdentity)`) to > > > encapsulate the attribute lookup, or is it better to keep the lookup > > inline > > > in each consumer? > > > > > > Let me know if I'm on the right track and I'll proceed with the new PR. > > > > > > Thanks, > > > Prithvi > > > > > > On Mon, Jun 15, 2026 at 8:52 PM Alexandre Dutra <[email protected]> > > wrote: > > > > > > > Hi Prithvi, > > > > > > > > Thanks for starting this thread! > > > > > > > > My stance on this matter remains unchanged: > > > > > > > > - PolarisPrincipal and PrincipalEntity should be decoupled. We must > > > > avoid the assumption that a PrincipalEntity exists for every > > > > principal, especially in cases involving federated or detached > > > > principals. > > > > > > > > - In my view, the most effective method for propagating the > > > > PrincipalEntity is to treat it as an *optional* attribute within the > > > > SecurityIdentity. > > > > > > > > > how should we ensure user-defined properties are reliably forwarded > > > > across all authenticator implementations, not just > > > `DefaultAuthenticator`? > > > > > > > > Such a guarantee is likely unattainable. Because authenticators are > > > > designed to be pluggable, custom implementations may be completely > > > > unaware of PrincipalEntity. Consequently, any logic that requires the > > > > presence of a PrincipalEntity within the SecurityIdentity is > > > > fundamentally flawed and should be refactored to remove that > > > > requirement. > > > > > > > > What is your use case for retrieving the PrincipalEntity's attributes > > > > at runtime? Is it for auditing purposes or are you building some > > > > business logic on top of it? If the former, it should be fine to just > > > > log "null" if the entity is not present (and you can control the > > > > authenticator in use to make sure it will be present); if the latter, > > > > that would mean that your logic is now dependent on a specific > > > > authenticator's ability to produce a PrincipalEntity, which isn't > > > > great. In that case it may be safer to fetch the PrincipalEntity from > > > > the metastore explicitly. > > > > > > > > Thanks, > > > > Alex > > > > > > > > On Sun, Jun 14, 2026 at 8:56 PM Prithvi S < > [email protected] > > > > > > > wrote: > > > > > > > > > > Hi all, > > > > > > > > > > I'm opening this thread as suggested by @dimas-b in the review of > PR > > > > #4405 ( > > > > > https://github.com/apache/polaris/pull/4405), which touches > > > > authentication > > > > > and authorization behavior. @flyrain also flagged a prior dev-list > > > thread > > > > > from April 2026 on the same topic, so I want to make sure that > > > discussion > > > > > is continued here. > > > > > > > > > > Background > > > > > `PolarisPrincipal.of(PrincipalEntity, …)` currently forwards only > the > > > > > entity's *internal* properties (e.g. `client_id`) and silently > drops > > > any > > > > > *user-defined* properties set at principal creation time (e.g. > > > > > `region=northamerica`, `department=finance`). > > > > > > > > > > As a result, downstream consumers of > > `PolarisPrincipal.getProperties()` > > > > > never see user attributes, and ABAC policies written against them > > never > > > > > match. This affects: > > > > > > > > > > - `DefaultAuthenticator` : constructs `PolarisPrincipal` during > > > > > authentication > > > > > - `AuthenticatingAugmentor` : copies principal properties into > > > > > `QuarkusSecurityIdentity` attributes > > > > > - External authorizers : `OpaPolarisAuthorizer` and > > > > > `RangerUtils.getUserAttributes` consume these as user attributes > for > > > > policy > > > > > evaluation > > > > > > > > > > The existing OPA test suite already builds principals with > > user-defined > > > > > properties (e.g. `department=finance`), demonstrating the intended > > > > > contract, which the production code path cannot currently honor. > > > > > > > > > > What PR #4405 does > > > > > The fix adds a `mergeEntityProperties()` helper that combines the > > > > > `PrincipalEntity`'s user-defined and internal properties, and uses > > the > > > > > merged map when constructing `PolarisPrincipal` via the > > > > > `of(PrincipalEntity, …)` overload. > > > > > > > > > > The design question raised: > > > > > @flyrain pointed out that in April 2026, @adutra raised a concern > > that > > > > > `PolarisPrincipal` was *intentionally* decoupled from > > `PrincipalEntity` > > > > > (see PR #2307), and suggested an alternative approach: expose the > > > > persisted > > > > > `PrincipalEntity` as a `SecurityIdentity` attribute rather than > > > widening > > > > > `getProperties()`. > > > > > > > > > > PR #4405 takes the opposite direction by merging properties > directly > > > into > > > > > `PolarisPrincipal`. I want to understand whether this is acceptable > > or > > > > > whether the community prefers the `SecurityIdentity` attribute > > approach > > > > > instead. > > > > > > > > > > questions I have, > > > > > 1. Was the decoupling of `PolarisPrincipal` from `PrincipalEntity` > > (PR > > > > > #2307) intended to prevent exactly this kind of property merge? If > > so, > > > > what > > > > > is the preferred mechanism for exposing user-defined principal > > > attributes > > > > > to downstream auth consumers? > > > > > 2. Is the `SecurityIdentity` attribute approach (exposing > > > > `PrincipalEntity` > > > > > directly) the right path, or are there other concerns with that? > > > > > 3. Since authenticators are pluggable, how should we ensure > > > user-defined > > > > > properties are reliably forwarded across all authenticator > > > > implementations, > > > > > not just `DefaultAuthenticator`? > > > > > > > > > > Happy to revise the approach based on community feedback. > > > > > > > > > > Regards, > > > > > Prithvi S > > > > > > > > > >
