yuqi1129 commented on code in PR #13707:
URL: https://github.com/apache/gravitino/pull/13707#discussion_r4414064962
##########
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:
Fixed in bd113b9706.
##########
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:
Fixed in bd113b9706.
--
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]
