This is an automated email from the ASF dual-hosted git repository. yuqi4733 pushed a commit to branch internal-main in repository https://gitbox.apache.org/repos/asf/gravitino.git
commit 7f2162184af4f4282338bea35f6ed76b599f1957 Author: yangyang zhong <[email protected]> AuthorDate: Thu Dec 11 11:27:31 2025 +0800 [#9291] improvement(authz): Add comment explaining why deny privileges are injected into allowEnforcer (#9449) ### What changes were proposed in this pull request? Add comment explaining why deny privileges are injected into allowEnforcer ### Why are the changes needed? Fix: #9291 ### Does this PR introduce _any_ user-facing change? None ### How was this patch tested? None --- .../expression/AuthorizationExpressionConverter.java | 15 +++++++++++++++ .../server/authorization/jcasbin/JcasbinAuthorizer.java | 8 ++++++++ 2 files changed, 23 insertions(+) diff --git a/server-common/src/main/java/org/apache/gravitino/server/authorization/expression/AuthorizationExpressionConverter.java b/server-common/src/main/java/org/apache/gravitino/server/authorization/expression/AuthorizationExpressionConverter.java index 160f22069f..34648782b4 100644 --- a/server-common/src/main/java/org/apache/gravitino/server/authorization/expression/AuthorizationExpressionConverter.java +++ b/server-common/src/main/java/org/apache/gravitino/server/authorization/expression/AuthorizationExpressionConverter.java @@ -207,6 +207,21 @@ public class AuthorizationExpressionConverter { public static String replaceAnyPrivilege(String expression) { expression = expression.replaceAll("SERVICE_ADMIN", "authorizer.isServiceAdmin()"); expression = expression.replaceAll("METALAKE_USER", "authorizer.isMetalakeUser(METALAKE_NAME)"); + + // A single privilege (e.g., SELECT_TABLE) can be granted or denied at multiple namespace + // levels: metalake, catalog, schema, and table. + // + // Deny takes precedence over allow: if deny is set for the privilege at any level in the + // hierarchy, + // the user is not considered to have that privilege—even if an allow exists at a more specific + // level. + // + // Examples: + // - If role1 is allowed SELECT_TABLE on metalake1 but denied on catalog1, + // then SELECT_TABLE is denied for all objects under catalog1. + // - If role1 is denied SELECT_TABLE on metalake1, any allow on catalog1 (or deeper) is + // overridden, + // and SELECT_TABLE remains denied for catalog1 and its descendants. expression = expression.replaceAll( "ANY_USE_CATALOG", 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 034a742c48..68125aec56 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 @@ -535,6 +535,14 @@ public class JcasbinAuthorizer implements GravitinoAuthorizer { .toUpperCase(java.util.Locale.ROOT), AuthConstants.ALLOW); } + // Since different roles of a user may simultaneously hold both "allow" and "deny" + // permissions + // for the same privilege on a given MetadataObject, the allowEnforcer must also incorporate + // the "deny" privilege to ensure that the authorize method correctly returns false in such + // cases. For example, if role1 has an "allow" privilege for SELECT_TABLE on table1, while + // role2 has a "deny" privilege for the same action on table1, then a user assigned both + // roles should receive a false result when calling the authorize method. + allowEnforcer.addPolicy( String.valueOf(roleEntity.id()), securableObject.type().name(),
