Maybe our approaches are just different. As for me, making backend changes
first, adding API later is more "natural" :)
I definitely have the opposite approach. I’m very used to defining the
contract first, before a working backend implementation is complete, as
that usually allows dependent teams / UI developers / doc writers to begin
work in parallel while the backend work is being done. There’s no UI work
here, of course, but it’s a habit to work in this manner.
This is more of a blocker from my POV because the behaviour of the current
builder code is hard to predict without reading the implementation, which
IMHO developers do not have to do on a regular basis as builders have a
pretty simple API, where a set call should result in the argument becoming
the new actual value of the attribute.
I’ve addressed this in the PR, but IMHO, the “flexibility” to set this
field more than once is more likely to hide programming errors than to
actually allow any real world use cases. Again,
builder.setFederated(true).setFederated(false) does not feel like a
realistic use case that we should have to support. More likely, that
scenario arises when someone copies a long chain of builder.setFoo() calls
and adds a .setFederated() call at the end without realizing it was already
called earlier in the chain. Or when a builder is started in one method and
returned or passed to another method for completion without the caller
realizing setFederated was already called.
And a builder that doesn’t allow for fields to be set twice isn’t without
precedence. Even the Immutables library has “strict” mode, which throws
IllegalStateExceptions if a field is set twice -
https://immutables.github.io/immutable.html#strict-builder . The whole
point of strict builders is to catch programming errors early so engineers
don’t have to wonder “but I set it to federated here, why is the value
missing from the map…?” during debugging sessions.
All that said, as I stated in the PR comment, I feel this is a minor point.
The latest revision of the PR includes Alex’s suggestion of removing the
property from the map if it’s already present.
Mike
On Thu, May 1, 2025 at 8:35 AM Dmitri Bourlatchkov <di...@apache.org> wrote:
But even in the original code, there is a test asserting that federated
principals can't be created.
Yeah, I saw that, but this was actually the thing that felt awkward to me
in the PR.
I'd prefer to avoid making API changes that do not have an execution option
in runtime (i.e. not possible to create Federated Principals in the API and
not having SCIM either). My rationale is that it is generally easy to add
API but hard to remove/change what has been exposed to the public.
Maybe our approaches are just different. As for me, making backend changes
first, adding API later is more "natural" :)
That said, from my POV this specific API change that adds the "federated"
property to Principals is rather basic and I'd be ok with making it now
even if we do not have SCIM (while I'd prefer not to make it as explained
above).
https://github.com/apache/polaris/pull/1353#discussion_r2067815515 .
This is more of a blocker from my POV because the behaviour of the current
builder code is hard to predict without reading the implementation, which
IMHO developers do not have to do on a regular basis as builders have a
pretty simple API, where a set call should result in the argument becoming
the new actual value of the attribute.
Cheers,
Dmitri.
On Thu, May 1, 2025 at 12:48 AM Michael Collado <collado.m...@gmail.com>
wrote:
Hi Dmitri
I do have a revision ready to push. As of this morning (PDT), I was still
waiting for a response to this issue (I see you've responded since) -
https://github.com/apache/polaris/pull/1353#discussion_r2067815515 .
But even in the original code, there is a test asserting that federated
principals can't be created.
I think to have a discussion that moves forward, it's important that
people
feel heard. If someone has comments on the code or the proposed designs,
I'm happy to have the discussion, but if neither the code nor the e-mail
discussion are being read... I don't see how this conversation can move
forward.
Mike
On Wed, Apr 30, 2025 at 5:58 PM Dmitri Bourlatchkov <di...@apache.org>
wrote:
Hi Mike,
I guess some of the confusion may come from PR 1353 [1] and this thread
being a bit out of sync.
Would you mind removing federated Principals from the API in the PR for
the
sake of clarity?
From my POV, we can reconsider that if/when SCIM (or something similar)
comes.
Thanks,
Dmitri.
[1] https://github.com/apache/polaris/pull/1353
On Wed, Apr 30, 2025 at 11:50 AM Michael Collado <
collado.m...@gmail.com
wrote:
Hi Robert
Unsure if you read the previous emails in this thread. In my earlier
response, I wrote
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.
I feel that should address the majority of the concern you just
expressed.
Storing grants with references to some kind of FQN is certainly one
approach, but IMO, is a massive undertaking with little benefit. For
example, all of the grant APIs have to be modified to deal with
target
entities that don’t exist. How do we do validation for these APIs? Do
these
APIs now need to talk to external entities to ensure the caller
didn’t
misspell a role name or use all caps instead of mixed case? UI
developers
can’t simply populate a list of roles for a dropdown list unless they
also
connect directly to the idp. That makes it hard for someone to build
a
generic Polaris admin UI without also building support for Okta,
Google,
Apple, etc.
On the other hand, SCIM is an industry standard for a reason. Every
major
IdP supports it - and I doubt it’s because they just don’t know any
better.
Supporting federated roles + SCIM means all the existing grant APIs
Just
Work and building generic privilege management tools/UI could be done
today.
Given the choice between a massive rewrite of the grant and
authorization
primitives with no user-facing benefit and supporting federated roles
with
the option of SCIM support in the future, the answer seems clear to
me.
Mike
On Wed, Apr 30, 2025 at 7:04 AM Robert Stupp <sn...@snazy.de> wrote:
Hi,
thanks Mike for the effort.
However, I think that persisting principals, which are not "owned"
by
Polaris, being persisted in Polaris is a good idea. How would
changes
to
a principal in the IdP be propagated to Polaris? What happens if a
principal that's deleted/disabled in the IdP - how is that
reflected
in
Polaris? What would happen if two Polaris instances concurrently
"transparently create" the same "federated principal entity"?
Storing mirrored information that is only owned by an (external)
IdP
leads to a "what's the source-of-truth" problem.
A principal is (mostly) an individual user or service that can
access
Polaris. The actual access-decision is up to the IdP (external IdP
or
Polaris w/ its own principals). Each principal can also be uniquely
identified (IDP + principal-ID).
I really do not think that principals owned by "external IdPs"
should
ever be persisted in Polaris.
Similar for roles manages by external IdPs.
ACLs (grants) are (generally speaking) lists of
grantee+privilege(s)
tuples. It is IMHO better to use a qualified reference to "IDP +
principal/role-ID".
AFAIR it's only possbible to grant privileges to roles, not
individual
principals. And if that's the case, I do not see a requirement to
have
"federated principals" being persisted in Polaris at all.
I seems the underlying issue here is the very tight coupling of
principals + principal-roles + catalog-roles + grants that
requires a
unique integer-ID for each of those and using the same
database-oriented
approach for role-membership management and catalog-entity grants.
What's IMHO needed is a proper separation of those things, which
should
happen now and after that's done integrate external IdPs.
On 22.04.25 17:00, Alex Dutra wrote:
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
--
Robert Stupp
@snazy
