bharos commented on code in PR #10868:
URL: https://github.com/apache/gravitino/pull/10868#discussion_r3190981542
##########
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:
For Q1, yes I can do that, but it is better to always delete the user, as
it's just an in-memory operation on the enforcer. Deletion of user in enforcer
will not result in any extra DB calls, role policies stay cached in loadedRoles.
> If not, we shouldn't delete the user.
This is hard to find, as group info is only present in user token.
Otherwise, we will need to maintain a new cache: userId → Set<groupNames> from
previous token
WDYT
##########
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:
For Q1, yes I can do that, but it is better to always delete the user, as
it's just an in-memory operation on the enforcer. Deletion of user in enforcer
will not result in any extra DB calls, role policies stay cached in loadedRoles.
> If not, we shouldn't delete the user.
This is hard to find, as group info is only present in user token.
Otherwise, we will need to maintain a new cache: userId → Set<groupNames> from
previous token
WDYT
--
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]
