yuqi1129 commented on code in PR #12964:
URL: https://github.com/apache/gravitino/pull/12964#discussion_r4044371102
##########
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()))))
Review Comment:
[P2] Fall back to `AllValuesSelector` for a NULL relation value
`association.relationValue().orElseThrow()` turns a missing selector into a
`NoSuchElementException`, which neither `TagExceptionHandler` nor
`PolicyExceptionHandler` maps, so the whole `GET
.../tags/{tag}/policies?details=true` (and `GET
.../policies/{policy}/tags?details=true` in `PolicyOperations`) returns 500.
The schema explicitly allows NULL here (`scripts/*/schema-2.0.0-*.sql`:
`selector ... DEFAULT NULL COMMENT 'policy tag selector JSON, NULL matches tag
presence'`), and `ObjectPolicyResolver` already treats a missing value as
`AllValuesSelector`. The two list endpoints should follow the same rule instead
of throwing.
##########
docs/open-api/policies.yaml:
##########
@@ -441,7 +441,8 @@ components:
$ref: "./openapi.yaml#/components/schemas/Audit"
inherited:
type: boolean
- description: Whether the policy is inherited from the parent
metadata object
+ description: Whether the policy is selected only through tags
inherited from ancestor
Review Comment:
[P3] Align the `inherited` description and document the new endpoints
`PolicyManager.listPolicyInfosForMetadataObject` also marks legacy direct
relations on ancestors as `inherited=true`, so "selected only through tags
inherited from ancestor metadata objects" is no longer accurate.
Also, none of the new endpoints (`GET/POST/DELETE
/metalakes/{metalake}/tags/{tag}/policies[/{policy}]` and `GET
/metalakes/{metalake}/policies/{policy}/tags`) are in `tags.yaml` /
`policies.yaml`, so `./gradlew :docs:build` does not validate them.
##########
core/src/main/java/org/apache/gravitino/policy/PolicyManager.java:
##########
@@ -295,12 +299,28 @@ 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);
+ Map<Long, PolicyEntity> policiesById = new LinkedHashMap<>();
+ Arrays.stream(listDirectPoliciesForMetadataObject(metalake,
metadataObject, false))
+ .forEach(policy -> policiesById.put(policy.id(), policy));
+
+ Arrays.stream(objectPolicyResolver.resolve(metalake, metadataObject))
Review Comment:
[P2] Decide the `enabled` semantics of the merged result
`ObjectPolicyResolver` drops disabled policies before returning, but
`listDirectPoliciesForMetadataObject` keeps them, so the merged array is
inconsistent. With policy `P` disabled: `GET /objects/table/t/policies/P`
returns 200 when `P` is a legacy direct relation and 404
(`NoSuchPolicyException` from `getPolicyForMetadataObject`) when `P` is bound
through a tag.
Either keep disabled policies on both paths and let callers read `enabled`,
or filter them on both; a consumer such as TMS cannot rely on either behaviour
today.
##########
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(
Review Comment:
[P3] Enforce the allowed-values rule for `TAG_VALUE` selectors
`design-docs/policy-on-tag.md` (rule 5 under selector validation) says that
when the tag defines allowed values, the selector value must be one of them,
but nothing on this path checks it: neither `TagManager.addPolicyForTag` nor
`PolicyTagRelService`. `POST /tags/{tag}/policies/{policy}` with
`{"type":"TAG_VALUE","value":"nope"}` on a tag whose `allowedValues` is
`["finance"]` succeeds and creates a relation that can never match.
--
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]
