Add support for federated principal and role with block for manual role assignment #1353

collado-mike · 2025-04-10T22:12:29Z

First PR to begin implementing the design outlined in https://docs.google.com/document/d/15_3ZiRB6Lhzw0nxij341QUdxEIyFGTrI9_18bFIyJVo/edit?tab=t.0#heading=h.cu1a1acu4lc5 .

Implements #441

Adds a FEDERATED_ENTITY property, which can be defined in Principals and PrincipalRoles. FEDERATED Principals cannot be created via the API, but FEDERATED roles can be. This allows for creating roles which can be assigned privileges, while actual principal creation is delegated to the JIT creation mechanism (not in this PR).

…le assignment

polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisEntity.java

polaris-core/src/main/java/org/apache/polaris/core/entity/PrincipalRoleEntity.java

eric-maynard · 2025-04-11T07:04:37Z

polaris-core/src/main/java/org/apache/polaris/core/entity/PrincipalRoleEntity.java

@@ -65,6 +65,13 @@ public Builder(PrincipalRoleEntity original) {
      super(original);
    }

+    public Builder setFederated(Boolean isFederated) {
+      if (isFederated != null && isFederated) {


This logic doesn't look right to me. This entity would come out federated:

new PolarisEntity.Builder() . . . .setFederated(true) .setFederated(false) .build()

Yeah, the idea is to avoid adding any entry for non-federated identities. I don't think the logic snippet you posted is reasonable or should be supported. I'd be in favor of an IllegalStateException if someone typed that code. Happy to add that.

Why should it not be supported? It sounds like you're saying that .setFederated(false) is never a valid call, in which case setFederated shouldn't take a boolean arg. Perhaps you are even looking for something like buildFederated?

I'm saying the code example you suggested is a programming error. Ideally, that would be handled by the compiler, but an exception would also work.

I support @eric-maynard 's point here. Current code is very risky in terms of latent bugs, IMHO. A caller of .setFederated(false) can reasonably expect the property to become false regardless of any previous set* calls (which might have been made in a different context).

...us/service/src/test/java/org/apache/polaris/service/quarkus/admin/ManagementServiceTest.java

eric-maynard · 2025-04-11T07:09:24Z

...rc/main/java/org/apache/polaris/service/it/test/PolarisManagementServiceIntegrationTest.java

@@ -871,6 +873,42 @@ public void testCreatePrincipalAndRotateCredentials() {
    // rotation that makes the old secret fall off retention.
  }

+  @Test
+  public void testCreateFederatedPrincipalFails() {


Given how different the semantics for federated principal (role)s appear to be, I wonder if we should consider a subtype? It looks like most APIs are valid for nonfederated principal (role)s are not valid for federated ones. This is reminiscent of EXTERNAL catalogs, but maybe more extreme

External catalog has a separate type because it has an extra field. Federated identity types don't have any additional fields at this point, so I'd prefer not to add an extra type. If extra fields end up getting added, then it would make sense, but for now deserializing wouldn't even be able to distinguish what type it should deserialized to.

eric-maynard · 2025-04-11T07:10:58Z

spec/polaris-management-service.yml

@@ -1089,6 +1089,10 @@ components:
        clientId:
          type: string
          description: The output-only OAuth clientId associated with this principal if applicable
+        federated:


This part LGTM, but it should go to a ML vote

+1 to ML discussion.

What's the use case for exposing federated principal in the Polaris Management API? Acting as a proxy in front of an IdP sounds questionable to me.

The main case is to support adding federated roles so that privileges can be defined. Per the original design doc, federated identities are created on the fly when a user logs in, but if we don't allow creation of federated roles, we can't define any privileges for those users until they've logged in. That makes things hard for the admins, IMO.

For Principals, I don't think federated principals should be created via the API, for the same reasoning you suggest, but we should be able to return federated principals and report that they are federated. To make that clearer, maybe I can update the spec to return one of two types, but to only allow creation of one type.

I updated the spec to be more clear about when federated principals are expected as return types/arguments. This unfortunately cluttered the generated code more than I had hoped. I'll post a link on the mailing list re: this change.

Thanks for the clarification! The description of the property LGTM. However, I'm not not sure about exposing federated Principals in the Polaris API.

Granted, we have to expose federated Principal Roles in order to allow assignment to Catalog Roles, but do Principals have to be exposed?

The design doc does talk about Polaris entities for federated roles, but I did not see it mention having to define entities for federated principals. Per section Automatic User Provisioning there is no strict requirement for federated principal entities... hence my question.

As @eric-maynard suggested, it might be more efficient to revive the ML / doc discussion. GH comment threads are less convenient for long discussions.

dimas-b · 2025-04-14T22:16:26Z

service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java

@@ -771,6 +771,9 @@ public PrincipalWithCredentials createPrincipal(PolarisEntity entity) {
    PolarisAuthorizableOperation op = PolarisAuthorizableOperation.CREATE_PRINCIPAL;
    authorizeBasicRootOperationOrThrow(op);

+    if (PolarisEntity.isFederated(entity)) {


Why do we expose "federated" as a property settable via java API only to add validation that it cannot be set?

Also, REST API (which is the only caller of this method) does not allow setting the "federated" property, as far as I can tell, because it does not set internal properties on the PrincipalEntity 🤔

to clarify: My thinking was that the fact that make those entities "not creatable" via the Admin API is not the presence of a specific property (i.e. "federated") but the fact that they are managed in a different way (via IdP integrations, I believe).

Can this be made more explicit in code? I do not have a solid suggestion, but I wonder if some sort of a "manager" class might be suitable here. That "manager" would then make the decision about allowing or not allowing changes via Admin API. WDYT?

The presence of a specific property is the only way that the admin API can tell if an entity is managed by an IdP. Principal Roles, federated or not, must be present in the service in order to be granted privileges. That means they have to be persisted, so the persistence API is going to return them as entities. We have to be able to introspect those entities to know when it is or isn't ok to grant access to those roles. This was outlined in the design doc at https://docs.google.com/document/d/15_3ZiRB6Lhzw0nxij341QUdxEIyFGTrI9_18bFIyJVo/edit?tab=t.0#heading=h.w9dvdtp5mw5p

The design doc does talk about this, but I do not recall firm decisions on mentioned alternatives :)

Even if we create entities for federated principals, consider a use case when someone creates a principal called A (non-federated) and later someone else adds federation and federated principal with name A comes in (or the other way around). Where is this conflict resolved? This is what I mean be "manager".

Another use case: a federated principal B is created thought some IdP integration logic and brings in some properties from the IdP. A user updates principal B via Admin API. The doc mentions that being able to update principal properties is desirable, but where do we resolve conflicts between updates from the API and from the integration side?

dimas-b · 2025-04-14T22:22:36Z

spec/polaris-management-service.yml

@@ -1089,6 +1089,10 @@ components:
        clientId:
          type: string
          description: The output-only OAuth clientId associated with this principal if applicable
+        federated:


+1 to ML discussion.

What's the use case for exposing federated principal in the Polaris Management API? Acting as a proxy in front of an IdP sounds questionable to me.

Add support for federated principal and role with block for manual ro…

b591ae2

…le assignment

collado-mike requested review from adutra, ashvina, dennishuo, dimas-b, eric-maynard, jackye1995, jbonofre, vvcephei, snazy, RussellSpitzer, takidau, MonkeyCanCode, flyrain and ebyhr as code owners April 10, 2025 22:12

github-project-automation bot added this to Basic Kanban Board Apr 10, 2025

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Apr 10, 2025

eric-maynard reviewed Apr 11, 2025

View reviewed changes

polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisEntity.java Show resolved Hide resolved

eric-maynard reviewed Apr 11, 2025

View reviewed changes

polaris-core/src/main/java/org/apache/polaris/core/entity/PrincipalRoleEntity.java Show resolved Hide resolved

eric-maynard reviewed Apr 11, 2025

View reviewed changes

...us/service/src/test/java/org/apache/polaris/service/quarkus/admin/ManagementServiceTest.java Show resolved Hide resolved

eric-maynard reviewed Apr 11, 2025

View reviewed changes

dimas-b reviewed Apr 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for federated principal and role with block for manual role assignment #1353

Add support for federated principal and role with block for manual role assignment #1353

collado-mike commented Apr 10, 2025

eric-maynard Apr 11, 2025

collado-mike Apr 15, 2025

eric-maynard Apr 15, 2025

collado-mike Apr 16, 2025

dimas-b Apr 16, 2025 •

edited

Loading

eric-maynard Apr 11, 2025 •

edited

Loading

collado-mike Apr 15, 2025

eric-maynard Apr 11, 2025

dimas-b Apr 14, 2025

collado-mike Apr 15, 2025 •

edited

Loading

collado-mike Apr 16, 2025

dimas-b Apr 16, 2025 •

edited

Loading

dimas-b Apr 16, 2025 •

edited

Loading

dimas-b Apr 14, 2025

dimas-b Apr 15, 2025

collado-mike Apr 16, 2025

dimas-b Apr 16, 2025 •

edited

Loading

dimas-b Apr 14, 2025

Add support for federated principal and role with block for manual role assignment #1353

Are you sure you want to change the base?

Add support for federated principal and role with block for manual role assignment #1353

Conversation

collado-mike commented Apr 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimas-b Apr 16, 2025 • edited Loading

Choose a reason for hiding this comment

eric-maynard Apr 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

collado-mike Apr 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimas-b Apr 16, 2025 • edited Loading

Choose a reason for hiding this comment

dimas-b Apr 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimas-b Apr 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimas-b Apr 16, 2025 •

edited

Loading

eric-maynard Apr 11, 2025 •

edited

Loading

collado-mike Apr 15, 2025 •

edited

Loading

dimas-b Apr 16, 2025 •

edited

Loading

dimas-b Apr 16, 2025 •

edited

Loading

dimas-b Apr 16, 2025 •

edited

Loading