yuqi1129 commented on code in PR #12850:
URL: https://github.com/apache/gravitino/pull/12850#discussion_r3922166383
##########
server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java:
##########
@@ -238,6 +194,24 @@ public Object invoke(MethodInvocation methodInvocation)
throws Throwable {
secondaryExpression,
secondaryExpressionCondition,
expressionAnnotation.allowCheckExistence());
+
authorizationMetalakes.addAll(executor.getAuthorizationMetalakes());
+ }
+
+ for (String metalake : authorizationMetalakes) {
+ Optional<Response> validationFailure =
+ validateCurrentUserAndActiveRoles(
+ NameIdentifier.of(metalake),
+ authorizationRequestContext,
Review Comment:
This request context cannot be shared across different metalakes.
AuthorizationRequestContext keeps a single hasLoadRole flag and a single
prefetchedRoleVersions map, so after mlA is validated or authorized, role
loading for mlB can be skipped or the prefetched roles from mlA can be reused.
That makes cross-metalake lineage role authorization order-dependent, and
named-role validation can inspect the wrong metalake state. Please use one
AuthorizationRequestContext per metalake and pass the matching context when
authorizing each dataset. Add a role-based mlA + mlB regression test in both
dataset orders; the current isOwner(any()) test does not exercise this path.
##########
lineage/src/main/java/org/apache/gravitino/lineage/source/rest/LineageOperations.java:
##########
@@ -53,22 +56,28 @@ public LineageOperations(LineageDispatcher
lineageDispatcher) {
@Produces(MediaType.APPLICATION_JSON)
@Timed(name = "post-lineage." + MetricNames.HTTP_PROCESS_DURATION, absolute
= true)
@ResponseMetered(name = "post-lineage", absolute = true)
- public Response postLineage(OpenLineage.RunEvent event) {
- LOG.info(
- "Open lineage event, run id:{}, job name:{}",
- org.apache.gravitino.lineage.Utils.getRunID(event),
- org.apache.gravitino.lineage.Utils.getJobName(event));
-
+ @AuthorizationExpression(expression =
AuthorizationExpressionConstants.CAN_ACCESS_METADATA)
+ public Response postLineage(
+ @AuthorizationRequest(type = AuthorizationRequest.RequestType.LINEAGE)
+ OpenLineage.RunEvent event) {
try {
return Utils.doAs(
httpRequest,
() -> {
+ LineageEventValidator.validate(event);
+ LOG.info(
+ "Open lineage event, run id:{}, job name:{}",
+ org.apache.gravitino.lineage.Utils.getRunID(event),
+ org.apache.gravitino.lineage.Utils.getJobName(event));
if (lineageDispatcher.dispatchLineageEvent(event)) {
return Utils.created();
} else {
return Utils.tooManyRequests();
}
});
+ } catch (IllegalArgumentException e) {
Review Comment:
The IllegalArgumentException catch also covers
lineageDispatcher.dispatchLineageEvent. If a processor or dispatcher has an
internal bug or configuration error and throws IllegalArgumentException, this
endpoint now reports it as an invalid client event (400) instead of 500. Please
isolate the validation try/catch, or use a dedicated validation exception, and
add a regression test where the dispatcher throws IllegalArgumentException.
##########
server/src/main/java/org/apache/gravitino/server/web/filter/authorization/LineageAuthorizationExecutor.java:
##########
@@ -0,0 +1,187 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.gravitino.server.web.filter.authorization;
+
+import static
org.apache.gravitino.server.web.filter.ParameterUtil.extractFromParameters;
+
+import com.google.common.base.Preconditions;
+import io.openlineage.server.OpenLineage.Dataset;
+import io.openlineage.server.OpenLineage.DatasetFacet;
+import io.openlineage.server.OpenLineage.DatasetFacets;
+import io.openlineage.server.OpenLineage.RunEvent;
+import java.lang.reflect.Parameter;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.gravitino.Entity;
+import org.apache.gravitino.MetadataObject;
+import org.apache.gravitino.MetadataObjects;
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.authorization.AuthorizationRequestContext;
+import
org.apache.gravitino.server.authorization.expression.AuthorizationExpressionEvaluator;
+import org.apache.gravitino.utils.MetadataObjectUtil;
+import org.apache.gravitino.utils.NameIdentifierUtil;
+
+/** Authorization executor for every input and output dataset in an
OpenLineage event. */
+public class LineageAuthorizationExecutor implements AuthorizationExecutor {
+
+ private static final String DATASET_TYPE_FACET = "datasetType";
+
+ private final Parameter[] parameters;
+ private final Object[] args;
+ private final String expression;
+
+ /**
+ * Creates an authorization executor for an OpenLineage event.
+ *
+ * @param parameters parameters of the intercepted REST method
+ * @param args arguments passed to the intercepted REST method
+ * @param expression authorization expression to evaluate for every dataset
+ */
+ public LineageAuthorizationExecutor(Parameter[] parameters, Object[] args,
String expression) {
+ this.parameters = parameters;
+ this.args = args;
+ this.expression = expression;
+ }
+
+ @Override
+ public Set<String> getAuthorizationMetalakes() {
+ Object request = extractFromParameters(parameters, args);
+ if (!(request instanceof RunEvent)) {
+ return Set.of();
+ }
+
+ RunEvent event = (RunEvent) request;
+ if (!hasValidDatasetIdentifiers(event.getInputs())
+ || !hasValidDatasetIdentifiers(event.getOutputs())) {
+ return Set.of();
+ }
+
+ Set<String> metalakes = new LinkedHashSet<>();
+ addDatasetMetalakes(event.getInputs(), metalakes);
+ addDatasetMetalakes(event.getOutputs(), metalakes);
+ return metalakes;
+ }
+
+ @Override
+ public boolean execute(AuthorizationRequestContext context) {
+ Object request = extractFromParameters(parameters, args);
+ if (!(request instanceof RunEvent)) {
+ // Request validation is owned by LineageOperations and must still run
when authorization is
+ // disabled. Let it return HTTP 400 for a null or malformed event.
+ return true;
+ }
+
+ RunEvent event = (RunEvent) request;
+ if (!hasValidDatasetIdentifiers(event.getInputs())
+ || !hasValidDatasetIdentifiers(event.getOutputs())) {
+ return true;
+ }
+
+ AuthorizationExpressionEvaluator evaluator = new
AuthorizationExpressionEvaluator(expression);
+ context.setOriginalAuthorizationExpression(expression);
+
+ return authorizeDatasets(event.getInputs(), context, evaluator)
+ && authorizeDatasets(event.getOutputs(), context, evaluator);
+ }
+
+ static MetadataObject.Type getMetadataType(Dataset dataset) {
+ DatasetFacets facets = dataset.getFacets();
+ if (facets == null) {
+ return MetadataObject.Type.TABLE;
+ }
+
+ DatasetFacet datasetTypeFacet =
facets.getAdditionalProperties().get(DATASET_TYPE_FACET);
+ if (datasetTypeFacet == null) {
+ return MetadataObject.Type.TABLE;
+ }
+
+ Object datasetType =
datasetTypeFacet.getAdditionalProperties().get(DATASET_TYPE_FACET);
+ Preconditions.checkArgument(
+ datasetType instanceof String && StringUtils.isNotBlank((String)
datasetType),
+ "The datasetType facet must contain a non-blank datasetType");
+
+ return switch (((String) datasetType).toUpperCase(Locale.ROOT)) {
+ case "TABLE" -> MetadataObject.Type.TABLE;
+ case "VIEW" -> MetadataObject.Type.VIEW;
+ case "FILE", "FILESET" -> MetadataObject.Type.FILESET;
+ case "MODEL", "MODEL_VERSION" -> MetadataObject.Type.MODEL;
+ case "TOPIC" -> MetadataObject.Type.TOPIC;
+ default -> throw new IllegalArgumentException("Unsupported dataset type:
" + datasetType);
+ };
+ }
+
+ private static boolean hasValidDatasetIdentifiers(List<? extends Dataset>
datasets) {
+ if (datasets == null) {
+ return true;
+ }
+
+ return datasets.stream()
+ .allMatch(
+ dataset ->
+ dataset != null
+ && StringUtils.isNotBlank(dataset.getNamespace())
+ && StringUtils.isNotBlank(dataset.getName()));
+ }
+
+ private static void addDatasetMetalakes(List<? extends Dataset> datasets,
Set<String> metalakes) {
+ if (datasets != null) {
+ datasets.forEach(dataset -> metalakes.add(dataset.getNamespace()));
+ }
+ }
+
+ private static boolean authorizeDatasets(
+ List<? extends Dataset> datasets,
+ AuthorizationRequestContext context,
+ AuthorizationExpressionEvaluator evaluator) {
+ if (datasets == null) {
+ return true;
+ }
+
+ for (Dataset dataset : datasets) {
+ if (!authorizeDataset(dataset, context, evaluator)) {
+ return false;
+ }
+ }
+ return true;
+ }
+
+ private static boolean authorizeDataset(
+ Dataset dataset,
+ AuthorizationRequestContext context,
+ AuthorizationExpressionEvaluator evaluator) {
+ String metalake = dataset.getNamespace();
+ try {
+ MetadataObject.Type metadataType = getMetadataType(dataset);
+ MetadataObject metadataObject = MetadataObjects.parse(dataset.getName(),
metadataType);
+ Entity.EntityType entityType =
MetadataObjectUtil.toEntityType(metadataType);
+ NameIdentifier identifier = MetadataObjectUtil.toEntityIdent(metalake,
metadataObject);
+ Map<Entity.EntityType, NameIdentifier> metadataContext =
+ NameIdentifierUtil.splitNameIdentifier(metalake, entityType,
identifier);
+ return evaluator.evaluate(metadataContext, Map.of(), context,
Optional.of(entityType.name()));
+ } catch (IllegalArgumentException exception) {
Review Comment:
This catch conflates request target-resolution failures with authorization
failures and may also swallow IllegalArgumentException raised by the evaluator
or authorizer. The former should follow the chosen invalid-request contract;
the latter should remain a 500 rather than become 403. Please resolve and
validate the dataset target before evaluating authorization, catch only the
target-resolution error, and let authorization infrastructure failures
propagate. If returning 403 for malformed targets is an intentional
anti-enumeration policy, please document and test that contract explicitly.
Tests should distinguish a malformed name or facet, an ordinary denial, and an
evaluator failure.
--
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]
