yuqi1129 commented on code in PR #13707:
URL: https://github.com/apache/gravitino/pull/13707#discussion_r4386012356
##########
server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizer.java:
##########
@@ -835,6 +835,209 @@ public void
testPartialResultCannotOverwriteCompletedConcurrentLoad() throws Exc
assertFalse(
getAllowEnforcer(jcasbinAuthorizer).getFilteredPolicy(0,
roleIdStr).isEmpty(),
"the completed load's policies must survive a late partial result");
+ assertFalse(
+ getIncompleteRolePolicyIds(jcasbinAuthorizer).contains(roleIdStr),
+ "a late partial result must not mark a completely loaded role as
incomplete");
+ }
+
+ @Test
+ public void testCompleteResultAfterPartialLoadClearsIncompleteMarker()
throws Exception {
+ // Mirror of testPartialResultCannotOverwriteCompletedConcurrentLoad with
the opposite ordering:
+ // the partial result wins first, then a complete result for the same
version replaces it.
+ String roleIdStr = String.valueOf(ALLOW_ROLE_ID);
+ String[] policyRow =
+ new String[] {
+ roleIdStr,
+ MetadataObject.Type.CATALOG.name(),
+ String.valueOf(CATALOG_ID),
+ USE_CATALOG.name(),
+ AuthConstants.ALLOW
+ };
+ ResolvedRolePolicies complete =
+ new ResolvedRolePolicies(
+ ImmutableList.<String[]>of(policyRow),
+ Collections.emptyList(),
+ Collections.emptyList());
+ ResolvedRolePolicies partial =
+ new ResolvedRolePolicies(
+ Collections.emptyList(),
+ Collections.emptyList(),
+ ImmutableList.of("CATALOG:testCatalog"));
+ long roleVersion = 43L;
+
+ assertTrue(invokeReplaceRolePolicies(jcasbinAuthorizer, ALLOW_ROLE_ID,
roleVersion, partial));
+ assertTrue(
+ getIncompleteRolePolicyIds(jcasbinAuthorizer).contains(roleIdStr),
+ "a partial load must mark the role's policies as incomplete");
+ assertFalse(
+
getLoadedRolesCache(jcasbinAuthorizer).getIfPresent(ALLOW_ROLE_ID).isPresent(),
+ "a partial load must stay out of loadedRoles");
+
+ // The complete result replaces the partial rows, so nothing can be
missing any longer.
+ assertTrue(invokeReplaceRolePolicies(jcasbinAuthorizer, ALLOW_ROLE_ID,
roleVersion, complete));
+ assertFalse(
+ getIncompleteRolePolicyIds(jcasbinAuthorizer).contains(roleIdStr),
+ "a complete load must clear the incomplete marker");
+ assertFalse(
+ getAllowEnforcer(jcasbinAuthorizer).getFilteredPolicy(0,
roleIdStr).isEmpty(),
+ "the complete load's policies must be installed");
+
+ // An explicit invalidation drops the rows, so the marker must not outlive
them.
+ jcasbinAuthorizer.handleRolePrivilegeChange(ALLOW_ROLE_ID);
+ assertFalse(
+ getIncompleteRolePolicyIds(jcasbinAuthorizer).contains(roleIdStr),
+ "clearing a role's policies must also clear its incomplete marker");
+ }
+
+ @Test
+ public void testHasDenyPolicyReportsMissingDenyOfIncompleteRoleLoad() throws
Exception {
+ Principal currentPrincipal = PrincipalUtils.getCurrentPrincipal();
+
+ // The user holds a role that DENIES USE_CATALOG on a parent scope, but
the role's securable
+ // object cannot be resolved, so no deny row is installed. hasDenyPolicy
answers an existence
+ // question ("can a DENY hide one of the listed objects?"), and a row that
was never installed
+ // cannot prove there is none: reporting "no deny" lets list authorization
take the parent-scope
+ // short-circuit and skip the per-object checks that would have caught it.
Review Comment:
[Important] The per-object fallback does not necessarily catch the missing
DENY. I verified this through the actual
`MetadataAuthzHelper.filterByExpression` with the real authorizer: a user has a
metalake-level ALLOW USE_CATALOG and a catalog-level DENY; resolving the DENY's
catalog returns empty during role loading. This patch makes `hasDenyPolicy`
return true, but the subsequent per-object filter still includes the catalog
because the DENY row is absent. After a complete reload of the same DENY, the
filter excludes it.
This is pre-existing behavior, and #13691 explicitly preserves per-object
semantics, so keeping this PR narrowly scoped is reasonable. However, please
correct this comment and the PR description's claim that the denied objects
stay hidden, add a regression documenting the actual list result, and track the
remaining authorization gap separately. Disabling the list short-circuit alone
does not provide that stronger safety guarantee.
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -434,6 +457,27 @@ public boolean hasDenyPolicy(
String userIdStr = String.valueOf(userId);
rolePolicyLock.readLock().lock();
try {
+ // A role whose securable objects could not all be resolved installs
only the rows it could
+ // resolve, so its deny rows may be missing. This method answers an
existence question — "can
+ // a DENY on these privileges hide one of the listed objects?" — and
missing rows can never
+ // prove the absence of a DENY, so any bound role with incomplete
policies must be reported as
+ // holding one. The caller then falls back to per-object checks instead
of the parent-scope
+ // short-circuit. Both enforcers are consulted: a role bound only in the
allow enforcer has no
+ // inspectable deny rows at all, which is the same kind of gap. The
marker is added and
+ // removed with the role's policies, so — unlike partialRoleLoadBackoff
— it cannot expire
+ // while the partial policies are still installed.
+ Set<String> boundRoleIds = new
HashSet<>(denyEnforcer.getRolesForUser(userIdStr));
Review Comment:
[Important] Please adapt this check to the current request-scoped role model
when resolving the conflicts with main. #13747 removed request-specific
membership from the shared JCasbin grouping rows, so `getRolesForUser()` is no
longer the source of the current request's held roles.
The incompleteness check should run inside the existing
`evaluateWithLoadedRolePolicies` evaluation callback and inspect the current
request's full held-role subjects (including inactive roles, since DENY remains
global). Please retain the policy-clear/reload protection added by #13606 and
rerun the completeness regressions against the updated implementation. The
current head's tests exercise the older grouping-row model, so they do not
establish compatibility with main.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
