More readable 403s (#396)

apache · Oct 25, 2024 · 304d700 · 304d700
1 parent 301dc14
commit 304d700
Show file tree

Hide file tree

Showing 4 changed files with 41 additions and 39 deletions.
diff --git a/polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizer.java b/polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizer.java
@@ -93,11 +93,14 @@
 import com.google.common.collect.SetMultimap;
 import java.util.List;
 import java.util.Set;
+import java.util.stream.Collectors;
 import org.apache.iceberg.exceptions.ForbiddenException;
 import org.apache.polaris.core.PolarisConfiguration;
 import org.apache.polaris.core.PolarisConfigurationStore;
 import org.apache.polaris.core.context.CallContext;
+import org.apache.polaris.core.entity.PolarisBaseEntity;
 import org.apache.polaris.core.entity.PolarisEntityConstants;
+import org.apache.polaris.core.entity.PolarisEntityCore;
 import org.apache.polaris.core.entity.PolarisGrantRecord;
 import org.apache.polaris.core.entity.PolarisPrivilege;
 import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper;
@@ -482,21 +485,21 @@ public boolean matchesOrIsSubsumedBy(
 
   public void authorizeOrThrow(
       @NotNull AuthenticatedPolarisPrincipal authenticatedPrincipal,
-      @NotNull Set<Long> activatedGranteeIds,
+      @NotNull Set<PolarisBaseEntity> activatedEntities,
       @NotNull PolarisAuthorizableOperation authzOp,
       @Nullable PolarisResolvedPathWrapper target,
       @Nullable PolarisResolvedPathWrapper secondary) {
     authorizeOrThrow(
         authenticatedPrincipal,
-        activatedGranteeIds,
+        activatedEntities,
         authzOp,
         target == null ? null : List.of(target),
         secondary == null ? null : List.of(secondary));
   }
 
   public void authorizeOrThrow(
       @NotNull AuthenticatedPolarisPrincipal authenticatedPrincipal,
-      @NotNull Set<Long> activatedGranteeIds,
+      @NotNull Set<PolarisBaseEntity> activatedEntities,
       @NotNull PolarisAuthorizableOperation authzOp,
       @Nullable List<PolarisResolvedPathWrapper> targets,
       @Nullable List<PolarisResolvedPathWrapper> secondaries) {
@@ -514,12 +517,12 @@ public void authorizeOrThrow(
           "Principal '%s' is not authorized for op %s due to PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_STATE",
           authenticatedPrincipal.getName(), authzOp);
     } else if (!isAuthorized(
-        authenticatedPrincipal, activatedGranteeIds, authzOp, targets, secondaries)) {
+        authenticatedPrincipal, activatedEntities, authzOp, targets, secondaries)) {
       throw new ForbiddenException(
-          "Principal '%s' with activated PrincipalRoles '%s' and activated ids '%s' is not authorized for op %s",
+          "Principal '%s' with activated PrincipalRoles '%s' and activated grants via '%s' is not authorized for op %s",
           authenticatedPrincipal.getName(),
           authenticatedPrincipal.getActivatedPrincipalRoleNames(),
-          activatedGranteeIds,
+          activatedEntities.stream().map(PolarisEntityCore::getName).collect(Collectors.toSet()),
           authzOp);
     }
   }
