Copilot commented on code in PR #10930:
URL: https://github.com/apache/gravitino/pull/10930#discussion_r3168079982
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -482,36 +528,40 @@ private void loadRolePrivilege(
SupportsRelationOperations.Type.ROLE_USER_REL,
userNameIdentifier,
Entity.EntityType.USER);
+ Set<Long> roleIds = new HashSet<>(entities.size());
List<CompletableFuture<Void>> loadRoleFutures = new ArrayList<>();
for (RoleEntity role : entities) {
Long roleId = role.id();
+ roleIds.add(roleId);
allowEnforcer.addRoleForUser(String.valueOf(userId),
String.valueOf(roleId));
denyEnforcer.addRoleForUser(String.valueOf(userId),
String.valueOf(roleId));
if (loadedRoles.getIfPresent(roleId) != null) {
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);
+ CompletableFuture.runAsync(
+ () -> {
+ loadedRoles.get(
+ roleId,
+ unused -> {
+ try {
+ RoleEntity roleEntity =
+ entityStore.get(
+ NameIdentifierUtil.ofRole(metalake,
role.name()),
+ Entity.EntityType.ROLE,
+ RoleEntity.class);
+ return loadPolicyByRoleEntity(roleEntity);
+ } catch (Exception e) {
Review Comment:
In `loadRolePrivilege`, the role load path wraps
`entityStore.get(...)`/`loadPolicyByRoleEntity(...)` in a broad `catch
(Exception e)` and rethrows a `RuntimeException`. This makes it harder to
reason about which failures are expected (e.g., IOException) and violates the
project guideline to avoid generic exception catches. Catch the specific
checked exceptions this code can throw (and let unchecked exceptions propagate)
so error handling is more precise and debuggable.
```suggestion
} catch (IOException e) {
```
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -440,20 +447,59 @@ private boolean loadPrivilegeAndAuthorize(
return false;
}
loadRolePrivilege(metalake, username, userId, requestContext);
- return authorizeByJcasbin(userId, metadataObject, metadataId, privilege);
+ return authorizeByIndex(userId, metadataObject, metadataId, privilege,
requestContext);
}
- private boolean authorizeByJcasbin(
- Long userId, MetadataObject metadataObject, Long metadataId, String
privilege) {
+ /**
+ * Resolve a single privilege probe against the per-role policy index.
Replaces the previous
+ * {@code enforcer.enforce} call, which scanned every policy line in the
enforcer for each
+ * probe. Per-request cost goes from {@code O(total_policies)} to {@code
O(roles_per_user)} hash
+ * probes.
+ */
+ private boolean authorizeByIndex(
+ Long userId,
+ MetadataObject metadataObject,
+ Long metadataId,
+ String privilege,
+ AuthorizationRequestContext requestContext) {
if (AuthConstants.OWNER.equals(privilege)) {
Optional<Long> owner = ownerRel.getIfPresent(metadataId);
return Objects.equals(Optional.of(userId), owner);
}
- return enforcer.enforce(
- String.valueOf(userId),
- String.valueOf(metadataObject.type()),
- String.valueOf(metadataId),
- privilege);
+ Set<Long> roleIds = requestContext.getUserRoleIds();
+ if (roleIds.isEmpty()) {
+ return false;
+ }
+ PolicyKey key = new PolicyKey(metadataObject.type().name(), metadataId,
privilege);
Review Comment:
`authorizeByIndex` builds the `PolicyKey` using the raw `privilege` string
from the request, but `loadPolicyByRoleEntity` normalizes stored policy
privilege names via
`AuthorizationUtils.replaceLegacyPrivilegeName(...).name().toUpperCase(Locale.ROOT)`.
This can cause lookups to fail for deprecated/legacy privilege names (e.g.,
CREATE_MODEL vs REGISTER_MODEL) even though they are intended to be equivalent.
Normalize the request privilege the same way before constructing the
`PolicyKey` (excluding the special OWNER path) so legacy privilege probes match
the indexed policies.
```suggestion
String normalizedPrivilege =
AuthorizationUtils.replaceLegacyPrivilegeName(Privilege.Name.valueOf(privilege))
.name()
.toUpperCase(Locale.ROOT);
PolicyKey key =
new PolicyKey(metadataObject.type().name(), metadataId,
normalizedPrivilege);
```
--
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]
