yuqi1129 commented on code in PR #13699:
URL: https://github.com/apache/gravitino/pull/13699#discussion_r4231003134
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizationLookups.java:
##########
@@ -131,11 +97,85 @@ public Optional<OwnerInfo> resolveOwnerId(
id -> ownerRelCache.get(id, k -> loadOwner(k, metadataType)));
}
+ // Preserve normalization failures as distinct from missing objects for
DENY-policy consumers.
+ MetadataIdResolution resolveMetadataIdResult(
+ MetadataObject metadataObject, String metalake,
AuthorizationRequestContext requestContext) {
+ MetadataObject cacheObject;
+ try {
+ // Use the same capability rules as ID resolution so hooks and peer
change-log replay
+ // evict every equivalent spelling from both cache tiers.
+ cacheObject =
+ requestContext.computeNormalizedMetadataObjectIfAbsent(
+ JcasbinAuthorizationCacheKeys.metadataIdCacheKey(metalake,
metadataObject),
+ ignored ->
MetadataIdConverter.normalizeMetadataObject(metadataObject, metalake));
+ } catch (NotFoundException e) {
+ return new MetadataIdResolution(Optional.empty(), false);
+ } catch (RuntimeException e) {
+ // Never fall back to the raw key: it can retain an ID after
canonical-name invalidation.
+ // Catch only normalization failures; entity-store and cache-loader
failures still propagate.
+ // Failures are not memoized, so a transient one can succeed on retry in
the same request.
+ // The cost: while a catalog cannot initialize, every object of that
catalog looked up in a
+ // request reloads it (connector initialization included) and logs this
warning again. A
+ // per-request, per-catalog failure memo or a once-per-request warning
would bound that.
+ LOG.warn(
+ "Cannot normalize metadata object {}:{} in metalake {};
authorization lookup is unresolved",
+ metadataObject.type(),
+ metadataObject.fullName(),
+ metalake,
+ e);
+ return new MetadataIdResolution(Optional.empty(), true);
Review Comment:
Fixed in 9b680b41a0. Capability failures are now remembered per (metalake,
catalog) for the lifetime of the request. Normalization is serialized per
catalog, so concurrent lookups of different objects also make only one failed
attempt and emit one normalization WARN. The next request can retry. Catalog-ID
lookups remain available to install the conservative DENY guard, and
entity-store failures still propagate.
Tests cover persistent failure across repeated and distinct objects,
concurrent failure deduplication, healthy-catalog isolation, catalog-ID
availability, and recovery in the next request. Disabling failure memory makes
the concurrent regression fail (two attempts instead of one). Updated the
request-retry contract and PR description.
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -1861,37 +1869,91 @@ private ResolvedRolePolicies resolveRolePolicies(
List<String> unresolvedObjects = new ArrayList<>();
for (SecurableObject securableObject : securableObjects) {
- Optional<Long> metadataId =
- lookups.resolveMetadataId(securableObject, metalake, requestContext);
+ JcasbinAuthorizationLookups.MetadataIdResolution resolution =
+ lookups.resolveMetadataIdResult(securableObject, metalake,
requestContext);
+ Optional<Long> metadataId = resolution.metadataId();
if (!metadataId.isPresent()) {
unresolvedObjects.add(securableObject.type().name() + ":" +
securableObject.fullName());
+ if (resolution.normalizationFailed()) {
+ addUnresolvedDenyPolicies(
+ securableObject, metalake, roleIdStr, requestContext, allowRows,
denyRows);
+ }
continue;
}
- String metadataIdStr = String.valueOf(metadataId.get());
- for (Privilege privilege : securableObject.privileges()) {
- Privilege.Condition condition = privilege.condition();
- String action =
- AuthorizationUtils.replaceLegacyPrivilegeName(privilege.name())
- .name()
- .toUpperCase(Locale.ROOT);
- if (AuthConstants.DENY.equalsIgnoreCase(condition.name())) {
- denyRows.add(
- new String[] {
- roleIdStr, securableObject.type().name(), metadataIdStr,
action, AuthConstants.ALLOW
- });
- }
+ addPolicyRows(
+ roleIdStr,
+ securableObject.type(),
+ metadataId.get(),
+ securableObject.privileges(),
+ allowRows,
+ denyRows);
+ }
+ return new ResolvedRolePolicies(allowRows, denyRows, unresolvedObjects);
+ }
- allowRows.add(
+ private void addUnresolvedDenyPolicies(
+ SecurableObject object,
+ String metalake,
+ String roleId,
+ AuthorizationRequestContext requestContext,
+ List<String[]> allowRows,
+ List<String[]> denyRows) {
+ List<Privilege> denies =
+ object.privileges().stream()
+ .filter(privilege -> privilege.condition() ==
Privilege.Condition.DENY)
+ .collect(Collectors.toList());
+ if (denies.isEmpty()) {
+ return;
+ }
+ // An unresolved child DENY must not disappear while an ancestor or
another role grants
+ // ALLOW. Conservatively deny the same privileges throughout its catalog
until the existing
+ // partial-role retry resolves the precise object again. Catalog IDs need
no capability lookup,
+ // and other catalogs remain usable. A missing entity alone does not
broaden existing policies.
+ MetadataObject catalog =
+ NameIdentifierUtil.toMetadataObject(
+ NameIdentifierUtil.getCatalogIdentifier(
+ MetadataObjectUtil.toEntityIdent(metalake, object)),
+ Entity.EntityType.CATALOG);
+ Optional<Long> catalogId = lookups.resolveMetadataId(catalog, metalake,
requestContext);
+ if (!catalogId.isPresent()) {
+ // Without a catalog ID even a catalog-scoped guard cannot be installed
safely. A missing
+ // catalog makes normalization report a missing object instead, so this
is only reachable
+ // when the catalog is dropped between the two lookups. Skipping the
DENY could drop it for
+ // a catalog recreated under the same name, so fail this load; the next
request retries.
+ throw new IllegalStateException(
+ "Cannot resolve catalog for unresolved deny policy on " +
object.fullName());
Review Comment:
Fixed in 9b680b41a0. If the catalog ID required for the DENY guard cannot be
resolved, role-policy resolution now returns an unavailable result. The caller
adds that role to unreadableRoleIds and skips publishing its policies, so
initial loading and later reloading both fail closed through the existing
channel rather than throwing this IllegalStateException.
The regression verifies denied authorization despite an ancestor ALLOW, the
conservative hasDenyPolicy result, no published policy rows or loaded marker,
and retry in a fresh request. Dropping this failure handling makes the
regression fail. The PR description now documents this path.
--
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]
