Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+270 −12
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 46 additions & 3 deletions ...main/java/org/apache/polaris/service/it/test/PolarisManagementServiceIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
*/
package org.apache.polaris.service.it.test;

import static javax.ws.rs.core.Response.Status.BAD_REQUEST;
import static javax.ws.rs.core.Response.Status.CREATED;
import static javax.ws.rs.core.Response.Status.FORBIDDEN;
import static org.apache.polaris.service.it.env.PolarisClient.polarisClient;
import static org.apache.polaris.service.it.test.PolarisApplicationIntegrationTest.PRINCIPAL_ROLE_ALL;
Expand Down Expand Up @@ -871,6 +873,42 @@ public void testCreatePrincipalAndRotateCredentials() {
// rotation that makes the old secret fall off retention.
}

@Test
public void testCreateFederatedPrincipalFails() {
Copy link
Contributor

@eric-maynard eric-maynard Apr 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Create a federated Principal
Principal federatedPrincipal =
new Principal(client.newEntityName("federatedPrincipal"), "abc", true, Map.of(), 0L, 0L, 1);

// Attempt to create the federated Principal using the managementApi
try (Response createPResponse =
managementApi
.request("v1/principals")
.post(Entity.json(new CreatePrincipalRequest(federatedPrincipal, false)))) {
assertThat(createPResponse).returns(BAD_REQUEST.getStatusCode(), Response::getStatus);
}
}

@Test
public void testCreateFederatedPrincipalRoleSucceeds() {
// Create a federated Principal Role
PrincipalRole federatedPrincipalRole =
new PrincipalRole(
client.newEntityName("federatedRole"),
true,
Map.of(),
Instant.now().toEpochMilli(),
Instant.now().toEpochMilli(),
1);

// Attempt to create the federated Principal using the managementApi
try (Response createResponse =
managementApi
.request("v1/principal-roles")
.post(Entity.json(new CreatePrincipalRoleRequest(federatedPrincipalRole)))) {
assertThat(createResponse).returns(CREATED.getStatusCode(), Response::getStatus);
}
}

