The confusion between "scopes" and actually *validated* "activatedRoles" is unfortunate. Originally the AuthenticatedPolarisPrincipal really was supposed to only contain "requestedScopes", and grant-resolution was just in the Resolver. In fact I was trying to do some digging to figure out why there ended up being code in PolarisResolutionManifest that backfilled the activatedRoles with the Resolver-validated ones, but couldn't quite find the originating commit (it predated the initial move into the Apache project, apparently).
In general I think if we can make it clear in the code everywhere whether we're dealing with "requestedScopes" or "validatedActiveRoles" and never reuse the same Collection to mean both that'd help maintainability long-term, even if federated-identity scenarios mean we end up storing the same list twice; I see the PR does already add some of that terminology with the "requestedRoles" variable, so that's good. Overall I'll defer to Michael on judging service-provider migration requirements, but at a high level this refactor seems workable for our use case. Thanks Alex for providing the mini refactor-guidance comment in your PR! I think that's a good example we can point at for setting some precedent on requirements for SPI evolution PRs. On Tue, Aug 19, 2025 at 6:49 AM Alexandre Dutra <adu...@apache.org> wrote: > Hi all, > > I went ahead and created a PR implementing the proposed changes: > > https://github.com/apache/polaris/pull/2390 > > Please take a look and let me know what you think. > > Thanks, > Alex > > On Wed, Aug 13, 2025 at 9:58 AM Robert Stupp <sn...@snazy.de> wrote: > > > > Merging those two things SGTM. > > It's what Quarkus/Vert.X > > 'HttpAuthenticationMechanism'/'SecurityIdentity' do (right). > > > > On Wed, Aug 13, 2025 at 1:55 AM Dmitri Bourlatchkov <di...@apache.org> > wrote: > > > > > > Thanks for starting this thread, Alex! > > > > > > I fully support merging Authenticator and ActiveRolesProvider. > > > > > > Aside from the issues you mentioned, from my perspective, it also makes > > > sense conceptually. Authenticating a request implies establishing the > > > principal's > > > identity and consequently its roles. It is a single integration point. > > > Mixing one > > > Authenticator implementation with another Role Resolver looks really > > > awkward. > > > > > > If some Authenticator implementations need to break this process down > into > > > two > > > stages, it is ok, but Polaris core still receives one (immutable) > Principal > > > per request > > > with relevant roles in it. > > > > > > Cheers, > > > Dmitri. > > > > > > On Mon, Aug 4, 2025 at 6:45 AM Alexandre Dutra <adu...@apache.org> > wrote: > > > > > > > Hi all, > > > > > > > > ActiveRolesProvider was introduced back in January, in order to > enrich > > > > SecurityContext with valid roles for a given principal. > > > > > > > > But that was before the introduction of Quarkus, and the introduction > > > > of external authentication with Quarkus Security and OIDC. > > > > > > > > TLDR: ActiveRolesProvider became problematic since (see details > > > > below), and I propose to merge ActiveRolesProvider into > Authenticator. > > > > This would make Authenticator the central component for resolving > > > > principals and principal roles, significantly simplifying both code > > > > and configuration. > > > > > > > > That's it :-) I'm eager to know what the community thinks. > > > > > > > > Thanks, > > > > Alex > > > > > > > > --------- > > > > > > > > Now, for the interested readers, some low-level details. > > > > ActiveRolesProvider is problematic because of its signature, and the > > > > reliance on conventions: > > > > > > > > - Its signature expects an AuthenticatedPolarisPrincipal, thus > forcing > > > > the Authenticator to create a "temporary" instance of > > > > AuthenticatedPolarisPrincipal, *potentially containing invalid > roles*. > > > > This makes the code error-prone. For example, let's assume a > principal > > > > has been granted "catalog_admin", but presents a JWT containing also > > > > "service_admin": > > > > > > > > 1) JWT roles = [PRINCIPAL:ROLE:catalog_admin, > PRINCIPAL:ROLE:service_admin] > > > > 2) Authenticator: creates a temporary > > > > AuthenticatedPolarisPrincipal{roles:[PRINCIPAL:ROLE:catalog_admin, > > > > PRINCIPAL:ROLE:service_admin]} > > > > 3) This temporary instance contains HIGHER privileges than the user > was > > > > granted! > > > > 4) ActiveRolesProvider: removes the wrong role > > > > 5) Final SecurityContext: > > > > AuthenticatedPolarisPrincipal{roles:[PRINCIPAL_ROLE:catalog_admin]} > > > > > > > > While the final security context is correct and does not contain > > > > "service_admin", the *temporary one does contain "service_admin" and > > > > is thus dangerous*. It must not be leaked (injected or used). > > > > > > > > - Even worse, if this temporary instance contains invalid roles, by > > > > convention the default Authenticator would filter out those roles, > > > > while the default ActiveRolesProvider would (again, by convention) > > > > assume that, since no roles were presented, then all roles should be > > > > activated. This could allow a user to obtain more roles than the JWT > > > > allows them to. Let's suppose a user has "service_admin", but > presents > > > > a token with wrong roles: > > > > > > > > 1) JWT roles = [foo, bar] > > > > 2) Authenticator: was expecting "PRINCIPAL_ROLE:XYZ" => removes the > roles > > > > 3) Authenticator: creates a temporary > > > > AuthenticatedPolarisPrincipal{roles:[]} > > > > 4) ActiveRolesProvider: roles is empty => assumes PRINCIPAL_ROLE:ALL > > > > => activates all roles > > > > 5) Final SecurityContext: > > > > AuthenticatedPolarisPrincipal{roles:[PRINCIPAL_ROLE:service_admin]} > > > > > > > > In the above scenario, the reliance on conventions results in the > > > > principal getting higher activated roles than the JWT allowed (the > JWT > > > > above should have resulted in no roles activated). It's not a serious > > > > security issue because only roles previously granted can be > activated, > > > > but it is concerning, because the original JWT did not contain such > > > > roles. > > > > > > > > Merging ActiveRolesProvider into Authenticator fixes both of the > issues. > > > > >
