aicam commented on PR #6872:
URL: https://github.com/apache/texera/pull/6872#issuecomment-5413967288
### `createModelVersion` returns file paths keyed on the caller's email, not
the owner's
`ModelResource.scala:553` builds the response tree with the requester's
identity:
```scala
LakeFSFileNode.fromLakeFSRepositoryCommittedObjects(
ResourceType.Model,
Map((user.getEmail, modelName, newVersionName) -> fileNodes)
)
```
The owner-email segment of `/model/<ownerEmail>/<name>/<version>/<file>` is
a lookup key, not a label:
`FileResolver.lookupModel` resolves it by joining `MODEL.OWNER_UID ->
USER.EMAIL`. So the only value that
resolves is the owner's. The guard on this endpoint is `userHasWriteAccess`,
not ownership, which makes a
collaborator creating a version the normal case — and the case that breaks.
Reproduced against a build of this branch (`eafd888`), file-service on a
local LakeFS + MinIO + Postgres,
one model owned by `texera` and shared `WRITE` with `[email protected]`:
```
POST /model/8/version/create (owner texera) ->
/model/texera/resnet50/v1 - initial/weights.bin
POST /model/8/version/create (collaborator, WRITE) ->
/model/[email protected]/resnet50/v2 - from-collab/weights.bin
GET /model/8/version/latest (the same version) ->
/model/texera/resnet50/v2 - from-collab/weights.bin
select mid from model join "user" u on u.uid = owner_uid
where u.email = '[email protected]' and model.name = 'resnet50'; -> 0 rows
```
One version, two different paths depending on which endpoint you ask, and
the one from `version/create`
resolves to nothing — `FileNotFoundException` for every file in that
response.
The two sibling read paths already do it correctly: `versionRootFileNodes`
uses `getOwner(ctx, mid).getEmail`
(line 731) and `fetchModelVersionRootFileNodes` uses `model.ownerEmail`
(line 750). So the fix is to make the
third agree:
```scala
Map((getOwner(ctx, mid).getEmail, modelName, newVersionName) -> fileNodes)
```
`getOwner` is already imported and `ctx` is in scope.
### The same line is in `DatasetResource`, and it would be good to fix it
here too
`DatasetResource.scala:507` is character-for-character the same expression,
and `createModelVersion` is
clearly derived from `createDatasetVersion` — this PR inherited the defect
rather than introducing it.
It reproduces identically on datasets:
```
POST /dataset/5/version/create (owner texera) ->
/dataset/texera/tweets/v1 - initial/part1.csv
POST /dataset/5/version/create (collaborator, WRITE) ->
/dataset/[email protected]/tweets/v2 - from-collab/part1.csv
GET /dataset/5/version/latest (the same version) ->
/dataset/texera/tweets/v2 - from-collab/part1.csv
```
It is currently latent in the UI, but only by accident:
`dataset.service.ts:148` does attach those paths to
the returned version, and the live caller
(`dataset-detail.component.ts:306`) just happens to ignore the
response and refetch from `/version/latest`. The other caller
(`user-dataset-version-creator.component.ts:176`) does forward them, but
sits behind `isCreatingVersion`,
which no opener currently sets to `true`. Either of those changing surfaces
the bug.
`DatasetResource` is outside this PR's diff, so this is a request rather
than a blocker — but since it is the
same one-line change (`getOwner(ctx, did).getEmail`, already used at lines
326 and 1038 of that file), fixing
both here keeps the model and dataset versioning paths from diverging, and
avoids leaving a known-bad line to
be copied a third time.
Worth a regression test either way: `ModelUploadResourceSpec` cannot catch
this today, because its only
session user is `sessionUser = new SessionUser(ownerUser)` (line 57) and
line 151 asserts with
`ownerUser.getEmail` — caller and owner are the same identity, so the
assertion passes either way. A case
that creates the version as a second user holding `WRITE` would pin it.
--
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]
