tanishqgandhi1908 commented on code in PR #7937:
URL: https://github.com/apache/texera/pull/7937#discussion_r3874770882
##########
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
--
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]
