Skip to content

Prefer PolarisPrincipal.getRoles in Resolver #2925

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

Merged
merged 1 commit into from
Oct 31, 2025
Merged

Prefer PolarisPrincipal.getRoles in Resolver #2925

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
29 changes: 13 additions & 16 deletions polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.stream.Collectors;
import org.apache.polaris.core.PolarisCallContext;
Expand Down Expand Up @@ -69,7 +70,6 @@ public class Resolver {

// the id of the principal making the call or 0 if unknown
private final @Nonnull PolarisPrincipal polarisPrincipal;
private final @Nonnull SecurityContext securityContext;

// reference catalog name for name resolution
private final String referenceCatalogName;
Expand Down Expand Up @@ -137,7 +137,6 @@ public Resolver(
this.diagnostics = diagnostics;
this.polarisMetaStoreManager = polarisMetaStoreManager;
this.cache = cache;
this.securityContext = securityContext;
this.referenceCatalogName = referenceCatalogName;

// validate inputs
Expand Down Expand Up @@ -467,11 +466,11 @@ private void updateResolved() {

// update all principal roles with latest
if (!this.resolvedCallerPrincipalRoles.isEmpty()) {
List<ResolvedPolarisEntity> refreshedResolvedCallerPrincipalRoles =
new ArrayList<>(this.resolvedCallerPrincipalRoles.size());
this.resolvedCallerPrincipalRoles.forEach(
ce -> refreshedResolvedCallerPrincipalRoles.add(this.getFreshlyResolved(ce)));
this.resolvedCallerPrincipalRoles = refreshedResolvedCallerPrincipalRoles;
this.resolvedCallerPrincipalRoles =
resolvedCallerPrincipalRoles.stream()
.map(this::getFreshlyResolved)
.filter(Objects::nonNull)
.collect(Collectors.toList());
}

// update referenced catalog
Expand Down Expand Up @@ -776,13 +775,11 @@ private ResolverStatus resolveCallerPrincipalAndPrincipalRoles(
/**
* Resolve all principal roles that the principal has grants for
*
* @param toValidate
* @param resolvedCallerPrincipal1
* @return the list of resolved principal roles the principal has grants for
*/
private List<ResolvedPolarisEntity> resolveAllPrincipalRoles(
List<ResolvedPolarisEntity> toValidate, ResolvedPolarisEntity resolvedCallerPrincipal1) {
return resolvedCallerPrincipal1.getGrantRecordsAsGrantee().stream()
List<ResolvedPolarisEntity> toValidate, ResolvedPolarisEntity callerPrincipal) {
return callerPrincipal.getGrantRecordsAsGrantee().stream()
.filter(gr -> gr.getPrivilegeCode() == PolarisPrivilege.PRINCIPAL_ROLE_USAGE.getCode())
.map(
gr ->
Expand All @@ -791,22 +788,22 @@ private List<ResolvedPolarisEntity> resolveAllPrincipalRoles(
PolarisEntityType.PRINCIPAL_ROLE,
PolarisEntityConstants.getRootEntityId(),
gr.getSecurableId()))
.filter(Objects::nonNull)
.collect(Collectors.toList());
}

/**
* Resolve the specified list of principal roles. The SecurityContext is used to determine whether
* the principal actually has the roles specified.
* Resolve the specified list of principal roles. The PolarisPrincipal is used to determine
* whether the principal actually has the roles specified.
*
* @param toValidate
* @param roleNames
* @return the filtered list of resolved principal roles
*/
private List<ResolvedPolarisEntity> resolvePrincipalRolesByName(
Copy link
Contributor

@adutra adutra Oct 28, 2025

Choose a reason for hiding this comment

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

I still think that there is something fishy with the fact that we re-resolve the principal entity and principal role entities here, at each pass.

The only authoritative source of truth for principal names and roles should be the SecurityIdentity produced during authentication.

If the principal and/or principal roles are modified after the authentication by some other process, we could get in the awkward situation where we would fetch different entities from the database, causing a mismatch with what SecurityIdentity exposes.

IMHO, we should stop resolving principals and principal roles altogether here. That's a prerogative of the Authenticator. This class should focus on resolving catalog roles and paths.

Copy link
Contributor

@dimas-b dimas-b Oct 28, 2025

Choose a reason for hiding this comment

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

Good point, @adutra ! I agree that roles should be established during authentication and not re-resolved after that.

However, I may be a pretty big change. IIRC, in case of Polaris-manages Principals the idea is that the state of permission assignments should be consistent with catalog data during request execution. This probably means that the roles entities used by the authenticator should be made available to the Resolver in order to take their version numbers into account when traversing grant records.

I think the change to use Principal role names in this PR is consistent with current code. Let's merge this PR and refactor more in follow-up PRs. WDYT?

Copy link
Contributor

@adutra adutra Oct 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Sure that works for me.

This probably means that the roles entities used by the authenticator should be made available to the Resolver in order to take their version numbers into account when traversing grant records.

Maybe, but this needs more investigation imho.

I still think it should possible to avoid that. Looking at the Resolver#resolveByName() method, we see that it ultimately calls PolarisMetaStoreManager#loadResolvedEntityByName() which returns the grant records for the entity. That creates a "snapshot" of [principal name + principal roles + catalog roles + grant records] that is consistent for a given pass, afaict (at least as consistent as it can be, given that the operation implies several reads that are not in the same transaction).

List<ResolvedPolarisEntity> toValidate, Set<String> roleNames) {
return roleNames.stream()
.filter(securityContext::isUserInRole)
.filter(roleName -> polarisPrincipal.getRoles().contains(roleName))
Copy link
Contributor

@dimas-b dimas-b Oct 28, 2025

Choose a reason for hiding this comment

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

I agree that from the polaris-core perspective using Principal roles is preferable to using securityContext::isUserInRole as it reduces the scope of components that need to be in sync WRT auth data (note: SecurityContext is well-defined only for Web / REST applications).

.map(roleName -> resolveByName(toValidate, PolarisEntityType.PRINCIPAL_ROLE, roleName))
.filter(Objects::nonNull)
Copy link
Contributor Author

@XN137 XN137 Oct 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

polaris/polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/BaseResolverTest.java

Lines 182 to 183 in 2c77fbf

// With a non-existing principal roles, nothing gets activated
this.resolveDriver(this.cache, Set.of("NOT_EXISTING"), "test", Set.of());

this test was failing with NPE without null filtering... tbh i am not sure i understand it... it's creating a Principal with a non-existing role... shouldnt the Resolver be unsuccessful in that case?
currently it silently does not resolve that role.

Copy link
Contributor

@dimas-b dimas-b Oct 28, 2025

Choose a reason for hiding this comment

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

It is generally the responsibility of the Authenticator to provide role names. However, given that it is a pluggable component and Polaris supports identity federation, core code should be resilient to roles being not present in the Polaris database.

I do not think the resolver should fail in this case. PolarisAuthorizer may deny the request if the unresolved role is critical for reaching the necessary permissions, though.

.collect(Collectors.toList());
}

Expand Down
29 changes: 0 additions & 29 deletions runtime/service/src/test/java/org/apache/polaris/service/admin/PolarisAuthzTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
import java.util.Optional;
import java.util.Set;
import java.util.function.Function;
import java.util.stream.Collectors;
import org.apache.iceberg.CatalogProperties;
import org.apache.iceberg.Schema;
import org.apache.iceberg.catalog.Catalog;
Expand Down Expand Up @@ -69,15 +68,13 @@
import org.apache.polaris.core.entity.CatalogEntity;
import org.apache.polaris.core.entity.CatalogRoleEntity;
import org.apache.polaris.core.entity.PolarisBaseEntity;
import org.apache.polaris.core.entity.PolarisEntityType;
import org.apache.polaris.core.entity.PolarisPrivilege;
import org.apache.polaris.core.entity.PrincipalEntity;
import org.apache.polaris.core.entity.PrincipalRoleEntity;
import org.apache.polaris.core.identity.provider.ServiceIdentityProvider;
import org.apache.polaris.core.persistence.MetaStoreManagerFactory;
import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
import org.apache.polaris.core.persistence.dao.entity.BaseResult;
import org.apache.polaris.core.persistence.dao.entity.EntityResult;
import org.apache.polaris.core.persistence.dao.entity.PrivilegeResult;
import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifest;
import org.apache.polaris.core.persistence.resolver.ResolutionManifestFactory;
Expand Down Expand Up @@ -464,34 +461,9 @@ protected static void assertSuccess(BaseResult result) {
protected @Nonnull SecurityContext securityContext(PolarisPrincipal p) {
SecurityContext securityContext = Mockito.mock(SecurityContext.class);
Mockito.when(securityContext.getUserPrincipal()).thenReturn(p);
Set<String> principalRoleNames = loadPrincipalRolesNames(p);
Mockito.when(securityContext.isUserInRole(Mockito.anyString()))
.thenAnswer(invocation -> principalRoleNames.contains(invocation.getArgument(0)));
return securityContext;
}

protected @Nonnull Set<String> loadPrincipalRolesNames(PolarisPrincipal p) {
PolarisBaseEntity principal =
metaStoreManager
.findPrincipalByName(callContext.getPolarisCallContext(), p.getName())
.orElseThrow();
return metaStoreManager
.loadGrantsToGrantee(callContext.getPolarisCallContext(), principal)
.getGrantRecords()
.stream()
.filter(gr -> gr.getPrivilegeCode() == PolarisPrivilege.PRINCIPAL_ROLE_USAGE.getCode())
.map(
gr ->
metaStoreManager.loadEntity(
callContext.getPolarisCallContext(),
0L,
gr.getSecurableId(),
PolarisEntityType.PRINCIPAL_ROLE))
.map(EntityResult::getEntity)
.map(PolarisBaseEntity::getName)
.collect(Collectors.toSet());
}

protected @Nonnull PrincipalEntity rotateAndRefreshPrincipal(
PolarisMetaStoreManager metaStoreManager,
String principalName,
Expand Down Expand Up @@ -524,7 +496,6 @@ private void initBaseCatalog() {
}
SecurityContext securityContext = Mockito.mock(SecurityContext.class);
Mockito.when(securityContext.getUserPrincipal()).thenReturn(authenticatedRoot);
Mockito.when(securityContext.isUserInRole(Mockito.anyString())).thenReturn(true);
PolarisPassthroughResolutionView passthroughView =
new PolarisPassthroughResolutionView(
resolutionManifestFactory, securityContext, CATALOG_NAME);
Expand Down
1 change: 0 additions & 1 deletion ...va/org/apache/polaris/service/catalog/generic/AbstractPolarisGenericTableCatalogTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,6 @@ public void before(TestInfo testInfo) {

securityContext = Mockito.mock(SecurityContext.class);
when(securityContext.getUserPrincipal()).thenReturn(authenticatedRoot);
when(securityContext.isUserInRole(isA(String.class))).thenReturn(true);

PolarisAuthorizer authorizer = new PolarisAuthorizerImpl(realmConfig);
ReservedProperties reservedProperties = ReservedProperties.NONE;
Expand Down
1 change: 0 additions & 1 deletion .../src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,6 @@ public void before(TestInfo testInfo) {

securityContext = Mockito.mock(SecurityContext.class);
when(securityContext.getUserPrincipal()).thenReturn(authenticatedRoot);
when(securityContext.isUserInRole(isA(String.class))).thenReturn(true);

PolarisAuthorizer authorizer = new PolarisAuthorizerImpl(realmConfig);
reservedProperties = new ReservedProperties() {};
Expand Down
1 change: 0 additions & 1 deletion .../test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogViewTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,6 @@ public void before(TestInfo testInfo) {

SecurityContext securityContext = Mockito.mock(SecurityContext.class);
when(securityContext.getUserPrincipal()).thenReturn(authenticatedRoot);
when(securityContext.isUserInRole(Mockito.anyString())).thenReturn(true);

PolarisAuthorizer authorizer = new PolarisAuthorizerImpl(realmConfig);
ReservedProperties reservedProperties = ReservedProperties.NONE;
Expand Down
1 change: 0 additions & 1 deletion ...ce/src/test/java/org/apache/polaris/service/catalog/policy/AbstractPolicyCatalogTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,6 @@ public void before(TestInfo testInfo) {

securityContext = Mockito.mock(SecurityContext.class);
when(securityContext.getUserPrincipal()).thenReturn(authenticatedRoot);
when(securityContext.isUserInRole(isA(String.class))).thenReturn(true);

PolarisAuthorizer authorizer = new PolarisAuthorizerImpl(realmConfig);
ReservedProperties reservedProperties = ReservedProperties.NONE;
Expand Down