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