Container scoping for NAb including automation

Author: labkey-klum

flow/src/org/labkey/flow/controllers/editscript/EditScriptForm.java

@@ -97,6 +97,7 @@ public void reset()
             {
                 throw new NotFoundException("scriptId not found: " + scriptIdStr);
             }
+            // GitHub Issue #1892: validate container
             flowObject.checkContainer(getContainer(), getUser(), url);
             _runCount = flowObject.getRunCount();
             step = FlowProtocolStep.fromRequest(getRequest());

flow/src/org/labkey/flow/controllers/run/RunController.java

@@ -232,6 +232,7 @@ public void validate(DownloadRunForm form, BindException errors)
                 errors.reject(ERROR_MSG, "run not found");
                 return;
             }
+            // GitHub Issue #1892: validate container
             _run.checkContainer(getContainer(), getUser(), getActionURL());
 
             FlowWell[] wells = _run.getWells(true);
@@ -465,6 +466,7 @@ else if (form.getSendTo() == ExportAnalysisForm.SendTo.Script)
                     if (run == null)
                         throw new NotFoundException("Flow run not found");
 
+                    // GitHub Issue #1892: validate container
                     run.checkContainer(getContainer(), getUser(), getActionURL());
                     runs.add(run);
                 }
@@ -479,6 +481,7 @@ else if (wellId != null && wellId.length > 0)
                     if (well == null)
                         throw new NotFoundException("Flow well not found");
 
+                    // GitHub Issue #1892: validate container
                     well.checkContainer(getContainer(), getUser(), getActionURL());
                     wells.add(well);
                 }
@@ -944,6 +947,7 @@ public static class DownloadAttachmentAction extends BaseDownloadAction<Attachme
             {
                 throw new NotFoundException();
             }
+            // GitHub Issue #1892: validate container
             run.checkContainer(getContainer(), getUser(), getViewContext().getActionURL());
 
             return new Pair<>(new ExpRunAttachmentParent(run.getExperimentRun()), form.getName());

flow/src/org/labkey/flow/data/FlowProtocol.java

