yuqi1129 commented on code in PR #12964:
URL: https://github.com/apache/gravitino/pull/12964#discussion_r4021876607
##########
core/src/main/java/org/apache/gravitino/policy/PolicyManager.java:
##########
@@ -300,7 +304,12 @@ public PolicyEntity[] listPolicyInfosForMetadataObject(
MetadataObjectUtil.checkMetadataObject(metalake, metadataObject);
checkMetalake(NameIdentifier.of(metalake), entityStore);
- return listDirectPoliciesForMetadataObject(entityIdent, entityType,
metadataObject);
+ Map<Long, PolicyEntity> policiesById = new LinkedHashMap<>();
+ Arrays.stream(listDirectPoliciesForMetadataObject(entityIdent, entityType,
metadataObject))
+ .forEach(policy -> policiesById.putIfAbsent(policy.id(), policy));
+ Arrays.stream(objectPolicyResolver.resolve(metalake, metadataObject))
Review Comment:
[P1] Filter inherited results before returning tag-derived policies
Adding resolved policies here also feeds them into the existing ancestor
loop in MetadataObjectPolicyOperations.listPoliciesForMetadataObject. That
method filters the first result with LOAD_POLICY_AUTHORIZATION_EXPRESSION, but
appends each ancestor's result without applying that filter.
For example:
1. A schema has a tag linked to policy P, and a table inherits that tag.
2. A user can access the table but has no right to view P.
3. The table lookup returns P and the authorization filter removes it.
4. The schema lookup returns P again, and the ancestor loop adds it back to
the response without checking policy visibility.
With details=false this exposes the policy name; with details=true it
exposes the policy content. The unfiltered ancestor loop already exists, but
this change newly makes tag-derived policies reachable through it.
I reproduced this with the actual manager/resolver/resource code and a
policy-visibility filter stubbed to reject all policies: the list still
returned one policy instead of zero.
Suggested fix: apply the visibility filter to the complete merged result
before producing either response shape, or filter every ancestor result. Please
add a regression test covering both details values. This should be fixed in
this PR.
##########
server/src/main/java/org/apache/gravitino/server/web/rest/TagOperations.java:
##########
@@ -325,6 +333,127 @@ public Response listMetadataObjectsForTag(
}
}
+ @GET
+ @Path("{tag}/policies")
+ @Produces("application/vnd.gravitino.v1+json")
+ @Timed(name = "list-policies-for-tag." + MetricNames.HTTP_PROCESS_DURATION,
absolute = true)
+ @ResponseMetered(name = "list-policies-for-tag", absolute = true)
+ @AuthorizationExpression(
+ expression =
AuthorizationExpressionConstants.LOAD_TAG_AUTHORIZATION_EXPRESSION)
+ public Response listPoliciesForTag(
+ @PathParam("metalake") @AuthorizationMetadata(type =
Entity.EntityType.METALAKE)
+ String metalake,
+ @PathParam("tag") @AuthorizationMetadata(type = Entity.EntityType.TAG)
String tagName,
+ @QueryParam("details") @DefaultValue("false") boolean verbose) {
+ LOG.info("Received list policy associations for tag: {} under metalake:
{}", tagName, metalake);
+ try {
+ return Utils.doAs(
+ httpRequest,
+ () -> {
+ RelationalEntity<?>[] associations =
+ tagDispatcher.listPolicyAssociationsForTag(metalake, tagName);
+ associations =
+ MetadataAuthzHelper.filterByExpression(
+ metalake,
+
AuthorizationExpressionConstants.LOAD_POLICY_AUTHORIZATION_EXPRESSION,
+ Entity.EntityType.POLICY,
+ associations,
+ association ->
+ NameIdentifierUtil.ofPolicy(metalake,
association.targetEntity().name()));
+ if (!verbose) {
+ String[] names =
+ Arrays.stream(associations)
+ .map(association -> association.targetEntity().name())
+ .toArray(String[]::new);
+ return Utils.ok(new NameListResponse(names));
+ }
+
+ PolicyForTagAssociationDTO[] associationDTOs =
+ Arrays.stream(associations)
+ .map(
+ association ->
+ new PolicyForTagAssociationDTO(
+ PolicyOperations.toDTO(
+ (PolicyEntity) association.targetEntity(),
Optional.empty()),
+ PolicyAssociationSelectorDTO.fromSelector(
+ PolicyAssociationSelectorSerde.deserialize(
+
association.relationValue().orElseThrow()))))
+ .toArray(PolicyForTagAssociationDTO[]::new);
+ return Utils.ok(new
PolicyForTagAssociationListResponse(associationDTOs));
+ });
+ } catch (Exception e) {
+ return ExceptionHandlers.handleTagException(OperationType.LIST, tagName,
metalake, e);
+ }
+ }
+
+ @POST
+ @Path("{tag}/policies/{policy}")
+ @Produces("application/vnd.gravitino.v1+json")
+ @Timed(name = "add-policy-for-tag." + MetricNames.HTTP_PROCESS_DURATION,
absolute = true)
+ @ResponseMetered(name = "add-policy-for-tag", absolute = true)
+ @AuthorizationExpression(
+ expression =
+ "METALAKE::OWNER || ((TAG::OWNER || ANY_APPLY_TAG) && (POLICY::OWNER
|| ANY_APPLY_POLICY))")
+ public Response addPolicyForTag(
+ @PathParam("metalake") @AuthorizationMetadata(type =
Entity.EntityType.METALAKE)
+ String metalake,
+ @PathParam("tag") @AuthorizationMetadata(type = Entity.EntityType.TAG)
String tagName,
+ @PathParam("policy") @AuthorizationMetadata(type =
Entity.EntityType.POLICY)
+ String policyName,
+ PolicyTagAddRequest request) {
+ LOG.info(
+ "Received add policy: {} for tag: {} under metalake: {}", policyName,
tagName, metalake);
+ try {
+ return Utils.doAs(
+ httpRequest,
+ () -> {
+ PolicyTagAddRequest effectiveRequest =
+ request == null ? new PolicyTagAddRequest() : request;
+ effectiveRequest.validate();
+ tagDispatcher.addPolicyForTag(
+ metalake, tagName, policyName, effectiveRequest.selector());
+ return Utils.ok(
+ new PolicyTagAssociationResponse(
+ policyName,
+ tagName,
+
PolicyAssociationSelectorDTO.fromSelector(effectiveRequest.selector())));
+ });
+ } catch (Exception e) {
+ return ExceptionHandlers.handleTagException(OperationType.ASSOCIATE,
tagName, metalake, e);
Review Comment:
[P2] Preserve the duplicate policy-association error on both server and
client
Adding the same policy/tag association again makes
TagManager.addPolicyForTag throw PolicyAlreadyAssociatedException. This catch
passes it to handleTagException, whose handler recognizes
TagAlreadyExistsException and TagAlreadyAssociatedException but not
PolicyAlreadyAssociatedException. The request therefore returns HTTP 500
instead of the documented 409 conflict.
There is also a matching Java-client issue:
GravitinoMetalake.addPolicyForTag uses ErrorHandlers.tagErrorHandler(). Even
after the server returns 409, that handler does not recognize the
PolicyAlreadyAssociatedException error type and throws a plain
AlreadyExistsException. Callers catching the exception declared by the tag API
will miss it.
I reproduced both cases: the REST resource returned 500 when its dispatcher
threw PolicyAlreadyAssociatedException; the client tag handler turned a 409
response carrying that type into AlreadyExistsException.
Suggested fix: handle PolicyAlreadyAssociatedException in both the server
and client tag error handlers, or introduce a handler for this association API
that preserves both tag and policy errors. Add tests for repeating the
association with the same selector and with a different selector, plus a client
test for the exact exception type. This should be fixed in this PR.
##########
core/src/main/java/org/apache/gravitino/policy/PolicyManager.java:
##########
@@ -300,7 +304,12 @@ public PolicyEntity[] listPolicyInfosForMetadataObject(
MetadataObjectUtil.checkMetadataObject(metalake, metadataObject);
checkMetalake(NameIdentifier.of(metalake), entityStore);
- return listDirectPoliciesForMetadataObject(entityIdent, entityType,
metadataObject);
+ Map<Long, PolicyEntity> policiesById = new LinkedHashMap<>();
+ Arrays.stream(listDirectPoliciesForMetadataObject(entityIdent, entityType,
metadataObject))
+ .forEach(policy -> policiesById.putIfAbsent(policy.id(), policy));
+ Arrays.stream(objectPolicyResolver.resolve(metalake, metadataObject))
+ .forEach(policy -> policiesById.putIfAbsent(policy.id(), policy));
Review Comment:
[P1] Resolve tag-derived policies for the requested object only
This method now returns policies resolved from effective tags, including
inherited tags. However, both
MetadataObjectPolicyOperations.listPoliciesForMetadataObject and
getPolicyForObject still call it again for each ancestor. Those separate
lookups can bring back policies that do not apply to the requested object.
Example: a schema assigns tag domain=finance, and its table overrides the
same tag with domain=risk. Policy finance_policy is linked to that tag with
selector TAG_VALUE("finance"). The resolver correctly excludes the policy for
the table. The REST code then resolves the schema separately and returns
finance_policy anyway.
The regression checks confirmed that the manager returns zero policies for
the table, while REST list returns one and REST get returns 200 instead of 404.
This also affects callers such as GravitinoStrategyProvider, which uses the
object's listPolicies/getPolicy APIs to select maintenance strategies.
Suggested fix: keep ancestor traversal for legacy direct policy
associations, but resolve tag-derived policies once, against the actual
requested object. Use the same rule for list and get. Simply removing ancestor
traversal would break existing direct-policy inheritance.
Please cover parent/child assignments with different values, including a
parent selector that does not match the child's effective value. 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]
