Copilot commented on code in PR #10279:
URL: https://github.com/apache/gravitino/pull/10279#discussion_r2894173459
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -209,9 +218,15 @@ public boolean isOwner(
String metalake,
MetadataObject metadataObject,
AuthorizationRequestContext requestContext) {
+ Long userId;
boolean result;
try {
- result = checkOwner(metalake, metadataObject, principal.getName());
+ Long metadataId = MetadataIdConverter.getID(metadataObject, metalake);
+ loadOwnerPolicy(metalake, metadataObject, metadataId);
+ UserEntity userEntity = getUserEntity(principal.getName(), metalake);
+ userId = userEntity.id();
+ metadataId = MetadataIdConverter.getID(metadataObject, metalake);
Review Comment:
`isOwner` recomputes `metadataId` twice inside the try block. This is
redundant and can be simplified by computing it once and reusing it for both
`loadOwnerPolicy` and the cache lookup.
```suggestion
```
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -506,19 +533,19 @@ private boolean checkOwner(String metalake,
MetadataObject metadataObject, Strin
SupportsRelationOperations.Type.OWNER_REL,
entityIdent,
Entity.EntityType.valueOf(metadataObject.type().name()));
-
- for (Entity ownerEntity : owners) {
- if (ownerEntity instanceof UserEntity) {
- UserEntity user = (UserEntity) ownerEntity;
- if (user.name().equals(userName)) {
- return true;
+ if (owners.isEmpty()) {
+ ownerRel.put(metadataId, Optional.empty());
+ } else {
+ for (Entity ownerEntity : owners) {
+ if (ownerEntity instanceof UserEntity) {
+ UserEntity user = (UserEntity) ownerEntity;
+ ownerRel.put(metadataId, Optional.of(user.id()));
}
}
Review Comment:
`loadOwnerPolicy` only populates the cache when the OWNER relation resolves
to a `UserEntity`. If the relation exists but is a non-user (e.g., group owner)
the cache remains empty and every `isOwner` call will re-query the backend.
Consider explicitly handling group owners (or at least caching
`Optional.empty()` when no `UserEntity` owner is found) to avoid repeated loads
and make behavior deterministic.
```suggestion
} else {
boolean userOwnerFound = false;
for (Entity ownerEntity : owners) {
if (ownerEntity instanceof UserEntity) {
UserEntity user = (UserEntity) ownerEntity;
ownerRel.put(metadataId, Optional.of(user.id()));
userOwnerFound = true;
break;
}
}
if (!userOwnerFound) {
// No UserEntity owner found; cache empty to avoid repeated
backend lookups
ownerRel.put(metadataId, Optional.empty());
}
```
##########
core/src/main/java/org/apache/gravitino/authorization/GravitinoAuthorizer.java:
##########
@@ -152,4 +152,16 @@ default void handleRolePrivilegeChange(String metalake,
String roleName) {
throw new RuntimeException("Can not get Role Entity", e);
}
}
+
+ /**
+ * This method is called to clear the owner relationship in jcasbin when the
owner of the metadata
+ * changes.
+ *
+ * @param metalake metalake;
+ * @param oldOwnerId The old owner id;
+ * @param nameIdentifier The metadata name identifier;
+ * @param type entity type
+ */
+ void handleMetadataOwnerChange(
+ String metalake, Long oldOwnerId, NameIdentifier nameIdentifier,
Entity.EntityType type);
Review Comment:
Adding a new abstract method to `GravitinoAuthorizer` is source/binary
incompatible for external implementations. If this interface is intended for
extension outside this repo, consider making `handleMetadataOwnerChange(...)` a
`default` no-op method (similar to `handleRolePrivilegeChange(String, String)`)
to avoid breaking custom authorizers.
```suggestion
default void handleMetadataOwnerChange(
String metalake, Long oldOwnerId, NameIdentifier nameIdentifier,
Entity.EntityType type) {}
```
##########
core/src/main/java/org/apache/gravitino/authorization/OwnerManager.java:
##########
@@ -118,6 +120,7 @@ public void setOwner(
metadataObject,
authorizationPlugin ->
authorizationPlugin.onOwnerSet(metadataObject,
originOwner.orElse(null), newOwner));
+ originOwner.ifPresent(owner -> notifyOwnerChange(owner, metalake,
metadataObject));
Review Comment:
Owner cache invalidation is only triggered when `originOwner` is present. If
the owner was previously unset, `JcasbinAuthorizer` may have cached
`Optional.empty()` for this metadata and will keep returning non-owner until
cache expiry. Consider notifying/invalidation unconditionally after
`insertRelation` (pass `null` for `oldOwnerId` when absent) so ownership
changes take effect immediately.
```suggestion
notifyOwnerChange(originOwner.orElse(null), metalake, metadataObject);
```
##########
core/src/main/java/org/apache/gravitino/authorization/OwnerManager.java:
##########
@@ -133,6 +136,33 @@ public void setOwner(
}
}
+ private void notifyOwnerChange(Owner oldOwner, String metalake,
MetadataObject metadataObject) {
+ GravitinoAuthorizer gravitinoAuthorizer =
GravitinoEnv.getInstance().gravitinoAuthorizer();
+ if (gravitinoAuthorizer != null) {
+ if (oldOwner.type() == Owner.Type.USER) {
+ try {
+ UserEntity userEntity =
+ GravitinoEnv.getInstance()
+ .entityStore()
+ .get(
+ NameIdentifierUtil.ofUser(metalake, oldOwner.name()),
+ Entity.EntityType.USER,
+ UserEntity.class);
+ gravitinoAuthorizer.handleMetadataOwnerChange(
+ metalake,
+ userEntity.id(),
+ MetadataObjectUtil.toEntityIdent(metalake, metadataObject),
+ Entity.EntityType.valueOf(metadataObject.type().name()));
+ } catch (IOException e) {
+ LOG.warn(e.getMessage(), e);
+ }
+ } else {
+ throw new UnsupportedOperationException(
+ "Notification for Group Owner is not supported yet.");
Review Comment:
`notifyOwnerChange` throws `UnsupportedOperationException` when the previous
owner is a GROUP. Since `getOwner()` can return group owners, this can make
`setOwner()` fail when migrating/changing ownership from a group-owned object.
Prefer a non-throwing behavior (e.g., best-effort invalidation or logging +
return) or implement group-owner invalidation.
```suggestion
} else if (oldOwner.type() == Owner.Type.GROUP) {
LOG.warn(
"Skipping owner change notification for group owner '{}' on
metadata object '{}', "
+ "group owner notification is not supported yet.",
oldOwner.name(),
metadataObject.fullName());
```
##########
docs/security/access-control.md:
##########
@@ -446,17 +446,20 @@ To enable access control in Gravitino, configure the
following settings in your
| `gravitino.authorization.serviceAdmins` | Comma-separated
list of service administrator usernames | (none) | Yes
(when authorization is enabled) | 0.5.0 |
| `gravitino.authorization.jcasbin.cacheExpirationSecs` | The expiration
time in seconds for authorization cache entries | `3600` | No
| 1.1.1 |
| `gravitino.authorization.jcasbin.roleCacheSize` | The maximum size
of the role cache for authorization | `10000` | No
| 1.1.1 |
+| `gravitino.authorization.jcasbin.ownerCacheSize` | The maximum size
of the owner cache for authorization | `100000` | No
| 1.1.1 |
### Authorization Cache
-Gravitino uses Caffeine caches to improve authorization performance by caching
role information. The cache configuration options allow you to tune the cache
behavior:
+Gravitino uses Caffeine caches to improve authorization performance by caching
role and owner information. The cache configuration options allow you to tune
the cache behavior:
- **`cacheExpirationSecs`**: Controls how long cache entries remain valid.
After this time, entries are automatically evicted and reloaded from the
backend on the next access. Lower values provide more up-to-date authorization
decisions but may increase load on the backend.
- **`roleCacheSize`**: Controls the maximum number of role entries that can be
cached. When the cache reaches this size, the least recently used entries are
evicted.
+- **`ownerCacheSize`**: Controls the maximum number of owner relationship
entries that can be cached. This cache maps metadata object IDs to their owner
IDs.
+
:::info
-When role privileges are changed through the Gravitino API, the corresponding
cache entries are automatically invalidated to ensure authorization decisions
reflect the latest state.
+When role privileges or ownership are changed through the Gravitino API, the
corresponding cache entries are automatically invalidated to ensure
authorization decisions reflect the latest state.
Review Comment:
Docs state that ownership changes automatically invalidate the corresponding
cache entries, but the current invalidation path in `OwnerManager.setOwner`
only notifies when there was an existing owner. Consider clarifying this
behavior in docs or adjusting the implementation so first-time owner assignment
also triggers invalidation.
```suggestion
When role privileges are changed through the Gravitino API, the
corresponding role cache entries are automatically invalidated to ensure
authorization decisions reflect the latest state. For ownership, cache entries
are automatically invalidated when an existing ownership relationship is
updated or removed; initial owner assignment may rely on normal cache
expiration and reload semantics.
```
--
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]
