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