Skip to content

Add new PolaisAuthorizer.authorizeOrThrow for Fine Grained AuthorizableOperations #2767

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
+73 −30
Open

Add new PolaisAuthorizer.authorizeOrThrow for Fine Grained AuthorizableOperations #2767

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
14 changes: 14 additions & 0 deletions polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,18 @@ void authorizeOrThrow(
@Nonnull PolarisAuthorizableOperation authzOp,
@Nullable List<PolarisResolvedPathWrapper> targets,
@Nullable List<PolarisResolvedPathWrapper> secondaries);

void authorizeOrThrow(
Copy link
Contributor

@dimas-b dimas-b Oct 7, 2025

Choose a reason for hiding this comment

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

Would it be possible to add default (possibly inefficient) implementations to the new methods to simplify transition in downstream projects? This is just a matter of convenience, so optional.

@Nonnull PolarisPrincipal polarisPrincipal,
@Nonnull Set<PolarisBaseEntity> activatedEntities,
@Nonnull Set<PolarisAuthorizableOperation> authzOps,
@Nullable PolarisResolvedPathWrapper target,
@Nullable PolarisResolvedPathWrapper secondary);

void authorizeOrThrow(
@Nonnull PolarisPrincipal polarisPrincipal,
@Nonnull Set<PolarisBaseEntity> activatedEntities,
@Nonnull Set<PolarisAuthorizableOperation> authzOps,
@Nullable List<PolarisResolvedPathWrapper> targets,
@Nullable List<PolarisResolvedPathWrapper> secondaries);
Comment on lines +45 to +57
Copy link
Contributor

@singhpk234 singhpk234 Oct 7, 2025

Choose a reason for hiding this comment

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

since this is a public interface, do you wanna may be add some function docs for this ?

Copy link
Contributor

@singhpk234 singhpk234 Oct 7, 2025

Choose a reason for hiding this comment

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

also do we wanna mark the existing API's deprecated ? since these APIs can superseed them

Copy link
Contributor Author

@HonahX HonahX Oct 7, 2025

Choose a reason for hiding this comment

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

do you wanna may be add some function docs for this ?

Sounds great!

also do we wanna mark the existing API's deprecated ? since these APIs can superseed them

I think we should keep the existing ones since they’re widely used across the codebase — they serve as polymorphic helpers.
However, we could provide a default implementation that delegates to the only version requiring an actual implementation. WDYT?

void authorizeOrThrow(
      @Nonnull PolarisPrincipal polarisPrincipal,
      @Nonnull Set<PolarisBaseEntity> activatedEntities,
      @Nonnull Set<PolarisAuthorizableOperation> authzOps,
      @Nullable List<PolarisResolvedPathWrapper> targets,
      @Nullable List<PolarisResolvedPathWrapper> secondaries);

Copy link
Contributor

@collado-mike collado-mike Oct 7, 2025

Choose a reason for hiding this comment

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

I think this probably deserves a mailing list discussion, but I wonder if this API change is really enough to support batching everything together into one call. This API makes the assumption that all operations have the same primary and secondary entities. Is that always going to be the case? or are we fine to kick that can down the road?

Copy link
Contributor Author

@HonahX HonahX Oct 7, 2025

Choose a reason for hiding this comment

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

Sounds good, will start a discussion!

IMHO, batch AuthorizableOperations should mainly be used for fine-grained cases like updateTable, where a single endpoint handles multiple types of updates that each require distinct authorization controls. The original intention behind AuthorizableOperation was that each operation corresponds to one API call, and updateTable is a special exception that needs finer-grained handling.

In the updateTable case, there’s a sort of internal inheritance — for example, a fine-grained operation like TABLE_ADD_SNAPSHOT conceptually extends a broader one such as TABLE_WRITE_PROPERTIES. All these fine-grained operations should share the same set of primary and secondary securable entities; it’s just that this “inheritance” relationship hasn’t been reflected in the code yet.

If two operations involve different sets of securables, they should ideally be authorized through separate API calls, each likely corresponding to its own endpoint.

If in the future the above assumption does not hold, I assume we will need large change anyway and thus a new set of apis will be introduced.

But the above are just my thoughts, let's discuss in dev mailing list to collect thoughts on the new api and see what we want to support : )

}
75 changes: 53 additions & 22 deletions polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@
import jakarta.annotation.Nonnull;
import jakarta.annotation.Nullable;
import jakarta.inject.Inject;
import java.util.EnumSet;
import java.util.List;
import java.util.Set;
import java.util.stream.Collectors;
Expand Down Expand Up @@ -774,33 +775,63 @@ public void authorizeOrThrow(
@Nonnull PolarisAuthorizableOperation authzOp,
@Nullable List<PolarisResolvedPathWrapper> targets,
@Nullable List<PolarisResolvedPathWrapper> secondaries) {
authorizeOrThrow(
polarisPrincipal, activatedEntities, EnumSet.of(authzOp), targets, secondaries);
}

