From 6b99dec1ce2e770f6cf42fa12169593858f67db1 Mon Sep 17 00:00:00 2001 From: cnathe Date: Thu, 27 Aug 2026 08:57:39 -0500 Subject: [PATCH 1/3] GitHub Issue #1391: SignalDataImportTask update to isUnderAnyPipelineRoot to check for job user ReadPermission in container --- .../pipeline/SignalDataImportTask.java | 29 +++-- .../signaldata/SignalDataFileWatcherTest.java | 104 +++++++++++++++++- 2 files changed, 118 insertions(+), 15 deletions(-) diff --git a/signalData/src/org/labkey/signaldata/pipeline/SignalDataImportTask.java b/signalData/src/org/labkey/signaldata/pipeline/SignalDataImportTask.java index 571c7c910..34589b98f 100644 --- a/signalData/src/org/labkey/signaldata/pipeline/SignalDataImportTask.java +++ b/signalData/src/org/labkey/signaldata/pipeline/SignalDataImportTask.java @@ -31,6 +31,8 @@ import org.labkey.api.query.ValidationException; import org.labkey.api.reader.DataLoader; import org.labkey.api.reader.DataLoaderFactory; +import org.labkey.api.security.User; +import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.util.DateUtil; import org.labkey.api.util.FileType; import org.labkey.api.util.FileUtil; @@ -170,9 +172,9 @@ public RecordedActionSet run() { Path resolvedPath = Path.of(dataFilePath).toAbsolutePath().normalize(); - if (!isUnderAnyPipelineRoot(resolvedPath)) + if (!isReadableUnderAnyPipelineRoot(job.getUser(), resolvedPath)) { - log.error("DataFile '{}' is not under a server-managed pipeline root", dataFilePath); + log.error("DataFile '{}' is not under a pipeline root readable by this user", dataFilePath); row.remove(INPUT_DATA_FILE); continue; } @@ -330,34 +332,37 @@ private FileLike getTargetFolder(Container container, Logger log) throws IOExcep } /** - * Determine whether the given path falls under a pipeline root for some container, using the same semantics as - * {@link PipelineService#findPipelineRoot(Container)} (which includes the default file-root fallback, not just - * explicitly configured pipeline roots). First try to resolve the path directly to its owning container(s); if - * that comes up empty (e.g. a container with a custom, non-default file root that the path-resolution logic does - * not yet handle), fall back to scanning every container's pipeline root. + * Determine whether the given path falls under a pipeline root for some container that the user can read, using the + * same semantics as {@link PipelineService#findPipelineRoot(Container)} (which includes the default file-root + * fallback, not just explicitly configured pipeline roots). First try to resolve the path directly to its owning + * container(s); if that comes up empty (e.g. a container with a custom, non-default file root that the + * path-resolution logic does not yet handle), fall back to scanning every container's pipeline root. */ - private boolean isUnderAnyPipelineRoot(Path resolvedPath) + private boolean isReadableUnderAnyPipelineRoot(User user, Path resolvedPath) { for (Container c : FileContentService.get().getContainersForFilePath(resolvedPath)) { - if (isUnderPipelineRoot(c, resolvedPath)) + if (isReadableUnderPipelineRoot(c, user, resolvedPath)) return true; } // Path could not be resolved to a container directly; fall back to scanning all containers for (Container c : ContainerManager.getAllChildren(ContainerManager.getRoot())) { - if (isUnderPipelineRoot(c, resolvedPath)) + if (isReadableUnderPipelineRoot(c, user, resolvedPath)) return true; } return false; } - private boolean isUnderPipelineRoot(Container container, Path resolvedPath) + /** + * GitHub Issue #1391: reaching a path through a container's pipeline root additionally requires ReadPermission in that container. + */ + private boolean isReadableUnderPipelineRoot(Container container, User user, Path resolvedPath) { PipeRoot root = PipelineService.get().findPipelineRoot(container); - return root != null && root.isUnderRoot(resolvedPath); + return root != null && root.isUnderRoot(resolvedPath) && root.hasPermission(container, user, ReadPermission.class); } public static class Factory extends AbstractTaskFactory diff --git a/signalData/test/src/org/labkey/test/tests/signaldata/SignalDataFileWatcherTest.java b/signalData/test/src/org/labkey/test/tests/signaldata/SignalDataFileWatcherTest.java index 4b214dc7f..3b33c4dce 100644 --- a/signalData/test/src/org/labkey/test/tests/signaldata/SignalDataFileWatcherTest.java +++ b/signalData/test/src/org/labkey/test/tests/signaldata/SignalDataFileWatcherTest.java @@ -22,12 +22,18 @@ import org.junit.experimental.categories.Category; import org.labkey.api.util.FileUtil; import org.labkey.api.util.Path; +import org.labkey.remoteapi.query.ContainerFilter; +import org.labkey.remoteapi.query.Filter; import org.labkey.test.BaseWebDriverTest; import org.labkey.test.Locator; +import org.labkey.test.TestFileUtils; +import org.labkey.test.TestTimeoutException; import org.labkey.test.categories.Git; import org.labkey.test.components.pipeline.PipelineTriggerWizard; import org.labkey.test.pages.signaldata.SignalDataAssayBeginPage; +import org.labkey.test.util.ApiPermissionsHelper; import org.labkey.test.util.DataRegionTable; +import org.labkey.test.util.PermissionsHelper; import org.labkey.test.util.PipelineStatusTable; import org.labkey.test.util.PortalHelper; import org.labkey.test.util.core.webdav.WebDavUploadHelper; @@ -44,6 +50,7 @@ import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; +import static org.labkey.test.util.PermissionsHelper.FOLDER_ADMIN_ROLE; @Category({Git.class}) @BaseWebDriverTest.ClassTimeout(minutes = 10) @@ -66,6 +73,15 @@ public class SignalDataFileWatcherTest extends BaseWebDriverTest // referenced by WebDAV path rather than by bare name. private static final String ALT_DATA_DIR = "altData"; + // A separate project, whose pipeline root is disjoint from the test project's, holding a data file that an import + // running in the test project must refuse to reach (GitHub Issue #1391). + private static final String FOREIGN_PROJECT = PROJECT_NAME + "Other"; + private static final String FOREIGN_FILENAME = "FOREIGN01.TXT"; + + // Folder admin in the test project with no role in FOREIGN_PROJECT. A trigger's "Run as" user becomes the import + // job's user, which is how the import runs without the site admin's read access to every container. + private static final String LIMITED_USER = "signaldata_limited@signaldatafilewatcher.test"; + @Nullable @Override protected final String getProjectName() @@ -101,6 +117,21 @@ public static void doSetup() throws Exception uploadHelper.uploadFile(file, ""); uploadHelper.uploadFile(file, ALT_DATA_DIR); } + + test._containerHelper.createProject(FOREIGN_PROJECT, null); + WebDavUploadHelper foreignUploadHelper = new WebDavUploadHelper(FOREIGN_PROJECT); + foreignUploadHelper.putText(FOREIGN_FILENAME, "foreign signal data"); + assertTrue("Test requires a data file in the foreign project's file root", foreignUploadHelper.fileExists(FOREIGN_FILENAME)); + + test._userHelper.createUser(LIMITED_USER); + new ApiPermissionsHelper(test).addMemberToRole(LIMITED_USER, FOLDER_ADMIN_ROLE, PermissionsHelper.MemberType.user, test.getProjectName()); + } + + @Override + protected void doCleanup(boolean afterTest) throws TestTimeoutException + { + _userHelper.deleteUsers(false, LIMITED_USER); + super.doCleanup(afterTest); } @Before @@ -218,7 +249,67 @@ public void testDataFileOutsidePipelineRootIsRejected() throws IOException assertTrue("Expected one of the pipeline jobs to be in error, statuses were: " + statuses, errorRow >= 0); statusTable.clickStatusLink(errorRow) - .waitForError(String.format("DataFile '%s' is not under a server-managed pipeline root", outsidePath)); + .waitForError(String.format("DataFile '%s' is not under a pipeline root readable by this user", outsidePath)); + + // The rejection is logged as an error and surfaces in the server error log; account for it so the harness's + // post-test error check does not fail this test. + deleteAllPipelineJobs(); + checkExpectedErrors(1); + } + + /** + * GitHub Issue #1391: an absolute server-side path may resolve through any container's pipeline root, but only one + * the job's user can read. Runs the import as LIMITED_USER, who has no role in the foreign project. + */ + @Test + public void testServerPathDataFileFromUnreadableProjectIsRejected() throws IOException + { + File foreignFile = FileUtil.appendName(TestFileUtils.getDefaultFileRoot(FOREIGN_PROJECT), FOREIGN_FILENAME); + assertTrue("Test requires the foreign data file to exist at " + foreignFile, foreignFile.exists()); + + verifyForeignDataFileRejected("foreignServerPathDatafiles.tsv", "Foreign server path trigger", + _userHelper.getDisplayNameForEmail(LIMITED_USER), foreignFile.getAbsolutePath(), + "is not under a pipeline root readable by this user"); + } + + /** + * Imports a metadata file holding one resolvable row and one row pointing at the foreign project, then verifies the + * foreign row was rejected. + * + * @param runAsDisplayName display name for the trigger's "Run as" user, or null to run as the current user + */ + private void verifyForeignDataFileRejected(String metadataFileName, String triggerName, @Nullable String runAsDisplayName, + String foreignDataFilePath, String expectedError) throws IOException + { + List> rows = new ArrayList<>(); + rows.add(List.of("Name", "DataFile")); + rows.add(List.of(RESULT_FILENAME_1, RESULT_FILENAME_1)); + rows.add(List.of(FOREIGN_FILENAME, foreignDataFilePath)); + File metadataFile = TestDataUtils.writeRowsToTsv(metadataFileName, rows); + + log("Configure a file watcher trigger for the Signal Data import pipeline"); + createImportTrigger(triggerName, metadataFile.getName(), runAsDisplayName); + + log("Drop a metadata file referencing a data file in another project"); + goToProjectHome(); + _fileBrowserHelper.dragDropUpload(metadataFile); + + log("Wait for the file watcher import job to finish"); + goToDataPipeline(); + waitForPipelineJobsToFinish(2); + + log("Verify the import job logged the rejection of the cross-project data file"); + PipelineStatusTable statusTable = new PipelineStatusTable(getDriver()); + List statuses = statusTable.getColumnDataAsText("Status"); + int errorRow = statuses.indexOf("ERROR"); + assertTrue("Expected one of the pipeline jobs to be in error, statuses were: " + statuses, errorRow >= 0); + + statusTable.clickStatusLink(errorRow) + .waitForError(String.format("DataFile '%s' %s", foreignDataFilePath, expectedError)); + + assertEquals("A data file from another project must not be imported into this project", 0, + executeSelectRowCommand("exp", "Data", ContainerFilter.CurrentAndSubfolders, "/" + getProjectName(), + List.of(new Filter("Name", FOREIGN_FILENAME))).getRowCount().intValue()); // The rejection is logged as an error and surfaces in the server error log; account for it so the harness's // post-test error check does not fail this test. @@ -227,6 +318,11 @@ public void testDataFileOutsidePipelineRootIsRejected() throws IOException } private void createImportTrigger(String name, String filePattern) + { + createImportTrigger(name, filePattern, null); + } + + private void createImportTrigger(String name, String filePattern, @Nullable String runAsDisplayName) { goToProjectHome(); goToFolderManagement().goToImportTab(); @@ -235,8 +331,10 @@ private void createImportTrigger(String name, String filePattern) PipelineTriggerWizard wizard = new PipelineTriggerWizard(getDriver()); wizard.setName(name) .setTask(IMPORT_PIPELINE_TASK) - .setEnabled(true) - .goToConfiguration() + .setEnabled(true); + if (runAsDisplayName != null) + wizard.setUsername(runAsDisplayName); + wizard.goToConfiguration() .setLocation(".") .setFilePattern(filePattern) // The 'protocolName' custom field declared by SignalDataImportTask's pipeline; the wizard binds From 5ae09e50b4dc12fda7abe45e87f7eb09e6094446 Mon Sep 17 00:00:00 2001 From: cnathe Date: Thu, 27 Aug 2026 09:57:41 -0500 Subject: [PATCH 2/3] Apply the container permissions check to the WebDav resolution branch as well --- .../pipeline/SignalDataImportTask.java | 38 ++-- .../signaldata/SignalDataFileWatcherTest.java | 212 ++++++++++++++---- 2 files changed, 192 insertions(+), 58 deletions(-) diff --git a/signalData/src/org/labkey/signaldata/pipeline/SignalDataImportTask.java b/signalData/src/org/labkey/signaldata/pipeline/SignalDataImportTask.java index 34589b98f..bc818a989 100644 --- a/signalData/src/org/labkey/signaldata/pipeline/SignalDataImportTask.java +++ b/signalData/src/org/labkey/signaldata/pipeline/SignalDataImportTask.java @@ -160,29 +160,37 @@ public RecordedActionSet run() } else { - // check to see if it's a webdav url WebdavResource resource = WebdavService.get().lookup(dataFilePath); - if (resource != null) + File resourceFile = resource != null ? resource.getFile() : null; + Path resolvedPath = (resourceFile != null ? resourceFile.toPath() : Path.of(dataFilePath)) + .toAbsolutePath().normalize(); + + // GitHub Issue #1391: a webdav resource's own policy can be stricter than its container's, since a + // pipeline root carries its own ACL + if (resourceFile != null && !resource.canRead(job.getUser(), true)) { - sourceFile = FileSystemLike.wrapFile(resource.getFile()); + log.error("DataFile '{}' is not readable by this user", dataFilePath); + row.remove(INPUT_DATA_FILE); + continue; } - // check to see if it's a server-side path - if (sourceFile == null) + if (!isReadableUnderAnyPipelineRoot(job.getUser(), resolvedPath)) { - Path resolvedPath = Path.of(dataFilePath).toAbsolutePath().normalize(); - - if (!isReadableUnderAnyPipelineRoot(job.getUser(), resolvedPath)) - { - log.error("DataFile '{}' is not under a pipeline root readable by this user", dataFilePath); - row.remove(INPUT_DATA_FILE); - continue; - } - sourceFile = FileSystemLike.wrapFile(resolvedPath.toFile()); + log.error("DataFile '{}' is not under a pipeline root readable by this user", dataFilePath); + row.remove(INPUT_DATA_FILE); + continue; } + sourceFile = FileSystemLike.wrapFile(resolvedPath.toFile()); + } + + if (sourceFile == null) + { + log.error("Unable to resolve DataFile '{}' for row '{}'", dataFilePath, name); + row.remove(INPUT_DATA_FILE); + continue; } - if (sourceFile != null && !sourceFile.exists()) + if (!sourceFile.exists()) { log.info("Data file not found: {}", sourceFile.getPath()); row.remove(INPUT_DATA_FILE); diff --git a/signalData/test/src/org/labkey/test/tests/signaldata/SignalDataFileWatcherTest.java b/signalData/test/src/org/labkey/test/tests/signaldata/SignalDataFileWatcherTest.java index 3b33c4dce..12adaa82b 100644 --- a/signalData/test/src/org/labkey/test/tests/signaldata/SignalDataFileWatcherTest.java +++ b/signalData/test/src/org/labkey/test/tests/signaldata/SignalDataFileWatcherTest.java @@ -49,8 +49,11 @@ import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; import static org.labkey.test.util.PermissionsHelper.FOLDER_ADMIN_ROLE; +import static org.labkey.test.util.PermissionsHelper.READER_ROLE; @Category({Git.class}) @BaseWebDriverTest.ClassTimeout(minutes = 10) @@ -73,14 +76,25 @@ public class SignalDataFileWatcherTest extends BaseWebDriverTest // referenced by WebDAV path rather than by bare name. private static final String ALT_DATA_DIR = "altData"; - // A separate project, whose pipeline root is disjoint from the test project's, holding a data file that an import - // running in the test project must refuse to reach (GitHub Issue #1391). + private static final String METADATA_FILE_BY_NAME = "datafiles.tsv"; + private static final String METADATA_FILE_WEBDAV = "webdavDatafiles.tsv"; + private static final String METADATA_FILE_OUTSIDE_ROOT = "outsideRoot.tsv"; + private static final String METADATA_FILE_DENIED_WEBDAV = "crossProjectDeniedWebDav.tsv"; + private static final String METADATA_FILE_DENIED_SERVER_PATH = "crossProjectDeniedServerPath.tsv"; + private static final String METADATA_FILE_ALLOWED_WEBDAV = "crossProjectAllowedWebDav.tsv"; + private static final String METADATA_FILE_ALLOWED_SERVER_PATH = "crossProjectAllowedServerPath.tsv"; + + // A separate project, whose pipeline root is disjoint from the test project's, holding data files that an import + // running in the test project reaches only when its user can read that project (GitHub Issue #1391). private static final String FOREIGN_PROJECT = PROJECT_NAME + "Other"; - private static final String FOREIGN_FILENAME = "FOREIGN01.TXT"; + private static final String FOREIGN_REJECTED_FILENAME = "FOREIGN01.TXT"; + private static final String FOREIGN_WEBDAV_FILENAME = "FOREIGN02.TXT"; + private static final String FOREIGN_SERVER_PATH_FILENAME = "FOREIGN03.TXT"; - // Folder admin in the test project with no role in FOREIGN_PROJECT. A trigger's "Run as" user becomes the import - // job's user, which is how the import runs without the site admin's read access to every container. + // Both are folder admin in the test project, which the file watcher requires of a trigger's "Run as" user; that + // user becomes the import job's user. Only CROSS_READER_USER can read FOREIGN_PROJECT. private static final String LIMITED_USER = "signaldata_limited@signaldatafilewatcher.test"; + private static final String CROSS_READER_USER = "signaldata_crossreader@signaldatafilewatcher.test"; @Nullable @Override @@ -98,6 +112,8 @@ public List getAssociatedModules() @BeforeClass public static void doSetup() throws Exception { + assertMetadataFileNamesAreDistinct(); + SignalDataFileWatcherTest test = getCurrentTest(); SignalDataInitializer initializer = new SignalDataInitializer(test, test.getProjectName()); initializer.setupProject(); @@ -120,18 +136,26 @@ public static void doSetup() throws Exception test._containerHelper.createProject(FOREIGN_PROJECT, null); WebDavUploadHelper foreignUploadHelper = new WebDavUploadHelper(FOREIGN_PROJECT); - foreignUploadHelper.putText(FOREIGN_FILENAME, "foreign signal data"); - assertTrue("Test requires a data file in the foreign project's file root", foreignUploadHelper.fileExists(FOREIGN_FILENAME)); + for (String dataFile : List.of(FOREIGN_REJECTED_FILENAME, FOREIGN_WEBDAV_FILENAME, FOREIGN_SERVER_PATH_FILENAME)) + { + foreignUploadHelper.putText(dataFile, "foreign signal data"); + assertTrue("Test requires a data file in the foreign project's file root: " + dataFile, foreignUploadHelper.fileExists(dataFile)); + } test._userHelper.createUser(LIMITED_USER); - new ApiPermissionsHelper(test).addMemberToRole(LIMITED_USER, FOLDER_ADMIN_ROLE, PermissionsHelper.MemberType.user, test.getProjectName()); + test._userHelper.createUser(CROSS_READER_USER); + ApiPermissionsHelper permissionsHelper = new ApiPermissionsHelper(test); + permissionsHelper.addMemberToRole(LIMITED_USER, FOLDER_ADMIN_ROLE, PermissionsHelper.MemberType.user, test.getProjectName()); + permissionsHelper.addMemberToRole(CROSS_READER_USER, FOLDER_ADMIN_ROLE, PermissionsHelper.MemberType.user, test.getProjectName()); + permissionsHelper.addMemberToRole(CROSS_READER_USER, READER_ROLE, PermissionsHelper.MemberType.user, FOREIGN_PROJECT); } @Override protected void doCleanup(boolean afterTest) throws TestTimeoutException { - _userHelper.deleteUsers(false, LIMITED_USER); - super.doCleanup(afterTest); + _userHelper.deleteUsers(false, LIMITED_USER, CROSS_READER_USER); + _containerHelper.deleteProject(FOREIGN_PROJECT, afterTest); + _containerHelper.deleteProject(getProjectName(), afterTest); } @Before @@ -149,7 +173,7 @@ public void preTest() throws Exception @Test public void testMetadataFileWatcherImport() { - File metadataFile = getFile("RunsMetadata/datafiles.tsv"); + File metadataFile = getFile("RunsMetadata/" + METADATA_FILE_BY_NAME); log("Configure a file watcher trigger for the Signal Data import pipeline"); createImportTrigger("Signal Data import trigger", metadataFile.getName()); @@ -189,7 +213,7 @@ public void testWebDavDataFilePaths() throws IOException rows.add(List.of(RESULT_FILENAME_1, webDavPath(RESULT_FILENAME_1), "StringOne", "1")); rows.add(List.of(RESULT_FILENAME_2, webDavPath(RESULT_FILENAME_2), "StringTwo", "2")); rows.add(List.of(RESULT_FILENAME_3, webDavPath(RESULT_FILENAME_3), "StringThree", "3")); - File metadataFile = TestDataUtils.writeRowsToTsv("webdavDatafiles.tsv", rows); + File metadataFile = TestDataUtils.writeRowsToTsv(METADATA_FILE_WEBDAV, rows); log("Configure a file watcher trigger for the Signal Data import pipeline"); createImportTrigger("WebDav paths trigger", metadataFile.getName()); @@ -229,7 +253,7 @@ public void testDataFileOutsidePipelineRootIsRejected() throws IOException rows.add(List.of("Name", "DataFile")); rows.add(List.of(RESULT_FILENAME_1, RESULT_FILENAME_1)); rows.add(List.of("OutsideRoot.TXT", outsidePath)); - File metadataFile = TestDataUtils.writeRowsToTsv("outsideRoot.tsv", rows); + File metadataFile = TestDataUtils.writeRowsToTsv(METADATA_FILE_OUTSIDE_ROOT, rows); log("Configure a file watcher trigger for the Signal Data import pipeline"); createImportTrigger("Outside root trigger", metadataFile.getName()); @@ -258,37 +282,116 @@ public void testDataFileOutsidePipelineRootIsRejected() throws IOException } /** - * GitHub Issue #1391: an absolute server-side path may resolve through any container's pipeline root, but only one - * the job's user can read. Runs the import as LIMITED_USER, who has no role in the foreign project. + * GitHub Issue #1391: a data file in a project the job's user cannot read must not be pulled into this one. + */ + @Test + public void testWebDavDataFileFromUnreadableProjectIsRejected() throws IOException + { + verifyForeignDataFileRejected(METADATA_FILE_DENIED_WEBDAV, "Unreadable WebDav path trigger", + FOREIGN_REJECTED_FILENAME, foreignWebDavPath(FOREIGN_REJECTED_FILENAME)); + } + + /** + * GitHub Issue #1391: same as above for an absolute server-side path. */ @Test public void testServerPathDataFileFromUnreadableProjectIsRejected() throws IOException { - File foreignFile = FileUtil.appendName(TestFileUtils.getDefaultFileRoot(FOREIGN_PROJECT), FOREIGN_FILENAME); - assertTrue("Test requires the foreign data file to exist at " + foreignFile, foreignFile.exists()); + verifyForeignDataFileRejected(METADATA_FILE_DENIED_SERVER_PATH, "Unreadable server path trigger", + FOREIGN_REJECTED_FILENAME, foreignFile(FOREIGN_REJECTED_FILENAME).getAbsolutePath()); + } + + /** + * GitHub Issue #1391: resolving a data file through another project's pipeline root is intended behavior when the + * job's user can read that project. + */ + @Test + public void testWebDavDataFileFromReadableProjectIsImported() throws IOException + { + verifyForeignDataFileImported(METADATA_FILE_ALLOWED_WEBDAV, "Readable WebDav path trigger", + FOREIGN_WEBDAV_FILENAME, foreignWebDavPath(FOREIGN_WEBDAV_FILENAME)); + } + + /** + * GitHub Issue #1391: same as above for an absolute server-side path. + */ + @Test + public void testServerPathDataFileFromReadableProjectIsImported() throws IOException + { + verifyForeignDataFileImported(METADATA_FILE_ALLOWED_SERVER_PATH, "Readable server path trigger", + FOREIGN_SERVER_PATH_FILENAME, foreignFile(FOREIGN_SERVER_PATH_FILENAME).getAbsolutePath()); + } + + /** + * Runs the cross-project import as LIMITED_USER and verifies the foreign row was rejected. The assertion matches + * only the data file path, since either the resource ACL or the pipeline root containment check can reject it + * depending on how the path was spelled. + */ + private void verifyForeignDataFileRejected(String metadataFileName, String triggerName, String foreignFileName, + String foreignDataFilePath) throws IOException + { + runForeignDataFileImport(metadataFileName, triggerName, LIMITED_USER, foreignFileName, foreignDataFilePath); + + log("Verify the import job logged the rejection of the cross-project data file"); + PipelineStatusTable statusTable = new PipelineStatusTable(getDriver()); + List statuses = statusTable.getColumnDataAsText("Status"); + int errorRow = statuses.indexOf("ERROR"); + assertTrue("Expected one of the pipeline jobs to be in error, statuses were: " + statuses, errorRow >= 0); + + statusTable.clickStatusLink(errorRow) + .waitForError(String.format("DataFile '%s'", foreignDataFilePath)); + + assertEquals("A data file from an unreadable project must not be imported into this project", 0, + foreignDataRowCount(foreignFileName)); + + // The rejection is logged as an error and surfaces in the server error log; account for it so the harness's + // post-test error check does not fail this test. + deleteAllPipelineJobs(); + checkExpectedErrors(1); + } + + /** + * Runs the same cross-project import as CROSS_READER_USER, who can read the foreign project, and verifies both rows + * imported and the foreign data file was registered in this project. + */ + private void verifyForeignDataFileImported(String metadataFileName, String triggerName, String foreignFileName, + String foreignDataFilePath) throws IOException + { + runForeignDataFileImport(metadataFileName, triggerName, CROSS_READER_USER, foreignFileName, foreignDataFilePath); - verifyForeignDataFileRejected("foreignServerPathDatafiles.tsv", "Foreign server path trigger", - _userHelper.getDisplayNameForEmail(LIMITED_USER), foreignFile.getAbsolutePath(), - "is not under a pipeline root readable by this user"); + log("Verify no pipeline job failed"); + List statuses = new PipelineStatusTable(getDriver()).getColumnDataAsText("Status"); + assertFalse("No pipeline job should be in error, statuses were: " + statuses, statuses.contains("ERROR")); + + log("Verify the run imported both the local and the cross-project data file"); + SignalDataAssayBeginPage beginPage = navigateToAssayLandingPage(SignalDataInitializer.RAW_SignalData_ASSAY); + beginPage.setSearchBox(getImportedRunIdentifier(beginPage)); + assertEquals("Incorrect number of rows imported by the file watcher", 2, beginPage.getRowCount()); + + // Sorted, because the grid's default order is not part of what this test is asserting. + List names = new ArrayList<>(beginPage.getDataRegionTable().getColumnDataAsText("Name")); + Collections.sort(names); + assertEquals("Incorrect Name values for the imported run", List.of(foreignFileName, RESULT_FILENAME_1), names); + + assertEquals("The cross-project data file should be registered in this project", 1, + foreignDataRowCount(foreignFileName)); } /** - * Imports a metadata file holding one resolvable row and one row pointing at the foreign project, then verifies the - * foreign row was rejected. - * - * @param runAsDisplayName display name for the trigger's "Run as" user, or null to run as the current user + * Drops a metadata file holding one row resolvable in this project and one row pointing at FOREIGN_PROJECT, then + * waits for the file watcher's move and import jobs. */ - private void verifyForeignDataFileRejected(String metadataFileName, String triggerName, @Nullable String runAsDisplayName, - String foreignDataFilePath, String expectedError) throws IOException + private void runForeignDataFileImport(String metadataFileName, String triggerName, String runAsUser, + String foreignFileName, String foreignDataFilePath) throws IOException { List> rows = new ArrayList<>(); rows.add(List.of("Name", "DataFile")); rows.add(List.of(RESULT_FILENAME_1, RESULT_FILENAME_1)); - rows.add(List.of(FOREIGN_FILENAME, foreignDataFilePath)); + rows.add(List.of(foreignFileName, foreignDataFilePath)); File metadataFile = TestDataUtils.writeRowsToTsv(metadataFileName, rows); - log("Configure a file watcher trigger for the Signal Data import pipeline"); - createImportTrigger(triggerName, metadataFile.getName(), runAsDisplayName); + log("Configure a file watcher trigger for the Signal Data import pipeline, running as " + runAsUser); + createImportTrigger(triggerName, metadataFile.getName(), _userHelper.getDisplayNameForEmail(runAsUser)); log("Drop a metadata file referencing a data file in another project"); goToProjectHome(); @@ -297,24 +400,47 @@ private void verifyForeignDataFileRejected(String metadataFileName, String trigg log("Wait for the file watcher import job to finish"); goToDataPipeline(); waitForPipelineJobsToFinish(2); + } - log("Verify the import job logged the rejection of the cross-project data file"); - PipelineStatusTable statusTable = new PipelineStatusTable(getDriver()); - List statuses = statusTable.getColumnDataAsText("Status"); - int errorRow = statuses.indexOf("ERROR"); - assertTrue("Expected one of the pipeline jobs to be in error, statuses were: " + statuses, errorRow >= 0); + private int foreignDataRowCount(String foreignFileName) throws IOException + { + return executeSelectRowCommand("exp", "Data", ContainerFilter.CurrentAndSubfolders, "/" + getProjectName(), + List.of(new Filter("Name", foreignFileName))).getRowCount().intValue(); + } - statusTable.clickStatusLink(errorRow) - .waitForError(String.format("DataFile '%s' %s", foreignDataFilePath, expectedError)); + /** + * The server-relative WebDAV resource path for a data file at the top of FOREIGN_PROJECT's file root. + */ + private String foreignWebDavPath(String fileName) + { + return String.format("/_webdav/%s/@files/%s", FOREIGN_PROJECT, fileName); + } - assertEquals("A data file from another project must not be imported into this project", 0, - executeSelectRowCommand("exp", "Data", ContainerFilter.CurrentAndSubfolders, "/" + getProjectName(), - List.of(new Filter("Name", FOREIGN_FILENAME))).getRowCount().intValue()); + private File foreignFile(String fileName) + { + File file = FileUtil.appendName(TestFileUtils.getDefaultFileRoot(FOREIGN_PROJECT), fileName); + assertTrue("Test requires the foreign data file to exist at " + file, file.exists()); + return file; + } - // The rejection is logged as an error and surfaces in the server error log; account for it so the harness's - // post-test error check does not fail this test. - deleteAllPipelineJobs(); - checkExpectedErrors(1); + /** + * The file watcher matches its file pattern with Matcher.find(), so a trigger watching for one metadata file also + * picks up a leftover file whose name merely contains that pattern, importing two runs where the test expects one. + */ + private static void assertMetadataFileNamesAreDistinct() + { + List names = List.of(METADATA_FILE_BY_NAME, METADATA_FILE_WEBDAV, METADATA_FILE_OUTSIDE_ROOT, + METADATA_FILE_DENIED_WEBDAV, METADATA_FILE_DENIED_SERVER_PATH, METADATA_FILE_ALLOWED_WEBDAV, + METADATA_FILE_ALLOWED_SERVER_PATH); + for (String pattern : names) + { + for (String other : names) + { + if (!pattern.equals(other) && other.contains(pattern)) + fail(String.format("Metadata file name '%s' contains '%s', so the trigger watching for '%s' would also import '%s'", + other, pattern, pattern, other)); + } + } } private void createImportTrigger(String name, String filePattern) From 66c7208a46b2f537d6fc8960d60d9bf27580f29c Mon Sep 17 00:00:00 2001 From: cnathe Date: Thu, 27 Aug 2026 10:22:49 -0500 Subject: [PATCH 3/3] code cleanup --- .../pipeline/SignalDataImportTask.java | 2 -- .../signaldata/SignalDataFileWatcherTest.java | 22 ------------------- 2 files changed, 24 deletions(-) diff --git a/signalData/src/org/labkey/signaldata/pipeline/SignalDataImportTask.java b/signalData/src/org/labkey/signaldata/pipeline/SignalDataImportTask.java index bc818a989..1845008e6 100644 --- a/signalData/src/org/labkey/signaldata/pipeline/SignalDataImportTask.java +++ b/signalData/src/org/labkey/signaldata/pipeline/SignalDataImportTask.java @@ -165,8 +165,6 @@ public RecordedActionSet run() Path resolvedPath = (resourceFile != null ? resourceFile.toPath() : Path.of(dataFilePath)) .toAbsolutePath().normalize(); - // GitHub Issue #1391: a webdav resource's own policy can be stricter than its container's, since a - // pipeline root carries its own ACL if (resourceFile != null && !resource.canRead(job.getUser(), true)) { log.error("DataFile '{}' is not readable by this user", dataFilePath); diff --git a/signalData/test/src/org/labkey/test/tests/signaldata/SignalDataFileWatcherTest.java b/signalData/test/src/org/labkey/test/tests/signaldata/SignalDataFileWatcherTest.java index 12adaa82b..934c5020e 100644 --- a/signalData/test/src/org/labkey/test/tests/signaldata/SignalDataFileWatcherTest.java +++ b/signalData/test/src/org/labkey/test/tests/signaldata/SignalDataFileWatcherTest.java @@ -112,8 +112,6 @@ public List getAssociatedModules() @BeforeClass public static void doSetup() throws Exception { - assertMetadataFileNamesAreDistinct(); - SignalDataFileWatcherTest test = getCurrentTest(); SignalDataInitializer initializer = new SignalDataInitializer(test, test.getProjectName()); initializer.setupProject(); @@ -423,26 +421,6 @@ private File foreignFile(String fileName) return file; } - /** - * The file watcher matches its file pattern with Matcher.find(), so a trigger watching for one metadata file also - * picks up a leftover file whose name merely contains that pattern, importing two runs where the test expects one. - */ - private static void assertMetadataFileNamesAreDistinct() - { - List names = List.of(METADATA_FILE_BY_NAME, METADATA_FILE_WEBDAV, METADATA_FILE_OUTSIDE_ROOT, - METADATA_FILE_DENIED_WEBDAV, METADATA_FILE_DENIED_SERVER_PATH, METADATA_FILE_ALLOWED_WEBDAV, - METADATA_FILE_ALLOWED_SERVER_PATH); - for (String pattern : names) - { - for (String other : names) - { - if (!pattern.equals(other) && other.contains(pattern)) - fail(String.format("Metadata file name '%s' contains '%s', so the trigger watching for '%s' would also import '%s'", - other, pattern, pattern, other)); - } - } - } - private void createImportTrigger(String name, String filePattern) { createImportTrigger(name, filePattern, null);