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

eric-maynard · 2025-04-11T07:02:57Z

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

@@ -182,6 +182,12 @@ public static List<NameAndId> toNameAndIdList(List<EntityNameLookupRecord> entit
        .orElse(null);
  }

+  public static boolean isFederated(PolarisBaseEntity entity) {


If only a subset of entity types support this, I don't think it should be present in the base type PolarisEntity

I tend to agree with @eric-maynard 's point here

eric-maynard · 2025-04-11T07:03:30Z

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

-    return principalRole;
+    return new PrincipalRole(
+        getName(),
+        PolarisEntity.isFederated(this),


If we have to add an argument, it's best to add it at the end

Normally, I'd agree, but here I am trying to stick to the convention of keeping the entity-specific args at the front and the general, id/created/updated-at timestamps at the end. That's the convention that every entity type follows.

Makes sense, it sounds like this folds into another comment above

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?

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

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

+  public void testCannotAddFederatedPrincipalToNonFederatedRole() {
+    PolarisMetaStoreManager metaStoreManager = setupMetaStoreManager();
+    PolarisCallContext callContext = setupCallContext(metaStoreManager);
+    PolarisAdminService polarisAdminService =
+        setupPolarisAdminService(metaStoreManager, callContext);
+
+    PrincipalEntity federatedPrincipal =
+        createPrincipal(metaStoreManager, callContext, "federated_id", true);
+    metaStoreManager.createPrincipal(callContext, federatedPrincipal);
+
+    PrincipalRoleEntity nonFederatedRole =
+        createRole(metaStoreManager, callContext, "non_federated_role", false);
+    EntityResult result =
+        metaStoreManager.createEntityIfNotExists(callContext, null, nonFederatedRole);
+    assertThat(result.isSuccess()).isTrue();
+
+    assertThatThrownBy(
+            () ->
+                polarisAdminService.assignPrincipalRole(
+                    federatedPrincipal.getName(), nonFederatedRole.getName()))
+        .isInstanceOf(ValidationException.class);
+  }
+
+  @Test
+  public void testCannotAddNonFederatedPrincipalToFederatedRole() {


It could make sense to use a parameterized test here. IIUC there is basically a table like the following:

Principal Type Principal Role Type Can Attach

Non-Federated Non-Federated Yes

Non-Federated Federated No

Federated Non-Federated No

Federated Federated No

The restrictions proposed in the table above make sense to me 👍

I'll skip the first row because it's already covered exhaustively elsewhere. Personally, I tend to shy away from parameterized tests, as I think they're overused and oftentimes obfuscate the test code with a bunch of if/else statements, but I'm ok adding it here.

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.

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?

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

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

dimas-b Apr 14, 2025

eric-maynard Apr 11, 2025

collado-mike Apr 15, 2025

eric-maynard Apr 15, 2025

eric-maynard Apr 11, 2025

collado-mike Apr 15, 2025

eric-maynard Apr 15, 2025

eric-maynard Apr 11, 2025

dimas-b Apr 14, 2025

collado-mike Apr 15, 2025

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

dimas-b Apr 14, 2025

dimas-b Apr 15, 2025

dimas-b Apr 14, 2025

Principal Type	Principal Role Type	Can Attach
Non-Federated	Non-Federated	Yes
Non-Federated	Federated	No
Federated	Non-Federated	No
Federated	Federated	No

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-maynard Apr 11, 2025 •

edited

Loading

collado-mike Apr 15, 2025 •

edited

Loading