From 3ca67d6ad4e6bbe36a41ffa22fc640b964d72da2 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Wed, 17 Jun 2026 13:08:22 -0700 Subject: [PATCH 1/2] Improve checks for file move and pipeline status API --- .../api/webdav/AbstractWebdavResource.java | 9 ++ .../labkey/api/webdav/FileSystemResource.java | 50 ++++++--- .../org/labkey/api/webdav/WebdavResource.java | 8 ++ .../api/webdav/WebdavResourceReadOnly.java | 6 + .../org/labkey/core/webdav/DavController.java | 105 +++++++++++++++++- .../pipeline/status/StatusController.java | 17 +-- 6 files changed, 169 insertions(+), 26 deletions(-) diff --git a/api/src/org/labkey/api/webdav/AbstractWebdavResource.java b/api/src/org/labkey/api/webdav/AbstractWebdavResource.java index 9bb1ccafa7c..44c267477a0 100644 --- a/api/src/org/labkey/api/webdav/AbstractWebdavResource.java +++ b/api/src/org/labkey/api/webdav/AbstractWebdavResource.java @@ -404,6 +404,15 @@ public boolean canDelete(User user, boolean forDelete, /* OUT */ @Nullable List< return perms.contains(DeletePermission.class); } + @Override + public boolean canMove(User user) + { + // A MOVE removes the resource from its source location, so by default it requires the same rights + // as deleting it from there. Resource types where moving and deleting differ (see FileSystemResource) + // override this. + return canDelete(user, true, null); + } + @Override public boolean canRename(User user, boolean forRename) { diff --git a/api/src/org/labkey/api/webdav/FileSystemResource.java b/api/src/org/labkey/api/webdav/FileSystemResource.java index 8a1b316a4f8..be7a88b24b8 100644 --- a/api/src/org/labkey/api/webdav/FileSystemResource.java +++ b/api/src/org/labkey/api/webdav/FileSystemResource.java @@ -507,27 +507,38 @@ public boolean canCreate(User user, boolean forCreate) } + // Shared access checks for canDelete() and canMove(): Delete permission plus a writable file on disk. + // Deliberately does NOT apply the "imported by an assay" restriction, which is specific to outright + // deletion - a file that has been imported may still be relocated within the file root. + private boolean hasDeletePermissionAndWritableFile(User user, boolean forDelete, @Nullable List message) + { + if (!super.canDelete(user, forDelete, message) || !hasFileSystem()) + return false; + File f = getFile(); + if (null == f) + return false; + if (!f.canWrite()) + { + SecurityLogger.log("File.canWrite()==false",user,null,false); + if (forDelete) + { + if (null != message) + message.add("File is not writable on server"); + _log.warn(user.getEmail() + " attempted to delete file that is not writable by LabKey Server. This may be a configuration problem. file: " + f.getPath()); + } + return false; + } + return true; + } + + @Override public boolean canDelete(User user, boolean forDelete, @Nullable List message) { try { - if (!super.canDelete(user, forDelete, message) || !hasFileSystem()) - return false; - File f = getFile(); - if (null == f) + if (!hasDeletePermissionAndWritableFile(user, forDelete, message)) return false; - if (!f.canWrite()) - { - SecurityLogger.log("File.canWrite()==false",user,null,false); - if (forDelete) - { - if (null != message) - message.add("File is not writable on server"); - _log.warn(user.getEmail() + " attempted to delete file that is not writable by LabKey Server. This may be a configuration problem. file: " + f.getPath()); - } - return false; - } // can't delete if already processed if (!getActions(user).isEmpty()) { @@ -544,6 +555,15 @@ public boolean canDelete(User user, boolean forDelete, @Nullable List me } + @Override + public boolean canMove(User user) + { + // A MOVE removes the file from its source location, so it requires Delete permission and a writable + // file. Unlike canDelete(), it does NOT require the file to be eligible for outright deletion: a file + // that has been imported by an assay may still be relocated within the file root. + return hasDeletePermissionAndWritableFile(user, false, null); + } + @Override public boolean canRename(User user, boolean forRename) { diff --git a/api/src/org/labkey/api/webdav/WebdavResource.java b/api/src/org/labkey/api/webdav/WebdavResource.java index 5b503d49f1f..db1592e4ccc 100644 --- a/api/src/org/labkey/api/webdav/WebdavResource.java +++ b/api/src/org/labkey/api/webdav/WebdavResource.java @@ -189,6 +189,14 @@ public interface WebdavResource extends Resource */ boolean canDelete(User user, boolean forDelete); boolean canDelete(User user, boolean forDelete, /* OUT */ List message); + /** + * A MOVE removes the resource from its source location, so it requires Delete permission there. + * Unlike {@link #canDelete}, it does not require the resource to be eligible for outright deletion + * (for example, a file that has been imported by an assay may still be relocated within the file root). + * @param user authenticated user + * @return true if the user has permission and server has capability + */ + boolean canMove(User user); /** * @param user authenticated user * @param forRename true if user wants to rename, false if checking capabilities (affects logging) diff --git a/api/src/org/labkey/api/webdav/WebdavResourceReadOnly.java b/api/src/org/labkey/api/webdav/WebdavResourceReadOnly.java index e5cdd8c3def..fe32e3fc7ce 100644 --- a/api/src/org/labkey/api/webdav/WebdavResourceReadOnly.java +++ b/api/src/org/labkey/api/webdav/WebdavResourceReadOnly.java @@ -291,6 +291,12 @@ public boolean canDelete(User user, boolean forDelete, List message) return false; } + @Override + public boolean canMove(User user) + { + return false; + } + @Override public boolean canRename(User user, boolean forRename) { diff --git a/core/src/org/labkey/core/webdav/DavController.java b/core/src/org/labkey/core/webdav/DavController.java index 4a71e7b134e..5358a18038b 100644 --- a/core/src/org/labkey/core/webdav/DavController.java +++ b/core/src/org/labkey/core/webdav/DavController.java @@ -50,12 +50,17 @@ import org.labkey.api.data.ContainerManager; import org.labkey.api.data.ConvertHelper; import org.labkey.api.data.Sort; +import org.labkey.api.exp.api.DataType; import org.labkey.api.exp.api.ExpData; +import org.labkey.api.exp.api.ExpProtocol; +import org.labkey.api.exp.api.ExpRun; import org.labkey.api.exp.api.ExperimentService; import org.labkey.api.files.FileContentService; import org.labkey.api.miniprofiler.MiniProfiler; import org.labkey.api.miniprofiler.RequestInfo; import org.labkey.api.module.ModuleLoader; +import org.labkey.api.pipeline.PipeRoot; +import org.labkey.api.pipeline.PipelineService; import org.labkey.api.premium.AntiVirusService; import org.labkey.api.premium.PremiumService; import org.labkey.api.query.ValidationException; @@ -3727,8 +3732,8 @@ WebdavStatus doMethod() throws DavException, IOException if (!src.canRead(getUser(), true)) return unauthorized(src); - // MOVE is effectively a delete operation from the source's perspective so confirm access - if (!src.canDelete(getUser(), true)) + // MOVE removes the resource from the source location, so confirm the caller could delete it there. + if (!src.canMove(getUser())) return unauthorized(src); if (exists && !dest.canWrite(getUser(),true) || !exists && !dest.canCreate(getUser(),true)) return unauthorized(dest); @@ -6727,6 +6732,54 @@ public void testMoveActionRequiresTargetCreate() throws Exception 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). + // Such a file may not be deleted outright, but it may still be relocated. This pins that divergence at + // the resource level, independent of the HTTP layer, as an admin who has every relevant permission. + @Test + public void testImportedFileCanMoveButNotDelete() throws Exception + { + File dir = ensureFilesDir(_folder); + File srcFile = writeFile(dir, "imported.txt"); + importFileIntoRun(_folder, srcFile); + + User admin = getAdmin(); + WebdavResource resource = WebdavService.get().lookup(filesPath(_folder).append("imported.txt")); + assertNotNull("Imported file should resolve through the resolver", resource); + assertTrue("Imported file should exist", resource.exists()); + assertFalse("Precondition: a file used by a run must report a non-empty action list", resource.getActions(admin).isEmpty()); + + List messages = new ArrayList<>(); + assertFalse("Admin must NOT be able to delete a file that has been imported by an assay", resource.canDelete(admin, true, messages)); + assertTrue("canDelete() should explain that the file was imported by an assay", messages.stream().anyMatch(m -> m.contains("imported by an assay"))); + 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 + // by DELETE (SC_FORBIDDEN) but relocated by MOVE (SC_CREATED). This is the platform-level analog of the + // PanoramaPublicMoveSkyDocTest scenario and guards against a recurrence of that regression. + @Test + public void testMoveActionMovesImportedFileButDeleteForbidden() throws Exception + { + File dir = ensureFilesDir(_folder); + + // DELETE of a file imported by a run is forbidden, and the file survives. + File importedForDelete = writeFile(dir, "imported-delete.txt"); + importFileIntoRun(_folder, importedForDelete); + MockHttpServletResponse deleteResp = doDelete(_folder, filesPath(_folder).append("imported-delete.txt"), getAdmin()); + 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. + File importedForMove = writeFile(dir, "imported-move.txt"); + importFileIntoRun(_folder, importedForMove); + Path src = filesPath(_folder).append("imported-move.txt"); + Path dest = filesPath(_folder).append("moved.txt"); + MockHttpServletResponse moveResp = doMove(_folder, src, dest, getAdmin()); + assertEquals("MOVE of a file imported by an assay must succeed", HttpServletResponse.SC_CREATED, moveResp.getStatus()); + assertFalse("Source file should no longer exist after a successful move", importedForMove.exists()); + assertTrue("Destination file should exist after a successful move", new File(dir, "moved.txt").exists()); + } + private static Path filesPath(Container c) { return WebdavService.getPath().append(c.getParsedPath()).append(FileContentService.FILES_LINK); @@ -6745,7 +6798,12 @@ private static File ensureFilesDir(Container c) private static File writeFile(File dir) throws IOException { - File f = FileUtil.appendName(dir, "secret.txt"); + return writeFile(dir, "secret.txt"); + } + + private static File writeFile(File dir, String name) throws IOException + { + File f = FileUtil.appendName(dir, name); try (FileOutputStream os = new FileOutputStream(f)) { os.write("secret".getBytes(StandardCharsets.UTF_8)); @@ -6753,6 +6811,47 @@ private static File writeFile(File dir) throws IOException return f; } + // Registers the file as experiment data used by a run, so the resolver's WebdavResource reports a + // non-empty action list - the same state that, in production, blocks deletion of an imported file + // while still permitting a move. + private void importFileIntoRun(Container c, File file) throws Exception + { + ExperimentService expSvc = ExperimentService.get(); + User admin = getAdmin(); + + ExpData data = expSvc.createData(c, new DataType("Data"), file.getName()); + data.setDataFileURI(file.toURI()); + data.save(admin); + + ExpRun run = expSvc.createExperimentRun(c, "import of " + file.getName()); + PipeRoot pipeRoot = PipelineService.get().findPipelineRoot(c); + assertNotNull("Test requires a pipeline root for " + c.getName(), pipeRoot); + run.setFilePathRoot(pipeRoot.getRootPath()); + ExpProtocol protocol = expSvc.ensureSampleDerivationProtocol(admin); + run.setProtocol(protocol); + + ViewBackgroundInfo info = new ViewBackgroundInfo(c, admin, null); + // Wiring the data as a run input is enough for getRunsUsingDatas() to associate the run with the file. + expSvc.saveSimpleExperimentRun(run, Collections.emptyMap(), Map.of(data, "Data"), Collections.emptyMap(), + Collections.emptyMap(), Collections.emptyMap(), info, null, false); + } + + private static MockHttpServletResponse doDelete(Container sourceContainer, Path srcResource, User user) throws Exception + { + String srcWebdav = srcResource.toString(); + String servletPath = "/" + WebdavService.getServletPath(); + + HttpServletRequest base = ViewServlet.mockRequest("DELETE", new ActionURL(name, "delete", sourceContainer), user, Map.of(), null); + MockHttpServletRequest req = (MockHttpServletRequest) base; + req.setServletPath(servletPath); + req.setPathInfo(srcWebdav.substring(servletPath.length())); + req.setRequestURI(srcWebdav); + + MockHttpServletResponse resp = new MockHttpServletResponse(); + new WebdavServlet(false).service(req, resp); + return resp; + } + private static MockHttpServletResponse doMove(Container sourceContainer, Path srcResource, Path destResource, User user) throws Exception { String srcWebdav = srcResource.toString(); diff --git a/pipeline/src/org/labkey/pipeline/status/StatusController.java b/pipeline/src/org/labkey/pipeline/status/StatusController.java index 1e5050799a8..eacd8bf1fc8 100644 --- a/pipeline/src/org/labkey/pipeline/status/StatusController.java +++ b/pipeline/src/org/labkey/pipeline/status/StatusController.java @@ -484,7 +484,7 @@ public Object execute(StatusDetailsForm form, BindException errors) throws Excep Container c = getContainerCheckAdmin(); PipelineStatusFile psf = getStatusFile(form.getRowId()); - if (psf == null || !getContainer().equals(psf.lookupContainer())) + if (psf == null || psf.lookupContainer() == null || !psf.lookupContainer().hasPermission(getUser(), ReadPermission.class)) throw new NotFoundException("Could not find status file for rowId " + form.getRowId()); var status = StatusDetailsBean.create(c, psf, form.getOffset(), form.getCount()); @@ -1120,15 +1120,16 @@ public void testStatusDetailsContainerScoping() throws Exception ActionURL foreignUrl = new ActionURL(StatusDetailsAction.class, folderA).addParameter("rowId", String.valueOf(rowId)); - // The API is scoped to its own container: addressing B's job through folder A is 404, regardless of the - // caller's rights in B. This is the case that fails without the fix (the unscoped action would serve B's - // job through folder A). - // A caller who can read folder A but NOT folder B: + // 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 + // someone without rights to its container. This is the case that fails without the fix. assertStatus(HttpServletResponse.SC_NOT_FOUND, get(foreignUrl, readerA)); - // ...and a site admin, who CAN read folder B, still gets 404 through folder A (no cross-container redirect). - assertStatus(HttpServletResponse.SC_NOT_FOUND, get(foreignUrl, admin)); + // A site admin, who CAN read folder B, is served B's job through folder A -- the supported cross-container reference. + assertStatus(HttpServletResponse.SC_OK, get(foreignUrl, admin)); - // Positive control: addressing the job through its own container still succeeds. + // Positive control: addressing the job through its own container also succeeds for a caller who can read it. ActionURL ownUrl = new ActionURL(StatusDetailsAction.class, folderB).addParameter("rowId", String.valueOf(rowId)); assertStatus(HttpServletResponse.SC_OK, get(ownUrl, admin)); } From 9dbcc5bbf15239a77a227b8e10c84d15b408b167 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Wed, 17 Jun 2026 13:26:59 -0700 Subject: [PATCH 2/2] Handle overrides better --- .../org/labkey/core/webdav/DavController.java | 32 +++++++++++++++++++ .../experiment/ScriptsResourceProvider.java | 6 ++++ .../pipeline/PipelineWebdavProvider.java | 8 +++++ 3 files changed, 46 insertions(+) diff --git a/core/src/org/labkey/core/webdav/DavController.java b/core/src/org/labkey/core/webdav/DavController.java index 5358a18038b..9af60ed35ff 100644 --- a/core/src/org/labkey/core/webdav/DavController.java +++ b/core/src/org/labkey/core/webdav/DavController.java @@ -6780,11 +6780,43 @@ public void testMoveActionMovesImportedFileButDeleteForbidden() throws Exception assertTrue("Destination file should exist after a successful move", new File(dir, "moved.txt").exists()); } + // Regression guard for the canMove()/canDelete() divergence introduced alongside the MOVE fix. A + // FileSystemResource subclass whose canDelete() forbids deletion outright (PipelineFolderResource returns + // false) must also forbid MOVE. Otherwise the new FileSystemResource.canMove() default - Delete permission + // plus a writable file - would let an admin relocate a node that was never deletable, because that default + // calls super.canDelete() and so bypasses the subclass override entirely. + @Test + public void testNonDeletableNodeIsNotMovable() throws Exception + { + // Configure an explicit pipeline root so the @pipeline webdav node resolves to a PipelineFolderResource. + File fileRoot = ensureFilesDir(_folder).getParentFile(); + File pipelineDir = new File(fileRoot, "pipelineOverrideRoot"); + if (!pipelineDir.exists()) + assertTrue("Test requires a writable pipeline root directory", pipelineDir.mkdirs()); + PipelineService.get().setPipelineRoot(getAdmin(), _folder, PipelineService.PRIMARY_ROOT, false, pipelineDir.toURI()); + + WebdavResource pipelineNode = WebdavService.get().lookup(pipelinePath(_folder)); + assertNotNull("Test requires the @pipeline webdav node to resolve to a PipelineFolderResource", pipelineNode); + assertNotNull("The pipeline node must be backed by a writable file root", pipelineNode.getFile()); + + User admin = getAdmin(); + // An admin has Delete permission and the pipeline root is writable, so the inherited + // FileSystemResource.canMove() would return true. The override must keep canMove() aligned with the + // categorical canDelete()==false, so the node remains immovable. + assertFalse("The pipeline node must never be deletable", pipelineNode.canDelete(admin, true, null)); + assertFalse("A node that is not deletable must also not be movable", pipelineNode.canMove(admin)); + } + private static Path filesPath(Container c) { return WebdavService.getPath().append(c.getParsedPath()).append(FileContentService.FILES_LINK); } + private static Path pipelinePath(Container c) + { + return WebdavService.getPath().append(c.getParsedPath()).append(FileContentService.PIPELINE_LINK); + } + private static File ensureFilesDir(Container c) { WebdavResource filesNode = WebdavService.get().lookup(filesPath(c)); diff --git a/experiment/src/org/labkey/experiment/ScriptsResourceProvider.java b/experiment/src/org/labkey/experiment/ScriptsResourceProvider.java index 0ec70c020ab..0cbc665d68e 100644 --- a/experiment/src/org/labkey/experiment/ScriptsResourceProvider.java +++ b/experiment/src/org/labkey/experiment/ScriptsResourceProvider.java @@ -146,6 +146,12 @@ public boolean canList(User user, boolean forRead) return hasAccess(user); } + @Override + public boolean canMove(User user) + { + return hasAccess(user); + } + @Override public boolean shouldIndex() { diff --git a/pipeline/src/org/labkey/pipeline/PipelineWebdavProvider.java b/pipeline/src/org/labkey/pipeline/PipelineWebdavProvider.java index dcd298bc74a..dc256cb85e6 100644 --- a/pipeline/src/org/labkey/pipeline/PipelineWebdavProvider.java +++ b/pipeline/src/org/labkey/pipeline/PipelineWebdavProvider.java @@ -106,6 +106,14 @@ public boolean canDelete(User user, boolean forDelete, List msg) return false; } + @Override + public boolean canMove(User user) + { + // The pipeline folder node is never deletable (see canDelete()), so it must not be movable either. + // FileSystemResource.canMove() relaxes only the assay-import restriction, not this categorical block. + return false; + } + @Override protected boolean hasAccess(User user) {