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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
