yuqi1129 commented on code in PR #12850:
URL: https://github.com/apache/gravitino/pull/12850#discussion_r3924722588
##########
lineage/src/main/java/org/apache/gravitino/lineage/source/rest/LineageOperations.java:
##########
@@ -53,16 +56,25 @@ public LineageOperations(LineageDispatcher
lineageDispatcher) {
@Produces(MediaType.APPLICATION_JSON)
Review Comment:
**Blocking, please verify before merge.** With this `@Produces`, I could not
reach this method at all. I ran `POST /api/lineage` against a real server on
this branch and on `main`, with three different `Accept` values, and every
single one came back **406**:
```
[PR] Accept: application/vnd.gravitino.v1+json -> 406
[MAIN] Accept: application/vnd.gravitino.v1+json -> 406
[MAIN] Accept: application/json -> 406 {"message":"Not
Acceptable","url":"/api/lineage"}
[MAIN] Accept: */* -> 406
```
`VersioningFilter` is registered on `/api/*` (`GravitinoServer.java:203`).
When the request carries no Gravitino version it **replaces** `Accept` with
`application/vnd.gravitino.v1+json` (`MutableHttpServletRequest.putHeader`
overwrites, it does not append). This method produces plain `application/json`,
so the two never match. Every other REST resource in the repo declares
`@Produces("application/vnd.gravitino.v1+json")`, this one does not.
The 406 comes from Jersey's method matching, before the resource method
runs, so the hk2 interceptor never fires either, which means the whole
authorization path this PR adds is unreachable as the server is currently
wired. It returns 406 rather than 404, so registration itself is fine, it is
purely content negotiation.
This is pre-existing, not introduced here, but two things follow:
1. It is one line in a file this PR already touches, so I would fix
`@Produces` here and add an integration test, otherwise nothing proves this fix
works.
2. I could not reproduce #12840's stated repro (both requests returning `201
Created`). Could you confirm how you reached 201? If the issue's premise does
not hold, the scope may need revisiting.
##########
lineage/src/main/java/org/apache/gravitino/lineage/source/rest/LineageOperations.java:
##########
@@ -53,16 +56,25 @@ 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)
Review Comment:
`CAN_ACCESS_METADATA` resolves to `LOAD_TABLE` for a table, i.e. SELECT or
MODIFY or owner. So a user holding only SELECT can list a table under
`outputs`, that is, assert "this job wrote this table".
The description calls this out as intentional ("metadata-visibility
permissions"), but it does not explain the asymmetry. Lineage feeds impact
analysis and governance, so a write-claim is arguably not the same as a read.
Worth either requiring MODIFY-level rights for `outputs`, or writing down why
visibility is enough.
##########
server/src/main/java/org/apache/gravitino/server/web/filter/authorization/LineageAuthorizationExecutor.java:
##########
@@ -0,0 +1,179 @@
+/*
+ * 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.ArrayList;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Optional;
+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.lineage.source.rest.LineageEventValidator;
+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;
+ private List<AuthorizationTarget> authorizationTargets = List.of();
+ private boolean authorizationTargetsResolved;
+
+ /**
+ * 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 Optional<String> getAuthorizationMetalake() {
+ RunEvent event = extractRunEvent();
+ resolveAuthorizationTargets(event);
+ return Optional.of(event.getJob().getNamespace());
+ }
+
+ @Override
+ public boolean execute(AuthorizationRequestContext context) {
+ if (!authorizationTargetsResolved) {
+ resolveAuthorizationTargets(extractRunEvent());
+ }
+
+ AuthorizationExpressionEvaluator evaluator = new
AuthorizationExpressionEvaluator(expression);
+ context.setOriginalAuthorizationExpression(expression);
+
+ for (AuthorizationTarget target : authorizationTargets) {
+ if (!evaluator.evaluate(
+ target.metadataContext, Map.of(), context,
Optional.of(target.entityType.name()))) {
+ return false;
+ }
+ }
+ return true;
+ }
+
+ 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;
Review Comment:
`MODEL_VERSION` maps to `MetadataObject.Type.MODEL`, whose full name is
three parts (`catalog.schema.model`), while a model version identifier
naturally has four. So `MetadataObjects.parse` below will reject any name a
producer would actually emit for a model version, and the caller gets a 400
they cannot act on.
Either drop `MODEL_VERSION` from this switch, or handle the four-part form
explicitly.
Separately: `datasetType` is not a standard OpenLineage facet, and reading
it as `facets["datasetType"].additionalProperties["datasetType"]` is a
Gravitino-specific convention. Documenting it in the OpenAPI spec is the right
move, it just means producers must be taught to emit it before anything other
than `TABLE` can be authorized.
##########
server/src/main/java/org/apache/gravitino/server/web/filter/authorization/LineageAuthorizationExecutor.java:
##########
@@ -0,0 +1,179 @@
+/*
+ * 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.ArrayList;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Optional;
+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.lineage.source.rest.LineageEventValidator;
+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;
+ private List<AuthorizationTarget> authorizationTargets = List.of();
+ private boolean authorizationTargetsResolved;
+
+ /**
+ * 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 Optional<String> getAuthorizationMetalake() {
Review Comment:
Small readability point: the name says "get metalake", but this also
validates the event and resolves every authorization target as a side effect,
and it is the first thing the interceptor calls. Doing the resolution lazily in
`execute()` and returning `event.getJob().getNamespace()` directly here would
make the contract match the name.
##########
server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java:
##########
@@ -267,6 +256,65 @@ public Object invoke(MethodInvocation methodInvocation)
throws Throwable {
}
}
+ private Optional<Response> validateCurrentUserAndActiveRoles(
+ NameIdentifier metalakeIdent,
+ AuthorizationRequestContext authorizationRequestContext,
+ AuthorizationExpression expressionAnnotation,
+ Map<Entity.EntityType, NameIdentifier> metadataContext,
+ Method method,
+ String expression) {
+ String currentUser = PrincipalUtils.getCurrentUserName();
+ try {
+ AuthorizationUtils.checkCurrentUser(
+ metalakeIdent.name(), currentUser, authorizationRequestContext);
+ } catch (NoSuchMetalakeException e) {
Review Comment:
For lineage this branch is the most likely real-world failure, and its
output is not usable.
When `job.namespace` is not a metalake, `checkCurrentUser` throws
`NoSuchMetalakeException` and we land here, returning a **403**. The lineage
annotation leaves `accessMetadataType` at its default `METALAKE`, and
`metadataContext` is empty for that endpoint, so `accessMetadataName` is null
and the message renders as `User 'x' is not authorized to perform operation
'postLineage' ` with a trailing space and no indication of which namespace was
wrong.
Since this PR redefines `job.namespace` to mean "metalake", getting it wrong
will be the number one support question. It should be a 400 that names the
field, not an anonymous 403.
##########
server/src/main/java/org/apache/gravitino/server/web/filter/authorization/LineageAuthorizationExecutor.java:
##########
@@ -0,0 +1,179 @@
+/*
+ * 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.ArrayList;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Optional;
+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.lineage.source.rest.LineageEventValidator;
+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;
+ private List<AuthorizationTarget> authorizationTargets = List.of();
+ private boolean authorizationTargetsResolved;
+
+ /**
+ * 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 Optional<String> getAuthorizationMetalake() {
+ RunEvent event = extractRunEvent();
+ resolveAuthorizationTargets(event);
+ return Optional.of(event.getJob().getNamespace());
+ }
+
+ @Override
+ public boolean execute(AuthorizationRequestContext context) {
+ if (!authorizationTargetsResolved) {
+ resolveAuthorizationTargets(extractRunEvent());
+ }
+
+ AuthorizationExpressionEvaluator evaluator = new
AuthorizationExpressionEvaluator(expression);
+ context.setOriginalAuthorizationExpression(expression);
+
+ for (AuthorizationTarget target : authorizationTargets) {
+ if (!evaluator.evaluate(
+ target.metadataContext, Map.of(), context,
Optional.of(target.entityType.name()))) {
+ return false;
+ }
+ }
+ return true;
Review Comment:
When the event carries no inputs and no outputs, `authorizationTargets` is
empty, the loop body never runs, and this returns `true`. So a well-formed but
dataset-less event is dispatched to the sinks with **no privilege check at
all**, the only gate left is `checkCurrentUser`, i.e. "are you a member of this
metalake".
The `{}` case from the issue is now caught by validation, but this one is
not. Whether that is acceptable is a product call, but it should be explicit
and covered by a test, right now nothing pins it.
##########
server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java:
##########
@@ -238,6 +193,40 @@ public Object invoke(MethodInvocation methodInvocation)
throws Throwable {
secondaryExpression,
secondaryExpressionCondition,
expressionAnnotation.allowCheckExistence());
+ try {
+ Optional<String> dynamicMetalake =
executor.getAuthorizationMetalake();
+ if (dynamicMetalake.isPresent()
+ && authorizationMetalake.isPresent()
+ &&
!dynamicMetalake.get().equals(authorizationMetalake.get())) {
+ throw new IllegalArgumentException(
+ String.format(
+ "Authorization request metalake '%s' does not match
path metalake '%s'",
+ dynamicMetalake.get(), authorizationMetalake.get()));
+ }
+ if (dynamicMetalake.isPresent()) {
+ authorizationMetalake = dynamicMetalake;
+ }
+ } catch (IllegalArgumentException exception) {
+ LOG.warn("Invalid authorization request", exception);
+ return Utils.illegalArguments(exception.getMessage(), exception);
+ }
+ }
+
+ if (authorizationMetalake.isPresent()) {
+ Optional<Response> validationFailure =
+ validateCurrentUserAndActiveRoles(
Review Comment:
This moves user and active-role validation from **before** executor
construction to **after** it, and that affects every endpoint, not only lineage.
Executor construction is not side-effect free:
`CreateSchemaAuthorizationExecutor` calls `injectParentSchema` in its
constructor, and `LoadTableAuthorizationExecutor` does `(String)
ParameterUtil.extractFromParameters(...)`. I went through both and they are
defensive enough that I could not build a failing case today, so this is not a
live bug. But the invariant "authenticate and authorize before parsing the
request body" is gone for all endpoints, and a caller who is not a member of
the metalake can now receive a 400/500 shaped by the request body where they
previously got a 403.
Minimal fix that keeps the new capability: validate the **path** metalake
where it was before, and only run the validation for a **dynamically resolved**
metalake after the executor is built. Existing endpoints then keep their exact
ordering.
##########
docs/open-api/lineage.yaml:
##########
@@ -21,21 +21,34 @@ paths:
/lineage:
post:
summary: Post runEvent
- description: Updates a run state for a job.
+ description: |
+ Updates a run state for a job. When authorization is enabled,
`job.namespace`
+ identifies the Gravitino metalake (organization), and every input and
output
+ dataset namespace must match it. Dataset names must be Gravitino
metadata full
+ names. The optional `datasetType` facet defaults to `TABLE`; supported
values are
+ `TABLE`, `VIEW`, `FILE`, `FILESET`, `MODEL`, `MODEL_VERSION`, and
`TOPIC`.
+ Inputs and outputs both require metadata visibility. Unsupported or
external
+ dataset identifiers are rejected when authorization is enabled. When
authorization
+ is disabled, generic OpenLineage namespaces remain supported.
Review Comment:
This paragraph is the real design decision in the PR and I think it deserves
more than a doc note.
Standard OpenLineage producers (the Spark, Airflow and dbt integrations)
emit `dataset.namespace` as a data source URI such as `hive://host:9083` or
`postgres://...`, and `job.namespace` as the scheduler namespace. Under this
contract, turning authorization on converts all of those into 400/403. The
sentence "When authorization is disabled, generic OpenLineage namespaces remain
supported" says exactly that: the same deployment speaks two different wire
protocols depending on a flag that looks unrelated, and the failure mode is
lineage silently stopping.
Options worth weighing, roughly in increasing cost:
- make the strict identifier contract its own opt-in config instead of
something `enableAuthorization` implies, so operators see what they are
switching;
- add a pluggable dataset-identifier resolver so a deployment can map its
producers' naming onto Gravitino identifiers;
- authorize only the datasets that resolve to a Gravitino object and pass
the rest through, which I would not recommend since it is fail-open.
I would do the first here and leave the second as follow-up. Whichever you
pick, the trade-off belongs in the PR description, today it is invisible to
whoever flips the flag.
--
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]
