yuqi1129 commented on code in PR #12964:
URL: https://github.com/apache/gravitino/pull/12964#discussion_r4026816587
##########
server/src/main/java/org/apache/gravitino/server/web/rest/MetadataObjectPolicyOperations.java:
##########
@@ -237,6 +214,7 @@ public Response listPoliciesForMetadataObject(
}
}
+ @Deprecated
Review Comment:
[P1] Reject retired direct-association writes instead of reporting success
The intentional switch to tag-derived object policies leaves this POST
operational: `@Deprecated` does not change its runtime behavior, and
`associatePoliciesForMetadataObject` still writes `POLICY_METADATA_OBJECT_REL`
and returns HTTP 200. However, `PolicyManager.listPolicyInfosForMetadataObject`
now reads only `ObjectPolicyResolver`, so a successful association is
immediately absent from list/get. Existing clients can silently keep creating
ineffective policy assignments. `GravitinoStrategyProvider` also uses object
list/get to discover maintenance strategies.
I reproduced this with the real H2 store: direct association returns the
policy name, but the immediate object-policy list returns zero policies.
Please make the cutover coherent in this PR. If direct associations are
retired now, reject this mutation explicitly with a migration message, and
establish a migration/verification path for existing relations before switching
reads. If this is still a compatibility window, preserve an explicit compatible
read path. The concern is not the deliberate target-model change itself, but
continuing to acknowledge writes that the runtime no longer uses. Add a
regression test for the old client's associate-then-list/get sequence.
##########
server/src/main/java/org/apache/gravitino/server/web/rest/MetadataObjectPolicyOperations.java:
##########
@@ -179,48 +168,36 @@ public Response listPoliciesForMetadataObject(
MetadataObjects.parse(
fullName,
MetadataObject.Type.valueOf(type.toUpperCase(Locale.ROOT)));
- Set<PolicyDTO> policies = Sets.newHashSet();
- PolicyEntity[] nonInheritedPolicies =
+ PolicyEntity[] policies =
policyDispatcher.listPolicyInfosForMetadataObject(metalake,
object);
Review Comment:
[P2] Avoid disclosing hidden policy names through resolver conflict errors
The visibility filter below only protects successful results. This endpoint
requires `CAN_ACCESS_METADATA`, so a caller may reach it without permission to
view any policy. If a hidden policy has both a matching and a nonmatching
selector across the object's effective tags, `ObjectPolicyResolver` throws
before filtering, with `Policy <name> has conflicting selector results ...`.
`ExceptionHandlers` then includes that message and the original exception stack
in the HTTP 500 response.
A REST-method regression test confirms that the visibility filter is never
called on this path and the error response contains the hidden policy name.
This test exercises the exception boundary; a full authorization-enabled HTTP
test should additionally cover an object-readable but policy-unreadable caller,
for both details values.
Please use a recognizable conflict exception and sanitize its public
response at the REST boundary, including the stack payload, while retaining
details in server logs. Keep the resolver's enforcement conflict behavior
intact rather than ignoring the conflict. This should be fixed in this PR.
##########
core/src/main/java/org/apache/gravitino/policy/PolicyManager.java:
##########
@@ -295,12 +297,9 @@ public RelationalEntity<?>[]
listTagAssociationsForPolicy(String metalake, Strin
@Override
public PolicyEntity[] listPolicyInfosForMetadataObject(
String metalake, MetadataObject metadataObject) {
- NameIdentifier entityIdent = MetadataObjectUtil.toEntityIdent(metalake,
metadataObject);
- Entity.EntityType entityType =
MetadataObjectUtil.toEntityType(metadataObject);
MetadataObjectUtil.checkMetadataObject(metalake, metadataObject);
checkMetalake(NameIdentifier.of(metalake), entityStore);
-
- return listDirectPoliciesForMetadataObject(entityIdent, entityType,
metadataObject);
+ return objectPolicyResolver.resolve(metalake, metadataObject);
Review Comment:
[P1] Preserve tag identity before enabling the resolver on object-policy
reads
This activates a resolver that combines separate reads by tag name rather
than stable entity ID. `ObjectPolicyResolver.resolve` first loads effective
`TagEntity` instances, then builds name identifiers for the policy-relation
query and joins the results back through `relation.source().name()`.
A deterministic reproduction with the real H2 store and an injected
scheduling boundary is:
1. The object is assigned tag T1, and the resolver reads that assignment.
2. Delete T1 and create same-name T2 with a different ID.
3. Associate policy P with T2, without assigning T2 to the object.
4. Resume the policy-relation query: it finds T2 -> P by name and combines
it with T1's old assignment.
The resolver returns P even though neither committed state ever associated P
with this object; the test also confirms the object has no tag assignment after
the replacement. This is a fabricated association, not merely a stale snapshot.
Please query relations using the observed tag IDs, or carry and validate the
source IDs, before enabling this read path. A consistent database snapshot is
another option; process-local locking alone does not cover multiple servers.
Also check target IDs when PolicyTagRelService reloads policy/tag entities by
name, since those relation rows already contain stable IDs. Add a
deletion/recreation regression test that asserts no replacement entity's policy
is selected. This should be fixed in this PR.
--
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]
