Improve checks for file move and pipeline status API#7768
Merged
Conversation
labkey-susanh
approved these changes
Jun 17, 2026
| assertTrue("Destination file should exist after a successful move", FileUtil.appendName(targetDir, "moved.txt").exists()); | ||
| } | ||
|
|
||
| // A.1: canDelete() and canMove() diverge ONLY for a file that backs experiment data (used by a run). |
Contributor
There was a problem hiding this comment.
A.1:? What's that referring to?
| assertTrue("Admin MUST still be able to move a file that has been imported by an assay", resource.canMove(admin)); | ||
| } | ||
|
|
||
| // A.2: The same divergence end-to-end through WebdavServlet. A file that backs experiment data is refused |
| assertEquals("DELETE of a file imported by an assay must be forbidden", HttpServletResponse.SC_FORBIDDEN, deleteResp.getStatus()); | ||
| assertTrue("File must still exist after a forbidden delete", importedForDelete.exists()); | ||
|
|
||
| // MOVE of an equally-imported file succeeds, relocating it within the file root. |
Contributor
There was a problem hiding this comment.
equally-imported? What does that mean?
|
|
||
| PipelineStatusFile psf = getStatusFile(form.getRowId()); | ||
| if (psf == null || !getContainer().equals(psf.lookupContainer())) | ||
| if (psf == null || psf.lookupContainer() == null || !psf.lookupContainer().hasPermission(getUser(), ReadPermission.class)) |
Contributor
There was a problem hiding this comment.
The messaging for the case when the user does not have permission will not be quite right here. Seems like it should throw and UnauthorizedException instead.
| // This API authorizes against the job's OWN container (folder B), not the container in the URL, so it | ||
| // intentionally supports referencing a job from another container -- but only for a caller who can read the | ||
| // container the job actually lives in. | ||
| // A caller who can read folder A but NOT folder B must still get 404: the job must not be revealed to |
Contributor
There was a problem hiding this comment.
I would say the 404 is less appropriate here (see comment above), but I won't insist.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale
Previous scoping changes weren't correct given some important use cases for compability.
Changes
Tasks 📍
Manual Testing