Hi Mike, Thanks for the historical context, that is really helpful. And I completely agree with your last paragraph, and especially this:
My preference would be to make this an immutable class that has the > principal entity and all active principal roles all in the constructor. I was arriving at the exact same conclusion – it's a pity that SecurityContext doesn't expose the principal roles. I guess, the problem now is how to move forward with this suggestion – I imagine a change in AuthenticatedPolarisPrincipal or in the Resolver would create repercussions in a lot of places in the code base as well as in existing PRs. Thanks, Alex On Mon, Apr 21, 2025 at 10:30 PM Michael Collado <collado.m...@gmail.com> wrote: > Hey > > 1. I'm unsure why the Resolver was written to assume no role means all > roles. But I think it has to do with > 2. The Resolver was intended to be able to resolve principal roles, grants, > and target entities all at one "snapshot" based on the entity and grant > versions returned by that loadEntitiesChangeTracking method. In order to do > this in one shot, the authenticator couldn't resolve the principal roles > actually granted to the principal, so it didn't resolve the > PRINCIPAL_ROLE:ALL scope in the token. But that meant the Resolver needed > to verify the grants existed for the roles to be assumed by the principal > (otherwise, a caller could just put whatever role they want in the scope). > To avoid the lookup during authn, ALL needed to be expanded by the Resolver > based on the grants. This was an impedance to the federated role proposal, > which stated that the authn token is authoritative regarding which roles > are granted to the principal (e.g., using a groups claim > < > https://developer.okta.com/docs/guides/customize-tokens-groups-claim/main/ > >). > That's why > 3. I added the ActiveRolesProvider interface so that the default impl would > continue to validate the grants held by the principal for activating > principal roles but the federated alternative would delegate to the token > claims (yet to be committed). > > Hopefully, that history helps. In short, I agree that > SecurityIdentity.getRoles() should be the right way to determine activated > principal roles. The Resolver should rely on the authenticator to determine > the active set of principal roles and not rely on grant records. The > AuthenticatedPolarisPrincipal doesn't really need to hold role names > anymore, but the resolver does still need the list of active PrincipalRoles > to load the appropriate catalog roles. My preference would be to make this > an immutable class that has the principal entity and all active principal > roles all in the constructor. > > Mike > > On Sun, Apr 20, 2025 at 8:34 AM Alex Dutra <alex.du...@dremio.com.invalid> > wrote: > > > Hi Mike, > > > > I am generally fine with the spec changes. > > > > I have however some concerns around the way we handle principal roles > today > > in Polaris, and as I was preparing the PR with support for external IDPs > > [1], some oddities stood out: > > > > 1. The pseudo-role PRINCIPAL_ROLE:ALL seems, by convention, to be > > converted to an empty set of roles by the Authenticator; but I wonder > > how we will be able to distinguish a token that contained no role > from a > > token that contained the pseudo-role. I added some changes to my PR to > > be > > able to distinguish the two situations, but I think the general > > handling of > > this pseudo-role, as well as role mapping in general, deserves some > more > > thinking. The spec says this pseudo-role is "non-functional" – but I > > don't > > know what this implies exactly. > > 2. AuthenticatedPolarisPrincipal exposes principal roles and this is > > imho concerning for two reasons: first, the Resolver seems to update > the > > principal roles during resolution, which makes the class mutable, > > potentially changing the principal characteristics *after* > > authentication. > > And secondly, this creates an ambiguity as roles can be accessed in > two > > ways: SecurityIdentity.getRoles() vs > > AuthenticatedPolarisPrincipal.getActivatedPrincipalRoles(). I would be > > in favor of using the former consistently and completely removing the > > latter. (Note: SecurityContext.isUserInRole() relies on the former.) > > 3. There seems to be some overlap between the primary roles > resolution, > > done by the ActiveRolesProvider, and the role resolution done later by > > the Resolver, cf. method resolveCallerPrincipalAndPrincipalRoles(). I > > wonder if this second role resolution is really necessary? Ultimately, > > the > > source of truth for role membership will always be > > SecurityIdentity.hasRole() or its JAX-RS variant, > > SecurityContext.isUserInRole(). We should not deviate from that. > > > > Thanks, > > > > Alex > > > > [1] https://github.com/apache/polaris/pull/1397 > > > > On Sat, Apr 19, 2025 at 2:47 AM Dmitri Bourlatchkov <di...@apache.org> > > wrote: > > > > > Re 2: Thanks for the clarification, Mike! I guess my brain swapped out > a > > > large portion of that doc :) > > > > > > I'm still not sure how IdentityToPrincipalMapping can help with > resolving > > > changes from API and from IdP integration. > > > > > > The doc talks about namespaces, but in PR# 1353, it looks like API > calls > > > are free to change anything in federated principal roles. > > > > > > Should IdentityToPrincipalMapping extend to validating changes coming > > from > > > the Admin API? > > > > > > Alternatively, we could treat that as a new permission and allow all > > admin > > > users to edit federated roles via API. > > > > > > Re 1: Yes, I guess it would be helpful with SCIM, but then we'd need to > > > sync role membership, I guess. > > > > > > My understanding so far was that we'd get role membership at > > authentication > > > time and not persist it in Polaris. > > > > > > Is that still the plan? > > > > > > I think this relates to how we connect authentication and authorization > > in > > > Polaris. If Principals are external, we get role membership from the > > access > > > token and all is clear. If Principals are local, and we still get roles > > > from the access token, then the question arises about whether that data > > is > > > in sync with local data (and the other way around too). > > > > > > That could be resolved by Polaris owning access tokens (via token > > exchange) > > > and assuming control over role membership based on local data and > access > > > time (I commented on this in the doc too). > > > > > > All in all, since SCIM is not part of phase 1 here, maybe we could > defer > > > dealing with federated Principal persistence, indeed. > > > > > > Re 3: I guess it also applies to client secret generation code, but > let's > > > see whether it actually causes too many conditions in code when it > comes > > to > > > that. > > > > > > Cheers, > > > Dmitri. > > > > > > On Thu, Apr 17, 2025 at 7:42 PM Michael Collado < > collado.m...@gmail.com> > > > wrote: > > > > > > > Thanks for the response. > > > > > > > > 1. TBH, I have no strong feelings about persisting the federated > > > > principals. I think the biggest advantage I saw was to support SCIM > > along > > > > with 3p identity providers. But for this implementation, if we want > to > > > skip > > > > persisting principal entities, I'm ok with it. > > > > > > > > 2. In the doc I wrote the following, which I think addresses the > > concern > > > > around conflicts. > > > > > > > > 1. > > > > > > > > The IdentityToPrincipalMapping should supply a naming convention > so > > > that > > > > PrincipalRoles and Principals created from that source do not > > conflict > > > > with > > > > entities created by a different IdentityProvider > > > > > > > > In the case that a role happens to exist even with the naming > > convention > > > > applied, the caller would be unable to authenticate with that role. > > > > > > > > 3. I don't think we need special classes for federated roles. In > nearly > > > all > > > > of the Polaris code, they are treated as PolarisRoleEntity without > any > > > > issue. The only code that needs to be aware of federated roles is > > > > > > > > a) The Admin API, which prevents granting these roles to principals > and > > > > > > > > b) The federated roles provider, which verifies that the federated > > > > principal is assuming federated roles. > > > > > > > > The rest of the code (i.e., the resolver and authorizer) treat these > > > roles > > > > exactly as native Polaris roles. > > > > > > > > Mike > > > > > > > > On Thu, Apr 17, 2025 at 2:39 PM Dmitri Bourlatchkov < > di...@apache.org> > > > > wrote: > > > > > > > > > Thanks for reviving this discussion, Mike! > > > > > > > > > > The API spec change by itself LGTM, but I have related concerns in > > how > > > > this > > > > > feature is meant to work in general. > > > > > > > > > > 1) The need to expose Federated Principals in Polaris API. > > > > > > > > > > The design doc [1] discusses the possibility to expose Federated > > > > Principals > > > > > in Polaris API, but there are pros and cons. I do not think it is a > > > > trivial > > > > > decision. I'd like to discuss this in more detail before we commit > to > > > > doing > > > > > that. > > > > > > > > > > From my POV the main benefit of exposing Federated Principals is to > > be > > > > able > > > > > to alter their properties in Polaris (as mentioned in the doc). > > > However, > > > > > your email indicates that this is actually forbidden. > > > > > > > > > > What use cases do you envision for exposing read-only Federated > > > > Principals? > > > > > > > > > > 2) Reconciliation of Federated vs. Non-Federated changes. > > > > > > > > > > Let's focus on Principal Roles here (which are meant to be writable > > via > > > > > API). > > > > > > > > > > If users are able to make and change (I assume) Principal Roles via > > > API, > > > > > and federation code will also be creating and changing those roles, > > > what > > > > is > > > > > our approach to handling conflicts between those two streams of > > > changes? > > > > > > > > > > Some examples for context (also mentioned in GH): > > > > > > > > > > A) A user creates normal Principal Role "A", and later role "A" is > > > > > discovered through Federation. > > > > > > > > > > B) A user edits Federated Role "B" and later properties of "B" are > > > > updated > > > > > through Federation (or the other way around). > > > > > > > > > > Do we want to control property edits based on the origin of the > > > property > > > > > (spec out a "namespace" for Federated properties)? > > > > > > > > > > 3) OO types > > > > > > > > > > Currently Principals and Principal Roles are represented by one > java > > > type > > > > > each. When federation comes into play, would it make sense to > > develop a > > > > > (java) type hierarchy for working with that data in Polaris code? > My > > > main > > > > > concern here is avoiding a maze of if/else statements throughout > the > > > > > Polaris codebase for supporting Federation. > > > > > > > > > > I guess our approach to this depends largely on the outcome of > topic > > 2 > > > > > above. > > > > > > > > > > WDYT? > > > > > > > > > > Thanks, > > > > > Dmitri. > > > > > > > > > > [1] > > > > > > > > > > > > > > > > > > > > https://docs.google.com/document/d/15_3ZiRB6Lhzw0nxij341QUdxEIyFGTrI9_18bFIyJVo/edit?tab=t.0#heading=h.cu1a1acu4lc5 > > > > > > > > > > On Wed, Apr 16, 2025 at 6:44 PM Michael Collado < > > > collado.m...@gmail.com> > > > > > wrote: > > > > > > > > > > > Hey folks > > > > > > > > > > > > Some of you already know that I posted an initial PR to get > > federated > > > > > > principals/roles added. One thing that came out of the feedback > > was a > > > > > spec > > > > > > change to make it clear when federated identities can be used in > > the > > > > > APIs. > > > > > > Notably, federated principals cannot be created or updated, but > can > > > be > > > > > > returned in get/list calls, whereas federated roles *can* be > > created > > > by > > > > > the > > > > > > API. The latter is useful/necessary in order to be able to assign > > > > > > privileges to those roles without relying on the JIT creation on > > > login. > > > > > > > > > > > > Please check out the spec change here and let me know what you > > think > > > - > > > > > > > > > > > > > > > > > > > > > > > > > > > https://github.com/apache/polaris/pull/1353/files#diff-52444bc79608edfae86ed0b46d171f7ef63c20090860d877e4e135168311a986 > > > > > > > > > > > > Mike > > > > > > > > > > > > On Tue, Dec 17, 2024 at 5:15 PM Dmitri Bourlatchkov > > > > > > <dmitri.bourlatch...@dremio.com.invalid> wrote: > > > > > > > > > > > > > Hi Mike, > > > > > > > > > > > > > > I left some comments in the doc, but overall it looks good to > me > > :) > > > > > > > > > > > > > > I still think there are some hidden dependencies on > Persistence. > > > For > > > > > > > example, whether and how we can have composite keys for > persisted > > > > > > federated > > > > > > > entities... but I guess we can work that out later. > > > > > > > > > > > > > > Also, I think it is important for the Authorizer API to avoid > > > > assuming > > > > > > that > > > > > > > all principals are persisted. Specific authorizer > implementations > > > > > > > (including the default one) can certainly expect persisted > > > > principals, > > > > > > but > > > > > > > the API should require that for the sake of flexibility of > > possible > > > > > > AuthN/Z > > > > > > > extensions. WDYT? > > > > > > > > > > > > > > Cheers, > > > > > > > Dmitri. > > > > > > > > > > > > > > On Thu, Nov 14, 2024 at 7:43 PM Michael Collado < > > > > > collado.m...@gmail.com> > > > > > > > wrote: > > > > > > > > > > > > > > > Hey folks > > > > > > > > > > > > > > > > As discussed during the community sync, I've put together > some > > > > > thoughts > > > > > > > on > > > > > > > > how we'd add support for federated identities in Polaris. I > > > copied > > > > > over > > > > > > > > some of what I had in the issue at > > > > > > > > https://github.com/apache/polaris/issues/441 and put it into > > the > > > > doc > > > > > > > here: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://docs.google.com/document/d/15_3ZiRB6Lhzw0nxij341QUdxEIyFGTrI9_18bFIyJVo/edit?tab=t.0 > > > > > > > > . > > > > > > > > > > > > > > > > Please take a look when you get some time and let me know > what > > > you > > > > > > think. > > > > > > > > Given that our next community sync is scheduled for the > > > > Thanksgiving > > > > > > > > holiday in the US, it might be useful to schedule a meeting > > > > > > specifically > > > > > > > > for this. I can schedule that sync if needed. > > > > > > > > > > > > > > > > Mike > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
