This is an automated email from the ASF dual-hosted git repository.
roryqi pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new d4190675a0 [#12037] improvement(authz): Improve the JCasbin checks
(#12001)
d4190675a0 is described below
commit d4190675a01f402171ffdb7db5e3313f8da6c86b
Author: roryqi <[email protected]>
AuthorDate: Fri Jul 17 15:15:43 2026 +0800
[#12037] improvement(authz): Improve the JCasbin checks (#12001)
### What changes were proposed in this pull request?
Improve the JCasbin checks
### Why are the changes needed?
Fix: #12037
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
UT.
---
.../authorization/jcasbin/JcasbinAuthorizer.java | 154 ++++++-----
.../jcasbin/TestJcasbinAuthorizer.java | 295 ++++++++++++++++++++-
2 files changed, 385 insertions(+), 64 deletions(-)
diff --git
a/server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java
b/server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java
index 64774c8914..47108b3799 100644
---
a/server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java
+++
b/server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java
@@ -24,7 +24,6 @@ import java.io.InputStream;
import java.nio.charset.StandardCharsets;
import java.security.Principal;
import java.util.ArrayList;
-import java.util.Arrays;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashSet;
@@ -470,70 +469,102 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
public boolean hasSetOwnerPermission(
String metalake, String type, String fullName,
AuthorizationRequestContext requestContext) {
Principal currentPrincipal = PrincipalUtils.getCurrentPrincipal();
+ MetadataObject.Type metadataType =
MetadataObject.Type.valueOf(type.toUpperCase(Locale.ROOT));
MetadataObject metalakeObject =
MetadataObjects.of(ImmutableList.of(metalake),
MetadataObject.Type.METALAKE);
+
// metalake owner can set owner in metalake.
if (isOwner(currentPrincipal, metalake, metalakeObject, requestContext)) {
return true;
}
- MetadataObject.Type metadataType =
MetadataObject.Type.valueOf(type.toUpperCase(Locale.ROOT));
- MetadataObject metadataObject =
- MetadataObjects.of(Arrays.asList(fullName.split("\\.")), metadataType);
+
+ MetadataObject metadataObject = MetadataObjects.parse(fullName,
metadataType);
do {
if (isOwner(currentPrincipal, metalake, metadataObject, requestContext))
{
- MetadataObject.Type tempType = metadataObject.type();
- if (tempType == MetadataObject.Type.SCHEMA) {
- boolean hasCatalogUseCatalog =
- authorize(
- currentPrincipal,
- metalake,
- MetadataObjects.parent(metadataObject),
- Privilege.Name.USE_CATALOG,
- requestContext);
- boolean hasMetalakeUseCatalog =
- authorize(
- currentPrincipal,
- metalake,
- metalakeObject,
- Privilege.Name.USE_CATALOG,
- requestContext);
- return hasCatalogUseCatalog || hasMetalakeUseCatalog;
- }
- if (tempType == MetadataObject.Type.TABLE
- || tempType == MetadataObject.Type.VIEW
- || tempType == MetadataObject.Type.TOPIC
- || tempType == MetadataObject.Type.FILESET
- || tempType == MetadataObject.Type.MODEL) {
- boolean hasMetalakeUseSchema =
- authorize(
- currentPrincipal,
- metalake,
- metalakeObject,
- Privilege.Name.USE_SCHEMA,
- requestContext);
- MetadataObject schemaObject = MetadataObjects.parent(metadataObject);
- boolean hasCatalogUseSchema =
- authorize(
- currentPrincipal,
- metalake,
- MetadataObjects.parent(schemaObject),
- Privilege.Name.USE_SCHEMA,
- requestContext);
- boolean hasSchemaUseSchema =
- authorize(
- currentPrincipal,
- metalake,
- schemaObject,
- Privilege.Name.USE_SCHEMA,
- requestContext);
- return hasMetalakeUseSchema || hasCatalogUseSchema ||
hasSchemaUseSchema;
- }
- return true;
+ return hasParentUsagePermission(
+ currentPrincipal, metalake, metadataObject, metalakeObject,
requestContext);
}
} while ((metadataObject = MetadataObjects.parent(metadataObject)) !=
null);
return false;
}
+ private boolean hasAuthorizeWithoutDeny(
+ Principal principal,
+ String metalake,
+ List<MetadataObject> authorizeObjects,
+ List<MetadataObject> denyObjects,
+ Privilege.Name privilege,
+ AuthorizationRequestContext requestContext) {
+ return hasAuthorizeOnAny(principal, metalake, authorizeObjects, privilege,
requestContext)
+ && !hasDenyOnAny(principal, metalake, denyObjects, privilege,
requestContext);
+ }
+
+ private boolean hasAuthorizeOnAny(
+ Principal principal,
+ String metalake,
+ List<MetadataObject> metadataObjects,
+ Privilege.Name privilege,
+ AuthorizationRequestContext requestContext) {
+ for (MetadataObject metadataObject : metadataObjects) {
+ if (authorize(principal, metalake, metadataObject, privilege,
requestContext)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ private boolean hasDenyOnAny(
+ Principal principal,
+ String metalake,
+ List<MetadataObject> metadataObjects,
+ Privilege.Name privilege,
+ AuthorizationRequestContext requestContext) {
+ for (MetadataObject metadataObject : metadataObjects) {
+ if (deny(principal, metalake, metadataObject, privilege,
requestContext)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ private boolean hasParentUsagePermission(
+ Principal principal,
+ String metalake,
+ MetadataObject targetObject,
+ MetadataObject metalakeObject,
+ AuthorizationRequestContext requestContext) {
+ MetadataObject parentObject = MetadataObjects.parent(targetObject);
+ if (parentObject != null && parentObject.type() ==
MetadataObject.Type.CATALOG) {
+ List<MetadataObject> useCatalogObjects = ImmutableList.of(parentObject,
metalakeObject);
+ return hasAuthorizeWithoutDeny(
+ principal,
+ metalake,
+ useCatalogObjects,
+ useCatalogObjects,
+ Privilege.Name.USE_CATALOG,
+ requestContext);
+ }
+ if (parentObject != null && parentObject.type() ==
MetadataObject.Type.SCHEMA) {
+ MetadataObject catalogObject = MetadataObjects.parent(parentObject);
+ List<MetadataObject> useSchemaObjects =
+ ImmutableList.of(metalakeObject, catalogObject, parentObject);
+ return hasAuthorizeWithoutDeny(
+ principal,
+ metalake,
+ useSchemaObjects,
+ useSchemaObjects,
+ Privilege.Name.USE_SCHEMA,
+ requestContext)
+ && !hasDenyOnAny(
+ principal,
+ metalake,
+ ImmutableList.of(catalogObject, metalakeObject),
+ Privilege.Name.USE_CATALOG,
+ requestContext);
+ }
+ return true;
+ }
+
@Override
public boolean hasMetadataPrivilegePermission(
String metalake, String type, String fullName,
AuthorizationRequestContext requestContext) {
@@ -544,19 +575,20 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
} catch (IllegalArgumentException e) {
throw new IllegalArgumentException("Unknown metadata object type: " +
type, e);
}
+ MetadataObject targetObject = MetadataObjects.parse(fullName,
metadataType);
+ MetadataObject metalakeObject =
+ MetadataObjects.of(ImmutableList.of(metalake),
MetadataObject.Type.METALAKE);
List<MetadataObject> chain = new ArrayList<>();
- for (MetadataObject obj = MetadataObjects.parse(fullName, metadataType);
- obj != null;
- obj = MetadataObjects.parent(obj)) {
+ for (MetadataObject obj = targetObject; obj != null; obj =
MetadataObjects.parent(obj)) {
chain.add(obj);
}
- chain.add(MetadataObjects.of(ImmutableList.of(metalake),
MetadataObject.Type.METALAKE));
+ chain.add(metalakeObject);
- for (MetadataObject obj : chain) {
- if (authorize(
- currentPrincipal, metalake, obj, Privilege.Name.MANAGE_GRANTS,
requestContext)) {
- return true;
- }
+ if (hasAuthorizeWithoutDeny(
+ currentPrincipal, metalake, chain, chain,
Privilege.Name.MANAGE_GRANTS, requestContext)
+ && hasParentUsagePermission(
+ currentPrincipal, metalake, targetObject, metalakeObject,
requestContext)) {
+ return true;
}
return hasSetOwnerPermission(metalake, type, fullName, requestContext);
}
diff --git
a/server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizer.java
b/server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizer.java
index 9932a73858..d13437d5d9 100644
---
a/server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizer.java
+++
b/server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizer.java
@@ -19,6 +19,7 @@ package org.apache.gravitino.server.authorization.jcasbin;
import static org.apache.gravitino.authorization.Privilege.Name.SELECT_TABLE;
import static org.apache.gravitino.authorization.Privilege.Name.USE_CATALOG;
+import static org.apache.gravitino.authorization.Privilege.Name.USE_SCHEMA;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
@@ -1685,7 +1686,13 @@ public class TestJcasbinAuthorizer {
"metalakeGrantRole",
ImmutableList.of(
buildManageGrantsSecurableObject(
- metalakeGrantRoleId, MetadataObject.Type.METALAKE,
METALAKE)));
+ metalakeGrantRoleId, MetadataObject.Type.METALAKE,
METALAKE),
+ buildSecurableObject(
+ metalakeGrantRoleId,
+ MetadataObject.Type.SCHEMA,
+ "testCatalog.testSchema",
+ USE_SCHEMA,
+ "ALLOW")));
when(entityStore.get(
eq(NameIdentifierUtil.ofRole(METALAKE, metalakeGrantRole.name())),
eq(Entity.EntityType.ROLE),
@@ -1708,7 +1715,19 @@ public class TestJcasbinAuthorizer {
"catalogGrantRole",
ImmutableList.of(
buildManageGrantsSecurableObject(
- catalogGrantRoleId, MetadataObject.Type.CATALOG,
"testCatalog")));
+ catalogGrantRoleId, MetadataObject.Type.CATALOG,
"testCatalog"),
+ buildSecurableObject(
+ catalogGrantRoleId,
+ MetadataObject.Type.CATALOG,
+ "testCatalog",
+ USE_CATALOG,
+ "ALLOW"),
+ buildSecurableObject(
+ catalogGrantRoleId,
+ MetadataObject.Type.SCHEMA,
+ "testCatalog.testSchema",
+ USE_SCHEMA,
+ "ALLOW")));
when(entityStore.get(
eq(NameIdentifierUtil.ofRole(METALAKE, catalogGrantRole.name())),
eq(Entity.EntityType.ROLE),
@@ -1737,7 +1756,13 @@ public class TestJcasbinAuthorizer {
buildManageGrantsSecurableObject(
tableGrantRoleId,
MetadataObject.Type.TABLE,
- "testCatalog.testSchema.testTable")));
+ "testCatalog.testSchema.testTable"),
+ buildSecurableObject(
+ tableGrantRoleId,
+ MetadataObject.Type.SCHEMA,
+ "testCatalog.testSchema",
+ USE_SCHEMA,
+ "ALLOW")));
when(entityStore.get(
eq(NameIdentifierUtil.ofRole(METALAKE, tableGrantRole.name())),
eq(Entity.EntityType.ROLE),
@@ -1760,6 +1785,246 @@ public class TestJcasbinAuthorizer {
METALAKE, "INVALID_TYPE", "testCatalog", new
AuthorizationRequestContext()));
}
+ @Test
+ public void testHasMetadataPrivilegePermissionRejectsDenyManageGrants()
throws Exception {
+ makeCompletableFutureUseCurrentThread(jcasbinAuthorizer);
+
+ Long allowRoleId = 203L;
+ RoleEntity allowRole =
+ mockRoleInStore(
+ allowRoleId,
+ "allowManageGrantsRole",
+ ImmutableList.of(
+ buildSecurableObject(
+ allowRoleId,
+ MetadataObject.Type.TABLE,
+ "testCatalog.testSchema.testTable",
+ Privilege.Name.MANAGE_GRANTS,
+ "ALLOW"),
+ buildSecurableObject(
+ allowRoleId,
+ MetadataObject.Type.SCHEMA,
+ "testCatalog.testSchema",
+ USE_SCHEMA,
+ "ALLOW")));
+ Long denyRoleId = 204L;
+ RoleEntity denyRole =
+ mockRoleInStore(
+ denyRoleId,
+ "denyManageGrantsRole",
+ ImmutableList.of(
+ buildSecurableObject(
+ denyRoleId,
+ MetadataObject.Type.METALAKE,
+ METALAKE,
+ Privilege.Name.MANAGE_GRANTS,
+ "DENY")));
+ mockDirectUserRoles(allowRole, denyRole);
+
+ assertFalse(
+ jcasbinAuthorizer.hasMetadataPrivilegePermission(
+ METALAKE,
+ "TABLE",
+ "testCatalog.testSchema.testTable",
+ new AuthorizationRequestContext()),
+ "DENY MANAGE_GRANTS should override a narrower ALLOW MANAGE_GRANTS");
+ }
+
+ @Test
+ public void testHasMetadataPrivilegePermissionRejectsMissingParentUsage()
throws Exception {
+ makeCompletableFutureUseCurrentThread(jcasbinAuthorizer);
+
+ Long allowRoleId = 209L;
+ RoleEntity allowRole =
+ mockRoleInStore(
+ allowRoleId,
+ "allowManageGrantsWithoutUseRole",
+ ImmutableList.of(
+ buildSecurableObject(
+ allowRoleId,
+ MetadataObject.Type.TABLE,
+ "testCatalog.testSchema.testTable",
+ Privilege.Name.MANAGE_GRANTS,
+ "ALLOW")));
+ mockDirectUserRoles(allowRole);
+
+ assertFalse(
+ jcasbinAuthorizer.hasMetadataPrivilegePermission(
+ METALAKE,
+ "TABLE",
+ "testCatalog.testSchema.testTable",
+ new AuthorizationRequestContext()),
+ "MANAGE_GRANTS on a table should also require parent USE_SCHEMA");
+ }
+
+ @Test
+ public void
testHasMetadataPrivilegePermissionRejectsDenyParentUsageForFunction()
+ throws Exception {
+ makeCompletableFutureUseCurrentThread(jcasbinAuthorizer);
+
+ Long allowRoleId = 210L;
+ RoleEntity allowRole =
+ mockRoleInStore(
+ allowRoleId,
+ "allowFunctionManageGrantsRole",
+ ImmutableList.of(
+ buildSecurableObject(
+ allowRoleId,
+ MetadataObject.Type.FUNCTION,
+ "testCatalog.testSchema.testFunction",
+ Privilege.Name.MANAGE_GRANTS,
+ "ALLOW"),
+ buildSecurableObject(
+ allowRoleId,
+ MetadataObject.Type.SCHEMA,
+ "testCatalog.testSchema",
+ USE_SCHEMA,
+ "ALLOW")));
+ Long denyRoleId = 211L;
+ RoleEntity denyRole =
+ mockRoleInStore(
+ denyRoleId,
+ "denyUseSchemaForFunctionRole",
+ ImmutableList.of(
+ buildSecurableObject(
+ denyRoleId, MetadataObject.Type.METALAKE, METALAKE,
USE_SCHEMA, "DENY")));
+ mockDirectUserRoles(allowRole, denyRole);
+
+ assertFalse(
+ jcasbinAuthorizer.hasMetadataPrivilegePermission(
+ METALAKE,
+ "FUNCTION",
+ "testCatalog.testSchema.testFunction",
+ new AuthorizationRequestContext()),
+ "DENY USE_SCHEMA should override FUNCTION-level MANAGE_GRANTS");
+ }
+
+ @Test
+ public void
testHasMetadataPrivilegePermissionAllowsOwnerWithDenyManageGrants() throws
Exception {
+ makeCompletableFutureUseCurrentThread(jcasbinAuthorizer);
+
+ Long denyRoleId = 205L;
+ RoleEntity denyRole =
+ mockRoleInStore(
+ denyRoleId,
+ "denyManageGrantsForOwnerRole",
+ ImmutableList.of(
+ buildSecurableObject(
+ denyRoleId,
+ MetadataObject.Type.METALAKE,
+ METALAKE,
+ Privilege.Name.MANAGE_GRANTS,
+ "DENY")));
+ mockDirectUserRoles(denyRole);
+ GravitinoCache<Long, Optional<OwnerInfo>> ownerRelCache =
getOwnerRelCache(jcasbinAuthorizer);
+ ownerRelCache.invalidateAll();
+ ownerRelCache.put(CATALOG_ID, Optional.of(new OwnerInfo(USER_ID, "USER")));
+
+ assertTrue(
+ jcasbinAuthorizer.hasMetadataPrivilegePermission(
+ METALAKE, "CATALOG", "testCatalog", new
AuthorizationRequestContext()),
+ "Owner should be able to manage privileges without checking DENY
MANAGE_GRANTS");
+ }
+
+ @Test
+ public void testHasSetOwnerPermissionRejectsDenyUseCatalogForTableOwner()
throws Exception {
+ makeCompletableFutureUseCurrentThread(jcasbinAuthorizer);
+
+ Long allowRoleId = 206L;
+ RoleEntity allowRole =
+ mockRoleInStore(
+ allowRoleId,
+ "allowUseSchemaRole",
+ ImmutableList.of(
+ buildSecurableObject(
+ allowRoleId,
+ MetadataObject.Type.SCHEMA,
+ "testCatalog.testSchema",
+ USE_SCHEMA,
+ "ALLOW")));
+ Long denyRoleId = 207L;
+ RoleEntity denyRole =
+ mockRoleInStore(
+ denyRoleId,
+ "denyUseCatalogRole",
+ ImmutableList.of(
+ buildSecurableObject(
+ denyRoleId, MetadataObject.Type.METALAKE, METALAKE,
USE_CATALOG, "DENY")));
+ mockDirectUserRoles(allowRole, denyRole);
+ when(ownerMetaMapper.selectOwnerByMetadataObjectIdAndType(eq(CATALOG_ID),
eq("TABLE")))
+ .thenReturn(new OwnerInfo(USER_ID, "USER"));
+ getOwnerRelCache(jcasbinAuthorizer).invalidateAll();
+
+ assertFalse(
+ jcasbinAuthorizer.hasSetOwnerPermission(
+ METALAKE,
+ "TABLE",
+ "testCatalog.testSchema.testTable",
+ new AuthorizationRequestContext()),
+ "DENY USE_CATALOG should override table ownership when setting owner");
+ }
+
+ @Test
+ public void testHasSetOwnerPermissionRejectsDenyUseCatalogForFunctionOwner()
throws Exception {
+ makeCompletableFutureUseCurrentThread(jcasbinAuthorizer);
+
+ Long allowRoleId = 212L;
+ RoleEntity allowRole =
+ mockRoleInStore(
+ allowRoleId,
+ "allowUseSchemaForFunctionOwnerRole",
+ ImmutableList.of(
+ buildSecurableObject(
+ allowRoleId,
+ MetadataObject.Type.SCHEMA,
+ "testCatalog.testSchema",
+ USE_SCHEMA,
+ "ALLOW")));
+ Long denyRoleId = 213L;
+ RoleEntity denyRole =
+ mockRoleInStore(
+ denyRoleId,
+ "denyUseCatalogForFunctionOwnerRole",
+ ImmutableList.of(
+ buildSecurableObject(
+ denyRoleId, MetadataObject.Type.METALAKE, METALAKE,
USE_CATALOG, "DENY")));
+ mockDirectUserRoles(allowRole, denyRole);
+ when(ownerMetaMapper.selectOwnerByMetadataObjectIdAndType(eq(CATALOG_ID),
eq("FUNCTION")))
+ .thenReturn(new OwnerInfo(USER_ID, "USER"));
+ getOwnerRelCache(jcasbinAuthorizer).invalidateAll();
+
+ assertFalse(
+ jcasbinAuthorizer.hasSetOwnerPermission(
+ METALAKE,
+ "FUNCTION",
+ "testCatalog.testSchema.testFunction",
+ new AuthorizationRequestContext()),
+ "DENY USE_CATALOG should override function ownership when setting
owner");
+ }
+
+ @Test
+ public void testHasSetOwnerPermissionAllowsCatalogOwnerWithDenyUseCatalog()
throws Exception {
+ makeCompletableFutureUseCurrentThread(jcasbinAuthorizer);
+
+ Long denyRoleId = 208L;
+ RoleEntity denyRole =
+ mockRoleInStore(
+ denyRoleId,
+ "denyUseCatalogForCatalogOwnerRole",
+ ImmutableList.of(
+ buildSecurableObject(
+ denyRoleId, MetadataObject.Type.METALAKE, METALAKE,
USE_CATALOG, "DENY")));
+ mockDirectUserRoles(denyRole);
+ GravitinoCache<Long, Optional<OwnerInfo>> ownerRelCache =
getOwnerRelCache(jcasbinAuthorizer);
+ ownerRelCache.invalidateAll();
+ ownerRelCache.put(CATALOG_ID, Optional.of(new OwnerInfo(USER_ID, "USER")));
+
+ assertTrue(
+ jcasbinAuthorizer.hasSetOwnerPermission(
+ METALAKE, "CATALOG", "testCatalog", new
AuthorizationRequestContext()),
+ "Catalog owner should be able to set owner without checking DENY
USE_CATALOG");
+ }
+
/**
* Builds a {@link SecurableObject} carrying an ALLOW {@code MANAGE_GRANTS}
privilege bound to
* {@code type} with the shared test metadata ID ({@link #CATALOG_ID}).
@@ -1786,6 +2051,30 @@ public class TestJcasbinAuthorizer {
}
}
+ private static SecurableObject buildSecurableObject(
+ Long roleId,
+ MetadataObject.Type type,
+ String objectName,
+ Privilege.Name privilege,
+ String condition) {
+ try {
+ SecurableObjectPO po =
+ SecurableObjectPO.builder()
+ .withType(String.valueOf(type))
+ .withMetadataObjectId(CATALOG_ID)
+ .withRoleId(roleId)
+
.withPrivilegeNames(objectMapper.writeValueAsString(ImmutableList.of(privilege)))
+
.withPrivilegeConditions(objectMapper.writeValueAsString(ImmutableList.of(condition)))
+ .withDeletedAt(0L)
+ .withCurrentVersion(1L)
+ .withLastVersion(1L)
+ .build();
+ return POConverters.fromSecurableObjectPO(objectName, po, type);
+ } catch (JsonProcessingException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
@SuppressWarnings("unchecked")
private static GravitinoCache<Long, Long>
getLoadedRolesCache(JcasbinAuthorizer authorizer)
throws Exception {
