Copilot commented on code in PR #10996:
URL: https://github.com/apache/gravitino/pull/10996#discussion_r3273543250
##########
server-common/build.gradle.kts:
##########
@@ -25,6 +25,9 @@ plugins {
}
dependencies {
+ annotationProcessor(libs.lombok)
+ compileOnly(libs.lombok)
+
Review Comment:
Adding Lombok to server-common introduces a new compile-time dependency for
just two small value classes. Consider replacing the Lombok-based POJOs with
explicit fields/getters/constructors (or records, if allowed) so this module
doesn’t need Lombok at all.
##########
server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizer.java:
##########
@@ -846,17 +875,34 @@ private static void restoreDefaultPrincipal() {
.thenReturn(new UserPrincipal(USERNAME));
}
- /** Mocks the user as having no direct (ROLE_USER_REL) role assignments. */
+ /**
+ * Mocks the user as having no directly-assigned roles. Bumps
userMetaMapper.getUserUpdatedAt to
+ * force the userRoleCache to miss and re-read from the (empty)
roleMetaMapper.listRolesByUserId.
+ */
private static void mockNoDirectUserRoles() throws IOException {
- NameIdentifier userNameIdentifier = NameIdentifierUtil.ofUser(METALAKE,
USERNAME);
- when(supportsRelationOperations.listEntitiesByRelation(
- eq(SupportsRelationOperations.Type.ROLE_USER_REL),
- eq(userNameIdentifier),
- eq(Entity.EntityType.USER)))
- .thenReturn(ImmutableList.of());
+
when(roleMetaMapper.listRolesByUserId(eq(USER_ID))).thenReturn(ImmutableList.of());
+ when(userMetaMapper.getUserUpdatedAt(eq(METALAKE), eq(USERNAME)))
+ .thenReturn(new UserUpdatedAt(USER_ID, System.currentTimeMillis()));
}
- /** Builds a {@link RoleEntity} and registers it in the mocked entity store.
*/
+ /**
+ * Mocks the user as having the given roles directly assigned via the
version-validated cache
+ * path. Bumps {@code userMetaMapper.getUserUpdatedAt} to force a
userRoleCache miss.
+ */
+ private static void mockDirectUserRoles(RoleEntity... roles) {
+ List<RolePO> rolePOs = new ArrayList<>();
+ for (RoleEntity role : roles) {
+ rolePOs.add(buildRolePO(role.id(), role.name()));
+ }
+ when(roleMetaMapper.listRolesByUserId(eq(USER_ID))).thenReturn(rolePOs);
+ when(userMetaMapper.getUserUpdatedAt(eq(METALAKE), eq(USERNAME)))
+ .thenReturn(new UserUpdatedAt(USER_ID, System.currentTimeMillis()));
+ }
Review Comment:
These test helpers rely on System.currentTimeMillis() to “bump”
user_meta.updated_at, but calls can happen within the same millisecond, causing
the version not to advance and the userRoleCache to remain valid. That can make
tests flaky/non-deterministic. Prefer a monotonic counter (similar to
groupVersionCounter) or explicitly incrementing timestamps per call.
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -72,7 +73,39 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-/** The Jcasbin implementation of GravitinoAuthorizer. */
+/**
+ * The Jcasbin implementation of {@link GravitinoAuthorizer}.
+ *
+ * <h2>Cache architecture</h2>
+ *
+ * <p>Authorization decisions are read-mostly and run on the hot path, so this
class layers three
+ * cache families with different consistency models:
+ *
+ * <ol>
+ * <li><b>Per-request dedup</b> — fields on {@link
AuthorizationRequestContext} (user info, group
+ * info, name→id, owner). A fresh context is created for every HTTP
request; every underlying
+ * DB query runs at most once per request even when the same
authorize/isOwner pair is
+ * evaluated repeatedly for a single authorization expression.
+ * <li><b>Version-validated shared caches</b> (strong consistency) — {@link
#userRoleCache},
+ * {@link #groupRoleCache}, {@link #loadedRoles}. Each cached entry
carries the {@code
+ * *_meta.updated_at} value it was loaded against; every read issues a
lightweight version
+ * probe and discards the entry if the DB sentinel has advanced. No TTL
is relied on for
+ * correctness — {@code expireAfterAccess} only bounds memory.
Review Comment:
The class Javadoc says the strong-consistency caches rely on
“expireAfterAccess” only to bound memory, but userRoleCache/groupRoleCache are
CaffeineGravitinoCache instances which expireAfterWrite. Please update the
documentation to match the actual expiration policy (or clarify that TTL type
differs per cache).
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinLoadedRolesCache.java:
##########
@@ -0,0 +1,93 @@
+/*
+ * 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 com.github.benmanes.caffeine.cache.Cache;
+import com.github.benmanes.caffeine.cache.Caffeine;
+import java.util.Optional;
+import java.util.concurrent.TimeUnit;
+import org.apache.gravitino.cache.GravitinoCache;
+import org.casbin.jcasbin.main.Enforcer;
+
+/**
+ * A {@link GravitinoCache} of {@code roleId -> updated_at} that synchronously
deletes the role's
+ * JCasbin policies from both enforcers when a key is evicted (by TTL, size,
or explicit
+ * invalidate).
+ *
+ * <p>Uses a raw Caffeine cache internally so it can attach a removal listener
with {@code
+ * executor(Runnable::run)} — eviction and policy cleanup must happen on the
same thread, so the
+ * {@link JcasbinAuthorizer} never sees a role bound in the enforcer without a
backing policy.
+ */
+class JcasbinLoadedRolesCache implements GravitinoCache<Long, Long> {
+
+ private final Cache<Long, Long> cache;
+
+ JcasbinLoadedRolesCache(long ttlMs, long maxSize, Enforcer allowEnforcer,
Enforcer denyEnforcer) {
+ this.cache =
+ Caffeine.newBuilder()
+ .expireAfterAccess(ttlMs, TimeUnit.MILLISECONDS)
+ .maximumSize(maxSize)
+ .executor(Runnable::run)
+ .removalListener(
+ (roleId, value, cause) -> {
+ if (roleId != null) {
+ allowEnforcer.deleteRole(String.valueOf(roleId));
+ denyEnforcer.deleteRole(String.valueOf(roleId));
+ }
+ })
+ .build();
+ }
+
+ @Override
+ public Optional<Long> getIfPresent(Long key) {
+ return Optional.ofNullable(cache.getIfPresent(key));
+ }
+
+ @Override
+ public void put(Long key, Long value) {
+ cache.put(key, value);
+ }
+
+ @Override
+ public void invalidate(Long key) {
+ cache.invalidate(key);
+ }
+
+ @Override
+ public void invalidateAll() {
+ cache.invalidateAll();
+ }
+
+ @Override
+ public void invalidateByPrefix(String prefix) {
+ cache.asMap().keySet().removeIf(k -> k.toString().startsWith(prefix));
Review Comment:
GravitinoCache.invalidateByPrefix is documented as only meaningful when the
key type is String. This cache’s key type is Long, so implementing prefix
invalidation via k.toString().startsWith(prefix) is surprising and could remove
unintended entries if it’s ever called. Prefer a no-op implementation (or
delegate to invalidateAll / throw UnsupportedOperationException) to match the
contract.
##########
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 {
+
+ private final long userId;
+ private final long updatedAt;
+ private final List<Long> roleIds;
+}
Review Comment:
CachedUserRoles is only referenced within the jcasbin package (and its
tests). Making it public unnecessarily expands the server-common API surface;
consider making it package-private (and possibly final) if it’s meant to be an
internal helper.
##########
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 {
+
+ private final long groupId;
+ private final long updatedAt;
+ private final List<Long> roleIds;
+}
Review Comment:
CachedGroupRoles is only referenced within the jcasbin package (and its
tests). Making it public unnecessarily expands the server-common API surface;
consider making it package-private (and possibly final) if it’s meant to be an
internal helper.
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -557,86 +583,193 @@ 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 current user or
one of the user's
+ * groups.
+ */
+ private boolean ownerMatchesUserOrGroups(
+ Optional<OwnerInfo> owner, long userId, String metalake) {
+ if (!owner.isPresent()) {
+ return false;
+ }
+ OwnerInfo ownerInfo = owner.get();
+ if
(Entity.EntityType.USER.name().equalsIgnoreCase(ownerInfo.getOwnerType())) {
+ return ownerInfo.getOwnerId() == userId;
+ }
+ 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);
- }
+ // 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);
- }
+ // 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();
+ }
+
+ List<RolePO> rolePOs =
+ SessionUtils.getWithoutCommit(RoleMetaMapper.class, m ->
m.listRolesByGroupId(groupId));
+ List<Long> roleIds =
rolePOs.stream().map(RolePO::getRoleId).collect(Collectors.toList());
+
+ groupRoleCache.put(
+ groupCacheKey, new CachedGroupRoles(groupId, groupInfo.getUpdatedAt(),
roleIds));
+ bindUserRoles(userId, roleIds);
+ return roleIds;
+ }
+
+ /**
+ * Returns the current principal's group names as carried by the IdP-pushed
{@link UserPrincipal}.
+ * Returns an empty list when the principal is not a {@link UserPrincipal}
(e.g. service tokens)
+ * or has no groups.
+ */
+ private List<String> currentPrincipalGroupNames() {
+ Principal principal = PrincipalUtils.getCurrentPrincipal();
+ if (!(principal instanceof UserPrincipal)) {
+ return new ArrayList<>();
+ }
+ List<UserGroup> groups = ((UserPrincipal) principal).getGroups();
+ if (groups.isEmpty()) {
+ return new ArrayList<>();
+ }
+ return
groups.stream().map(UserGroup::getGroupname).collect(Collectors.toList());
+ }
+
/**
* Resolves GroupEntity objects for the current principal's groups, skipping
any that are stale or
- * not found in the store.
+ * not found in the store. Used by both {@link #isSelf} (ROLE branch) and
{@link
+ * #loadRolePrivilege} to discover group-inherited role assignments.
Review Comment:
This method’s Javadoc claims it is used by loadRolePrivilege, but
loadRolePrivilege now uses currentPrincipalGroupNames() + loadGroupRoles() and
never calls resolveCurrentUserGroups(). Please correct the Javadoc to avoid
misleading future readers.
--
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]
