Copilot commented on code in PR #10996: URL: https://github.com/apache/gravitino/pull/10996#discussion_r3279254067
########## server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/CachedGroupRoles.java: ########## @@ -0,0 +1,37 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.server.authorization.jcasbin; + +import java.util.List; +import lombok.AllArgsConstructor; +import lombok.Getter; + +/** + * Cached snapshot of a group's role assignments. The {@code updatedAt} timestamp corresponds to the + * {@code group_meta.updated_at} column and is used as a version sentinel: if the DB value is newer, + * the cached role list is stale and must be reloaded. + */ +@Getter +@AllArgsConstructor +public class CachedGroupRoles { Review Comment: CachedGroupRoles is only referenced inside the jcasbin package (JcasbinAuthorizer + same-package tests). Making it public increases the exposed API surface without an apparent need; consider reducing visibility to package-private to keep these cache snapshot details internal. ########## server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java: ########## @@ -652,93 +694,202 @@ 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(); Review Comment: userRoleCache validity check only compares cached.updatedAt against user_meta.updated_at, but does not verify the cached snapshot belongs to the current userId. If a user is deleted and recreated with the same username (new user_id) and a low/zero updated_at (default is 0 on insert), the old CachedUserRoles entry can be treated as valid and bind the previous user's roles to the new user, which is a privilege-escalation risk. Treat the cache entry as valid only when both userId and updatedAt match the current UserUpdatedAt (similar to the group cache’s groupId check), and add a regression test for delete/recreate username reuse. ########## server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/CachedUserRoles.java: ########## @@ -0,0 +1,37 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.server.authorization.jcasbin; + +import java.util.List; +import lombok.AllArgsConstructor; +import lombok.Getter; + +/** + * Cached snapshot of a user's direct role assignments. The {@code updatedAt} timestamp corresponds + * to the {@code user_meta.updated_at} column and is used as a version sentinel: if the DB value is + * newer, the cached role list is stale and must be reloaded. + */ +@Getter +@AllArgsConstructor +public class CachedUserRoles { Review Comment: CachedUserRoles appears to be an internal helper type only referenced within the jcasbin package (used as a cache value in JcasbinAuthorizer and in same-package tests). Keeping it public unnecessarily expands the server-common jar’s exported API surface; consider making the class package-private to signal it’s not a supported external API. -- 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]
