Copilot commented on code in PR #6869:
URL: https://github.com/apache/texera/pull/6869#discussion_r3798889277
##########
common/workflow-core/src/main/scala/org/apache/texera/amber/core/storage/model/LakeFSFileDocument.scala:
##########
@@ -36,21 +37,51 @@ object LakeFSFileDocument {
// In the local development or other architectures, this token can be empty.
lazy val userJwtToken: String =
sys.env.getOrElse(EnvironmentalVariable.ENV_USER_JWT_TOKEN, "").trim
+
+ private lazy val datasetPresignEndpoint: String =
+ sys.env
+ .getOrElse(
+
EnvironmentalVariable.ENV_FILE_SERVICE_GET_DATASET_PRESIGNED_URL_ENDPOINT,
+ "http://localhost:9092/api/dataset/presign-download";
+ )
+ .trim
+
+ private lazy val modelPresignEndpoint: String =
+ sys.env
+ .getOrElse(
+
EnvironmentalVariable.ENV_FILE_SERVICE_GET_MODEL_PRESIGNED_URL_ENDPOINT,
+ "http://localhost:9092/api/model/presign-download";
+ )
+ .trim
+
+ /**
+ * The file-service presign-download endpoint serving this resource type.
Each resource type
+ * owns an endpoint because they enforce different access control (a
dataset grant does not
+ * grant a model).
+ */
+ def presignEndpointOf(resourceType: ResourceType.Value): String =
+ resourceType match {
+ case ResourceType.Datasets => datasetPresignEndpoint
+ case ResourceType.Models => modelPresignEndpoint
Review Comment:
The model endpoint selected here is not implemented by the newly registered
`ModelResource`. With a user JWT, every model read therefore receives an HTTP
error and `asInputStream` catches it to perform a direct LakeFS fetch,
bypassing the new model authorization layer. Add the authenticated model
presign endpoint and do not fall back to direct storage on authorization/4xx
responses.
##########
sql/updates/38.sql:
##########
@@ -0,0 +1,39 @@
+/*
+ * 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.
+ */
+
+\c texera_db
+
+SET search_path TO texera_db;
+
+BEGIN;
+
+-- Per-user access control for models.
+-- Enables the model management API to grant/list/revoke READ/WRITE access.
+
+CREATE TABLE IF NOT EXISTS model_user_access
+(
+ mid INT NOT NULL,
+ uid INT NOT NULL,
+ privilege privilege_enum NOT NULL DEFAULT 'NONE',
+ PRIMARY KEY (mid, uid),
+ FOREIGN KEY (mid) REFERENCES model(mid) ON DELETE CASCADE,
+ FOREIGN KEY (uid) REFERENCES "user"(uid) ON DELETE CASCADE
+);
Review Comment:
This migration leaves models created under changeSet 37 without owner
grants. `ResourceAccess.listVisible` currently discovers private/owned models
through `model_user_access`, so such models disappear from their owner's
`/model/list` even though ownership still authorizes `/model/{mid}`. Backfill
owner WRITE rows when creating the table.
##########
file-service/src/main/scala/org/apache/texera/service/resource/ModelResource.scala:
##########
@@ -0,0 +1,411 @@
+/*
+ * 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.texera.service.resource
+
+import com.typesafe.scalalogging.LazyLogging
+import io.dropwizard.auth.Auth
+import jakarta.annotation.security.{PermitAll, RolesAllowed}
+import jakarta.ws.rs._
+import jakarta.ws.rs.core._
+import org.apache.texera.amber.core.storage.util.LakeFSStorageClient
+import org.apache.texera.auth.SessionUser
+import org.apache.texera.common.config.StorageConfig
+import org.apache.texera.dao.SqlServer
+import org.apache.texera.dao.SqlServer.withTransaction
+import org.apache.texera.dao.jooq.generated.enums.PrivilegeEnum
+import org.apache.texera.dao.jooq.generated.tables.Model.MODEL
+import org.apache.texera.dao.jooq.generated.tables.daos.{ModelDao,
ModelUserAccessDao}
+import org.apache.texera.dao.jooq.generated.tables.pojos.{Model,
ModelUserAccess}
+import org.apache.texera.service.resource.ManagedResource.{Model =>
MODEL_RESOURCE}
+import org.apache.texera.service.resource.ModelAccessResource._
+import org.apache.texera.service.resource.ModelResource.{context, _}
+import org.apache.texera.service.util.S3StorageClient
+import
org.apache.texera.service.util.LakeFSExceptionHandler.withLakeFSErrorHandling
+import org.jooq.{DSLContext, EnumType}
+
+object ModelResource {
+
+ // MVP supports a single framework; stored on the model so later frameworks
can be added.
+ private val DEFAULT_FRAMEWORK = "pytorch"
+
+ private def context =
+ SqlServer
+ .getInstance()
+ .createDSLContext()
+
+ /**
+ * Helper function to get the model from DB using mid
+ */
+ private def getModelByID(ctx: DSLContext, mid: Integer): Model = {
+ val modelDao = new ModelDao(ctx.configuration())
+ val model = modelDao.fetchOneByMid(mid)
+ if (model == null) {
+ throw new NotFoundException(f"Model $mid not found")
+ }
+ model
+ }
+
+ case class DashboardModel(
+ model: Model,
+ ownerEmail: String,
+ accessPrivilege: EnumType,
+ isOwner: Boolean,
+ size: Long
+ )
+
+ case class CreateModelRequest(
+ modelName: String,
+ modelDescription: String,
+ isModelPublic: Boolean,
+ isModelDownloadable: Boolean,
+ framework: String,
+ format: String
+ )
+
+ case class ModelDescriptionModification(mid: Integer, description: String)
+
+ case class ModelNameModification(mid: Integer, name: String)
+}
+
+@Produces(Array(MediaType.APPLICATION_JSON))
+@Path("/model")
+class ModelResource extends LazyLogging {
+ private val ERR_USER_HAS_NO_ACCESS_TO_MODEL_MESSAGE = "User has no access to
this model"
+
+ /**
+ * Helper function to get the model from DB with additional information
including
+ * user access privilege and owner email
+ */
+ private def getDashboardModel(
+ ctx: DSLContext,
+ mid: Integer,
+ requesterUid: Option[Integer]
+ ): DashboardModel = {
+ val targetModel = getModelByID(ctx, mid)
+
+ if (requesterUid.isEmpty && !targetModel.getIsPublic) {
+ throw new ForbiddenException(ERR_USER_HAS_NO_ACCESS_TO_MODEL_MESSAGE)
+ } else if (requesterUid.exists(uid => !userHasReadAccess(ctx, mid, uid))) {
+ throw new ForbiddenException(ERR_USER_HAS_NO_ACCESS_TO_MODEL_MESSAGE)
+ }
+
+ val userAccessPrivilege = requesterUid
+ .map(uid => getModelUserAccessPrivilege(ctx, mid, uid))
+ .getOrElse(PrivilegeEnum.READ)
+
+ val isOwner = requesterUid.contains(targetModel.getOwnerUid)
+
+ DashboardModel(
+ targetModel,
+ getOwner(ctx, mid).getEmail,
+ userAccessPrivilege,
+ isOwner,
+ withLakeFSErrorHandling(s"retrieving the size of model
'${targetModel.getName}'") {
+
LakeFSStorageClient.retrieveRepositorySize(targetModel.getRepositoryName)
+ }
+ )
+ }
+
+ @POST
+ @RolesAllowed(Array("REGULAR", "ADMIN"))
+ @Path("/create")
+ @Consumes(Array(MediaType.APPLICATION_JSON))
+ def createModel(
+ request: CreateModelRequest,
+ @Auth user: SessionUser
+ ): DashboardModel = {
+
+ withTransaction(context) { ctx =>
+ val uid = user.getUid
+ val modelUserAccessDao: ModelUserAccessDao = new
ModelUserAccessDao(ctx.configuration())
+
+ val modelName = request.modelName
+ val modelDescription = request.modelDescription
+ val isModelPublic = request.isModelPublic
+ val isModelDownloadable = request.isModelDownloadable
+
+ ResourceNaming.validateName(MODEL_RESOURCE.label, modelName)
+ ResourceNaming.requireNameAvailable(ctx, MODEL_RESOURCE, uid, modelName)
+
+ // insert the model into the database
+ val model = new Model()
+ model.setName(modelName)
+ model.setDescription(modelDescription)
+ model.setIsPublic(isModelPublic)
+ model.setIsDownloadable(isModelDownloadable)
+ model.setOwnerUid(uid)
+
model.setFramework(Option(request.framework).filter(_.nonEmpty).getOrElse(DEFAULT_FRAMEWORK))
+ model.setFormat(request.format)
+
+ // insert record and get created model with mid
+ val createdModel =
ResourceNaming.failOnDuplicateName(MODEL_RESOURCE.label) {
+ ctx
+ .insertInto(MODEL)
+ .set(ctx.newRecord(MODEL, model))
+ .returning()
+ .fetchOne()
+ }
+
+ // Initialize the repository in LakeFS
+ val repositoryName = s"model-${createdModel.getMid}"
+ try {
+ withLakeFSErrorHandling(s"creating the repository of model
'${model.getName}'") {
+ LakeFSStorageClient.initRepo(repositoryName)
Review Comment:
The external repository is created before the remaining database writes. If
updating `repository_name`, inserting the owner grant, or committing the
transaction fails, the database rolls back but `model-{mid}` remains
permanently orphaned in LakeFS. Add compensation for every post-initialization
failure, or provision from a committed pending/outbox state.
This issue also appears on line 221 of the same file.
##########
file-service/src/main/scala/org/apache/texera/service/resource/ResourceAccess.scala:
##########
@@ -0,0 +1,312 @@
+/*
+ * 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.texera.service.resource
+
+import jakarta.ws.rs.{BadRequestException, ForbiddenException}
+import jakarta.ws.rs.core.Response
+import org.apache.texera.dao.jooq.generated.Tables.USER
+import org.apache.texera.dao.jooq.generated.enums.PrivilegeEnum
+import org.apache.texera.dao.jooq.generated.tables.daos.UserDao
+import org.apache.texera.dao.jooq.generated.tables.pojos.User
+import org.jooq.{DSLContext, EnumType, Record}
+
+import scala.jdk.CollectionConverters._
+
+/**
+ * Ownership and privilege rules shared by every access-controlled resource.
+ *
+ * A resource is readable when it is public, or the caller owns it, or the
caller holds an
+ * explicit grant; it is writable when the caller owns it or holds a WRITE
grant. A missing
+ * resource resolves to "not public, unowned, ungranted" rather than an
error, so callers
+ * decide whether absence is a 403 or a 404.
+ */
+object ResourceAccess {
+
+ /** One shared grant, as returned by the access-list endpoints. */
+ case class AccessEntry(email: String, name: String, privilege: EnumType) {}
+
+ def isPublic[R <: Record, A <: Record](
+ ctx: DSLContext,
+ resource: ManagedResource[R, A],
+ id: Integer
+ ): Boolean =
+ Option(
+ ctx
+ .select(resource.isPublicField)
+ .from(resource.table)
+ .where(resource.idField.eq(id))
+ .fetchOne()
+ ).flatMap(record => Option(record.value1()))
+ .exists(_.booleanValue())
+
+ def userOwns[R <: Record, A <: Record](
+ ctx: DSLContext,
+ resource: ManagedResource[R, A],
+ id: Integer,
+ uid: Integer
+ ): Boolean =
+ Option(
+ ctx
+ .select(resource.ownerUidField)
+ .from(resource.table)
+ .where(resource.idField.eq(id))
+ .fetchOne()
+ ).flatMap(record => Option(record.value1()))
+ .contains(uid)
+
+ def privilegeOf[R <: Record, A <: Record](
+ ctx: DSLContext,
+ resource: ManagedResource[R, A],
+ id: Integer,
+ uid: Integer
+ ): PrivilegeEnum =
+ Option(
+ ctx
+ .select(resource.privilegeField)
+ .from(resource.accessTable)
+ .where(
+ resource.accessIdField
+ .eq(id)
+ .and(resource.accessUidField.eq(uid))
+ )
+ .fetchOneInto(classOf[PrivilegeEnum])
+ ).getOrElse(PrivilegeEnum.NONE)
+
+ def userHasWriteAccess[R <: Record, A <: Record](
+ ctx: DSLContext,
+ resource: ManagedResource[R, A],
+ id: Integer,
+ uid: Integer
+ ): Boolean =
+ userOwns(ctx, resource, id, uid) ||
+ privilegeOf(ctx, resource, id, uid) == PrivilegeEnum.WRITE
+
+ def userHasReadAccess[R <: Record, A <: Record](
+ ctx: DSLContext,
+ resource: ManagedResource[R, A],
+ id: Integer,
+ uid: Integer
+ ): Boolean =
+ isPublic(ctx, resource, id) ||
+ userHasWriteAccess(ctx, resource, id, uid) ||
+ privilegeOf(ctx, resource, id, uid) == PrivilegeEnum.READ
+
+ /** The owning user, or null when the resource does not exist. */
+ def owner[R <: Record, A <: Record](
+ ctx: DSLContext,
+ resource: ManagedResource[R, A],
+ id: Integer
+ ): User = {
+ val userDao = new UserDao(ctx.configuration())
+ Option(
+ ctx
+ .select(resource.ownerUidField)
+ .from(resource.table)
+ .where(resource.idField.eq(id))
+ .fetchOne()
+ ).flatMap(record => Option(record.value1()))
+ .map(ownerUid => userDao.fetchOneByUid(ownerUid))
+ .orNull
+ }
+
+ /** The owner's email, or an empty string when the resource does not exist.
*/
Review Comment:
This contract is incorrect: `requireReadAccess` throws for a missing
resource, so the method never returns an empty string in that case. Document
the authorization failure instead of promising an unreachable return value.
This issue also appears in the following locations of the same file:
- line 147
- line 197
- line 251
- line 275
--
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]
