dennismdejong commented on code in PR #13707:
URL: https://github.com/apache/gravitino/pull/13707#discussion_r4397146409
##########
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:
Corrected. The comment and the PR description now state the limited
guarantee: the marker only disables the parent-scope short-circuit; it does not
make a not-yet-resolved DENY effective. Added
`testListStillIncludesObjectDeniedByIncompleteRoleUntilCompleteReload`, which
drives the real `MetadataAuthzHelper.filterByExpression`: the catalog stays
listed while the deny row is missing, and is excluded only after a complete
reload installs it. The remaining gap is noted in the PR description as tracked
separately.
##########
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:
Done. The completeness check now runs inside the
`evaluateWithLoadedRolePolicies` callback and inspects
`roleSubjects(requestContext, false)` — the request's full held-role subjects,
including inactive roles — instead of `getRolesForUser()`. The #13606
clear/reload protection and the marker lifetime are unchanged. Rebased onto
current `main`, adapted the regressions to the request-scoped model, and reran
them: all 181 jcasbin tests pass.
--
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]
