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