Skip to content
10 changes: 10 additions & 0 deletions elispotassay/src/org/labkey/elispot/ElispotController.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package org.labkey.elispot;

import org.apache.commons.lang3.math.NumberUtils;
import org.jetbrains.annotations.Nullable;
import org.json.JSONArray;
import org.json.JSONObject;
Expand Down Expand Up @@ -543,6 +544,15 @@ public boolean handlePost(Object o, BindException errors) throws Exception
Set<String> selections = DataRegionSelection.getSelected(getViewContext(), true);
if (!selections.isEmpty())
{
// GitHub Kanban #1892: Verify each selected run belongs to the current container before queuing
for (String selection : selections)
Comment thread
labkey-nicka marked this conversation as resolved.
{
int rowId = NumberUtils.toInt(selection, -1);
ExpRun run = rowId != -1 ? ExperimentService.get().getExpRun(rowId, getContainer()) : null;
if (run == null)
throw new NotFoundException("Run " + selection + " does not exist.");
}

ViewBackgroundInfo info = new ViewBackgroundInfo(getContainer(), getUser(), getViewContext().getActionURL());
BackgroundSubtractionJob job = new BackgroundSubtractionJob(ElispotPipelineProvider.NAME, info,
PipelineService.get().findPipelineRoot(getContainer()), selections);
Expand Down
5 changes: 4 additions & 1 deletion flow/src/org/labkey/flow/FlowModule.java
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,10 @@ public Set<Class> getIntegrationTests()
FlowController.TestCase.class,
FlowProtocol.TestCase.class,
PersistTests.class,
FlowManager.TestCase.class
FlowManager.TestCase.class,
FlowManager.ContainerScopingTestCase.class,
WellController.WellContainerScopingTestCase.class,
AnalysisScriptController.AnalysisScriptContainerScopingTestCase.class
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@

package org.labkey.flow.controllers.editscript;

import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.fhcrc.cpas.flow.script.xml.ScriptDocument;
import org.jetbrains.annotations.NotNull;
import org.labkey.api.security.permissions.UpdatePermission;
Expand Down Expand Up @@ -92,6 +92,8 @@ public void reset()
{
throw new NotFoundException("scriptId not found: " + scriptIdStr);
}
// GitHub Issue #1892: validate container
flowObject.checkContainer(getContainer(), getUser(), getViewContext().getActionURL());
_runCount = flowObject.getRunCount();
step = FlowProtocolStep.fromRequest(getRequest());
_run = FlowRun.fromURL(getViewContext().getActionURL(), getRequest(), getViewContext().getContainer(), getUser());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,11 @@
import org.apache.commons.io.filefilter.IOFileFilter;
import org.apache.commons.lang3.StringUtils;
import org.apache.logging.log4j.Logger;
import org.fhcrc.cpas.flow.script.xml.ScriptDocument;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import org.junit.Before;
import org.junit.Test;
import org.labkey.api.action.FormViewAction;
import org.labkey.api.action.SimpleRedirectAction;
import org.labkey.api.action.SimpleViewAction;
Expand All @@ -36,11 +39,16 @@
import org.labkey.api.pipeline.PipelineUrls;
import org.labkey.api.pipeline.browse.PipelinePathForm;
import org.labkey.api.security.RequiresPermission;
import org.labkey.api.security.User;
import org.labkey.api.security.permissions.AbstractContainerScopingTest;
import org.labkey.api.security.permissions.InsertPermission;
import org.labkey.api.security.permissions.ReadPermission;
import org.labkey.api.security.permissions.UpdatePermission;
import org.labkey.api.security.roles.EditorRole;
import org.labkey.api.security.roles.ReaderRole;
import org.labkey.api.study.Study;
import org.labkey.api.study.StudyService;
import org.labkey.api.test.TestWhen;
import org.labkey.api.usageMetrics.SimpleMetricsService;
import org.labkey.api.util.FileUtil;
import org.labkey.api.util.PageFlowUtil;
Expand All @@ -54,6 +62,7 @@
import org.labkey.api.view.HttpView;
import org.labkey.api.view.JspView;
import org.labkey.api.view.NavTree;
import org.labkey.api.view.NotFoundException;
import org.labkey.api.view.RedirectException;
import org.labkey.api.view.ViewBackgroundInfo;
import org.labkey.api.view.template.PageConfig;
Expand Down Expand Up @@ -86,11 +95,14 @@
import org.labkey.flow.util.SampleUtil;
import org.labkey.vfs.FileLike;
import org.labkey.vfs.FileSystemLike;
import org.springframework.mock.web.MockHttpServletResponse;
import org.springframework.validation.BindException;
import org.springframework.validation.Errors;
import org.springframework.web.multipart.MultipartFile;
import org.springframework.web.servlet.ModelAndView;

import jakarta.servlet.http.HttpServletResponse;

import java.io.File;
import java.io.IOException;
import java.net.URI;
Expand Down Expand Up @@ -185,16 +197,20 @@ protected ModelAndView analyzeRuns(ChooseRunsToAnalyzeForm form, int[] runIds, S
DataRegionSelection.clearAll(getViewContext());

FlowExperiment experiment = FlowExperiment.fromLSID(experimentLSID);
checkContainer(experiment);
String experimentName = form.ff_analysisName;
if (experiment != null)
{
experimentName = experiment.getName();
}
FlowScript analysis = form.getProtocol();
checkContainer(analysis);
AnalyzeJob job = new AnalyzeJob(getViewBackgroundInfo(), experimentName, experimentLSID, FlowProtocol.ensureForContainer(getUser(), getContainer()), analysis, form.getProtocolStep(), runIds, PipelineService.get().findPipelineRoot(getContainer()));
if (form.getCompensationMatrixId() != 0)
{
job.setCompensationMatrix(FlowCompensationMatrix.fromCompId(form.getCompensationMatrixId()));
FlowCompensationMatrix comp = FlowCompensationMatrix.fromCompId(form.getCompensationMatrixId());
checkContainer(comp);
job.setCompensationMatrix(comp);
}
job.setCompensationExperimentLSID(form.getCompensationExperimentLSID());
return HttpView.redirect(executeScript(job));
Expand All @@ -216,6 +232,7 @@ public class ChooseRunsToAnalyzeAction extends BaseAnalyzeRunsAction
public ModelAndView getView(ChooseRunsToAnalyzeForm form, BindException errors)
{
script = form.getProtocol();
checkContainer(script);
return chooseRunsToAnalyze(form, errors);
}
}
Expand All @@ -227,12 +244,19 @@ public class AnalyzeSelectedRunsAction extends BaseAnalyzeRunsAction
public ModelAndView getView(ChooseRunsToAnalyzeForm form, BindException errors) throws Exception
{
script = form.getProtocol();
checkContainer(script);
int[] runIds = form.getSelectedRunIds();
if (runIds.length == 0)
{
errors.reject(ERROR_MSG, "Please select at least one run to analyze.");
return chooseRunsToAnalyze(form, errors);
}
for (int runId : runIds)
{
FlowRun run = FlowRun.fromRunId(runId);
if (run == null || !run.getContainer().hasPermission(getUser(), ReadPermission.class))
throw new NotFoundException("Run not found: " + runId);
}
String experimentLSID = form.getAnalysisLSID();
if (experimentLSID == null)
{
Expand Down Expand Up @@ -843,6 +867,12 @@ private SampleIdMap<FlowFCSFile> getSelectedFCSFiles(ImportAnalysisForm form, Er
return null;
}

if (!file.getContainer().equals(getContainer()))
{
errors.reject(ERROR_MSG, "Resolved FCS file '" + file.getName() + "' is not in this folder.");
return null;
}

if (!file.isOriginalFCSFile())
{
errors.reject(ERROR_MSG, "Resolved FCS file '" + file.getName() + "' is a FCS files created from importing an external analysis.");
Expand Down Expand Up @@ -1495,4 +1525,51 @@ public void addNavTrail(NavTree root)
root.addChild(display);
}
}

@TestWhen(TestWhen.When.BVT)
public static class AnalysisScriptContainerScopingTestCase extends AbstractContainerScopingTest
{
private User _admin;
private Container _folderA;
private Container _folderB;
private int _scriptIdB;

@Before
public void setUp() throws Exception
{
_admin = getAdmin();
_folderA = createContainer("A");
_folderB = createContainer("B");

// Pre-create the flow protocol/steps in B so the valid-case ChooseRunsToAnalyzeForm.populate() finds them.
FlowProtocol.ensureForContainer(_admin, _folderB);

// Create a flow analysis script with a single analysis step so populate() has a usable protocol step.
ScriptDocument doc = ScriptDocument.Factory.newInstance();
doc.addNewScript().addNewAnalysis();
_scriptIdB = FlowScript.create(_admin, _folderB, getClass().getSimpleName() + "-" + "scriptB", doc.toString()).getScriptId();
}

// ChooseRunsToAnalyzeForm resolves the analysis script by global rowId; a foreign script must be scoped.
@Test
public void testAnalysisScriptContainerScoping() throws Exception
{
ActionURL foreignUrl = new ActionURL(ChooseRunsToAnalyzeAction.class, _folderA).addParameter("scriptId", _scriptIdB);

User folderAEditor = createUserInRole(_folderA, EditorRole.class);
assertStatus(HttpServletResponse.SC_NOT_FOUND, get(foreignUrl, folderAEditor));

User folderBEditor = createUserInRole(_folderA, EditorRole.class);
grantRole(folderBEditor, _folderB, ReaderRole.class);
MockHttpServletResponse resp = get(foreignUrl, folderBEditor);
assertStatus(HttpServletResponse.SC_FOUND, resp);
String location = resp.getRedirectedUrl();
assertNotNull("Redirect must have a Location", location);
assertTrue("Redirect should target the script's own container, was: " + location, location.contains(_folderB.getPath()));

// valid case: the script in its own container is accepted (renders the choose-runs wizard).
assertStatus(HttpServletResponse.SC_OK,
get(new ActionURL(ChooseRunsToAnalyzeAction.class, _folderB).addParameter("scriptId", _scriptIdB), _admin));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public FlowProtocol getProtocol() throws UnauthorizedException
{
if (_protocol != null)
return _protocol;
_protocol = FlowProtocol.fromURLRedirectIfNull(getUser(), getViewContext().getActionURL(), getRequest());
_protocol = FlowProtocol.fromURLRedirectIfNull(getUser(), getViewContext(), getRequest());
return _protocol;
}

Expand Down
8 changes: 8 additions & 0 deletions flow/src/org/labkey/flow/controllers/run/RunController.java
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,8 @@ 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);
if (wells.length == 0)
Expand Down Expand Up @@ -464,6 +466,8 @@ 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);
}
_runs = runs;
Expand All @@ -477,6 +481,8 @@ 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);
}
_wells = wells;
Expand Down Expand Up @@ -941,6 +947,8 @@ 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());
}
Expand Down
Loading