bharos commented on code in PR #10868:
URL: https://github.com/apache/gravitino/pull/10868#discussion_r3184871650
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -504,40 +516,106 @@ private void loadRolePrivilege(
Entity.EntityType.USER);
List<CompletableFuture<Void>> loadRoleFutures = new ArrayList<>();
for (RoleEntity role : entities) {
- Long roleId = role.id();
- allowEnforcer.addRoleForUser(String.valueOf(userId),
String.valueOf(roleId));
- denyEnforcer.addRoleForUser(String.valueOf(userId),
String.valueOf(roleId));
- if (loadedRoles.getIfPresent(roleId) != null) {
+ addRoleForUserAndLoadPolicies(
+ userId, metalake, role.id(), role.name(), loadRoleFutures,
entityStore);
+ }
+
+ // Load roles inherited from the user's groups.
+ for (GroupEntity groupEntity : resolveCurrentUserGroups(metalake,
entityStore)) {
+ List<Long> roleIds = groupEntity.roleIds();
+ List<String> roleNames = groupEntity.roleNames();
+ if (roleIds == null || roleNames == null) {
+ continue;
+ }
+ if (roleIds.size() != roleNames.size()) {
+ LOG.warn(
+ "Group {} has mismatched roleIds ({}) and roleNames ({})
-- skipping",
+ groupEntity.name(),
+ roleIds.size(),
+ roleNames.size());
continue;
}
- CompletableFuture<Void> loadRoleFuture =
- CompletableFuture.supplyAsync(
- () -> {
- try {
- return entityStore.get(
- NameIdentifierUtil.ofRole(metalake,
role.name()),
- Entity.EntityType.ROLE,
- RoleEntity.class);
- } catch (Exception e) {
- throw new RuntimeException("Failed to load role:
" + role.name(), e);
- }
- },
- executor)
- .thenAcceptAsync(
- roleEntity -> {
- loadPolicyByRoleEntity(roleEntity);
- loadedRoles.put(roleId, true);
- },
- executor);
- loadRoleFutures.add(loadRoleFuture);
+ for (int i = 0; i < roleIds.size(); i++) {
+ addRoleForUserAndLoadPolicies(
+ userId,
+ metalake,
+ roleIds.get(i),
+ roleNames.get(i),
+ loadRoleFutures,
+ entityStore);
+ }
}
+
CompletableFuture.allOf(loadRoleFutures.toArray(new
CompletableFuture[0])).join();
} catch (IOException e) {
throw new RuntimeException(e);
}
});
}
+ /**
+ * Resolves GroupEntity objects for the current principal's groups, skipping
any that are stale or
+ * not found in the store.
+ */
+ private List<GroupEntity> resolveCurrentUserGroups(String metalake,
EntityStore entityStore) {
+ Principal principal = PrincipalUtils.getCurrentPrincipal();
+ List<GroupEntity> result = new ArrayList<>();
+ if (!(principal instanceof UserPrincipal)) {
+ return result;
+ }
+ for (UserGroup group : ((UserPrincipal) principal).getGroups()) {
+ try {
+ NameIdentifier groupIdent = NameIdentifierUtil.ofGroup(metalake,
group.getGroupname());
+ result.add(entityStore.get(groupIdent, Entity.EntityType.GROUP,
GroupEntity.class));
+ } catch (NoSuchEntityException e) {
+ LOG.debug("Group not found in store: {}", group.getGroupname());
+ } catch (Exception e) {
+ LOG.warn("Failed to resolve group: {}", group.getGroupname(), e);
+ }
+ }
+ return result;
+ }
+
+ /**
+ * Adds a role mapping for the given user in both enforcers and
asynchronously loads the role's
+ * policies if they are not already cached. When a role needs loading, the
resulting {@link
+ * CompletableFuture} is appended to {@code loadRoleFutures} so the caller
can join all futures
+ * after processing both direct and group-inherited roles.
+ */
+ private void addRoleForUserAndLoadPolicies(
+ Long userId,
+ String metalake,
+ Long roleId,
+ String roleName,
+ List<CompletableFuture<Void>> loadRoleFutures,
+ EntityStore entityStore) {
+ allowEnforcer.addRoleForUser(String.valueOf(userId),
String.valueOf(roleId));
Review Comment:
Q1: What happens if a user is removed from a group?
Group membership comes from the JWT token
(PrincipalUtils.getCurrentPrincipal().getGroups()). When the IdP removes the
user from the group, subsequent tokens won't include it, so
resolveCurrentUserGroups skips it and the group's roles are not re-added.
However, stale g-rows from a previous request persist in the enforcer
temporarily — until the role's cache entry is evicted (cache TTL expiry, or any
handleRolePrivilegeChange call that invalidates it). There is no immediate
signal from the IdP into Gravitino for group membership changes (unlike
revokeRolesFromGroup which fires handleRolePrivilegeChange right away). I've
added a TODO with a proposed fix (deleteUser() at the start of
loadRolePrivilege to clear stale g-rows before re-resolving) and a test that
documents this transient limitation - see
testUserRemovedFromGroupAtIdpDeniesAccess.
Q2: Role belongs to user and group — revoke from user, group keeps it
This works correctly. When revokeRolesFromUser fires,
handleRolePrivilegeChange(roleId) invalidates the cache → the removal listener
calls deleteRole on both enforcers, removing all g-rows and p-rows for that
role. On the next authorize call, loadRolePrivilege re-resolves both direct
roles (user no longer has it) and group roles (group still has it) — so the
role is re-loaded via the group path and access is preserved.
Added new test: testRoleSharedByUserAndGroupSurvivesUserRevocation
Also added couple of tests to verify deny vs allow case across user and
group: `testDenyRoleOnUserOverridesAllowFromGroup` and
`testDenyRoleFromGroupOverridesAllowOnUser`
--
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]
