aicam commented on code in PR #7937:
URL: https://github.com/apache/texera/pull/7937#discussion_r3874224855
##########
bin/single-node/nginx.conf:
##########
@@ -45,6 +45,20 @@ http {
proxy_set_header X-Real-IP $remote_addr;
}
+ # Trailing slash is required: a bare /api/model prefix would also match
+ # the LLM /api/models route below, which nginx matches byte-wise.
Review Comment:
The trailing slash is the right call, but the stated reason doesn't hold:
the LLM route below is `location = /api/models` (line 75), an **exact-match**
location. Exact matches are resolved before any prefix location and
short-circuit the search, so a bare `location /api/model` could never have
swallowed `/api/models` no matter how nginx compares the bytes.
The only path a bare prefix would actually have stolen is
`/api/models/<sub>` — and `LiteLLMModelsResource`
(`AccessControlResource.scala:359`) exposes a single `@GET` at the class path
with no subpaths, so even that isn't reachable today.
Worth keeping the slash for symmetry with `/api/access/dataset/` right
above, just with reasoning that will still be true when someone reads it next:
```suggestion
# Trailing slash for symmetry with /api/access/dataset/ below. The
LLM route
# is an exact match (location = /api/models), which outranks any
prefix
# location, so /api/models is unambiguous either way.
```
One consequence to be aware of either way: unlike `location /api/dataset`,
this won't proxy a bare `/api/model`. Harmless now — every `@Path` on
`ModelResource` has a further segment — but it becomes a silent 404 through
`location /api/` if a root endpoint is ever added.
##########
file-service/src/main/scala/org/apache/texera/service/resource/ModelResource.scala:
##########
@@ -911,10 +1015,114 @@ class ModelResource extends LazyLogging {
)
}
+ //
===========================================================================
+ // Cover image
+ //
===========================================================================
+
+ /** Points the model card at a committed image inside the model,
"<version>/<file>". */
+ @POST
+ @RolesAllowed(Array("REGULAR", "ADMIN"))
+ @Path("/{mid}/update/cover")
+ @Consumes(Array(MediaType.APPLICATION_JSON))
+ def updateModelCoverImage(
+ @PathParam("mid") mid: Integer,
+ request: CoverImageRequest,
+ @Auth sessionUser: SessionUser
+ ): Response = {
+ withTransaction(context) { ctx =>
+ val model = getModelByID(ctx, mid)
+ if (!userHasWriteAccess(ctx, mid, sessionUser.getUid)) {
+ throw new ForbiddenException(ERR_USER_HAS_NO_ACCESS_TO_MODEL_MESSAGE)
+ }
+
+ val normalized =
+ CoverImageUtils.validatePathOrThrow(request.coverImage,
CoverImageUtils.MAX_PATH_LENGTH)
+
+ val document = CoverImageUtils.openCoverOrBadRequest(
+ ResourceType.Model,
+ getOwner(ctx, mid).getEmail,
+ model.getName,
+ normalized
+ )
+
CoverImageUtils.requireWithinSizeLimit(CoverImageUtils.fileSizeOf(document,
normalized))
+
+ model.setCoverImage(normalized)
+ new ModelDao(ctx.configuration()).update(model)
+ Response.ok(Map("coverImage" -> normalized)).build()
+ }
+ }
+
+ /** 307 redirect to the cover's presigned S3 URL. */
+ @GET
+ @PermitAll
+ @Path("/{mid}/cover")
+ def getModelCover(
+ @PathParam("mid") mid: Integer,
+ @Auth sessionUser: Optional[SessionUser]
+ ): Response = {
+ withTransaction(context) { ctx =>
+ val model = requireCoverReadAccess(ctx, mid, sessionUser)
+ val coverImage = Option(model.getCoverImage).getOrElse(
+ throw new NotFoundException("No cover image")
+ )
+
+ val document = CoverImageUtils
+ .openCover(ResourceType.Model, getOwner(ctx, mid).getEmail,
model.getName, coverImage)
+ .getOrElse(throw new NotFoundException("No cover image"))
+
+ Response
+ .temporaryRedirect(new URI(CoverImageUtils.presignedUrl(document,
coverImage)))
+ .build()
+ }
+ }
+
+ /**
+ * Presigned cover URL as JSON. Needed for private models because `<img
src>`
+ * cannot attach the Authorization header that GET /{mid}/cover requires.
+ */
+ @GET
+ @PermitAll
+ @Path("/{mid}/cover-url")
+ @Produces(Array(MediaType.APPLICATION_JSON))
+ def getModelCoverUrl(
+ @PathParam("mid") mid: Integer,
+ @Auth sessionUser: Optional[SessionUser]
+ ): Response = {
+ withTransaction(context) { ctx =>
+ val model = requireCoverReadAccess(ctx, mid, sessionUser)
+
+ Option(model.getCoverImage) match {
+ case None => Response.ok(Map("url" -> null)).build()
+ case Some(coverImage) =>
+ val url = CoverImageUtils
+ .openCover(ResourceType.Model, getOwner(ctx, mid).getEmail,
model.getName, coverImage)
+ .map(CoverImageUtils.presignedUrl(_, coverImage))
+ Response.ok(Map("url" -> url.orNull)).build()
+ }
+ }
+ }
+
//
===========================================================================
// Private helpers
//
===========================================================================
+ /** A cover is readable by anyone for a public model, and by read-grantees
otherwise. */
+ private def requireCoverReadAccess(
+ ctx: DSLContext,
+ mid: Integer,
+ sessionUser: Optional[SessionUser]
+ ): Model = {
+ val model = getModelByID(ctx, mid)
+ val requesterUid = if (sessionUser.isPresent)
Some(sessionUser.get().getUid) else None
+
+ if (requesterUid.isEmpty && !model.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)
+ }
+ model
+ }
Review Comment:
This is a verbatim copy of the check in `getDashboardModel`
(`ModelResource.scala:203-214`) — same two branches, same message, same order.
`DatasetResource` inlines the identical five lines twice more, in
`getDatasetCover` and `getDatasetCoverUrl`. That's four copies of one
authorization rule across the two files.
The PR description argues, convincingly, that "a duplicated allowlist
drifts" and moves the extension check into `CoverImageUtils` for exactly that
reason. An authorization check has the same failure mode with a worse blast
radius: if someone later tightens `getDashboardModel` (say, to honour
`is_downloadable` or a ban flag), the cover endpoints keep serving presigned
URLs under the old rule and nothing fails.
Suggest one helper that both paths go through:
```suggestion
/** A cover is readable by anyone for a public model, and by read-grantees
otherwise. */
private def requireCoverReadAccess(
ctx: DSLContext,
mid: Integer,
sessionUser: Optional[SessionUser]
): Model =
requireReadAccess(
ctx,
mid,
if (sessionUser.isPresent) Some(sessionUser.get().getUid) else None
)
/**
* The single read rule for a model: an anonymous caller gets public
models only, a
* signed-in caller goes through `userHasReadAccess`. Shared with
`getDashboardModel`
* so the two cannot drift.
*/
private def requireReadAccess(
ctx: DSLContext,
mid: Integer,
requesterUid: Option[Integer]
): Model = {
val model = getModelByID(ctx, mid)
if (requesterUid.isEmpty && !model.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)
}
model
}
```
`getDashboardModel` then drops its own copy and opens with:
```scala
val targetModel = requireReadAccess(ctx, mid, requesterUid)
```
The two `DatasetResource` cover endpoints can take the same treatment
against `getDashboardDataset`, which would bring it to one copy per resource.
##########
file-service/src/main/scala/org/apache/texera/service/resource/ModelResource.scala:
##########
@@ -147,6 +172,13 @@ object ModelResource {
case class ModelNameModification(mid: Integer, name: String)
+ case class ModelFrameworkModification(mid: Integer, framework: String)
+
+ case class ModelFormatModification(mid: Integer, format: String)
+
+ /** Committed image, relative to the model root, e.g. "v1 - init/cover.jpg".
*/
+ case class CoverImageRequest(coverImage: String)
Review Comment:
This is now byte-identical to `DatasetResource.CoverImageRequest`
(`DatasetResource.scala:275`). Every other resource-agnostic half of the cover
endpoints moved into `CoverImageUtils` in this PR; the request DTO is the one
piece left behind in two places.
Suggest deleting the local copy:
```suggestion
```
and giving it a single home next to the validation it feeds, in
`CoverImageUtils`:
```scala
/** Committed image, relative to the resource root, e.g. "v1 -
init/cover.jpg". */
case class CoverImageRequest(coverImage: String)
```
Then both resources import it — `import
org.apache.texera.service.util.CoverImageUtils.CoverImageRequest` — and the two
endpoint signatures are unchanged. Lower stakes than the allowlist, but it's
the same argument, and doing it now costs three lines while doing it later
means touching both resources again.
--
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]
