tanishqgandhi1908 commented on code in PR #7937:
URL: https://github.com/apache/texera/pull/7937#discussion_r3874769642
##########
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:
You're right and my comment was wrong — `location = /api/models` is an exact
match, which nginx resolves before any prefix location, so a bare `/api/model`
could never have taken it. Confirmed `LiteLLMModelsResource` has a single
`@GET` at the class path too, so `/api/models/<sub>` isn't reachable either.
Took your wording. The k8s gateway comment repeated the same wrong claim, so
I fixed that one as well, and corrected the PR description.
Noted on the bare `/api/model` not being proxied. Leaving it as-is since
every `@Path` on `ModelResource` has a further segment, but worth remembering
if a root endpoint ever shows up.
##########
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:
Agreed, and it was worse than you counted — `getDashboardDataset` has the
same five lines, so it was five copies, not four. The blast-radius argument is
the right one: a later tightening of `getDashboardModel` would have left the
cover endpoints handing out presigned URLs under the old rule with nothing
failing.
Added `requireReadAccess` to both resources as you suggested.
`getDashboardModel`, `getDashboardDataset` and all three cover endpoints go
through it; `requireCoverReadAccess` stays as the `Optional[SessionUser]` to
`Option[Integer]` adapter. One copy per resource now.
300 tests pass, including the dataset permissions spec — that's the check
that mattered here, since this is authorization code.
--
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]
