Copilot commented on code in PR #11404:
URL: https://github.com/apache/gravitino/pull/11404#discussion_r3347025592
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -889,9 +890,9 @@ private boolean ownerMatchesUserOrGroups(
if
(!Entity.EntityType.GROUP.name().equalsIgnoreCase(ownerInfo.getOwnerType())) {
return false;
}
- EntityStore entityStore = GravitinoEnv.getInstance().entityStore();
- for (GroupEntity groupEntity : resolveCurrentUserGroups(metalake,
entityStore)) {
- if (Objects.equals(groupEntity.id(), ownerInfo.getOwnerId())) {
+ for (String groupname : currentPrincipalGroupNames()) {
+ Optional<GroupUpdatedAt> groupInfo = loadGroupInfo(metalake, groupname,
requestContext);
+ if (groupInfo.isPresent() && groupInfo.get().getGroupId() ==
ownerInfo.getOwnerId()) {
return true;
}
Review Comment:
ownerMatchesUserOrGroups() uses the passed-in Principal for USER owner
checks, but for GROUP owner checks it ignores that parameter and instead reads
PrincipalUtils.getCurrentPrincipal() via currentPrincipalGroupNames(). If
isOwner/authorize is ever invoked with an explicit principal that differs from
the thread-local current principal, GROUP ownership checks can return incorrect
results. Use the provided `principal` parameter to derive group names instead
of consulting global state.
--
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]