@Override
public void authorizeOrThrow(
@Nonnull PolarisPrincipal polarisPrincipal,
@Nonnull Set<PolarisBaseEntity> activatedEntities,
@Nonnull Set<PolarisAuthorizableOperation> authzOps,
@Nullable PolarisResolvedPathWrapper target,
@Nullable PolarisResolvedPathWrapper secondary) {
authorizeOrThrow(
Copy link
Contributor

@travis-bowen travis-bowen Oct 8, 2025

Choose a reason for hiding this comment

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

Honestly this and the other authorizeOrThrow (with single op and single target / secondary) would probably be nice to have a default as well to default to calling the option with a list of targets and secondaries.

The Polaris impls end up just delegating these other impls often do as well.

polarisPrincipal,
activatedEntities,
authzOps,
target == null ? null : List.of(target),
secondary == null ? null : List.of(secondary));
}

@Override
public void authorizeOrThrow(
@Nonnull PolarisPrincipal polarisPrincipal,
@Nonnull Set<PolarisBaseEntity> activatedEntities,
@Nonnull Set<PolarisAuthorizableOperation> authzOps,
@Nullable List<PolarisResolvedPathWrapper> targets,
@Nullable List<PolarisResolvedPathWrapper> secondaries) {
boolean enforceCredentialRotationRequiredState =
realmConfig.getConfig(
FeatureConfiguration.ENFORCE_PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_CHECKING);
boolean isRoot = getRootPrincipalName().equals(polarisPrincipal.getName());
if (enforceCredentialRotationRequiredState
&& polarisPrincipal
.getProperties()
.containsKey(PolarisEntityConstants.PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_STATE)
&& authzOp != PolarisAuthorizableOperation.ROTATE_CREDENTIALS) {
throw new ForbiddenException(
"Principal '%s' is not authorized for op %s due to PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_STATE",
polarisPrincipal.getName(), authzOp);
} else if (authzOp == PolarisAuthorizableOperation.RESET_CREDENTIALS) {
if (!isRoot) {
throw new ForbiddenException("Only Root principal(service-admin) can perform %s", authzOp);
for (PolarisAuthorizableOperation authzOp : authzOps) {
Copy link
Contributor

@dimas-b dimas-b Oct 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This loop could be done in a default impl. of the new method (redirecting to old methods). Performance-wise it would be the same, as far as I can tell, but it would have an easier migration path downstream.

if (enforceCredentialRotationRequiredState
&& polarisPrincipal
.getProperties()
.containsKey(PolarisEntityConstants.PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_STATE)
&& authzOp != PolarisAuthorizableOperation.ROTATE_CREDENTIALS) {
throw new ForbiddenException(
"Principal '%s' is not authorized for op %s due to PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_STATE",
polarisPrincipal.getName(), authzOp);
} else if (authzOp == PolarisAuthorizableOperation.RESET_CREDENTIALS) {
if (!isRoot) {
throw new ForbiddenException(
"Only Root principal(service-admin) can perform %s", authzOp);
}
LOGGER
.atDebug()
.addKeyValue("principalName", polarisPrincipal.getName())
.log("Root principal allowed to reset credentials");
} else if (!isAuthorized(
polarisPrincipal, activatedEntities, authzOp, targets, secondaries)) {
throw new ForbiddenException(
"Principal '%s' with activated PrincipalRoles '%s' and activated grants via '%s' is not authorized for op %s",
polarisPrincipal.getName(),
polarisPrincipal.getRoles(),
activatedEntities.stream().map(PolarisEntityCore::getName).collect(Collectors.toSet()),
authzOp);
}
LOGGER
.atDebug()
.addKeyValue("principalName", polarisPrincipal.getName())
.log("Root principal allowed to reset credentials");
} else if (!isAuthorized(polarisPrincipal, activatedEntities, authzOp, targets, secondaries)) {
throw new ForbiddenException(
"Principal '%s' with activated PrincipalRoles '%s' and activated grants via '%s' is not authorized for op %s",
polarisPrincipal.getName(),
polarisPrincipal.getRoles(),
activatedEntities.stream().map(PolarisEntityCore::getName).collect(Collectors.toSet()),
authzOp);
}
}

Expand Down
14 changes: 6 additions & 8 deletions runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,14 +275,12 @@ protected void authorizeBasicTableLikeOperationsOrThrow(
throwNotFoundExceptionForTableLikeEntity(identifier, List.of(subType));
}

for (PolarisAuthorizableOperation op : ops) {
authorizer.authorizeOrThrow(
polarisPrincipal,
resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
op,
target,
null /* secondary */);
}
authorizer.authorizeOrThrow(
polarisPrincipal,
resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
ops,
target,
null /* secondary */);

initializeCatalog();
}
Expand Down
Loading