Hi all, Thanks for the quick turnaround! The PR is merged. Nice velocity :-)
Thanks, Alex On Fri, Sep 25, 2026 at 9:03 PM Yufei Gu <[email protected]> wrote: > > Hi Alex, JB, Prithvi, > > Thanks for the discussion and for putting together #5612. That's fast! > Combining tenant resolution and credential mapping in OidcIdentityPreparer > makes sense to me. The PR also removes the ordering dependency I was > concerned about, which is great! > > Thanks, > > Yufei > > > On Fri, 25 Sep 2026 15:43:00 +0200, Alex Dutra <[email protected]> wrote: > > Hi all, > have the preparer return the identity unchanged when the > principal is not a JWT. Yes, that was what I had in mind when I mentioned > "not too invasive" :-) Since there seems to be some consensus, here is a PR > implementing the suggested changes: > https://github.com/apache/polaris/pull/5612 Thanks, Alex On Thu, Sep 24, > 2026 at 10:55 PM Prithvi S <[email protected]> wrote: > > Hi all, > > > Thanks for writing this up, and for keeping it separate from #5119. > > > I agree with Alex. Both OIDC augmentors should move into one > > OidcIdentityPreparer, and AuthenticatingAugmentor should call that, then > > authenticator.authenticate(). > > Both have to move together. > OidcPolarisCredentialAugmentor reads the tenant > config off the identity, > and DefaultPrincipalMapper and > DefaultPrincipalRolesMapper do too. If > tenant resolution stays its own > augmentor, we still depend on priority > 1200 running before 1000. The > preparer can resolve the tenant, store that > attribute under the same key, > then select the mappers and attach the > credential. The internal/external > credential split from #5119 belongs in > the preparer as well. > > On the coupling concern: have the preparer return > the identity unchanged > when the principal is not a JWT. Internal auth > already carries its > credential from InternalIdentityProvider, so it skips > the OIDC work, and > AuthenticatingAugmentor stays the same for every > source. > > JB, I checked the repo too. Nothing is registered between > priority 1000 and > 1200. The only other augmentor is the test > RootPrincipalAugmentor, at the > default priority, and it only handles > anonymous identities. The tenant > resolver, the two mappers, and > Authenticator stay the extension points. If > a deployment has an augmentor > in that gap, or compiles against those > priority constants, it would be > good to know. Otherwise I think we can drop > it. > > The external-idp dev > notes still describe the three augmentor steps, so > they should be updated > with the change. > > Thanks, > Prithvi S > > On Thu, Sep 24, 2026 at > 9:53 PM Alex Dutra <[email protected]> wrote: > > > Hi all, > > > > I agree > that the authentication layer may seem complex at first sight. > > I also > agree that ideally, augmentors should remain loosely coupled, > > without > dependencies on execution order relative to other augmentors. > > In > Polaris, augmentors are indeed a bit too tightly coupled. > > > > My main > reservation on the proposed simplification is that Injecting > > the > concrete OIDC component into AuthenticatingAugmentor makes the > > > otherwise source-neutral authentication stage know about one > > particular > external mechanism. That blurs the line between internal > > and external > auth paths. > > > > Also, the proposal only mentions removing > > > OidcPolarisCredentialAugmentor, but OidcTenantResolvingAugmentor would > > > need to be removed as well. > > > > Here is a slightly modified proposal: > we could consolidate all > > Polaris-owned OIDC preparation (tenant > resolution and credential > > mapping) into a single focused CDI bean (e.g. > OidcIdentityPreparer). > > Then, AuthenticatingAugmentor would become the > single top-level > > augmentor, which roughly executes the following > workflow: > > > > 1. If source == OIDC, invoke OidcIdentityPreparer, which > will: > > a. Invoke OidcTenantResolver > > b. Invoke PrincipalMapper > > c. > Invoke PrincipalRolesMapper > > 2. Invoke > DefaultAuthenticator.authenticate() for all sources > > > > The OIDC > coupling is still there, but not too invasive. I could live with > > that. > > > > > Thanks, > > Alex > > > > On Thu, Sep 24, 2026 at 7:41 AM > Jean-Baptiste Onofré <[email protected]> > > wrote: > > > > > > Hi Yufei > > > > > > > +1 about the OidcPolarisCredentialAugmentor as CDI component and > call it > > > via AuthenticatingAugmentor. > > > > > > The current wiring > (OidcPolarisCredentialAugmentor at priority 1100 > > handing > > > off a > PolarisCredential via SecurityIdentity to AuthenticatingAugmentor > > at > > > > priority 1000) only works because of the comment on > > > > OidcPolarisCredentialAugmentor saying it "must run before the > > > > authenticating augmentor": that's an invariant the compiler can't check, > > > > and it's easy to break by editing a priority constant without realizing > > > the > > > ordering matters. Making it an explicit call > > > > (credentialAugmentor.augment(identity) then > > > > authenticator.authenticate(identity)) turns that into something the type > > > > and a reader can verify. > > > > > > I checked and I don't see anything > else registered between priority 1000 > > > and 1100 > (OidcTenantResolvingAugmentor is 1200), so nothing depends on > > > > OidcPolarisCredentialAugmentor running as a distinct > > > > SecurityIdentityAugmentor. > > > One thing I don't know is whether any > downstream/vendor deployment has > > > added its own augmentor that expects > to observe or mutate the > > > PolarisCredential between these two steps, > turning it into a plain CDI > > bean > > > removes that extension point. If > nobody is aware of this use case, we are > > > good to move forward. > > > > > > > Regards > > > JB > > > > > > On Thu, Sep 24, 2026 at 7:28 AM Yufei Gu > <[email protected]> wrote: > > > > > > > Hi all, > > > > > > > > While > reviewing #5119 <https://github.com/apache/polaris/pull/5119>, I > > > > > found the authentication flow a little confusing: > > > > > > > > - > > > > > > > > > InternalIdentityProvider and OidcPolarisCredentialAugmentor both > > > attach > > > > a PolarisCredential to the identity, for their respective > > > authentication > > > > paths. > > > > - > > > > > > > > > AuthenticatingAugmentor then consumes that credential to produce a > > > > > PolarisPrincipal. > > > > > > > > Logically, the first two are > source-specific preparation steps, > > followed by > > > > a shared > authentication step. But the OIDC preparation and shared > > > > > authentication steps both implement SecurityIdentityAugmentor. Their > > > > > dependency is hidden in priorities and the credential passed through > > > > > SecurityIdentity, rather than expressed as a direct call. > > > > > > > > > Could we turn OidcPolarisCredentialAugmentor into a regular CDI > > > component > > > > and have AuthenticatingAugmentor call it explicitly > first, then call > > > > authenticator.authenticate(identity)? Internal > authentication would > > skip > > > > that mapping because its credential > is already present. > > > > > > > > Do downstream extensions rely on > running between these two augmentors? > > > > Otherwise, > > > > would this > make the flow clearer? > > > > > > > > To be clear, this is not a blocker > for PR 5119. > > > > Yufei > > > > > >