@@ -161,6 +161,7 @@ static public FlowProtocol fromURL(User user, ViewContext context, HttpServletRe
         if (ret == null)
             return null;
 
+        // GitHub Issue #1892: validate container
         ret.checkContainer(context.getContainer(), user, url);
         if (!ret.getContainer().hasPermission(user, ReadPermission.class))
         {

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

@@ -1835,22 +1835,17 @@ public void setup()
         @Test
         public void testScriptContainerScoping() throws Exception
         {
-            // A flow script that lives in folder B.
+            // GitHub Issue #1892: Regression test for FLOW-1.
             FlowScript script = FlowScript.create(_admin, _folderB, "scope-test",
                     "<script xmlns=\"http://www.labkey.org/data/xml/flowScript\"/>");
             int scriptId = script.getScriptId();
 
-            // DownloadAction resolves the script through EditScriptForm; dispatch it against folder A while pointing
-            // scriptId at B's script -- the FLOW-1 attack shape (reaching a script that lives in another folder).
             ActionURL foreignUrl = new ActionURL(ScriptController.DownloadAction.class, _folderA)
                     .addParameter(FlowParam.scriptId.toString(), scriptId);
 
             User readerAonly = createUserInRole(_folderA, ReaderRole.class);
             assertStatus(HttpServletResponse.SC_NOT_FOUND, get(foreignUrl, readerAonly));
 
-            // Redirect branch: a caller who can read both folders is redirected to B (where its own permissions are
-            // re-enforced) instead of being served B's script from A's context. This is the core vulnerability:
-            // without checkContainer() in EditScriptForm.reset() the action streams B's script from folder A (200).
             User readerAreaderB = createUserInRole(_folderA, ReaderRole.class);
             grantRole(readerAreaderB, _folderB, ReaderRole.class);
             MockHttpServletResponse resp = get(foreignUrl, readerAreaderB);
@@ -1868,22 +1863,16 @@ public void testScriptContainerScoping() throws Exception
         @Test
         public void testProtocolContainerScoping() throws Exception
         {
-            // Regression test for FLOW-2. A FlowProtocol is accessed by a global experimentId.
-            // A flow protocol that lives in folder B.
+            // GitHub Issue #1892: Regression test for FLOW-2. A FlowProtocol is accessed by a global experimentId.
             FlowProtocol protocolB = FlowProtocol.ensureForContainer(_admin, _folderB);
             int experimentId = protocolB.getProtocol().getRowId();
 
-            // EditFCSAnalysisFilter is an UpdatePermission action; dispatch it against folder A while pointing
-            // experimentId at B's protocol -- the FLOW-2 attack shape (editor in A reaching a protocol in B).
             ActionURL foreignUrl = new ActionURL(ProtocolController.EditFCSAnalysisFilterAction.class, _folderA)
                     .addParameter(FlowParam.experimentId.toString(), experimentId);
 
             User editorAonly = createUserInRole(_folderA, EditorRole.class);
             assertStatus(HttpServletResponse.SC_NOT_FOUND, get(foreignUrl, editorAonly));
 
-            // Redirect branch: a caller who can edit folder A and read folder B is redirected to B (where its own
-            // permissions are re-enforced) instead of operating on B's protocol from A's context. This is the core
-            // vulnerability: without checkContainer() in fromURL() the action proceeds in folder A (HTTP 200).
             User editorAreaderB = createUserInRole(_folderA, EditorRole.class);
             grantRole(editorAreaderB, _folderB, ReaderRole.class);
             MockHttpServletResponse resp = get(foreignUrl, editorAreaderB);
@@ -1901,7 +1890,7 @@ public void testProtocolContainerScoping() throws Exception
         @Test
         public void testExportAnalysisContainerScoping() throws Exception
         {
-            // Regression test for FLOW-3 RunController.ExportAnalysis resolves
+            // GitHub Issue #1892: Regression test for FLOW-3. RunController.ExportAnalysis resolves
             // URL supplied runId/wellId via the global FlowRun.
             ExpData data = ExperimentService.get().createData(_folderB, FlowDataType.FCSFile, "scope-test-well");
             URI dataFileURI = new URI("file:///attributes.flowdata.xml");
@@ -1912,18 +1901,12 @@ public void testExportAnalysisContainerScoping() throws Exception
             AttributeSetHelper.save(attrs, _admin, data);
             int wellId = data.getRowId();
 
-            // Point wellId at B's well while dispatching the export against folder A -- the FLOW-3 attack shape.
             ActionURL foreignUrl = new ActionURL(RunController.ExportAnalysis.class, _folderA)
                     .addParameter(FlowParam.wellId.toString(), wellId);
 
-            // Deny branch: a caller who can read folder A but has no rights in folder B must not learn B's well exists,
-            // nor export it -> 404.
             User readerAonly = createUserInRole(_folderA, ReaderRole.class);
             assertStatus(HttpServletResponse.SC_NOT_FOUND, post(foreignUrl, readerAonly));
 
-            // Redirect branch: a caller who can read both folders is redirected to B (where its own permissions are
-            // re-enforced) instead of having B's well exported from A's context. This is the core vulnerability: without
-            // checkContainer() in validateCommand() the action exports B's well from folder A.
             User readerAreaderB = createUserInRole(_folderA, ReaderRole.class);
             grantRole(readerAreaderB, _folderB, ReaderRole.class);
             MockHttpServletResponse resp = post(foreignUrl, readerAreaderB);
@@ -1940,8 +1923,8 @@ public void testExportAnalysisContainerScoping() throws Exception
         @Test
         public void testRunDownloadContainerScoping() throws Exception
         {
-            // Regression test for FLOW-4. RunController.DownloadAction (and
-            // DownloadAttachmentAction) resolve a run via RunForm.getRun() -> FlowRun.fromRunId() from a global runId
+            // GitHub Issue #1892: Regression test for FLOW-4. RunController.DownloadAction (and
+            // DownloadAttachmentAction) resolve a run via RunForm.getRun() -> FlowRun.fromRunId() from a global runId.
             FlowProtocol protocolB = FlowProtocol.ensureForContainer(_admin, _folderB);
             ExpRun expRun = ExperimentService.get().createExperimentRun(_folderB, "scope-test-run");
             expRun.setProtocol(protocolB.getProtocol());
@@ -1954,9 +1937,6 @@ public void testRunDownloadContainerScoping() throws Exception
             User readerAonly = createUserInRole(_folderA, ReaderRole.class);
             assertStatus(HttpServletResponse.SC_NOT_FOUND, get(foreignUrl, readerAonly));
 
-            // Redirect branch: a caller who can read both folders is redirected to B (where its own permissions are
-            // re-enforced) instead of having B's run streamed from A's context. This is the core vulnerability: without
-            // checkContainer() in DownloadAction.validate() the action streams B's run from folder A.
             User readerAreaderB = createUserInRole(_folderA, ReaderRole.class);
             grantRole(readerAreaderB, _folderB, ReaderRole.class);
             MockHttpServletResponse resp = get(foreignUrl, readerAreaderB);

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

@@ -401,9 +401,33 @@ public ActionURL getGraphRenderURL()
         }
     }
 
+    private static void _verifyObjectIdsReadable(User user, int[] ids)
+    {
+        if (ids == null)
+            return;
+
+        // GitHub Issue #1892: (NAB-9) The object ids come straight from the request and getDilutionSummaries() resolves them to runs
+        // via a global, cross-container lookup.
+        for (int id : ids)
+        {
+            ExpRun run = NabManager.get().getNAbRunByObjectId(id);
+            if (run == null)
+                throw new NotFoundException("One or more requested specimens do not exist.");
+            if (run.getContainer().hasPermission(user, ReadPermission.class))
+                continue;
+            throw new NotFoundException("One or more requested specimens do not exist.");
+        }
+    }
+
     @RequiresPermission(ReadPermission.class)
     public static class NabGraphSelectedAction extends GraphSelectedAction<GraphSelectedForm>
     {
+        @Override
+        protected void verifyObjectIdsReadable(int[] ids)
+        {
+            _verifyObjectIdsReadable(getUser(), ids);
+        }
+
         @Override
         protected GraphSelectedBean createSelectionBean(ViewContext context, ExpProtocol protocol, int[] cutoffs, int[] dataObjectIds, String caption, String title)
         {
@@ -493,6 +517,11 @@ public URLHelper getSuccessURL(DeleteRunForm form)
     @ContextualRoles(RunDatasetContextualRoles.class)
     public static class NabMultiGraphAction extends MultiGraphAction<GraphSelectedForm>
     {
+        @Override
+        protected void verifyObjectIdsReadable(int[] ids)
+        {
+            _verifyObjectIdsReadable(getUser(), ids);
+        }
     }
 
     @RequiresPermission(ReadPermission.class)

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

@@ -16,11 +16,8 @@
 
 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;
@@ -41,18 +38,13 @@
 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;
@@ -214,70 +206,4 @@ 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/NabModule.java

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