@@ -531,24 +534,26 @@ public void authorizeOrThrow(
    */
   public boolean isAuthorized(
       @NotNull AuthenticatedPolarisPrincipal authenticatedPolarisPrincipal,
-      @NotNull Set<Long> activatedGranteeIds,
+      @NotNull Set<PolarisBaseEntity> activatedEntities,
       @NotNull PolarisAuthorizableOperation authzOp,
       @Nullable PolarisResolvedPathWrapper target,
       @Nullable PolarisResolvedPathWrapper secondary) {
     return isAuthorized(
         authenticatedPolarisPrincipal,
-        activatedGranteeIds,
+        activatedEntities,
         authzOp,
         target == null ? null : List.of(target),
         secondary == null ? null : List.of(secondary));
   }
 
   public boolean isAuthorized(
       @NotNull AuthenticatedPolarisPrincipal authenticatedPolarisPrincipal,
-      @NotNull Set<Long> activatedGranteeIds,
+      @NotNull Set<PolarisBaseEntity> activatedEntities,
       @NotNull PolarisAuthorizableOperation authzOp,
       @Nullable List<PolarisResolvedPathWrapper> targets,
       @Nullable List<PolarisResolvedPathWrapper> secondaries) {
+    Set<Long> entityIdSet =
+        activatedEntities.stream().map(PolarisEntityCore::getId).collect(Collectors.toSet());
     for (PolarisPrivilege privilegeOnTarget : authzOp.getPrivilegesOnTarget()) {
       // If any privileges are required on target, the target must be non-null.
       Preconditions.checkState(
@@ -558,7 +563,7 @@ public boolean isAuthorized(
           privilegeOnTarget);
       for (PolarisResolvedPathWrapper target : targets) {
         if (!hasTransitivePrivilege(
-            authenticatedPolarisPrincipal, activatedGranteeIds, privilegeOnTarget, target)) {
+            authenticatedPolarisPrincipal, entityIdSet, privilegeOnTarget, target)) {
           // TODO: Collect missing privileges to report all at the end and/or return to code
           // that throws NotAuthorizedException for more useful messages.
           return false;
@@ -573,7 +578,7 @@ public boolean isAuthorized(
           privilegeOnSecondary);
       for (PolarisResolvedPathWrapper secondary : secondaries) {
         if (!hasTransitivePrivilege(
-            authenticatedPolarisPrincipal, activatedGranteeIds, privilegeOnSecondary, secondary)) {
+            authenticatedPolarisPrincipal, entityIdSet, privilegeOnSecondary, secondary)) {
           return false;
         }
       }

diff --git a/...src/main/java/org/apache/polaris/core/persistence/resolver/PolarisResolutionManifest.java b/...src/main/java/org/apache/polaris/core/persistence/resolver/PolarisResolutionManifest.java
@@ -243,28 +243,25 @@ public PolarisResolvedPathWrapper getPassthroughResolvedPath(
     return resolvedPath;
   }
 
-  public Set<Long> getAllActivatedCatalogRoleAndPrincipalRoleIds() {
-    Set<Long> activatedIds = new HashSet<>();
+  public Set<PolarisBaseEntity> getAllActivatedCatalogRoleAndPrincipalRoles() {
+    Set<PolarisBaseEntity> activatedRoles = new HashSet<>();
     primaryResolver.getResolvedCallerPrincipalRoles().stream()
         .map(EntityCacheEntry::getEntity)
-        .map(PolarisBaseEntity::getId)
-        .forEach(activatedIds::add);
+        .forEach(activatedRoles::add);
     if (primaryResolver.getResolvedCatalogRoles() != null) {
       primaryResolver.getResolvedCatalogRoles().values().stream()
           .map(EntityCacheEntry::getEntity)
-          .map(PolarisBaseEntity::getId)
-          .forEach(activatedIds::add);
+          .forEach(activatedRoles::add);
     }
-    return activatedIds;
+    return activatedRoles;
   }
 
-  public Set<Long> getAllActivatedPrincipalRoleIds() {
-    Set<Long> activatedIds = new HashSet<>();
+  public Set<PolarisBaseEntity> getAllActivatedPrincipalRoleEntities() {
+    Set<PolarisBaseEntity> activatedEntities = new HashSet<>();
     primaryResolver.getResolvedCallerPrincipalRoles().stream()
         .map(EntityCacheEntry::getEntity)
-        .map(PolarisBaseEntity::getId)
-        .forEach(activatedIds::add);
-    return activatedIds;
+        .forEach(activatedEntities::add);
+    return activatedEntities;
   }
 
   public void setSimulatedResolvedRootContainerEntity(

diff --git a/polaris-service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java b/polaris-service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java
@@ -153,7 +153,7 @@ private void authorizeBasicRootOperationOrThrow(PolarisAuthorizableOperation op)
         resolutionManifest.getResolvedRootContainerEntityAsPath();
     authorizer.authorizeOrThrow(
         authenticatedPrincipal,
-        resolutionManifest.getAllActivatedPrincipalRoleIds(),
+        resolutionManifest.getAllActivatedPrincipalRoleEntities(),
         op,
         rootContainerWrapper,
         null /* secondary */);
@@ -198,7 +198,7 @@ private void authorizeBasicTopLevelEntityOperationOrThrow(
     }
     authorizer.authorizeOrThrow(
         authenticatedPrincipal,
-        resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoleIds(),
+        resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
         op,
         topLevelEntityWrapper,
         null /* secondary */);
@@ -218,7 +218,7 @@ private void authorizeBasicCatalogRoleOperationOrThrow(
     }
     authorizer.authorizeOrThrow(
         authenticatedPrincipal,
-        resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoleIds(),
+        resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
         op,
         target,
         null /* secondary */);
@@ -248,7 +248,7 @@ private void authorizeGrantOnRootContainerToPrincipalRoleOperationOrThrow(
 
     authorizer.authorizeOrThrow(
         authenticatedPrincipal,
-        resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoleIds(),
+        resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
         op,
         rootContainerWrapper,
         principalRoleWrapper);
@@ -284,7 +284,7 @@ private void authorizeGrantOnTopLevelEntityToPrincipalRoleOperationOrThrow(
 
     authorizer.authorizeOrThrow(
         authenticatedPrincipal,
-        resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoleIds(),
+        resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
         op,
         topLevelEntityWrapper,
         principalRoleWrapper);
@@ -314,7 +314,7 @@ private void authorizeGrantOnPrincipalRoleToPrincipalOperationOrThrow(
 
     authorizer.authorizeOrThrow(
         authenticatedPrincipal,
-        resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoleIds(),
+        resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
         op,
         principalRoleWrapper,
         principalWrapper);
@@ -352,7 +352,7 @@ private void authorizeGrantOnCatalogRoleToPrincipalRoleOperationOrThrow(
 
     authorizer.authorizeOrThrow(
         authenticatedPrincipal,
-        resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoleIds(),
+        resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
         op,
         catalogRoleWrapper,
         principalRoleWrapper);
@@ -381,7 +381,7 @@ private void authorizeGrantOnCatalogOperationOrThrow(
         resolutionManifest.getResolvedPath(catalogRoleName, true);
     authorizer.authorizeOrThrow(
         authenticatedPrincipal,
-        resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoleIds(),
+        resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
         op,
         catalogWrapper,
         catalogRoleWrapper);
@@ -420,7 +420,7 @@ private void authorizeGrantOnNamespaceOperationOrThrow(
 
     authorizer.authorizeOrThrow(
         authenticatedPrincipal,
-        resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoleIds(),
+        resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
         op,
         namespaceWrapper,
         catalogRoleWrapper);
@@ -464,7 +464,7 @@ private void authorizeGrantOnTableLikeOperationOrThrow(
 
     authorizer.authorizeOrThrow(
         authenticatedPrincipal,
-        resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoleIds(),
+        resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
         op,
         tableLikeWrapper,
         catalogRoleWrapper);

diff --git a/...ervice/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java b/...ervice/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java
@@ -213,7 +213,7 @@ private void authorizeBasicNamespaceOperationOrThrow(
     }
     authorizer.authorizeOrThrow(
         authenticatedPrincipal,
-        resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoleIds(),
+        resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
         op,
         target,
         null /* secondary */);
@@ -246,7 +246,7 @@ private void authorizeCreateNamespaceUnderNamespaceOperationOrThrow(
     }
     authorizer.authorizeOrThrow(
         authenticatedPrincipal,
-        resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoleIds(),
+        resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
         op,
         target,
         null /* secondary */);
@@ -283,7 +283,7 @@ private void authorizeCreateTableLikeUnderNamespaceOperationOrThrow(
     }
     authorizer.authorizeOrThrow(
         authenticatedPrincipal,
-        resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoleIds(),
+        resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
         op,
         target,
         null /* secondary */);
@@ -315,7 +315,7 @@ private void authorizeBasicTableLikeOperationOrThrow(
     }
     authorizer.authorizeOrThrow(
         authenticatedPrincipal,
-        resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoleIds(),
+        resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
         op,
         target,
         null /* secondary */);
@@ -368,7 +368,7 @@ private void authorizeCollectionOfTableLikeOperationOrThrow(
             .toList();
     authorizer.authorizeOrThrow(
         authenticatedPrincipal,
-        resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoleIds(),
+        resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
         op,
         targets,
         null /* secondaries */);
@@ -428,7 +428,7 @@ private void authorizeRenameTableLikeOperationOrThrow(
         resolutionManifest.getResolvedPath(dst.namespace(), true);
     authorizer.authorizeOrThrow(
         authenticatedPrincipal,
-        resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoleIds(),
+        resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
         op,
         target,
         secondary);