yuqi1129 commented on code in PR #13707:
URL: https://github.com/apache/gravitino/pull/13707#discussion_r4413964077
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -488,6 +514,29 @@ public boolean hasDenyPolicy(
metalake,
requestContext,
() -> {
+ // The request holds these roles (direct + group-inherited);
inactive roles are
+ // included because a DENY remains in effect regardless of
active-role narrowing.
+ Set<String> heldRoleIds = roleSubjects(requestContext, false);
+
+ // 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 a held
role with incomplete
+ // policies must be reported as possibly holding one. The caller
then falls back to
+ // per-object checks instead of the parent-scope short-circuit.
Note that this only
+ // disables that short-circuit: per-object evaluation is
unchanged, so an unresolvable
+ // DENY still cannot hide an object it never resolved to.
+ for (String roleId : heldRoleIds) {
+ if (incompleteRolePolicyIds.contains(roleId)) {
Review Comment:
[Important] The PR description says the remaining gap is "tracked
separately", but I cannot find an issue for it. Please open one and link it in
the description.
The gap is real. One example: a role load reads the securable object names
(derived from ids), then a concurrent rename changes table `t` to `t2`.
Resolving `t` gets NotFound, so the DENY row for `t2` is missing until the
partial-load retry, and `t2` is listed.
[Question] Please also make the description clear about what this marker
changes today. When the marker is true but the deny scan finds no row, the
per-object path reads the same deny rows, so the list result is the same as
with the short-circuit (your list test asserts exactly this). Since #13699, a
normalization failure already installs a catalog-scoped deny guard that the
existing scan sees, and an entity-store failure fails closed (`orElse(true)`).
So the marker only matters for a genuine NotFound. Its value is that
`hasDenyPolicy` stays truthful, so a future fix for the gap cannot be bypassed
by the short-circuit. Not blocking; a sentence in the description is enough.
##########
server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizer.java:
##########
@@ -1210,6 +1210,337 @@ 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" would let list
authorization take the
+ // parent-scope short-circuit; reporting one forces the per-object
evaluation, which is the
+ // strongest guarantee this fix provides (it does not make the missing
DENY effective; see
+ // testListStillIncludesObjectDeniedByIncompleteRoleUntilCompleteReload).
+ mockRoleInStore(DENY_ROLE_ID, "denyRole",
ImmutableList.of(getDenySecurableObject()));
+ when(roleMetaMapper.listRolesByUserId(eq(USER_ID)))
+ .thenReturn(ImmutableList.of(buildRolePO(DENY_ROLE_ID, "denyRole")));
+ when(userMetaMapper.getUserUpdatedAt(eq(METALAKE), eq(USERNAME)))
+ .thenReturn(new UserUpdatedAt(USER_ID, nextUserVersion()));
+
+ metadataIdConverterMockedStatic
+ .when(() -> MetadataIdConverter.getID(any(), eq(METALAKE)))
+ .thenReturn(Optional.empty());
+ try {
+ assertTrue(
+ jcasbinAuthorizer.hasDenyPolicy(
+ currentPrincipal,
+ METALAKE,
+ ImmutableSet.of(USE_CATALOG),
+ new AuthorizationRequestContext()),
+ "an incomplete role load must be reported as possibly holding a
DENY");
+ assertTrue(
+ getDenyEnforcer(jcasbinAuthorizer)
+ .getFilteredNamedPolicy("p", 0, String.valueOf(DENY_ROLE_ID))
+ .isEmpty(),
+ "the deny row must really be absent, so the answer cannot come from
the policy scan");
+ } finally {
+ metadataIdConverterMockedStatic
+ .when(() -> MetadataIdConverter.getID(any(), eq(METALAKE)))
+ .thenReturn(Optional.of(CATALOG_ID));
+ }
+
+ // The retry throttle lapses independently of the partial policies. Even
when the retry cannot
+ // read the role at all (so it neither reloads nor re-arms the throttle),
the role still holds
+ // the partial row set and must keep reporting the possible deny.
+ getPartialRoleLoadBackoffCache(jcasbinAuthorizer).invalidate(DENY_ROLE_ID);
+ when(entityStore.get(
+ eq(NameIdentifierUtil.ofRole(METALAKE, "denyRole")),
+ eq(Entity.EntityType.ROLE),
+ eq(RoleEntity.class)))
+ .thenReturn(null);
+
+ assertTrue(
+ jcasbinAuthorizer.hasDenyPolicy(
+ currentPrincipal,
+ METALAKE,
+ ImmutableSet.of(USE_CATALOG),
+ new AuthorizationRequestContext()),
+ "the deny must still be reported after the retry throttle expired");
+ assertFalse(
+
getPartialRoleLoadBackoffCache(jcasbinAuthorizer).getIfPresent(DENY_ROLE_ID).isPresent(),
+ "the unreadable role must not have re-armed the throttle, proving the
marker carries"
+ + " the answer");
+ assertTrue(
+
getIncompleteRolePolicyIds(jcasbinAuthorizer).contains(String.valueOf(DENY_ROLE_ID)),
+ "the incomplete marker must survive the retry throttle");
+ }
+
+ @Test
+ public void testHasDenyPolicyRecoversAfterCompleteRoleLoad() throws
Exception {
+ Principal currentPrincipal = PrincipalUtils.getCurrentPrincipal();
+
+ // An ALLOW-only role so that the deny enforcer never holds a row: every
hasDenyPolicy answer
+ // below is driven solely by policy completeness, which makes the recovery
observable.
+ RoleEntity allowRole =
+ mockRoleInStore(ALLOW_ROLE_ID, "allowRole",
ImmutableList.of(getAllowSecurableObject()));
+ mockDirectUserRoles(allowRole);
+
+ metadataIdConverterMockedStatic
+ .when(() -> MetadataIdConverter.getID(any(), eq(METALAKE)))
+ .thenReturn(Optional.empty());
Review Comment:
[Nit] This test changes the shared `MetadataIdConverter.getID` static stub
(here and with `thenAnswer` below) without a try/finally, and never restores
`Optional.of(CATALOG_ID)`. Other tests in this file restore it in `finally`.
When I disabled the marker check, the 3 new tests failed as expected, but 8
unrelated tests (e.g. `testHasMetadataPrivilegePermission`,
`testHasDenyPolicyDetectsGroupInheritedDeny`) also failed because the stub
leaked. Please wrap it in try/finally so a future regression gives a clean
report.
##########
server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizer.java:
##########
@@ -1210,6 +1210,337 @@ 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" would let list
authorization take the
+ // parent-scope short-circuit; reporting one forces the per-object
evaluation, which is the
+ // strongest guarantee this fix provides (it does not make the missing
DENY effective; see
+ // testListStillIncludesObjectDeniedByIncompleteRoleUntilCompleteReload).
+ mockRoleInStore(DENY_ROLE_ID, "denyRole",
ImmutableList.of(getDenySecurableObject()));
+ when(roleMetaMapper.listRolesByUserId(eq(USER_ID)))
+ .thenReturn(ImmutableList.of(buildRolePO(DENY_ROLE_ID, "denyRole")));
+ when(userMetaMapper.getUserUpdatedAt(eq(METALAKE), eq(USERNAME)))
+ .thenReturn(new UserUpdatedAt(USER_ID, nextUserVersion()));
+
+ metadataIdConverterMockedStatic
+ .when(() -> MetadataIdConverter.getID(any(), eq(METALAKE)))
+ .thenReturn(Optional.empty());
+ try {
+ assertTrue(
+ jcasbinAuthorizer.hasDenyPolicy(
+ currentPrincipal,
+ METALAKE,
+ ImmutableSet.of(USE_CATALOG),
+ new AuthorizationRequestContext()),
+ "an incomplete role load must be reported as possibly holding a
DENY");
+ assertTrue(
+ getDenyEnforcer(jcasbinAuthorizer)
+ .getFilteredNamedPolicy("p", 0, String.valueOf(DENY_ROLE_ID))
+ .isEmpty(),
+ "the deny row must really be absent, so the answer cannot come from
the policy scan");
+ } finally {
+ metadataIdConverterMockedStatic
+ .when(() -> MetadataIdConverter.getID(any(), eq(METALAKE)))
+ .thenReturn(Optional.of(CATALOG_ID));
+ }
+
+ // The retry throttle lapses independently of the partial policies. Even
when the retry cannot
+ // read the role at all (so it neither reloads nor re-arms the throttle),
the role still holds
+ // the partial row set and must keep reporting the possible deny.
+ getPartialRoleLoadBackoffCache(jcasbinAuthorizer).invalidate(DENY_ROLE_ID);
+ when(entityStore.get(
+ eq(NameIdentifierUtil.ofRole(METALAKE, "denyRole")),
+ eq(Entity.EntityType.ROLE),
+ eq(RoleEntity.class)))
+ .thenReturn(null);
Review Comment:
[Nit] In production `entityStore.get` does not return `null`; a role that
cannot be read throws, is marked unreadable, and `hasDenyPolicy` already
returns `true` through `orElse(true)`. The case the marker really protects is
the backoff expiring by TTL after `loadRolePrivilege` but before the
evaluation, while the partial rows are still installed. Could you reword this
comment to describe that case, or at least note that `null` is only a test
stand-in to keep the retry from re-arming the throttle?
--
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]