@Test
public void testCreateListUpdateAndDeletePrincipal() {
Principal principal =
Expand Down Expand Up @@ -1022,7 +1060,7 @@ public void testGetPrincipalWithInvalidName() {
public void testCreateListUpdateAndDeletePrincipalRole() {
PrincipalRole principalRole =
new PrincipalRole(
client.newEntityName("myprincipalrole"), Map.of("custom-tag", "foo"), 0L, 0L, 1);
client.newEntityName("myprincipalrole"), false, Map.of("custom-tag", "foo"), 0L, 0L, 1);
managementApi.createPrincipalRole(principalRole);

// Second attempt to create the same entity should fail with CONFLICT.
Expand Down Expand Up @@ -1114,7 +1152,7 @@ public void testCreateListUpdateAndDeletePrincipalRole() {
public void testCreatePrincipalRoleInvalidName() {
String goodName = RandomStringUtils.random(MAX_IDENTIFIER_LENGTH, true, true);
PrincipalRole principalRole =
new PrincipalRole(goodName, Map.of("custom-tag", "good_principal_role"), 0L, 0L, 1);
new PrincipalRole(goodName, false, Map.of("custom-tag", "good_principal_role"), 0L, 0L, 1);
managementApi.createPrincipalRole(principalRole);

String longInvalidName = RandomStringUtils.random(MAX_IDENTIFIER_LENGTH + 1, true, true);
Expand All @@ -1130,7 +1168,12 @@ public void testCreatePrincipalRoleInvalidName() {
for (String invalidPrincipalRoleName : invalidPrincipalRoleNames) {
principalRole =
new PrincipalRole(
invalidPrincipalRoleName, Map.of("custom-tag", "bad_principal_role"), 0L, 0L, 1);
invalidPrincipalRoleName,
false,
Map.of("custom-tag", "bad_principal_role"),
0L,
0L,
1);

try (Response response =
managementApi
Expand Down
6 changes: 6 additions & 0 deletions polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisEntity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,12 @@ public static List<NameAndId> toNameAndIdList(List<EntityNameLookupRecord> entit
.orElse(null);
}

public static boolean isFederated(PolarisBaseEntity entity) {
return Optional.ofNullable(entity.getInternalPropertiesAsMap())
.map(map -> Boolean.parseBoolean(map.get(PolarisEntityConstants.FEDERATED_ENTITY)))
.orElse(false);
}

public PolarisEntity(@Nonnull PolarisBaseEntity sourceEntity) {
super(
sourceEntity.getCatalogId(),
Expand Down
2 changes: 2 additions & 0 deletions polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisEntityConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ public class PolarisEntityConstants {
public static final String PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_STATE =
"CREDENTIAL_ROTATION_REQUIRED";

public static final String FEDERATED_ENTITY = "federated";

/**
* Name format of storage integration for polaris entity: {@code
* POLARIS_<catalog_id>_<entity_id>}. This name format gives us flexibility to switch to use
Expand Down
9 changes: 9 additions & 0 deletions polaris-core/src/main/java/org/apache/polaris/core/entity/PrincipalEntity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ public static PrincipalEntity of(PolarisBaseEntity sourceEntity) {
public static PrincipalEntity fromPrincipal(Principal principal) {
return new Builder()
.setName(principal.getName())
.setFederated(principal.getFederated())
.setProperties(principal.getProperties())
.setClientId(principal.getClientId())
.build();
Expand All @@ -45,6 +46,7 @@ public Principal asPrincipal() {
return new Principal(
getName(),
getClientId(),
PolarisEntity.isFederated(this),
getPropertiesAsMap(),
getCreateTimestamp(),
getLastUpdateTimestamp(),
Expand Down Expand Up @@ -78,6 +80,13 @@ public Builder setCredentialRotationRequiredState() {
return this;
}

public Builder setFederated(Boolean isFederated) {
if (isFederated != null && isFederated) {
internalProperties.put(PolarisEntityConstants.FEDERATED_ENTITY, "true");
}
return this;
}

@Override
public PrincipalEntity build() {
return new PrincipalEntity(buildBase());
Expand Down
23 changes: 15 additions & 8 deletions polaris-core/src/main/java/org/apache/polaris/core/entity/PrincipalRoleEntity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,19 +38,19 @@ public static PrincipalRoleEntity of(PolarisBaseEntity sourceEntity) {
public static PrincipalRoleEntity fromPrincipalRole(PrincipalRole principalRole) {
return new Builder()
.setName(principalRole.getName())
.setFederated(principalRole.getFederated())
.setProperties(principalRole.getProperties())
.build();
}

public PrincipalRole asPrincipalRole() {
PrincipalRole principalRole =
new PrincipalRole(
getName(),
getPropertiesAsMap(),
getCreateTimestamp(),
getLastUpdateTimestamp(),
getEntityVersion());
return principalRole;
return new PrincipalRole(
getName(),
PolarisEntity.isFederated(this),
getPropertiesAsMap(),
getCreateTimestamp(),
getLastUpdateTimestamp(),
getEntityVersion());
}

public static class Builder extends PolarisEntity.BaseBuilder<PrincipalRoleEntity, Builder> {
Expand All @@ -65,6 +65,13 @@ public Builder(PrincipalRoleEntity original) {
super(original);
}

public Builder setFederated(Boolean isFederated) {
if (isFederated != null && isFederated) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@dimas-b dimas-b Apr 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

internalProperties.put(PolarisEntityConstants.FEDERATED_ENTITY, "true");
}
return this;
}

@Override
public PrincipalRoleEntity build() {
return new PrincipalRoleEntity(buildBase());
Expand Down
161 changes: 161 additions & 0 deletions ...service/src/test/java/org/apache/polaris/service/quarkus/admin/ManagementServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,14 @@
import static org.assertj.core.api.Assertions.assertThatThrownBy;

import jakarta.ws.rs.core.Response;
import jakarta.ws.rs.core.SecurityContext;
import java.security.Principal;
import java.time.Clock;
import java.time.Instant;
import java.util.List;
import java.util.Map;
import java.util.Set;
import org.apache.iceberg.exceptions.ValidationException;
import org.apache.polaris.core.PolarisCallContext;
import org.apache.polaris.core.admin.model.AwsStorageConfigInfo;
import org.apache.polaris.core.admin.model.Catalog;
Expand All @@ -34,8 +39,18 @@
import org.apache.polaris.core.admin.model.PolarisCatalog;
import org.apache.polaris.core.admin.model.StorageConfigInfo;
import org.apache.polaris.core.admin.model.UpdateCatalogRequest;
import org.apache.polaris.core.auth.AuthenticatedPolarisPrincipal;
import org.apache.polaris.core.auth.PolarisAuthorizerImpl;
import org.apache.polaris.core.context.CallContext;
import org.apache.polaris.core.context.RealmContext;
import org.apache.polaris.core.entity.PrincipalEntity;
import org.apache.polaris.core.entity.PrincipalRoleEntity;
import org.apache.polaris.core.persistence.MetaStoreManagerFactory;
import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
import org.apache.polaris.core.persistence.dao.entity.EntityResult;
import org.apache.polaris.service.TestServices;
import org.apache.polaris.service.admin.PolarisAdminService;
import org.apache.polaris.service.config.DefaultConfigurationStore;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.mockito.Mockito;
Expand Down Expand Up @@ -158,4 +173,150 @@ public void testUpdateCatalogWithDisallowedStorageConfig() {
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Unsupported storage type: FILE");
}

private PolarisMetaStoreManager setupMetaStoreManager() {
MetaStoreManagerFactory metaStoreManagerFactory = services.metaStoreManagerFactory();
RealmContext realmContext = services.realmContext();
return metaStoreManagerFactory.getOrCreateMetaStoreManager(realmContext);
}

private PolarisCallContext setupCallContext(PolarisMetaStoreManager metaStoreManager) {
MetaStoreManagerFactory metaStoreManagerFactory = services.metaStoreManagerFactory();
RealmContext realmContext = services.realmContext();
return new PolarisCallContext(
metaStoreManagerFactory.getOrCreateSessionSupplier(realmContext).get(),
services.polarisDiagnostics());
}

private PolarisAdminService setupPolarisAdminService(
PolarisMetaStoreManager metaStoreManager, PolarisCallContext callContext) {
RealmContext realmContext = services.realmContext();
return new PolarisAdminService(
CallContext.of(realmContext, callContext),
services.entityManagerFactory().getOrCreateEntityManager(realmContext),
metaStoreManager,
new SecurityContext() {
@Override
public Principal getUserPrincipal() {
return new AuthenticatedPolarisPrincipal(
new PrincipalEntity.Builder().setName("root").build(), Set.of("service_admin"));
}

@Override
public boolean isUserInRole(String role) {
return true;
}

@Override
public boolean isSecure() {
return false;
}

@Override
public String getAuthenticationScheme() {
return "";
}
},
new PolarisAuthorizerImpl(new DefaultConfigurationStore(Map.of())));
}

private PrincipalEntity createPrincipal(
PolarisMetaStoreManager metaStoreManager,
PolarisCallContext callContext,
String name,
boolean isFederated) {
return new PrincipalEntity.Builder()
.setFederated(isFederated)
.setName(name)
.setCreateTimestamp(Instant.now().toEpochMilli())
.setId(metaStoreManager.generateNewEntityId(callContext).getId())
.build();
}

private PrincipalRoleEntity createRole(
PolarisMetaStoreManager metaStoreManager,
PolarisCallContext callContext,
String name,
boolean isFederated) {
return new PrincipalRoleEntity.Builder()
.setId(metaStoreManager.generateNewEntityId(callContext).getId())
.setName(name)
.setFederated(isFederated)
.setProperties(Map.of())
.setCreateTimestamp(Instant.now().toEpochMilli())
.setLastUpdateTimestamp(Instant.now().toEpochMilli())
.build();
}

@Test
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() {
PolarisMetaStoreManager metaStoreManager = setupMetaStoreManager();
PolarisCallContext callContext = setupCallContext(metaStoreManager);
PolarisAdminService polarisAdminService =
setupPolarisAdminService(metaStoreManager, callContext);

PrincipalEntity nonFederatedPrincipal =
createPrincipal(metaStoreManager, callContext, "non_federated_id", false);
metaStoreManager.createPrincipal(callContext, nonFederatedPrincipal);

PrincipalRoleEntity federatedRole =
createRole(metaStoreManager, callContext, "federated_role", true);
EntityResult result =
metaStoreManager.createEntityIfNotExists(callContext, null, federatedRole);
assertThat(result.isSuccess()).isTrue();

assertThatThrownBy(
() ->
polarisAdminService.assignPrincipalRole(
nonFederatedPrincipal.getName(), federatedRole.getName()))
.isInstanceOf(ValidationException.class);
}

@Test
public void testCannotAddFederatedPrincipalToFederatedRole() {
PolarisMetaStoreManager metaStoreManager = setupMetaStoreManager();
PolarisCallContext callContext = setupCallContext(metaStoreManager);
PolarisAdminService polarisAdminService =
setupPolarisAdminService(metaStoreManager, callContext);

PrincipalEntity federatedPrincipal =
createPrincipal(metaStoreManager, callContext, "federated_principal", true);
metaStoreManager.createPrincipal(callContext, federatedPrincipal);

PrincipalRoleEntity federatedRole =
createRole(metaStoreManager, callContext, "federated_role", true);
EntityResult result =
metaStoreManager.createEntityIfNotExists(callContext, null, federatedRole);
assertThat(result.isSuccess()).isTrue();

assertThatThrownBy(
() ->
polarisAdminService.assignPrincipalRole(
federatedPrincipal.getName(), federatedRole.getName()))
.isInstanceOf(ValidationException.class);
}
}
Loading