Copilot commented on code in PR #10996:
URL: https://github.com/apache/gravitino/pull/10996#discussion_r3273994553
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -652,93 +694,199 @@ private boolean authorizeByJcasbin(
}
}
- private static UserEntity getUserEntity(String username, String metalake)
throws IOException {
+ //
---------------------------------------------------------------------------
+ // User info / ownership helpers
+ //
---------------------------------------------------------------------------
+
+ /**
+ * Per-request {@link UserUpdatedAt} lookup. The underlying {@code
user_meta} query is issued at
+ * most once per (metalake, username) within a single request.
+ */
+ private Optional<UserUpdatedAt> loadUserInfo(
+ String metalake, String username, AuthorizationRequestContext
requestContext) {
+ String cacheKey = JcasbinAuthorizationCacheKeys.userRoleKey(metalake,
username);
+ return requestContext.computeUserInfoIfAbsent(
+ cacheKey,
+ k ->
+ Optional.ofNullable(
+ SessionUtils.getWithoutCommit(
+ UserMetaMapper.class, m -> m.getUserUpdatedAt(metalake,
username))));
+ }
+
+ /**
+ * Returns true when the cached owner type and ID match the given principal
or one of the
+ * principal's groups. The user id is resolved via the version-validated
{@link #loadUserInfo}
+ * cache so back-to-back ownership checks in the same request do not
re-query {@code user_meta}.
+ */
+ private boolean ownerMatchesUserOrGroups(
+ Optional<OwnerInfo> owner,
+ Principal principal,
+ String metalake,
+ AuthorizationRequestContext requestContext) {
+ if (!owner.isPresent()) {
+ return false;
+ }
+ OwnerInfo ownerInfo = owner.get();
+ if
(Entity.EntityType.USER.name().equalsIgnoreCase(ownerInfo.getOwnerType())) {
+ Optional<UserUpdatedAt> userInfo =
+ loadUserInfo(metalake, principal.getName(), requestContext);
+ return userInfo.isPresent() && userInfo.get().getUserId() ==
ownerInfo.getOwnerId();
+ }
+ if
(!Entity.EntityType.GROUP.name().equalsIgnoreCase(ownerInfo.getOwnerType())) {
+ return false;
+ }
EntityStore entityStore = GravitinoEnv.getInstance().entityStore();
- UserEntity userEntity =
- entityStore.get(
- NameIdentifierUtil.ofUser(metalake, username),
- Entity.EntityType.USER,
- UserEntity.class);
- return userEntity;
+ for (GroupEntity groupEntity : resolveCurrentUserGroups(metalake,
entityStore)) {
+ if (Objects.equals(groupEntity.id(), ownerInfo.getOwnerId())) {
+ return true;
+ }
+ }
+ return false;
}
+ //
---------------------------------------------------------------------------
+ // 4-step role loading with version validation
+ //
---------------------------------------------------------------------------
+
private void loadRolePrivilege(
- String metalake, String username, Long userId,
AuthorizationRequestContext requestContext) {
+ String metalake,
+ String username,
+ long userId,
+ UserUpdatedAt userInfo,
+ AuthorizationRequestContext requestContext) {
requestContext.loadRole(
() -> {
- EntityStore entityStore = GravitinoEnv.getInstance().entityStore();
- NameIdentifier userNameIdentifier =
NameIdentifierUtil.ofUser(metalake, username);
- List<RoleEntity> entities;
- try {
- entities =
- entityStore
- .relationOperations()
- .listEntitiesByRelation(
- SupportsRelationOperations.Type.ROLE_USER_REL,
- userNameIdentifier,
- Entity.EntityType.USER);
- List<CompletableFuture<Void>> loadRoleFutures = new ArrayList<>();
- Set<String> desiredRoleIds = new HashSet<>();
- for (RoleEntity role : entities) {
- desiredRoleIds.add(String.valueOf(role.id()));
- addRoleForUserAndLoadPolicies(
- userId,
- metalake,
- role.id(),
- role.name(),
- loadRoleFutures,
- entityStore,
- requestContext);
- }
+ // Step 1a: version-validated user-direct roles via cache.
+ List<Long> userDirectRoleIds = loadUserRoles(metalake, username,
userId, userInfo);
+
+ // Step 1b: version-validated group-inherited roles via cache. Group
membership comes
+ // from the IdP-pushed UserPrincipal; for each group we load its
roles via the same
+ // version-validated path as users (group_meta.updated_at as the
staleness sentinel).
+ List<Long> groupInheritedRoleIds = new ArrayList<>();
+ for (String groupname : currentPrincipalGroupNames()) {
+ groupInheritedRoleIds.addAll(
+ loadGroupRoles(metalake, groupname, userId, requestContext));
+ }
- // 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;
- }
- for (int i = 0; i < roleIds.size(); i++) {
- desiredRoleIds.add(String.valueOf(roleIds.get(i)));
- addRoleForUserAndLoadPolicies(
- userId,
- metalake,
- roleIds.get(i),
- roleNames.get(i),
- loadRoleFutures,
- entityStore,
- requestContext);
- }
+ // Prune stale g-rows: any role currently bound but no longer in the
desired
+ // set (e.g. user removed from a group at the IdP, or role
unassigned).
+ Set<String> desiredRoleIds = new HashSet<>();
+ for (Long id : userDirectRoleIds) {
+ desiredRoleIds.add(String.valueOf(id));
+ }
+ for (Long id : groupInheritedRoleIds) {
+ desiredRoleIds.add(String.valueOf(id));
+ }
+ String userIdStr = String.valueOf(userId);
+ for (String currentRole : allowEnforcer.getRolesForUser(userIdStr)) {
+ if (!desiredRoleIds.contains(currentRole)) {
+ allowEnforcer.deleteRoleForUser(userIdStr, currentRole);
+ denyEnforcer.deleteRoleForUser(userIdStr, currentRole);
}
+ }
- CompletableFuture.allOf(loadRoleFutures.toArray(new
CompletableFuture[0])).join();
-
- // Prune stale g-rows: remove role mappings that are no longer
valid
- // (e.g. user was removed from a group at the IdP level).
- String userIdStr = String.valueOf(userId);
- for (String currentRole :
allowEnforcer.getRolesForUser(userIdStr)) {
- if (!desiredRoleIds.contains(currentRole)) {
- allowEnforcer.deleteRoleForUser(userIdStr, currentRole);
- denyEnforcer.deleteRoleForUser(userIdStr, currentRole);
- }
- }
- } catch (IOException e) {
- throw new RuntimeException(e);
+ // Step 3: batch version-check all role IDs (direct +
group-inherited),
+ // load stale ones (1 query for the version probe).
+ List<Long> allRoleIds = new ArrayList<>(userDirectRoleIds);
+ allRoleIds.addAll(groupInheritedRoleIds);
+ if (!allRoleIds.isEmpty()) {
+ versionCheckAndLoadRoles(metalake, allRoleIds, requestContext);
}
});
}
+ private List<Long> loadUserRoles(
+ String metalake, String username, long userId, UserUpdatedAt userInfo) {
+ String userCacheKey = JcasbinAuthorizationCacheKeys.userRoleKey(metalake,
username);
+ Optional<CachedUserRoles> cachedOpt =
userRoleCache.getIfPresent(userCacheKey);
+
+ if (cachedOpt.isPresent() && cachedOpt.get().getUpdatedAt() >=
userInfo.getUpdatedAt()) {
+ // Cache is still valid
+ CachedUserRoles cached = cachedOpt.get();
+ bindUserRoles(userId, cached.getRoleIds());
+ return cached.getRoleIds();
+ }
+
+ // Cache miss or stale — reload from DB
Review Comment:
Cache validity check for userRoleCache only compares updatedAt. Because
user_meta.updated_at defaults to 0 on insert, a delete-and-recreate of the same
username (new user_id, updated_at=0) can cause an older CachedUserRoles entry
(higher updatedAt) to be treated as valid and bind stale roleIds to the new
userId, potentially granting privileges to the wrong principal. Treat the cache
entry as valid only if both updatedAt is fresh AND cached.getUserId() matches
the current userId (otherwise force reload + overwrite). A regression test for
the recreate scenario would help prevent future leaks.
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -652,93 +694,199 @@ private boolean authorizeByJcasbin(
}
}
- private static UserEntity getUserEntity(String username, String metalake)
throws IOException {
+ //
---------------------------------------------------------------------------
+ // User info / ownership helpers
+ //
---------------------------------------------------------------------------
+
+ /**
+ * Per-request {@link UserUpdatedAt} lookup. The underlying {@code
user_meta} query is issued at
+ * most once per (metalake, username) within a single request.
+ */
+ private Optional<UserUpdatedAt> loadUserInfo(
+ String metalake, String username, AuthorizationRequestContext
requestContext) {
+ String cacheKey = JcasbinAuthorizationCacheKeys.userRoleKey(metalake,
username);
+ return requestContext.computeUserInfoIfAbsent(
+ cacheKey,
+ k ->
+ Optional.ofNullable(
+ SessionUtils.getWithoutCommit(
+ UserMetaMapper.class, m -> m.getUserUpdatedAt(metalake,
username))));
+ }
+
+ /**
+ * Returns true when the cached owner type and ID match the given principal
or one of the
+ * principal's groups. The user id is resolved via the version-validated
{@link #loadUserInfo}
+ * cache so back-to-back ownership checks in the same request do not
re-query {@code user_meta}.
+ */
+ private boolean ownerMatchesUserOrGroups(
+ Optional<OwnerInfo> owner,
+ Principal principal,
+ String metalake,
+ AuthorizationRequestContext requestContext) {
+ if (!owner.isPresent()) {
+ return false;
+ }
+ OwnerInfo ownerInfo = owner.get();
+ if
(Entity.EntityType.USER.name().equalsIgnoreCase(ownerInfo.getOwnerType())) {
+ Optional<UserUpdatedAt> userInfo =
+ loadUserInfo(metalake, principal.getName(), requestContext);
+ return userInfo.isPresent() && userInfo.get().getUserId() ==
ownerInfo.getOwnerId();
+ }
+ if
(!Entity.EntityType.GROUP.name().equalsIgnoreCase(ownerInfo.getOwnerType())) {
+ return false;
+ }
EntityStore entityStore = GravitinoEnv.getInstance().entityStore();
- UserEntity userEntity =
- entityStore.get(
- NameIdentifierUtil.ofUser(metalake, username),
- Entity.EntityType.USER,
- UserEntity.class);
- return userEntity;
+ for (GroupEntity groupEntity : resolveCurrentUserGroups(metalake,
entityStore)) {
+ if (Objects.equals(groupEntity.id(), ownerInfo.getOwnerId())) {
+ return true;
+ }
+ }
+ return false;
}
+ //
---------------------------------------------------------------------------
+ // 4-step role loading with version validation
+ //
---------------------------------------------------------------------------
+
private void loadRolePrivilege(
- String metalake, String username, Long userId,
AuthorizationRequestContext requestContext) {
+ String metalake,
+ String username,
+ long userId,
+ UserUpdatedAt userInfo,
+ AuthorizationRequestContext requestContext) {
requestContext.loadRole(
() -> {
- EntityStore entityStore = GravitinoEnv.getInstance().entityStore();
- NameIdentifier userNameIdentifier =
NameIdentifierUtil.ofUser(metalake, username);
- List<RoleEntity> entities;
- try {
- entities =
- entityStore
- .relationOperations()
- .listEntitiesByRelation(
- SupportsRelationOperations.Type.ROLE_USER_REL,
- userNameIdentifier,
- Entity.EntityType.USER);
- List<CompletableFuture<Void>> loadRoleFutures = new ArrayList<>();
- Set<String> desiredRoleIds = new HashSet<>();
- for (RoleEntity role : entities) {
- desiredRoleIds.add(String.valueOf(role.id()));
- addRoleForUserAndLoadPolicies(
- userId,
- metalake,
- role.id(),
- role.name(),
- loadRoleFutures,
- entityStore,
- requestContext);
- }
+ // Step 1a: version-validated user-direct roles via cache.
+ List<Long> userDirectRoleIds = loadUserRoles(metalake, username,
userId, userInfo);
+
+ // Step 1b: version-validated group-inherited roles via cache. Group
membership comes
+ // from the IdP-pushed UserPrincipal; for each group we load its
roles via the same
+ // version-validated path as users (group_meta.updated_at as the
staleness sentinel).
+ List<Long> groupInheritedRoleIds = new ArrayList<>();
+ for (String groupname : currentPrincipalGroupNames()) {
+ groupInheritedRoleIds.addAll(
+ loadGroupRoles(metalake, groupname, userId, requestContext));
+ }
- // 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;
- }
- for (int i = 0; i < roleIds.size(); i++) {
- desiredRoleIds.add(String.valueOf(roleIds.get(i)));
- addRoleForUserAndLoadPolicies(
- userId,
- metalake,
- roleIds.get(i),
- roleNames.get(i),
- loadRoleFutures,
- entityStore,
- requestContext);
- }
+ // Prune stale g-rows: any role currently bound but no longer in the
desired
+ // set (e.g. user removed from a group at the IdP, or role
unassigned).
+ Set<String> desiredRoleIds = new HashSet<>();
+ for (Long id : userDirectRoleIds) {
+ desiredRoleIds.add(String.valueOf(id));
+ }
+ for (Long id : groupInheritedRoleIds) {
+ desiredRoleIds.add(String.valueOf(id));
+ }
+ String userIdStr = String.valueOf(userId);
+ for (String currentRole : allowEnforcer.getRolesForUser(userIdStr)) {
+ if (!desiredRoleIds.contains(currentRole)) {
+ allowEnforcer.deleteRoleForUser(userIdStr, currentRole);
+ denyEnforcer.deleteRoleForUser(userIdStr, currentRole);
}
+ }
- CompletableFuture.allOf(loadRoleFutures.toArray(new
CompletableFuture[0])).join();
-
- // Prune stale g-rows: remove role mappings that are no longer
valid
- // (e.g. user was removed from a group at the IdP level).
- String userIdStr = String.valueOf(userId);
- for (String currentRole :
allowEnforcer.getRolesForUser(userIdStr)) {
- if (!desiredRoleIds.contains(currentRole)) {
- allowEnforcer.deleteRoleForUser(userIdStr, currentRole);
- denyEnforcer.deleteRoleForUser(userIdStr, currentRole);
- }
- }
- } catch (IOException e) {
- throw new RuntimeException(e);
+ // Step 3: batch version-check all role IDs (direct +
group-inherited),
+ // load stale ones (1 query for the version probe).
+ List<Long> allRoleIds = new ArrayList<>(userDirectRoleIds);
+ allRoleIds.addAll(groupInheritedRoleIds);
+ if (!allRoleIds.isEmpty()) {
+ versionCheckAndLoadRoles(metalake, allRoleIds, requestContext);
}
});
}
+ private List<Long> loadUserRoles(
+ String metalake, String username, long userId, UserUpdatedAt userInfo) {
+ String userCacheKey = JcasbinAuthorizationCacheKeys.userRoleKey(metalake,
username);
+ Optional<CachedUserRoles> cachedOpt =
userRoleCache.getIfPresent(userCacheKey);
+
+ if (cachedOpt.isPresent() && cachedOpt.get().getUpdatedAt() >=
userInfo.getUpdatedAt()) {
+ // Cache is still valid
+ CachedUserRoles cached = cachedOpt.get();
+ bindUserRoles(userId, cached.getRoleIds());
+ return cached.getRoleIds();
+ }
+
+ // Cache miss or stale — reload from DB
+ List<RolePO> rolePOs =
+ SessionUtils.getWithoutCommit(RoleMetaMapper.class, m ->
m.listRolesByUserId(userId));
+ List<Long> roleIds =
rolePOs.stream().map(RolePO::getRoleId).collect(Collectors.toList());
+
+ userRoleCache.put(userCacheKey, new CachedUserRoles(userId,
userInfo.getUpdatedAt(), roleIds));
+ bindUserRoles(userId, roleIds);
+ return roleIds;
+ }
+
+ /**
+ * Per-request {@link GroupUpdatedAt} lookup, mirroring {@link
#loadUserInfo}. The {@code
+ * group_meta} probe runs at most once per (metalake, groupname) within a
single request.
+ */
+ private Optional<GroupUpdatedAt> loadGroupInfo(
+ String metalake, String groupname, AuthorizationRequestContext
requestContext) {
+ String cacheKey = JcasbinAuthorizationCacheKeys.groupRoleKey(metalake,
groupname);
+ return requestContext.computeGroupInfoIfAbsent(
+ cacheKey,
+ k ->
+ Optional.ofNullable(
+ SessionUtils.getWithoutCommit(
+ GroupMetaMapper.class, m -> m.getGroupUpdatedAt(metalake,
groupname))));
+ }
+
+ /**
+ * Version-validated group-role load, mirroring {@link #loadUserRoles}. Uses
{@code
+ * group_meta.updated_at} as the staleness sentinel: if the cached snapshot
is at least as fresh
+ * as the DB version, we reuse it; otherwise we reload from {@code
role_meta}. In both cases the
+ * resulting role IDs are bound to the user's jcasbin g-rows so that the
enforcer sees inherited
+ * privileges. Groups missing from the DB return an empty list.
+ */
+ private List<Long> loadGroupRoles(
+ String metalake, String groupname, long userId,
AuthorizationRequestContext requestContext) {
+ Optional<GroupUpdatedAt> groupInfoOpt = loadGroupInfo(metalake, groupname,
requestContext);
+ if (!groupInfoOpt.isPresent()) {
+ return new ArrayList<>();
+ }
+ GroupUpdatedAt groupInfo = groupInfoOpt.get();
+ long groupId = groupInfo.getGroupId();
+ String groupCacheKey =
JcasbinAuthorizationCacheKeys.groupRoleKey(metalake, groupname);
+ Optional<CachedGroupRoles> cachedOpt =
groupRoleCache.getIfPresent(groupCacheKey);
+
+ if (cachedOpt.isPresent() && cachedOpt.get().getUpdatedAt() >=
groupInfo.getUpdatedAt()) {
+ CachedGroupRoles cached = cachedOpt.get();
+ bindUserRoles(userId, cached.getRoleIds());
+ return cached.getRoleIds();
Review Comment:
Cache validity check for groupRoleCache only compares updatedAt. Since
group_meta.updated_at can be 0 on insert, a group delete-and-recreate with the
same name (new group_id, updated_at=0) could incorrectly reuse an older
CachedGroupRoles snapshot and bind stale roleIds, granting inherited privileges
from the prior group. Treat the cache entry as valid only if both updatedAt is
fresh AND cached.getGroupId() matches the current groupId; otherwise reload
roles and overwrite the cache. Adding a test for group recreate would guard
against this.
--
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]
