Container scoping for NAb

Author: labkey-klum

flow/src/org/labkey/flow/persist/AbstractContainerScopingTest.java

@@ -58,6 +58,7 @@
 import org.labkey.api.query.QueryService;
 import org.labkey.api.query.UserSchema;
 import org.labkey.api.security.User;
+import org.labkey.api.security.permissions.AbstractContainerScopingTest;
 import org.labkey.api.security.roles.EditorRole;
 import org.labkey.api.security.roles.ReaderRole;
 import org.labkey.api.util.DateUtil;

flow/src/org/labkey/flow/persist/FlowManager.java

@@ -234,6 +234,11 @@ public ModelAndView getView(RenderAssayForm form, BindException errors) throws E
             {
                 throw new NotFoundException("Run " + form.getRowId() + " does not exist.");
             }
+
+            // GitHub Issue #1892: getExpRun() resolves by global rowId; ensure the run belongs to the current container
+            if (!run.getContainer().equals(getContainer()))
+                throw new NotFoundException("Run " + form.getRowId() + " does not exist.");
+
             File file = getDataHandler(run).getDataFile(run);
             if (file == null)
             {
@@ -449,6 +454,10 @@ public void validateCommand(DeleteRunForm form, Errors errors)
             if (_run == null)
                 throw new NotFoundException("Run " + form.getRowId() + " does not exist.");
 
+            // GitHub Issue #1892: getExpRun() resolves by global rowId; ensure the run belongs to the current container
+            if (!_run.getContainer().equals(getContainer()))
+                throw new NotFoundException("Run " + form.getRowId() + " does not exist.");
+
             if (form.isReupload())
             {
                 _file = getDataHandler(_run).getDataFile(_run);

nab/src/org/labkey/nab/NabAssayController.java

@@ -16,8 +16,11 @@
 
 package org.labkey.nab;
 
+import jakarta.servlet.http.HttpServletResponse;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
+import org.junit.Before;
+import org.junit.Test;
 import org.labkey.api.assay.AssayService;
 import org.labkey.api.data.ColumnInfo;
 import org.labkey.api.data.Container;
@@ -38,13 +41,18 @@
 import org.labkey.api.query.FieldKey;
 import org.labkey.api.query.QueryService;
 import org.labkey.api.security.User;
+import org.labkey.api.security.permissions.AbstractContainerScopingTest;
+import org.labkey.api.security.roles.EditorRole;
+import org.labkey.api.security.roles.ReaderRole;
+import org.labkey.api.view.ActionURL;
 import org.labkey.api.study.Dataset;
 import org.labkey.api.study.Study;
 import org.labkey.api.study.StudyService;
 import org.labkey.api.util.PageFlowUtil;
 import org.labkey.api.util.Pair;
 import org.labkey.nab.query.NabProtocolSchema;
 import org.labkey.nab.query.NabRunDataTable;
+import org.springframework.mock.web.MockHttpServletResponse;
 
 import java.sql.SQLException;
 import java.util.Collection;
@@ -206,4 +214,70 @@ public void getDataPropertiesFromNabRunData(NabRunDataTable nabRunDataTable, Str
             throw new RuntimeSQLException(e);
         }
     }
+
+    public static class ContainerScopingTestCase extends AbstractContainerScopingTest
+    {
+        private Container _folderA;
+        private Container _folderB;
+        private User _admin;
+
+        @Before
+        public void setup()
+        {
+            _admin = getAdmin();
+            _folderA = createContainer("A");
+            _folderB = createContainer("B");
+        }
+
+        @Test
+        public void testDeleteRunContainerScoping() throws Exception
+        {
+            // Regression test for NAB-1. NabAssayController.DeleteRunAction resolves a
+            // run via ExperimentService.getExpRun(rowId) from a global rowId and then deletes it.
+            ExpProtocol protocol = ExperimentService.get().ensureSampleDerivationProtocol(_admin);
+            ExpRun run = ExperimentService.get().createExperimentRun(_folderB, "scope-test-run");
+            run.setProtocol(protocol);
+            run.save(_admin);
+            int rowId = run.getRowId();
+
+            ActionURL foreignUrl = new ActionURL(NabAssayController.DeleteRunAction.class, _folderA)
+                    .addParameter("rowId", rowId);
+
+            User deleterAonly = createUserInRole(_folderA, EditorRole.class);
+            assertStatus(HttpServletResponse.SC_NOT_FOUND, post(foreignUrl, deleterAonly));
+            assertNotNull("Foreign-container run must not be deleted", ExperimentService.get().getExpRun(rowId));
+
+            ActionURL ownUrl = new ActionURL(NabAssayController.DeleteRunAction.class, _folderB)
+                    .addParameter("rowId", rowId);
+            assertStatus(HttpServletResponse.SC_FOUND, post(ownUrl, _admin));
+            assertNull("Run should have been deleted from its own container", ExperimentService.get().getExpRun(rowId));
+        }
+
+        @Test
+        public void testDownloadDatafileContainerScoping() throws Exception
+        {
+            // Regression test for NAB-2. NabAssayController.DownloadDatafileAction resolves a run via
+            // ExperimentService.getExpRun(rowId) from a global rowId
+            ExpProtocol protocol = ExperimentService.get().ensureSampleDerivationProtocol(_admin);
+            ExpRun run = ExperimentService.get().createExperimentRun(_folderB, "scope-test-download-run");
+            run.setProtocol(protocol);
+            run.save(_admin);
+            int rowId = run.getRowId();
+
+            User readerAonly = createUserInRole(_folderA, ReaderRole.class);
+            ActionURL foreignUrl = new ActionURL(NabAssayController.DownloadDatafileAction.class, _folderA)
+                    .addParameter("rowId", rowId);
+            MockHttpServletResponse foreignResponse = get(foreignUrl, readerAonly);
+            assertStatus(HttpServletResponse.SC_NOT_FOUND, foreignResponse);
+            assertTrue("Foreign-container download must be rejected by the container check",
+                    foreignResponse.getContentAsString().contains("does not exist"));
+
+            ActionURL ownUrl = new ActionURL(NabAssayController.DownloadDatafileAction.class, _folderB)
+                    .addParameter("rowId", rowId);
+            MockHttpServletResponse ownResponse = get(ownUrl, _admin);
+            assertStatus(HttpServletResponse.SC_NOT_FOUND, ownResponse);
+            assertTrue("Same-container request should advance past the container check to the data handler",
+                    ownResponse.getContentAsString().contains("is not a NAb run"));
+        }
+    }
 }

nab/src/org/labkey/nab/NabManager.java

@@ -122,4 +122,11 @@ public Set<Class> getUnitTests()
     {
         return Set.of(PlateParserTests.class);
     }
+
+    @NotNull
+    @Override
+    public Set<Class> getIntegrationTests()
+    {
+        return Set.of(NabManager.ContainerScopingTestCase.class);
+    }
 }