From 65d29f9452d2d2a77f76b635fd4dba5a5be3d13e Mon Sep 17 00:00:00 2001 From: Lum Date: Tue, 9 Jun 2026 16:26:43 -0700 Subject: [PATCH 01/11] Validate container permissions --- .../controllers/editscript/EditScriptForm.java | 8 +++++++- .../flow/controllers/protocol/ProtocolForm.java | 2 +- flow/src/org/labkey/flow/data/FlowProtocol.java | 16 ++++++++++------ 3 files changed, 18 insertions(+), 8 deletions(-) diff --git a/flow/src/org/labkey/flow/controllers/editscript/EditScriptForm.java b/flow/src/org/labkey/flow/controllers/editscript/EditScriptForm.java index b8632153ff..cf9cb49bbd 100644 --- a/flow/src/org/labkey/flow/controllers/editscript/EditScriptForm.java +++ b/flow/src/org/labkey/flow/controllers/editscript/EditScriptForm.java @@ -73,7 +73,12 @@ public void reset() { try { - String scriptIdStr = getRequest().getParameter("scriptId"); + ActionURL url = getViewContext().getActionURL(); + // Read scriptId from the action URL (falling back to the request parameter) so it resolves under both + // real browser requests and in-JVM mock dispatch, mirroring how runId/compId below are read from the URL. + String scriptIdStr = url.getParameter("scriptId"); + if (scriptIdStr == null) + scriptIdStr = getRequest().getParameter("scriptId"); if (scriptIdStr == null) { throw new NotFoundException("scriptId required"); @@ -92,6 +97,7 @@ public void reset() { throw new NotFoundException("scriptId not found: " + scriptIdStr); } + flowObject.checkContainer(getContainer(), getUser(), url); _runCount = flowObject.getRunCount(); step = FlowProtocolStep.fromRequest(getRequest()); _run = FlowRun.fromURL(getViewContext().getActionURL(), getRequest(), getViewContext().getContainer(), getUser()); diff --git a/flow/src/org/labkey/flow/controllers/protocol/ProtocolForm.java b/flow/src/org/labkey/flow/controllers/protocol/ProtocolForm.java index b1b7d75dc0..311d04c42c 100644 --- a/flow/src/org/labkey/flow/controllers/protocol/ProtocolForm.java +++ b/flow/src/org/labkey/flow/controllers/protocol/ProtocolForm.java @@ -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; } diff --git a/flow/src/org/labkey/flow/data/FlowProtocol.java b/flow/src/org/labkey/flow/data/FlowProtocol.java index c58bf74a9e..7cb03fbac6 100644 --- a/flow/src/org/labkey/flow/data/FlowProtocol.java +++ b/flow/src/org/labkey/flow/data/FlowProtocol.java @@ -16,6 +16,7 @@ package org.labkey.flow.data; +import jakarta.servlet.http.HttpServletRequest; import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -70,6 +71,7 @@ import org.labkey.api.view.RedirectException; import org.labkey.api.view.UnauthorizedException; import org.labkey.api.view.ViewBackgroundInfo; +import org.labkey.api.view.ViewContext; import org.labkey.flow.controllers.FlowController; import org.labkey.flow.controllers.FlowParam; import org.labkey.flow.controllers.protocol.ProtocolController; @@ -79,7 +81,6 @@ import org.labkey.flow.query.FlowTableType; import org.labkey.flow.script.KeywordsJob; -import jakarta.servlet.http.HttpServletRequest; import java.io.File; import java.sql.ResultSet; import java.sql.SQLException; @@ -149,15 +150,18 @@ static public boolean isDefaultProtocol(ExpProtocol protocol) DEFAULT_PROTOCOL_NAME.equals(protocol.getName()); } - static public FlowProtocol fromURL(User user, ActionURL url, HttpServletRequest request) throws UnauthorizedException + static public FlowProtocol fromURL(User user, ViewContext context, HttpServletRequest request) throws UnauthorizedException { + ActionURL url = context.getActionURL(); FlowProtocol ret = fromProtocolId(getIntParam(url, request, FlowParam.experimentId)); if (ret == null) { - ret = FlowProtocol.getForContainer(ContainerManager.getForPath(url.getExtraPath())); + ret = FlowProtocol.getForContainer(context.getContainer()); } if (ret == null) return null; + + ret.checkContainer(context.getContainer(), user, url); if (!ret.getContainer().hasPermission(user, ReadPermission.class)) { throw new UnauthorizedException(); @@ -165,11 +169,11 @@ static public FlowProtocol fromURL(User user, ActionURL url, HttpServletRequest return ret; } - public static FlowProtocol fromURLRedirectIfNull(User user, ActionURL url, HttpServletRequest request) + public static FlowProtocol fromURLRedirectIfNull(User user, ViewContext context, HttpServletRequest request) { - FlowProtocol protocol = fromURL(user, url, request); + FlowProtocol protocol = fromURL(user, context, request); if (protocol == null) - throw new RedirectException(url.clone().setAction(FlowController.BeginAction.class)); + throw new RedirectException(context.getActionURL().clone().setAction(FlowController.BeginAction.class)); return protocol; } From 21b78586bb9c5830cc11b675a15c20f05429a989 Mon Sep 17 00:00:00 2001 From: Lum Date: Wed, 10 Jun 2026 12:20:21 -0700 Subject: [PATCH 02/11] More container scoping plus unit tests. --- flow/src/org/labkey/flow/FlowModule.java | 3 +- .../flow/controllers/run/RunController.java | 4 + .../persist/AbstractContainerScopingTest.java | 146 +++++++++++++++ .../org/labkey/flow/persist/FlowManager.java | 167 ++++++++++++++++++ 4 files changed, 319 insertions(+), 1 deletion(-) create mode 100644 flow/src/org/labkey/flow/persist/AbstractContainerScopingTest.java diff --git a/flow/src/org/labkey/flow/FlowModule.java b/flow/src/org/labkey/flow/FlowModule.java index cbe2d66b21..64ad66dc75 100644 --- a/flow/src/org/labkey/flow/FlowModule.java +++ b/flow/src/org/labkey/flow/FlowModule.java @@ -342,7 +342,8 @@ public Set getIntegrationTests() FlowController.TestCase.class, FlowProtocol.TestCase.class, PersistTests.class, - FlowManager.TestCase.class + FlowManager.TestCase.class, + FlowManager.ContainerScopingTestCase.class ); } diff --git a/flow/src/org/labkey/flow/controllers/run/RunController.java b/flow/src/org/labkey/flow/controllers/run/RunController.java index 01dbc2a010..d1b2c9090e 100644 --- a/flow/src/org/labkey/flow/controllers/run/RunController.java +++ b/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; } + _run.checkContainer(getContainer(), getUser(), getActionURL()); FlowWell[] wells = _run.getWells(true); if (wells.length == 0) @@ -464,6 +465,7 @@ else if (form.getSendTo() == ExportAnalysisForm.SendTo.Script) if (run == null) throw new NotFoundException("Flow run not found"); + run.checkContainer(getContainer(), getUser(), getActionURL()); runs.add(run); } _runs = runs; @@ -477,6 +479,7 @@ else if (wellId != null && wellId.length > 0) if (well == null) throw new NotFoundException("Flow well not found"); + well.checkContainer(getContainer(), getUser(), getActionURL()); wells.add(well); } _wells = wells; @@ -941,6 +944,7 @@ public static class DownloadAttachmentAction extends BaseDownloadAction(new ExpRunAttachmentParent(run.getExperimentRun()), form.getName()); } diff --git a/flow/src/org/labkey/flow/persist/AbstractContainerScopingTest.java b/flow/src/org/labkey/flow/persist/AbstractContainerScopingTest.java new file mode 100644 index 0000000000..4368acfec7 --- /dev/null +++ b/flow/src/org/labkey/flow/persist/AbstractContainerScopingTest.java @@ -0,0 +1,146 @@ +package org.labkey.flow.persist; + +import org.junit.After; +import org.junit.Assert; +import org.labkey.api.data.Container; +import org.labkey.api.data.ContainerManager; +import org.labkey.api.security.MutableSecurityPolicy; +import org.labkey.api.security.SecurityManager; +import org.labkey.api.security.SecurityPolicyManager; +import org.labkey.api.security.User; +import org.labkey.api.security.UserManager; +import org.labkey.api.security.ValidEmail; +import org.labkey.api.security.roles.Role; +import org.labkey.api.util.JunitUtil; +import org.labkey.api.util.TestContext; +import org.labkey.api.view.ActionURL; +import org.labkey.api.view.ViewServlet; +import org.springframework.mock.web.MockHttpServletResponse; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; + +/** + * Base class for "container scoping" (a.k.a. broken-object-level-authorization / BOLA / IDOR) integration tests. These + * tests verify that an action whose {@code @RequiresPermission} annotation is correct for the current container still + * rejects an object resolved by a global id that belongs to a different container. + * + *

The repeated scaffolding lives here so each subclass keeps only its data fixture and the action under test: + *

    + *
  • {@link #createContainer(String)} — make a throwaway child of the junit container (auto-cleaned).
    • + *
  • {@link #createUserInRole(Container, Class)} — make a user with a role assigned in one folder only + * (auto-cleaned). Use this to obtain a caller who is, say, admin in folder A but has no rights in folder B.
    • + *
  • {@link #get(ActionURL, User)} / {@link #post(ActionURL, User)} — dispatch an in-JVM request as a given user + * and inspect the {@link MockHttpServletResponse} status. Parameters travel on the {@link ActionURL}.
    • + *
+ * + *

Note: WebDAV verbs (MOVE, PROPPATCH, ...) are not served through {@link ViewServlet} dispatch, so a WebDAV test + * should still use this class for its container/user fixtures but drive the verb through {@code WebdavServlet} itself. + */ +public abstract class AbstractContainerScopingTest extends Assert +{ + private static final Map FORM_HEADERS = Map.of("Content-Type", "application/x-www-form-urlencoded"); + + private final List _containers = new ArrayList<>(); + private final List _users = new ArrayList<>(); + + /** The site-admin user (from {@link TestContext}) that owns the test fixtures. */ + protected User getAdmin() + { + return TestContext.get().getUser(); + } + + /** + * Create a throwaway child container of the junit container, named uniquely per test class, registered for + * automatic cleanup. Callers pass a short local name (e.g. "A"/"B"/"Source"); the class name is prepended so two + * test classes can both ask for "A" without colliding. + */ + protected Container createContainer(String name) + { + Container junit = JunitUtil.getTestContainer(); + Container c = ContainerManager.ensureContainer(junit.getParsedPath().append(getClass().getSimpleName() + "-" + name, true), getAdmin()); + _containers.add(c); + return c; + } + + /** + * Create a user that has {@code role} assigned in {@code scope} only (it has no rights in any other + * container), registered for automatic cleanup. This is the canonical way to build a caller who is privileged in + * one folder but not another. Do not use {@code LimitedUser} for this — that grants roles unconditionally in every + * container. + */ + protected User createUserInRole(Container scope, Class role) throws Exception + { + String email = getClass().getSimpleName().toLowerCase() + "-" + _users.size() + "@containerscoping.test"; + User user = SecurityManager.addUser(new ValidEmail(email), null).getUser(); + _users.add(user); + grantRole(user, scope, role); + return user; + } + + /** + * Grant {@code role} to an existing {@code user} in {@code scope}, on top of any roles it already holds in that + * container. Use this to build a caller with different roles in different folders (e.g. delete rights in a source + * folder but only read access in a target folder). + */ + protected void grantRole(User user, Container scope, Class role) throws Exception + { + MutableSecurityPolicy policy = new MutableSecurityPolicy(scope.getPolicy()); + policy.addRoleAssignment(user, role); + SecurityPolicyManager.savePolicyForTests(policy, getAdmin()); + } + + /** + * Dispatch a GET to the action addressed by {@code url} as {@code user}. Put request parameters on the URL. No + * request-body Content-Type is sent: a GET carries no body, and an "application/json" Content-Type would make an + * API action ({@code ReadOnlyApiAction}) try to parse the empty body as JSON and fail with 400 before its + * {@code execute()} runs. With no Content-Type the form binds from the URL parameters, as a real GET would. + */ + protected MockHttpServletResponse get(ActionURL url, User user) throws Exception + { + return ViewServlet.GET(url, user, Map.of()); + } + + /** Dispatch a POST to the action addressed by {@code url} as {@code user}. Put request parameters on the URL. */ + protected MockHttpServletResponse post(ActionURL url, User user) throws Exception + { + return ViewServlet.POST(url, user, FORM_HEADERS, null); + } + + /** Assert that a dispatched response has the expected HTTP status code. */ + protected void assertStatus(int expected, MockHttpServletResponse response) + { + assertEquals("Unexpected HTTP status", expected, response.getStatus()); + } + + @After + public void cleanupContainerScopingFixtures() + { + User admin = getAdmin(); + + for (User user : _users) + { + try + { + UserManager.deleteUser(user.getUserId()); + } + catch (Exception ignored) + { + } + } + _users.clear(); + + for (Container c : _containers) + { + try + { + ContainerManager.deleteAll(c, admin); + } + catch (Exception ignored) + { + } + } + _containers.clear(); + } +} diff --git a/flow/src/org/labkey/flow/persist/FlowManager.java b/flow/src/org/labkey/flow/persist/FlowManager.java index 6e43a77375..1ed647064e 100644 --- a/flow/src/org/labkey/flow/persist/FlowManager.java +++ b/flow/src/org/labkey/flow/persist/FlowManager.java @@ -16,12 +16,14 @@ package org.labkey.flow.persist; +import jakarta.servlet.http.HttpServletResponse; import org.apache.commons.collections4.iterators.ArrayIterator; import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import org.junit.Before; import org.junit.Test; import org.labkey.api.audit.AuditLogService; import org.labkey.api.data.Aggregate; @@ -46,6 +48,7 @@ import org.labkey.api.exp.OntologyManager; import org.labkey.api.exp.PropertyDescriptor; import org.labkey.api.exp.api.ExpData; +import org.labkey.api.exp.api.ExpRun; import org.labkey.api.exp.api.ExpSampleType; import org.labkey.api.exp.api.ExperimentService; import org.labkey.api.exp.query.SamplesSchema; @@ -55,20 +58,30 @@ import org.labkey.api.query.QueryService; import org.labkey.api.query.UserSchema; import org.labkey.api.security.User; +import org.labkey.api.security.roles.EditorRole; +import org.labkey.api.security.roles.ReaderRole; import org.labkey.api.util.DateUtil; import org.labkey.api.util.FileUtil; import org.labkey.api.util.UnexpectedException; +import org.labkey.api.view.ActionURL; import org.labkey.flow.FlowModule; import org.labkey.flow.analysis.web.GraphSpec; import org.labkey.flow.analysis.web.StatisticSpec; +import org.labkey.flow.controllers.FlowParam; +import org.labkey.flow.controllers.editscript.ScriptController; import org.labkey.flow.controllers.executescript.AnalysisEngine; +import org.labkey.flow.controllers.protocol.ProtocolController; +import org.labkey.flow.controllers.run.RunController; import org.labkey.flow.data.AttributeType; import org.labkey.flow.data.FlowDataObject; +import org.labkey.flow.data.FlowDataType; import org.labkey.flow.data.FlowProperty; import org.labkey.flow.data.FlowProtocol; +import org.labkey.flow.data.FlowScript; import org.labkey.flow.data.ICSMetadata; import org.labkey.flow.query.FlowSchema; import org.labkey.flow.query.FlowTableType; +import org.springframework.mock.web.MockHttpServletResponse; import java.net.URI; import java.sql.ResultSet; @@ -1802,4 +1815,158 @@ public void metrics() assertNotNull(metrics.get("flowTempTableCount")); } } + + public static class ContainerScopingTestCase extends AbstractContainerScopingTest + { + private Container _folderA; + private Container _folderB; + private User _admin; + + @Before + public void setup() + { + // Regression test for FLOW-1. A FlowScript is addressed by a global scriptId. + _admin = getAdmin(); + _folderA = createContainer("A"); + _folderB = createContainer("B"); + } + + @Test + public void testScriptContainerScoping() throws Exception + { + // A flow script that lives in folder B. + FlowScript script = FlowScript.create(_admin, _folderB, "scope-test", + "