Hi Alex, +1 to this design. I think it allows future extensions to be introduced without disrupting existing code.
I also approved it in GH. Cheers, Dmitri. On Fri, Jul 17, 2026 at 9:35 AM Alexandre Dutra <[email protected]> wrote: > Hi again, > > In order to validate my own suggestion I went ahead and created the > following PR: > > https://github.com/apache/polaris/pull/5085 > > Compared to what I had in mind initially, the main difference is that > PolarisPrincipal must stay the "lingua franca" for everything related > to authn and authz, since it's the only component declared in > polaris-core. So in the final design, PolarisPrincipal simply exposes > a generic attribute map, which may contain the PrincipalEntity as an > attribute. The SecurityIdentity exposes the same attributes for > completeness. > > Let me know your thoughts on this design. > > Thanks, > Alex > > On Fri, Jul 17, 2026 at 11:13 AM Alexandre Dutra <[email protected]> > wrote: > > > > Hi Prithvi, Dmitri, > > > > I was OOO last week and I'm catching up with this discussion, so my > > apologies for the delay. > > > > @ Prithvi : your description of the approach is spot on and > > corresponds to what I had in mind. To your questions: > > > > > 1. Is the attribute key naming convention above acceptable? > > > > Yes. Quarkus tends to use simple attribute keys like "tenant-id", but > > I think Polaris should use namespaced attributes. > > > > > 2. For `DefaultAuthenticator` [...] should it also add the > `PrincipalEntity` to `SecurityIdentity` attributes at that point [...] ? > > > > Yes it should, and that's indeed the annoying part. The current > > authenticate method signature is: > > > > PolarisPrincipal authenticate(PolarisCredential credentials) throws > > NotAuthorizedException, ServiceFailureException; > > > > This signature is too rigid and does not easily allow attaching a > > PrincipalEntity. We'd need to change it to something like: > > > > SecurityIdentity authenticate(SecurityIdentity credentials) throws > > AuthenticationFailedException; > > > > This would allow the authenticator impl to: > > > > - read all the credentials presented by the client (and not only > > PolarisCredential) > > - attach the PrincipalEntity to the SecurityIdentity, if it exists > > - allow easy authentication of federated principals, as a future > possibility > > > > > 3. For the OPA test suite [...] should I update those tests to instead > set the `PrincipalEntity` attribute on `SecurityIdentity` [...] ? > > > > Yes, that would be the right thing to do. > > > > > 4. Should I add a helper method somewhere [...] to encapsulate the > attribute lookup [...] ? > > > > I like that idea, +1. > > > > My problem with [5032] is that I'm not sure it's going in the right > > direction, given the above. There are two methods today in > > PolarisPrincipal that I think should be removed: > > > > Map<String, String> getProperties(); > > Optional<String> getToken(); > > > > As they could easily be replaced by injecting SecurityIdentity (also, > > getToken is currently unused). > > > > What do you think of this plan? > > > > Thanks, > > Alex > > > > [5032]: https://github.com/apache/polaris/pull/5032 > > > > On Fri, Jul 17, 2026 at 12:44 AM Prithvi S <[email protected]> > wrote: > > > > > > Hi JB, Dmitri, > > > > > > Got it, sure. Thanks for trying to reopen #4405. I updated it in > > > https://github.com/apache/polaris/pull/5032. > > > > > > Regards, > > > Prithvi > > > > > > On Thu, Jul 16, 2026 at 4:42 PM Jean-Baptiste Onofré <[email protected]> > > > wrote: > > > > > > > Yup, I suggest to re-open with a fresh PR. > > > > > > > > Regards > > > > JB > > > > > > > > On Thu, Jul 16, 2026 at 5:10 AM Dmitri Bourlatchkov < > [email protected]> > > > > wrote: > > > > > > > > > > Hi Prithvi, > > > > > > > > > > Sorry, but GH refuses to reopen PR 4405 because its branch has been > > > > > force-pushed or something like that. > > > > > > > > > > In the interest of making progress, let's not worry about it. > Please open > > > > > fresh PRs for all changes that got stuck in erroneously closed PRs > and > > > > > cross-reference them. > > > > > > > > > > Cheers, > > > > > Dmitri. > > > > > > > > > > On Fri, Jul 10, 2026 at 7:25 PM Prithvi S < > [email protected]> > > > > > wrote: > > > > > > > > > > > Hi Dmitri, > > > > > > > > > > > > Ah, okay, got it. I closed > https://github.com/apache/polaris/pull/5032 > > > > . > > > > > > Can > > > > > > you try to reopen https://github.com/apache/polaris/pull/4405 > now? > > > > Thanks! > > > > > > Also this access privilege is confusing. This must be a project > > > > setting? > > > > > > Can we have access to reopen it if it closes due to stale > activity? > > > > > > > > > > > > Regards, > > > > > > Prithvi S > > > > > > > > > > > > On Sat, Jul 11, 2026 at 4:30 AM Dmitri Bourlatchkov < > [email protected]> > > > > > > wrote: > > > > > > > > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
