diff --git a/api/src/org/labkey/api/security/permissions/AbstractContainerScopingTest.java b/api/src/org/labkey/api/security/permissions/AbstractContainerScopingTest.java new file mode 100644 index 00000000000..bb5db4d34a6 --- /dev/null +++ b/api/src/org/labkey/api/security/permissions/AbstractContainerScopingTest.java @@ -0,0 +1,161 @@ +/* + * Copyright (c) 2026 LabKey Corporation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.labkey.api.security.permissions; + +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: + *

+ * + *

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/core/src/org/labkey/core/CoreModule.java b/core/src/org/labkey/core/CoreModule.java index 76db398883d..bd342ae5afa 100644 --- a/core/src/org/labkey/core/CoreModule.java +++ b/core/src/org/labkey/core/CoreModule.java @@ -1394,11 +1394,13 @@ public Set getIntegrationTests() AdminController.SerializationTest.class, AdminController.TestCase.class, AdminController.WorkbookDeleteTestCase.class, + AdminController.ImportFolderSourceScopingTestCase.class, AllowListType.TestCase.class, AttachmentServiceImpl.TestCase.class, CoreController.TestCase.class, DataRegion.TestCase.class, DavController.TestCase.class, + DavController.MoveActionContainerScopingTestCase.class, EmailServiceImpl.TestCase.class, FilesSiteSettingsAction.TestCase.class, LoggerController.TestCase.class, diff --git a/core/src/org/labkey/core/admin/AdminController.java b/core/src/org/labkey/core/admin/AdminController.java index 3ae8e54dee7..8868dcbf947 100644 --- a/core/src/org/labkey/core/admin/AdminController.java +++ b/core/src/org/labkey/core/admin/AdminController.java @@ -209,6 +209,7 @@ import org.labkey.api.security.impersonation.RoleImpersonationContextFactory; import org.labkey.api.security.impersonation.UserImpersonationContextFactory; import org.labkey.api.security.permissions.AbstractActionPermissionTest; +import org.labkey.api.security.permissions.AbstractContainerScopingTest; import org.labkey.api.security.permissions.AdminOperationsPermission; import org.labkey.api.security.permissions.AdminPermission; import org.labkey.api.security.permissions.ApplicationAdminPermission; @@ -5393,6 +5394,10 @@ public boolean handlePost(ImportFolderForm form, BindException errors) throws Ex if (!StringUtils.isEmpty(form.getSourceTemplateFolder())) { fiConfig = getFolderImportConfigFromTemplateFolder(form, pipelineUnzipDir, errors); + if (fiConfig == null || errors.hasErrors()) + { + return false; + } } else { @@ -5488,10 +5493,16 @@ public boolean handlePost(ImportFolderForm form, BindException errors) throws Ex private FolderImportConfig getFolderImportConfigFromTemplateFolder(final ImportFolderForm form, final Path pipelineUnzipDir, final BindException errors) throws Exception { - // user choose to import from a template source folder + // user chose to import from a template source folder Container sourceContainer = form.getSourceTemplateFolderContainer(); - // In order to support the Advanced import options to import into multiple target folders we need to zip + if (sourceContainer == null || !sourceContainer.hasPermission(getUser(), AdminPermission.class)) + { + errors.reject(SpringActionController.ERROR_MSG, "You do not have permission to import from the specified source folder."); + return null; + } + + // To support the Advanced import options to import into multiple target folders we need to zip // the source template folder so that the zip file can be passed to the pipeline processes. FolderExportContext ctx = new FolderExportContext(getUser(), sourceContainer, getRegisteredFolderWritersForImplicitExport(sourceContainer), "new", false, @@ -12286,4 +12297,28 @@ protected static void doCleanup() throws Exception } } } + + public static class ImportFolderSourceScopingTestCase extends AbstractContainerScopingTest + { + @Test + public void testImportFromTemplateRequiresSourceAdmin() throws Exception + { + Container dest = createContainer("Dest"); + Container source = createContainer("Source"); + User destAdminOnly = createUserInRole(dest, FolderAdminRole.class); + + ActionURL url = new ActionURL(ImportFolderAction.class, dest) + .addParameter("sourceTemplateFolder", source.getPath()) + .addParameter("sourceTemplateFolderId", source.getId()); + MockHttpServletResponse resp = post(url, destAdminOnly); + + // The fix rejects the import and reshows the form (200) rather than redirecting to success (302), with a + // source-permission error message in the rendered content. + assertStatus(HttpServletResponse.SC_OK, resp); + assertTrue("Expected a source-permission rejection message, content was: " + resp.getContentAsString(), + resp.getContentAsString().contains("permission to import from the specified source folder")); + + // Positive control performed in S3ImportTest.testS3Import(). Difficult to mock here due to pipeline job + } + } } diff --git a/core/src/org/labkey/core/webdav/DavController.java b/core/src/org/labkey/core/webdav/DavController.java index 76fac322759..4b5590749db 100644 --- a/core/src/org/labkey/core/webdav/DavController.java +++ b/core/src/org/labkey/core/webdav/DavController.java @@ -32,6 +32,7 @@ import org.json.JSONObject; import org.json.JSONWriter; import org.junit.Assert; +import org.junit.Before; import org.junit.Test; import org.labkey.api.action.ApiUsageException; import org.labkey.api.action.BaseViewAction; @@ -66,8 +67,12 @@ import org.labkey.api.security.SecurityManager; import org.labkey.api.security.User; import org.labkey.api.security.UserManager; +import org.labkey.api.security.permissions.AbstractContainerScopingTest; import org.labkey.api.security.permissions.BrowserDeveloperPermission; import org.labkey.api.security.permissions.ReadPermission; +import org.labkey.api.security.roles.AuthorRole; +import org.labkey.api.security.roles.EditorRole; +import org.labkey.api.security.roles.ReaderRole; import org.labkey.api.settings.AppProps; import org.labkey.api.settings.LookAndFeelProperties; import org.labkey.api.settings.OptionalFeatureService; @@ -99,6 +104,8 @@ import org.labkey.vfs.FileLike; import org.springframework.beans.MutablePropertyValues; import org.springframework.beans.PropertyValues; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.validation.BindException; import org.springframework.web.multipart.MultipartException; import org.springframework.web.multipart.MultipartFile; @@ -122,10 +129,8 @@ import jakarta.servlet.http.HttpSession; import jakarta.servlet.http.Part; import javax.xml.parsers.DocumentBuilder; -import javax.xml.parsers.DocumentBuilderFactory; import javax.xml.parsers.ParserConfigurationException; import javax.xml.parsers.SAXParser; -import javax.xml.parsers.SAXParserFactory; import java.io.BufferedInputStream; import java.io.BufferedWriter; import java.io.ByteArrayInputStream; @@ -135,6 +140,7 @@ import java.io.File; import java.io.FileInputStream; import java.io.FileNotFoundException; +import java.io.FileOutputStream; import java.io.FilterInputStream; import java.io.FilterWriter; import java.io.IOException; @@ -3720,6 +3726,9 @@ WebdavStatus doMethod() throws DavException, IOException if (!src.canRead(getUser(), true)) return unauthorized(src); + // MOVE is effectively a delete operation from the source's perspective so confirm access + if (!src.canDelete(getUser(), true)) + return unauthorized(src); if (exists && !dest.canWrite(getUser(),true) || !exists && !dest.canCreate(getUser(),true)) return unauthorized(dest); @@ -6630,4 +6639,134 @@ public void testElementName() } } + + public static class MoveActionContainerScopingTestCase extends AbstractContainerScopingTest + { + private Container _folder; + private User _mover; + + @Before + public void setUp() throws Exception + { + _folder = createContainer("Folder"); + // Author = Read + Insert but NOT Delete. This lets the caller pass the source-read and destination-create + // checks so a same-folder MOVE's outcome turns solely on the source-delete guard under test. + _mover = createUserInRole(_folder, AuthorRole.class); + } + + // Documents the precondition the MOVE fix relies on: MOVE removes the source, so it requires Delete there. + // An Author can read and create but, lacking Delete, must not be able to rename/move. + @Test + public void testRenamePermissionInvariant() + { + WebdavResource files = WebdavService.get().lookup(filesPath(_folder)); + assertNotNull("Expected a @files webdav node for the test folder", files); + assertTrue("Author should be able to read @files", files.canRead(_mover, true)); + assertTrue("Author should be able to create in @files (so the destination check passes)", files.canCreate(_mover, true)); + assertFalse("Author must NOT be able to rename/move @files (lacks Delete)", files.canRename(_mover, true)); + assertTrue("Admin should be able to rename @files", files.canRename(getAdmin(), true)); + } + + // Drives an actual MOVE through WebdavServlet/DavController. The caller can read and create in the (single) + // folder but cannot delete, so only the source-delete guard can forbid the move. Without the fix the move + // succeeds (SC_CREATED), removing a file the caller has no right to delete; with the fix it is forbidden. + @Test + public void testMoveActionRequiresSourceDelete() throws Exception + { + File dir = ensureFilesDir(_folder); + File srcFile = writeFile(dir); + assertTrue("Source file should resolve through the resolver", WebdavService.get().lookup(filesPath(_folder).append("secret.txt")).exists()); + + Path src = filesPath(_folder).append("secret.txt"); + Path dest = filesPath(_folder).append("moved.txt"); + + MockHttpServletResponse resp = doMove(_folder, src, dest, _mover); + assertEquals("A caller without Delete on the source must be forbidden from MOVE", HttpServletResponse.SC_FORBIDDEN, resp.getStatus()); + assertTrue("Source file must still exist after a forbidden move", srcFile.exists()); + + // Positive control: the same MOVE driven by an admin (who has Delete, hence rename) succeeds, proving the + // guard forbids only the under-privileged caller rather than every MOVE. + MockHttpServletResponse adminResp = doMove(_folder, src, dest, getAdmin()); + assertEquals("An admin must be allowed to MOVE", HttpServletResponse.SC_CREATED, adminResp.getStatus()); + assertFalse("Source file should no longer exist after a successful move", srcFile.exists()); + assertTrue("Destination file should exist after a successful move", new File(dir, "moved.txt").exists()); + } + + // Ensure moves by a caller who CAN delete in the source but lacks Insert in the target are forbidden + @Test + public void testMoveActionRequiresTargetCreate() throws Exception + { + Container source = createContainer("DeleteSource"); + Container target = createContainer("NoInsertTarget"); + + // Editor in the source (Read+Insert+Update+Delete) -> passes the source read and delete checks. + // Reader in the target (no Insert) -> proves read access to the target is not enough; Insert is required. + User editorInSourceOnly = createUserInRole(source, EditorRole.class); + grantRole(editorInSourceOnly, target, ReaderRole.class); + + File srcDir = ensureFilesDir(source); + File targetDir = ensureFilesDir(target); + File srcFile = writeFile(srcDir); + + Path src = filesPath(source).append("secret.txt"); + Path dest = filesPath(target).append("moved.txt"); + + MockHttpServletResponse resp = doMove(source, src, dest, editorInSourceOnly); + assertEquals("A caller without Insert on the target must be forbidden from MOVE", HttpServletResponse.SC_FORBIDDEN, resp.getStatus()); + assertTrue("Source file must still exist after a forbidden move", srcFile.exists()); + assertFalse("Destination file must not have been created", FileUtil.appendName(targetDir, "moved.txt").exists()); + + // Positive control: a caller who is also Editor (hence Insert) in the target can complete the same + // cross-container MOVE, proving the destination guard forbids only the under-privileged caller. + User editorInBoth = createUserInRole(source, EditorRole.class); + grantRole(editorInBoth, target, EditorRole.class); + MockHttpServletResponse okResp = doMove(source, src, dest, editorInBoth); + assertEquals("With Insert on the target the MOVE must succeed", HttpServletResponse.SC_CREATED, okResp.getStatus()); + assertFalse("Source file should no longer exist after a successful move", srcFile.exists()); + assertTrue("Destination file should exist after a successful move", FileUtil.appendName(targetDir, "moved.txt").exists()); + } + + private static Path filesPath(Container c) + { + return WebdavService.getPath().append(c.getParsedPath()).append(FileContentService.FILES_LINK); + } + + private static File ensureFilesDir(Container c) + { + WebdavResource filesNode = WebdavService.get().lookup(filesPath(c)); + assertNotNull("Test requires a @files node for " + c.getName(), filesNode); + File dir = filesNode.getFile(); + assertNotNull("Test requires a file root for " + c.getName(), dir); + if (!dir.exists()) + dir.mkdirs(); + return dir; + } + + private static File writeFile(File dir) throws IOException + { + File f = FileUtil.appendName(dir, "secret.txt"); + try (FileOutputStream os = new FileOutputStream(f)) + { + os.write("secret".getBytes(StandardCharsets.UTF_8)); + } + return f; + } + + private static MockHttpServletResponse doMove(Container sourceContainer, Path srcResource, Path destResource, User user) throws Exception + { + String srcWebdav = srcResource.toString(); + String destWebdav = destResource.toString(); + String servletPath = "/" + WebdavService.getServletPath(); + + HttpServletRequest base = ViewServlet.mockRequest("MOVE", new ActionURL(name, "move", sourceContainer), user, Map.of("Destination", destWebdav), null); + MockHttpServletRequest req = (MockHttpServletRequest) base; + req.setServletPath(servletPath); + req.setPathInfo(srcWebdav.substring(servletPath.length())); + req.setRequestURI(srcWebdav); + + MockHttpServletResponse resp = new MockHttpServletResponse(); + new WebdavServlet(false).service(req, resp); + return resp; + } + } } diff --git a/experiment/src/org/labkey/experiment/ExperimentModule.java b/experiment/src/org/labkey/experiment/ExperimentModule.java index 7dfb0c6dbd6..810160a20c4 100644 --- a/experiment/src/org/labkey/experiment/ExperimentModule.java +++ b/experiment/src/org/labkey/experiment/ExperimentModule.java @@ -949,6 +949,7 @@ public Collection getSummary(Container c) public Set getIntegrationTests() { return Set.of( + ExperimentController.ContainerScopingTestCase.class, DomainImpl.TestCase.class, DomainPropertyImpl.TestCase.class, ExpDataTableImpl.TestCase.class, diff --git a/experiment/src/org/labkey/experiment/ExternalDocsURLCustomPropertyRenderer.java b/experiment/src/org/labkey/experiment/ExternalDocsURLCustomPropertyRenderer.java index e6544c8755a..604b1cf6768 100644 --- a/experiment/src/org/labkey/experiment/ExternalDocsURLCustomPropertyRenderer.java +++ b/experiment/src/org/labkey/experiment/ExternalDocsURLCustomPropertyRenderer.java @@ -78,6 +78,6 @@ public String getValue(ObjectProperty prop, List siblingProperti { // That's OK, we won't try to do anything with the link } - return "" + PageFlowUtil.filter(label) + ""; + return "" + PageFlowUtil.filter(label) + ""; } } diff --git a/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java b/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java index 67c15cdc4cf..cc2ffd6445f 100644 --- a/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java +++ b/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java @@ -31,6 +31,7 @@ import org.json.JSONArray; import org.json.JSONException; import org.json.JSONObject; +import org.junit.Test; import org.labkey.api.action.ApiJsonWriter; import org.labkey.api.action.ApiResponse; import org.labkey.api.action.ApiSimpleResponse; @@ -59,6 +60,7 @@ import org.labkey.api.attachments.AttachmentParent; import org.labkey.api.attachments.AttachmentService; import org.labkey.api.attachments.BaseDownloadAction; +import org.labkey.api.attachments.ByteArrayAttachmentFile; import org.labkey.api.audit.AbstractAuditTypeProvider; import org.labkey.api.audit.AuditLogService; import org.labkey.api.audit.DetailedAuditTypeEvent; @@ -187,6 +189,7 @@ import org.labkey.api.security.RequiresPermission; import org.labkey.api.security.SecurableResource; import org.labkey.api.security.User; +import org.labkey.api.security.permissions.AbstractContainerScopingTest; import org.labkey.api.security.permissions.AdminPermission; import org.labkey.api.security.permissions.DeletePermission; import org.labkey.api.security.permissions.DesignDataClassPermission; @@ -198,6 +201,7 @@ import org.labkey.api.security.permissions.SiteAdminPermission; import org.labkey.api.security.permissions.TroubleshooterPermission; import org.labkey.api.security.permissions.UpdatePermission; +import org.labkey.api.security.roles.ReaderRole; import org.labkey.api.settings.AppProps; import org.labkey.api.settings.ConceptURIProperties; import org.labkey.api.sql.LabKeySql; @@ -298,6 +302,7 @@ import org.labkey.vfs.FileLike; import org.springframework.beans.PropertyValue; import org.springframework.beans.PropertyValues; +import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.validation.BindException; import org.springframework.validation.Errors; import org.springframework.validation.ObjectError; @@ -316,6 +321,7 @@ import java.io.InputStream; import java.io.OutputStream; import java.net.URI; +import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; import java.sql.SQLException; @@ -386,6 +392,10 @@ public static void ensureCorrectContainer(Container requestContainer, ExpObject Container objectContainer = object.getContainer(); if (!requestContainer.equals(objectContainer)) { + // Only redirect if the user can read the object's container; otherwise don't reveal that it exists + if (objectContainer == null || !objectContainer.hasPermission(viewContext.getUser(), ReadPermission.class)) + throw new NotFoundException(); + ActionURL url = viewContext.cloneActionURL(); url.setContainer(objectContainer); throw new RedirectException(url); @@ -1797,6 +1807,8 @@ public Pair getAttachment(AttachmentForm form) ExpData data = ExperimentServiceImpl.get().getExpData(lsid.toString()); if (data == null) throw new NotFoundException("Error: Data object not found for the given LSID: " + lsid); + // The LSID could be from another container. If so, redirect there + ensureCorrectContainer(getContainer(), data, getViewContext()); AttachmentParent parent = new ExpDataClassAttachmentParent(data.getContainer(), lsid); return new Pair<>(parent, form.getName()); @@ -8335,4 +8347,47 @@ public Object execute(Object o, BindException errors) throws Exception } } + public static class ContainerScopingTestCase extends AbstractContainerScopingTest + { + @Test + public void testDataClassAttachmentContainerScoping() throws Exception + { + User admin = getAdmin(); + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + User readerA = createUserInRole(folderA, ReaderRole.class); + + // A data object that lives in folder B, with a real attachment so the legitimate download path can be exercised. + String attachmentName = "attachment.txt"; + String lsid = ExperimentService.get().generateGuidLSID(folderB, ExpData.class); + ExpData data = ExperimentService.get().createData(folderB, "exp1-scope-test", lsid); + data.save(admin); + AttachmentParent parent = new ExpDataClassAttachmentParent(folderB, new Lsid(lsid)); + AttachmentService.get().addAttachments(parent, List.of(new ByteArrayAttachmentFile(attachmentName, "scope test".getBytes(StandardCharsets.UTF_8), "text/plain")), admin); + + ActionURL foreignUrl = new ActionURL(DataClassAttachmentDownloadAction.class, folderA) + .addParameter("lsid", lsid) + .addParameter("name", attachmentName); + + // Deny branch: a caller who can read folder A but NOT folder B must not learn that B's data exists -> 404, + // rather than being redirected (which would leak B's existence and path). + assertStatus(HttpServletResponse.SC_NOT_FOUND, get(foreignUrl, readerA)); + + // Redirect branch: a caller who CAN read folder B is redirected to its own container (where Read is + // re-enforced) rather than served from folder A. Fails without the ensureCorrectContainer call. + MockHttpServletResponse resp = get(foreignUrl, admin); + assertStatus(HttpServletResponse.SC_FOUND, resp); + String location = resp.getRedirectedUrl(); + assertNotNull("Redirect must have a Location", location); + assertTrue("Redirect should target the data's own container, was: " + location, location.contains(folderB.getPath())); + + // Positive control: addressing the attachment through its own container serves the file (200), proving the + // action isn't simply rejecting every request. + ActionURL ownUrl = new ActionURL(DataClassAttachmentDownloadAction.class, folderB) + .addParameter("lsid", lsid) + .addParameter("name", attachmentName); + assertStatus(HttpServletResponse.SC_OK, get(ownUrl, admin)); + } + } + } diff --git a/issues/src/org/labkey/issue/IssuesController.java b/issues/src/org/labkey/issue/IssuesController.java index f2b9b65184a..ee2eb357a1a 100644 --- a/issues/src/org/labkey/issue/IssuesController.java +++ b/issues/src/org/labkey/issue/IssuesController.java @@ -28,6 +28,7 @@ import org.jetbrains.annotations.Nullable; import org.json.JSONArray; import org.json.JSONObject; +import org.junit.Test; import org.labkey.api.action.ApiResponse; import org.labkey.api.action.ApiSimpleResponse; import org.labkey.api.action.BaseViewAction; @@ -79,6 +80,7 @@ import org.labkey.api.issues.IssuesListDefService; import org.labkey.api.issues.IssuesSchema; import org.labkey.api.issues.IssuesUrls; +import org.labkey.api.module.Module; import org.labkey.api.module.ModuleHtmlView; import org.labkey.api.module.ModuleLoader; import org.labkey.api.query.FieldKey; @@ -99,11 +101,14 @@ import org.labkey.api.security.SecurityManager; import org.labkey.api.security.User; import org.labkey.api.security.UserManager; +import org.labkey.api.security.permissions.AbstractContainerScopingTest; import org.labkey.api.security.permissions.AdminPermission; 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.FolderAdminRole; import org.labkey.api.security.roles.OwnerRole; +import org.labkey.api.security.roles.ReaderRole; import org.labkey.api.security.roles.RoleManager; import org.labkey.api.util.ButtonBuilder; import org.labkey.api.util.CSRFUtil; @@ -113,6 +118,7 @@ import org.labkey.api.util.JsonUtil; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.util.Pair; +import org.labkey.api.util.TestContext; import org.labkey.api.util.URLHelper; import org.labkey.api.util.InputBuilder; import org.labkey.api.view.ActionURL; @@ -1633,9 +1639,41 @@ public static class MoveAction extends MutatingApiAction @Override public ApiResponse execute(MoveIssueForm form, BindException errors) { + if (form.getIssueIds() == null || form.getIssueIds().length == 0) + throw new NotFoundException("No issues specified to move"); + + // The client supplies the destination; resolve it and validate it per issue below + Container dest = form.getTargetContainerId() != null ? ContainerManager.getForId(form.getTargetContainerId()) : null; + if (dest == null) + throw new NotFoundException("Target container not found"); + + if (!dest.hasPermission(getUser(), AdminPermission.class)) + throw new UnauthorizedException(); + + List issueIds = Arrays.asList(form.getIssueIds()); + for (Integer issueId : issueIds) + { + // getIssue(null, ...) resolves by global id, so each issue and the chosen destination must be + // authorized explicitly rather than relying on the current container's Admin grant. + IssueObject issue = IssueManager.getIssue(null, getUser(), issueId); + if (issue == null) + throw new NotFoundException("Issue not found: " + issueId); + + Container source = issue.getContainerFromId(); + // Moving an issue removes it from its source folder, so require Admin there. + if (source == null || !source.hasPermission(getUser(), AdminPermission.class)) + throw new UnauthorizedException(); + + // The destination must be a legitimate move target for this issue's list definition. This also + // confirms the user can access the matching issue list in the destination. + IssueListDef issueListDef = IssueManager.getIssueListDef(issue); + if (issueListDef == null || !IssueManager.getMoveDestinationContainers(source, getUser(), issueListDef.getName()).contains(dest)) + throw new UnauthorizedException(); + } + try { - IssueManager.moveIssues(getUser(), Arrays.asList(form.getIssueIds()), ContainerManager.getForId(form.getTargetContainerId())); + IssueManager.moveIssues(getUser(), issueIds, dest); } catch (IOException x) { @@ -2340,4 +2378,84 @@ public void setIssueId(int issueId) this.issueId = issueId; } } + + public static class MoveActionContainerScopingTestCase extends AbstractContainerScopingTest + { + @Test + public void testMoveRequiresSourceAdmin() throws Exception + { + // Admin in the destination only + // Positive control is in IssuesTest.moveIssueTest(). + assertCrossContainerMoveRejected(MoverScope.DESTINATION); + } + + @Test + public void testMoveRequiresDestinationAdmin() throws Exception + { + // Admin in the source only: driving the action through the source folder gets past @RequiresPermission and + // the source-admin guard, so the move's outcome turns solely on the destination-admin guard -> 403. + assertCrossContainerMoveRejected(MoverScope.SOURCE); + } + + private enum MoverScope { SOURCE, DESTINATION } + + /** + * Create a source and destination folder, put an issue in the source (owned by the site admin), then attempt to + * move it as a caller who is a folder admin in exactly one of the two folders ({@code moverScope}). + */ + private void assertCrossContainerMoveRejected(MoverScope moverScope) throws Exception + { + User admin = getAdmin(); + Container source = createContainer("Source"); // the issue lives here + Container dest = createContainer("Dest"); + + ensureIssuesEnabled(source); + + // Create an issue in the source folder (as the site admin) + IssueObject issue = new IssueObject(); + issue.open(source, admin); + issue.setAssignedTo(admin.getUserId()); + issue.setTitle("Scoping test issue"); + issue.setPriority("3"); + issue.setIssueDefName(IssueListDef.DEFAULT_ISSUE_LIST_NAME); + ObjectFactory.Registry.getFactory(IssueObject.class).toMap(issue, issue.getProperties()); + IssueManager.saveIssue(admin, source, issue); + int issueId = issue.getIssueId(); + + // A folder admin in exactly one of the two folders; drive the action through that folder + Container moverFolder = moverScope == MoverScope.SOURCE ? source : dest; + User mover = createUserInRole(moverFolder, FolderAdminRole.class); + // Make them a reader in the other folder + grantRole(mover, moverScope == MoverScope.SOURCE ? dest : source, ReaderRole.class); + + ActionURL url = new ActionURL(MoveAction.class, moverFolder) + .addParameter("issueIds", String.valueOf(issueId)) + .addParameter("targetContainerId", dest.getId()); + assertStatus(HttpServletResponse.SC_FORBIDDEN, post(url, mover)); + + // The issue must remain in its source container + assertNotNull("Issue should still exist in its source folder", IssueManager.getIssue(source, admin, issueId)); + } + + private static void ensureIssuesEnabled(Container c) + { + Module issueModule = ModuleLoader.getInstance().getModule(IssuesModule.NAME); + Set activeModules = c.getActiveModules(); + if (!activeModules.contains(issueModule)) + { + Set newActiveModules = new HashSet<>(activeModules); + newActiveModules.add(issueModule); + c.setActiveModules(newActiveModules); + } + if (IssueManager.getIssueListDef(c, IssueListDef.DEFAULT_ISSUE_LIST_NAME) == null) + { + IssueListDef def = new IssueListDef(); + def.setName(IssueListDef.DEFAULT_ISSUE_LIST_NAME); + def.setLabel(IssueListDef.DEFAULT_ISSUE_LIST_NAME); + def.setKind(IssueDefDomainKind.NAME); + def.beforeInsert(TestContext.get().getUser(), c.getId()); + def.save(TestContext.get().getUser()); + } + } + } } diff --git a/issues/src/org/labkey/issue/IssuesModule.java b/issues/src/org/labkey/issue/IssuesModule.java index 63c9a7aa91e..06630d6dbad 100644 --- a/issues/src/org/labkey/issue/IssuesModule.java +++ b/issues/src/org/labkey/issue/IssuesModule.java @@ -193,7 +193,10 @@ public ActionURL getTabURL(Container c, User user) @NotNull public Set getIntegrationTests() { - return Collections.singleton(org.labkey.issue.model.IssueManager.TestCase.class); + return Set.of( + org.labkey.issue.model.IssueManager.TestCase.class, + org.labkey.issue.IssuesController.MoveActionContainerScopingTestCase.class + ); } @Override diff --git a/mothership/src/org/labkey/mothership/MothershipController.java b/mothership/src/org/labkey/mothership/MothershipController.java index f3284469b78..398dec07b60 100644 --- a/mothership/src/org/labkey/mothership/MothershipController.java +++ b/mothership/src/org/labkey/mothership/MothershipController.java @@ -16,6 +16,7 @@ package org.labkey.mothership; +import jakarta.servlet.http.HttpServletResponse; import org.apache.commons.beanutils.ConversionException; import org.apache.commons.lang3.StringUtils; import org.apache.commons.validator.routines.InetAddressValidator; @@ -23,6 +24,7 @@ import org.jetbrains.annotations.NotNull; import org.json.JSONException; import org.json.JSONObject; +import org.junit.Test; import org.labkey.api.action.BaseApiAction; import org.labkey.api.action.BaseViewAction; import org.labkey.api.action.FormHandlerAction; @@ -49,6 +51,7 @@ import org.labkey.api.data.RenderContext; import org.labkey.api.data.SimpleFilter; import org.labkey.api.data.Sort; +import org.labkey.api.data.Table; import org.labkey.api.data.TableInfo; import org.labkey.api.data.TableSelector; import org.labkey.api.module.AllowedDuringUpgrade; @@ -64,6 +67,7 @@ import org.labkey.api.security.RequiresSiteAdmin; import org.labkey.api.security.User; import org.labkey.api.security.UserManager; +import org.labkey.api.security.permissions.AbstractContainerScopingTest; import org.labkey.api.security.permissions.AdminPermission; import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.security.permissions.UpdatePermission; @@ -562,6 +566,22 @@ public void validateCommand(ServerInstallationForm target, Errors errors) @Override public boolean handlePost(ServerInstallationForm form, BindException errors) throws Exception { + // Confirm the row belongs to the current container + Object pk = form.getPkVal(); + if (pk == null) + throw new NotFoundException("No server installation specified"); + int installationId; + try + { + installationId = Integer.parseInt(String.valueOf(pk)); + } + catch (NumberFormatException e) + { + throw new NotFoundException("Invalid server installation id: " + pk); + } + if (MothershipManager.get().getServerInstallation(installationId, getContainer()) == null) + throw new NotFoundException("Server installation not found in this folder"); + form.doUpdate(); return true; } @@ -1975,5 +1995,43 @@ public void setUptimeContainer(String uptimeContainer) _uptimeContainer = uptimeContainer; } } + + public static class ContainerScopingTestCase extends AbstractContainerScopingTest + { + @Test + public void testUpdateInstallationContainerScoping() throws Exception + { + User admin = getAdmin(); + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + + // An installation row that lives in folder B + ServerInstallation si = new ServerInstallation(); + si.setContainer(folderB.getId()); + si.setServerInstallationGUID(GUID.makeGUID()); + si.setNote("original"); + si = Table.insert(admin, MothershipManager.get().getTableInfoServerInstallation(), si); + int id = si.getServerInstallationId(); + + // Try to update it through folder A; the fix resolves the row in the current container and 404s on a miss + ActionURL url = new ActionURL(UpdateInstallationAction.class, folderA) + .addParameter("ServerInstallationId", String.valueOf(id)) + .addParameter("Note", "hacked"); + assertStatus(HttpServletResponse.SC_NOT_FOUND, post(url, admin)); + + // The row in folder B must be untouched + ServerInstallation reloaded = MothershipManager.get().getServerInstallation(id, folderB); + assertNotNull("Installation should still exist in its own container", reloaded); + assertEquals("Note must not have been overwritten", "original", reloaded.getNote()); + + // Positive control: updating through the row's own container (folder B) succeeds and persists the change, + // proving the guard rejects only the cross-container case, not every update. + ActionURL ownUrl = new ActionURL(UpdateInstallationAction.class, folderB) + .addParameter("ServerInstallationId", String.valueOf(id)) + .addParameter("Note", "updated"); + assertStatus(HttpServletResponse.SC_FOUND, post(ownUrl, admin)); + assertEquals("Note should have been updated through the row's own container", "updated", MothershipManager.get().getServerInstallation(id, folderB).getNote()); + } + } } diff --git a/mothership/src/org/labkey/mothership/MothershipModule.java b/mothership/src/org/labkey/mothership/MothershipModule.java index 471eecdde46..99f7cf6f574 100644 --- a/mothership/src/org/labkey/mothership/MothershipModule.java +++ b/mothership/src/org/labkey/mothership/MothershipModule.java @@ -116,6 +116,12 @@ public Set getUnitTests() return PageFlowUtil.set(ExceptionStackTrace.TestCase.class); } + @Override + public @NotNull Set getIntegrationTests() + { + return PageFlowUtil.set(MothershipController.ContainerScopingTestCase.class); + } + @Override public void doStartup(ModuleContext moduleContext) { diff --git a/pipeline/src/org/labkey/pipeline/PipelineModule.java b/pipeline/src/org/labkey/pipeline/PipelineModule.java index 2c692ed9485..a7781c0f7aa 100644 --- a/pipeline/src/org/labkey/pipeline/PipelineModule.java +++ b/pipeline/src/org/labkey/pipeline/PipelineModule.java @@ -339,6 +339,7 @@ public Set getIntegrationTests() PipelineQueueImpl.TestCase.class, PipelineServiceImpl.TestCase.class, StatusController.TestCase.class, + StatusController.ContainerScopingTestCase.class, ClusterStartup.TestCase.class ); } diff --git a/pipeline/src/org/labkey/pipeline/status/StatusController.java b/pipeline/src/org/labkey/pipeline/status/StatusController.java index b167d6bc75b..94965407a05 100644 --- a/pipeline/src/org/labkey/pipeline/status/StatusController.java +++ b/pipeline/src/org/labkey/pipeline/status/StatusController.java @@ -16,7 +16,9 @@ package org.labkey.pipeline.status; +import jakarta.servlet.http.HttpServletResponse; import org.jetbrains.annotations.Nullable; +import org.junit.Test; import org.labkey.api.action.ApiResponse; import org.labkey.api.action.FormHandlerAction; import org.labkey.api.action.FormViewAction; @@ -35,6 +37,7 @@ import org.labkey.api.data.ContainerManager; import org.labkey.api.data.DataRegion; import org.labkey.api.data.DataRegionSelection; +import org.labkey.api.data.Table; import org.labkey.api.pipeline.NoSuchJobException; import org.labkey.api.pipeline.PipeRoot; import org.labkey.api.pipeline.PipelineJob; @@ -51,11 +54,13 @@ import org.labkey.api.security.RequiresPermission; import org.labkey.api.security.User; import org.labkey.api.security.permissions.AbstractActionPermissionTest; +import org.labkey.api.security.permissions.AbstractContainerScopingTest; import org.labkey.api.security.permissions.AdminOperationsPermission; import org.labkey.api.security.permissions.DeletePermission; import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.security.permissions.TroubleshooterPermission; import org.labkey.api.security.permissions.UpdatePermission; +import org.labkey.api.security.roles.ReaderRole; import org.labkey.api.settings.AdminConsole; import org.labkey.api.util.FileUtil; import org.labkey.api.util.HtmlString; @@ -77,6 +82,7 @@ import org.labkey.api.view.template.PageConfig; import org.labkey.pipeline.PipelineController; import org.labkey.pipeline.analysis.AnalysisController; +import org.labkey.pipeline.api.PipelineSchema; import org.labkey.pipeline.api.PipelineServiceImpl; import org.labkey.pipeline.api.PipelineStatusFileImpl; import org.springframework.validation.BindException; @@ -84,6 +90,7 @@ import org.springframework.web.servlet.ModelAndView; import java.io.BufferedReader; +import java.io.File; import java.io.IOException; import java.io.PrintWriter; import java.nio.file.Files; @@ -470,7 +477,7 @@ public Object execute(StatusDetailsForm form, BindException errors) throws Excep Container c = getContainerCheckAdmin(); PipelineStatusFile psf = getStatusFile(form.getRowId()); - if (psf == null) + if (psf == null || !getContainer().equals(psf.lookupContainer())) throw new NotFoundException("Could not find status file for rowId " + form.getRowId()); var status = StatusDetailsBean.create(c, psf, form.getOffset(), form.getCount()); @@ -1076,4 +1083,39 @@ controller.new ForceRefreshAction() ); } } + + public static class ContainerScopingTestCase extends AbstractContainerScopingTest + { + @Test + public void testStatusDetailsContainerScoping() throws Exception + { + User admin = getAdmin(); + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + User readerA = createUserInRole(folderA, ReaderRole.class); + + // A status file that lives in folder B. FilePath is a required column; point it at a non-existent log so + // StatusDetailsBean skips reading it (it only reads when the file exists) without affecting the scoping check. + PipelineStatusFileImpl sf = new PipelineStatusFileImpl(); + sf.beforeInsert(admin, folderB.getId()); + sf.setStatus(PipelineJob.TaskStatus.complete.toString()); + sf.setFilePath(FileUtil.appendName(FileUtil.getTempDirectory(), "pipeline-scoping-test-" + folderB.getRowId() + ".log").getAbsolutePath()); + sf = Table.insert(admin, PipelineSchema.getInstance().getTableInfoStatusFiles(), sf); + long rowId = sf.getRowId(); + + ActionURL foreignUrl = new ActionURL(StatusDetailsAction.class, folderA).addParameter("rowId", String.valueOf(rowId)); + + // The API is scoped to its own container: addressing B's job through folder A is 404, regardless of the + // caller's rights in B. This is the case that fails without the fix (the unscoped action would serve B's + // job through folder A). + // A caller who can read folder A but NOT folder B: + assertStatus(HttpServletResponse.SC_NOT_FOUND, get(foreignUrl, readerA)); + // ...and a site admin, who CAN read folder B, still gets 404 through folder A (no cross-container redirect). + assertStatus(HttpServletResponse.SC_NOT_FOUND, get(foreignUrl, admin)); + + // Positive control: addressing the job through its own container still succeeds. + ActionURL ownUrl = new ActionURL(StatusDetailsAction.class, folderB).addParameter("rowId", String.valueOf(rowId)); + assertStatus(HttpServletResponse.SC_OK, get(ownUrl, admin)); + } + } } diff --git a/specimen/src/org/labkey/specimen/SpecimenModule.java b/specimen/src/org/labkey/specimen/SpecimenModule.java index 9dbcf3679d0..a83415169e8 100644 --- a/specimen/src/org/labkey/specimen/SpecimenModule.java +++ b/specimen/src/org/labkey/specimen/SpecimenModule.java @@ -72,6 +72,7 @@ import org.labkey.specimen.importer.SpecimenSettingsImporter; import org.labkey.specimen.model.SpecimenRequestEventType; import org.labkey.specimen.pipeline.SpecimenPipeline; +import org.labkey.specimen.requirements.SpecimenRequestRequirementProvider; import org.labkey.specimen.query.SpecimenPivotByDerivativeType; import org.labkey.specimen.query.SpecimenPivotByPrimaryType; import org.labkey.specimen.query.SpecimenPivotByRequestingLocation; @@ -333,7 +334,9 @@ public Set getUnitTests() public @NotNull Set getIntegrationTests() { return Set.of( - SpecimenImporter.TestCase.class + SpecimenImporter.TestCase.class, + SpecimenRequestRequirementProvider.ContainerScopingTestCase.class, + SpecimenController.ContainerScopingTestCase.class ); } } \ No newline at end of file diff --git a/specimen/src/org/labkey/specimen/actions/ShowGroupMembersAction.java b/specimen/src/org/labkey/specimen/actions/ShowGroupMembersAction.java index ddf84246704..f728e50de34 100644 --- a/specimen/src/org/labkey/specimen/actions/ShowGroupMembersAction.java +++ b/specimen/src/org/labkey/specimen/actions/ShowGroupMembersAction.java @@ -92,6 +92,10 @@ public boolean handlePost(UpdateGroupForm form, BindException errors) throws Exc SpecimenRequestActor actor = getActor(form); LocationImpl location = getLocation(form); + // getActor is container-scoped; null means the actorId doesn't belong to this folder + if (actor == null) + throw new NotFoundException(); + if (emailsToDelete != null && emailsToDelete.length > 0) { List invalidEmails = new ArrayList<>(); diff --git a/specimen/src/org/labkey/specimen/actions/SpecimenController.java b/specimen/src/org/labkey/specimen/actions/SpecimenController.java index 0682600a0c2..f3d79169d7e 100644 --- a/specimen/src/org/labkey/specimen/actions/SpecimenController.java +++ b/specimen/src/org/labkey/specimen/actions/SpecimenController.java @@ -14,6 +14,7 @@ import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.json.JSONObject; +import org.junit.Test; import org.labkey.api.action.ApiResponse; import org.labkey.api.action.ApiSimpleResponse; import org.labkey.api.action.ExportAction; @@ -82,6 +83,7 @@ import org.labkey.api.security.RequiresPermission; import org.labkey.api.security.User; import org.labkey.api.security.ValidEmail; +import org.labkey.api.security.permissions.AbstractContainerScopingTest; import org.labkey.api.security.permissions.AdminPermission; import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.security.permissions.UpdatePermission; @@ -4611,7 +4613,8 @@ public boolean handlePost(RequirementForm form, BindException errors) throws Exc { SpecimenRequestRequirement requirement = SpecimenRequestRequirementProvider.get().getRequirement(getContainer(), form.getRequirementId()); - if (requirement.getRequestId() == form.getId()) + // getRequirement is container-scoped; null means the requirementId doesn't belong to this folder + if (requirement != null && requirement.getRequestId() == form.getId()) { SpecimenRequestManager.get().deleteRequestRequirement(getUser(), requirement); return true; @@ -5177,6 +5180,9 @@ public boolean handlePost(EmailSpecimenListForm form, BindException errors) thro ids[i] = Integer.parseInt(idStrs[i]); LocationImpl originatingOrProvidingLocation = LocationManager.get().getLocation(getContainer(), ids[0]); SpecimenRequestActor notifyActor = SpecimenRequestRequirementProvider.get().getActor(getContainer(), ids[1]); + // getActor is container-scoped; null means the actorId doesn't belong to this folder + if (notifyActor == null) + throw new NotFoundException("No notification actor found for id " + ids[1] + " in this folder"); LocationImpl notifyLocation = null; if (notifyActor.isPerSite() && ids[2] >= 0) notifyLocation = LocationManager.get().getLocation(getContainer(), ids[2]); @@ -5757,4 +5763,89 @@ public void addNavTrail(NavTree root) root.addChild("Insert " + _form.getQueryName()); } } + + /** + * Verifies that actions resolving a specimen object by its global rowId reject ids that belong to a different + * container, even when the caller has the action's required permission in the current folder. These exercise the + * action-level guards that depend on the container scoping enforced by {@link SpecimenRequestRequirementProvider}. + */ + public static class ContainerScopingTestCase extends AbstractContainerScopingTest + { + // Actors are required by requirements through a NOT NULL foreign key, so several fixtures need one + private SpecimenRequestActor createActor(Container c, String label) + { + SpecimenRequestActor actor = new SpecimenRequestActor(); + actor.setContainer(c); + actor.setLabel(label); + return actor.create(getAdmin()); + } + + @Test + public void testShowGroupMembersActionRejectsForeignActor() throws Exception + { + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + + // An actor that lives in folder A + SpecimenRequestActor actor = createActor(folderA, "Group members scoping actor"); + int actorId = actor.getRowId(); + + // Addressing the action through folder B, where the actor does not live, must 404 rather than expose it + ActionURL foreignUrl = new ActionURL(ShowGroupMembersAction.class, folderB).addParameter("id", String.valueOf(actorId)); + assertStatus(HttpServletResponse.SC_NOT_FOUND, post(foreignUrl, getAdmin())); + + // Positive control: the same request through the actor's own folder is accepted (redirect on success), + // proving the guard rejects only the cross-container case rather than every request + ActionURL ownUrl = new ActionURL(ShowGroupMembersAction.class, folderA).addParameter("id", String.valueOf(actorId)); + assertStatus(HttpServletResponse.SC_FOUND, post(ownUrl, getAdmin())); + } + + @Test + public void testDeleteRequirementActionRejectsForeignRequirement() throws Exception + { + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + SpecimenRequestRequirementProvider provider = SpecimenRequestRequirementProvider.get(); + + // Build a real request chain in folder A: status -> request -> requirement. Deleting a requirement logs a + // request event whose RequestId is a foreign key into SampleRequest, so the request row must actually + // exist (a fake id would fail the FK and mask what this test is checking). Capture each insert's return so + // we have the generated rowId. + SpecimenRequestStatus status = new SpecimenRequestStatus(); + status.setContainer(folderA); + status.setLabel("Delete requirement scoping status"); + status.setSortOrder(1); // non-null and >= 0 so the bean isn't treated as a system status + status = Table.insert(getAdmin(), SpecimenSchema.get().getTableInfoSampleRequestStatus(), status); + + SpecimenRequest request = new SpecimenRequest(); + request.setContainer(folderA); + request.setStatusId(status.getRowId()); + request = SpecimenRequestManager.get().createRequest(getAdmin(), request, false); + int requestId = request.getRowId(); + + SpecimenRequestActor actor = createActor(folderA, "Delete requirement scoping actor"); + SpecimenRequestRequirement requirement = new SpecimenRequestRequirement(); + requirement.setContainer(folderA); + requirement.setRequestId(requestId); + requirement.setActorId(actor.getRowId()); + requirement.setDescription("Delete requirement scoping test"); + requirement = requirement.persist(getAdmin(), GUID.makeGUID()); + int requirementId = requirement.getRowId(); + + // Deleting through folder B, where the requirement does not live, must be a no-op: the row survives + ActionURL foreignUrl = new ActionURL(DeleteRequirementAction.class, folderB) + .addParameter("id", String.valueOf(requestId)) + .addParameter("requirementId", String.valueOf(requirementId)); + post(foreignUrl, getAdmin()); + assertNotNull("Cross-container delete must not remove the requirement", provider.getRequirement(folderA, requirementId)); + + // Positive control: deleting through the requirement's own folder removes it, proving the guard rejects + // only the cross-container case rather than every delete + ActionURL ownUrl = new ActionURL(DeleteRequirementAction.class, folderA) + .addParameter("id", String.valueOf(requestId)) + .addParameter("requirementId", String.valueOf(requirementId)); + post(ownUrl, getAdmin()); + assertNull("Same-container delete should remove the requirement", provider.getRequirement(folderA, requirementId)); + } + } } diff --git a/specimen/src/org/labkey/specimen/requirements/DefaultRequirementProvider.java b/specimen/src/org/labkey/specimen/requirements/DefaultRequirementProvider.java index 803e98fd4b5..2111948c60c 100644 --- a/specimen/src/org/labkey/specimen/requirements/DefaultRequirementProvider.java +++ b/specimen/src/org/labkey/specimen/requirements/DefaultRequirementProvider.java @@ -100,7 +100,11 @@ private synchronized String getDefaultRequirementPlaceholder(final Container con @Override public R getRequirement(Container container, Object requirementPrimaryKey) { - return new TableSelector(getRequirementTableInfo()).getObject(requirementPrimaryKey, _requirementClass); + R requirement = new TableSelector(getRequirementTableInfo()).getObject(requirementPrimaryKey, _requirementClass); + // The lookup is by global primary key; reject rows that don't belong to the requested container + if (requirement != null && !container.equals(requirement.getContainer())) + return null; + return requirement; } public R[] getRequirements(Container container, String ownerEntityId) @@ -133,7 +137,11 @@ public A[] getActors(Container c) @Override public A getActor(Container c, Object primaryKey) { - return new TableSelector(getActorTableInfo()).getObject(primaryKey, _actorClass); + A actor = new TableSelector(getActorTableInfo()).getObject(primaryKey, _actorClass); + // The lookup is by global primary key; reject rows that don't belong to the requested container + if (actor != null && !c.equals(actor.getContainer())) + return null; + return actor; } @Override diff --git a/specimen/src/org/labkey/specimen/requirements/SpecimenRequestRequirementProvider.java b/specimen/src/org/labkey/specimen/requirements/SpecimenRequestRequirementProvider.java index 4af4b5ab4d2..c7be11bb57c 100644 --- a/specimen/src/org/labkey/specimen/requirements/SpecimenRequestRequirementProvider.java +++ b/specimen/src/org/labkey/specimen/requirements/SpecimenRequestRequirementProvider.java @@ -16,9 +16,12 @@ package org.labkey.specimen.requirements; +import org.junit.Test; import org.labkey.api.data.Container; import org.labkey.api.data.TableInfo; +import org.labkey.api.security.permissions.AbstractContainerScopingTest; import org.labkey.api.specimen.SpecimenSchema; +import org.labkey.api.util.GUID; import org.labkey.specimen.model.SpecimenRequestActor; import java.util.Collection; @@ -99,4 +102,49 @@ public Set getActorsInUseSet(Container container) ids.add(actor.getRowId()); return ids; } + + public static class ContainerScopingTestCase extends AbstractContainerScopingTest + { + @Test + public void testGetActorContainerScoping() + { + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + SpecimenRequestRequirementProvider provider = SpecimenRequestRequirementProvider.get(); + + SpecimenRequestActor actor = new SpecimenRequestActor(); + actor.setContainer(folderA); + actor.setLabel("Scoping test actor"); + actor = actor.create(getAdmin()); + int rowId = actor.getRowId(); + + assertNotNull("Actor should be visible from its own container", provider.getActor(folderA, rowId)); + assertNull("Actor must NOT be visible from another container", provider.getActor(folderB, rowId)); + } + + @Test + public void testGetRequirementContainerScoping() + { + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + SpecimenRequestRequirementProvider provider = SpecimenRequestRequirementProvider.get(); + + // A requirement references an actor through a NOT NULL foreign key, so create one in the same folder first + SpecimenRequestActor actor = new SpecimenRequestActor(); + actor.setContainer(folderA); + actor.setLabel("Requirement scoping test actor"); + actor = actor.create(getAdmin()); + + SpecimenRequestRequirement requirement = new SpecimenRequestRequirement(); + requirement.setContainer(folderA); + requirement.setRequestId(-1); // no real request row is needed for a primary-key lookup + requirement.setActorId(actor.getRowId()); + requirement.setDescription("Requirement scoping test"); + requirement = requirement.persist(getAdmin(), GUID.makeGUID()); + int rowId = requirement.getRowId(); + + assertNotNull("Requirement should be visible from its own container", provider.getRequirement(folderA, rowId)); + assertNull("Requirement must NOT be visible from another container", provider.getRequirement(folderB, rowId)); + } + } } diff --git a/study/src/org/labkey/study/StudyModule.java b/study/src/org/labkey/study/StudyModule.java index c5a62276bfa..60bb3b9d968 100644 --- a/study/src/org/labkey/study/StudyModule.java +++ b/study/src/org/labkey/study/StudyModule.java @@ -717,7 +717,8 @@ public Set getIntegrationTests() StudyModule.TestCase.class, VisitImpl.TestCase.class, DatasetUpdateService.TestCase.class, - DatasetLsidImportHelper.TestCase.class); + DatasetLsidImportHelper.TestCase.class, + org.labkey.study.controllers.CreateChildStudyAction.ContainerScopingTestCase.class); } @Override diff --git a/study/src/org/labkey/study/controllers/CreateChildStudyAction.java b/study/src/org/labkey/study/controllers/CreateChildStudyAction.java index 9e502bf6a30..28f9be1907b 100644 --- a/study/src/org/labkey/study/controllers/CreateChildStudyAction.java +++ b/study/src/org/labkey/study/controllers/CreateChildStudyAction.java @@ -15,7 +15,9 @@ */ package org.labkey.study.controllers; +import jakarta.servlet.http.HttpServletResponse; import org.jetbrains.annotations.NotNull; +import org.junit.Test; import org.labkey.api.action.ApiResponse; import org.labkey.api.action.ApiSimpleResponse; import org.labkey.api.action.MutatingApiAction; @@ -31,19 +33,25 @@ import org.labkey.api.query.ValidationException; 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.AdminPermission; +import org.labkey.api.security.permissions.ReadPermission; +import org.labkey.api.security.roles.FolderAdminRole; import org.labkey.api.specimen.SpecimenSchema; import org.labkey.api.specimen.importer.ImportTemplate; import org.labkey.api.study.SpecimenTablesTemplate; import org.labkey.api.study.Study; +import org.labkey.api.study.StudySnapshotType; import org.labkey.api.study.TimepointType; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.util.Path; +import org.labkey.api.view.ActionURL; import org.labkey.study.StudyFolderType; import org.labkey.study.importer.CreateChildStudyPipelineJob; import org.labkey.study.model.ChildStudyDefinition; import org.labkey.study.model.StudyImpl; import org.labkey.study.model.StudyManager; +import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.validation.BindException; import org.springframework.validation.Errors; import org.springframework.validation.ObjectError; @@ -101,27 +109,54 @@ public ApiResponse execute(ChildStudyDefinition form, BindException errors) thro @Override public void validateForm(ChildStudyDefinition form, Errors errors) { - Container c = ContainerManager.getForPath(form.getDstPath()); - _destFolderCreated = c == null; + // Verify the user can read the source study and can administer the destination (or its parent, if the + // folder must be created) before we create any folder or queue the copy job. + Container sourceContainer = form.getSrcPath() != null ? ContainerManager.getForPath(form.getSrcPath()) : null; + if (sourceContainer == null || !sourceContainer.hasPermission(getUser(), ReadPermission.class)) + { + errors.reject(SpringActionController.ERROR_MSG, "Unable to locate the parent study from location : " + form.getSrcPath()); + } + else + { + _sourceStudy = StudyManager.getInstance().getStudy(sourceContainer); + if (_sourceStudy == null) + errors.reject(SpringActionController.ERROR_MSG, "Unable to locate the parent study from location : " + form.getSrcPath()); + } + + Container existingDst = form.getDstPath() != null ? ContainerManager.getForPath(form.getDstPath()) : null; + _destFolderCreated = existingDst == null; + + boolean dstAuthorized; + if (existingDst != null) + { + // Existing destination folder: require Admin on it. + dstAuthorized = existingDst.hasPermission(getUser(), AdminPermission.class); + } + else if (form.getDstPath() != null) + { + // Folder will be created: require Admin on the parent so an arbitrary folder can't be grafted in. + Container dstParent = ContainerManager.getForPath(Path.parse(form.getDstPath()).getParent()); + dstAuthorized = dstParent != null && dstParent.hasPermission(getUser(), AdminPermission.class); + } + else + { + dstAuthorized = false; + } - // make sure the folder, if already existing doesn't already contain a study - _dstContainer = ContainerManager.ensureContainer(Path.parse(form.getDstPath()), getUser()); - if (_dstContainer != null) + if (!dstAuthorized) + { + errors.reject(SpringActionController.ERROR_MSG, "Invalid destination folder."); + } + // Only create the destination folder once the rest of the form (including the source-read check above) has + // validated, so a rejected publish doesn't leave a stray empty folder behind. + else if (!errors.hasErrors()) { + // make sure the folder, if already existing doesn't already contain a study + _dstContainer = ContainerManager.ensureContainer(Path.parse(form.getDstPath()), getUser()); Study study = StudyManager.getInstance().getStudy(_dstContainer); if (study != null) - { errors.reject(SpringActionController.ERROR_MSG, "A study already exists in the destination folder."); - } } - else - errors.reject(SpringActionController.ERROR_MSG, "Invalid destination folder."); - - Container sourceContainer = ContainerManager.getForPath(form.getSrcPath()); - _sourceStudy = StudyManager.getInstance().getStudy(sourceContainer); - - if (_sourceStudy == null) - errors.reject(SpringActionController.ERROR_MSG, "Unable to locate the parent study from location : " + form.getSrcPath()); if (form.getMode() == null) errors.reject(SpringActionController.ERROR_MSG, "Unable to locate a study snapshot type from specified mode"); @@ -178,4 +213,39 @@ private StudyImpl createNewStudy(ChildStudyDefinition form, BindException errors return study; } + + public static class ContainerScopingTestCase extends AbstractContainerScopingTest + { + @Test + public void testPublishRequiresSourceRead() throws Exception + { + User admin = getAdmin(); + Container dest = createContainer("Dest"); // the limited user is admin here + Container source = createContainer("Source"); // holds a real study; the limited user has no rights + + // A real study in the source, so the only reason validateForm can reject is the missing source-read grant. + // createStudy() defaults the required subject noun/column fields, so we only set what this test cares about. + StudyImpl study = new StudyImpl(source, "Source Study"); + study.setTimepointType(TimepointType.VISIT); + StudyManager.getInstance().createTestStudy(admin, study); + + // A user who is a folder admin in the destination only (no rights in the source) + User destAdminOnly = createUserInRole(dest, FolderAdminRole.class); + + ActionURL url = new ActionURL(CreateChildStudyAction.class, dest) + .addParameter("srcPath", source.getPath()) + .addParameter("dstPath", dest.getPath()) + .addParameter("mode", StudySnapshotType.publish.name()); + MockHttpServletResponse resp = post(url, destAdminOnly); + + // The publish must not succeed because the user can't read the source study + assertNotEquals("Publish from an unreadable source study must not succeed", HttpServletResponse.SC_OK, resp.getStatus()); + + // No study should have been published into the destination, and the source study must be untouched + assertNull("No study should have been created in the destination", StudyManager.getInstance().getStudy(dest)); + assertNotNull("The source study must be untouched", StudyManager.getInstance().getStudy(source)); + + // Positive scenario covered by StudyPublishTest + } + } } diff --git a/study/src/org/labkey/study/model/DatasetImportTestCase.jsp b/study/src/org/labkey/study/model/DatasetImportTestCase.jsp index c6c58c6570c..7cdb76bc5dc 100644 --- a/study/src/org/labkey/study/model/DatasetImportTestCase.jsp +++ b/study/src/org/labkey/study/model/DatasetImportTestCase.jsp @@ -97,13 +97,9 @@ public void createStudy() c.setFolderType(FolderTypeManager.get().getFolderType(StudyFolderType.NAME), _context.getUser()); StudyImpl s = new StudyImpl(c, this.getClass().getName()); s.setTimepointType(TimepointType.DATE); - s.setStartDate(new Date(DateUtil.parseDateTime(c, "2001-01-01"))); - s.setSubjectColumnName("SubjectID"); - s.setSubjectNounPlural("Subjects"); - s.setSubjectNounSingular("Subject"); s.setSecurityType(SecurityType.BASIC_WRITE); - s.setStartDate(new Date(DateUtil.parseDateTime(c, "1 Jan 2000"))); - _studyDateBased = StudyManager.getInstance().createStudy(_context.getUser(), s); + s.setStartDate(new Date(DateUtil.parseDateTime("1 Jan 2000"))); + _studyDateBased = StudyManager.getInstance().createTestStudy(_context.getUser(), s); MvUtil.assignMvIndicators(c, new String[] {"X", "Y", "Z"}, @@ -115,12 +111,9 @@ public void createStudy() Container c = ContainerManager.createContainer(junit, name, _context.getUser()); StudyImpl s = new StudyImpl(c, "Junit Study"); s.setTimepointType(TimepointType.VISIT); - s.setStartDate(new Date(DateUtil.parseDateTime(c, "2001-01-01"))); - s.setSubjectColumnName("SubjectID"); - s.setSubjectNounPlural("Subjects"); - s.setSubjectNounSingular("Subject"); + s.setStartDate(new Date(DateUtil.parseDateTime("2001-01-01"))); s.setSecurityType(SecurityType.BASIC_WRITE); - _studyVisitBased = StudyManager.getInstance().createStudy(_context.getUser(), s); + _studyVisitBased = StudyManager.getInstance().createTestStudy(_context.getUser(), s); MvUtil.assignMvIndicators(c, new String[] {"X", "Y", "Z"}, diff --git a/study/src/org/labkey/study/model/DatasetLsidImportHelper.java b/study/src/org/labkey/study/model/DatasetLsidImportHelper.java index 57c23240057..053af1a7652 100644 --- a/study/src/org/labkey/study/model/DatasetLsidImportHelper.java +++ b/study/src/org/labkey/study/model/DatasetLsidImportHelper.java @@ -150,12 +150,9 @@ private StudyImpl createStudy(TimepointType timepointType, String name) Container c = ContainerManager.createContainer(JunitUtil.getTestContainer(), GUID.makeHash(), context.getUser()); StudyImpl study = new StudyImpl(c, name); study.setTimepointType(timepointType); - study.setStartDate(new Date(DateUtil.parseDateTime(c, "2025-04-01"))); - study.setSubjectColumnName("SubjectID"); - study.setSubjectNounPlural("Subjects"); - study.setSubjectNounSingular("Subject"); + study.setStartDate(new Date(DateUtil.parseDateTime("2025-04-01"))); - return StudyManager.getInstance().createStudy(context.getUser(), study); + return StudyManager.getInstance().createTestStudy(context.getUser(), study); } @After diff --git a/study/src/org/labkey/study/model/StudyImpl.java b/study/src/org/labkey/study/model/StudyImpl.java index eabc4999273..4b60f1abb6e 100644 --- a/study/src/org/labkey/study/model/StudyImpl.java +++ b/study/src/org/labkey/study/model/StudyImpl.java @@ -1262,13 +1262,9 @@ public void createStudy() Container c = ContainerManager.createContainer(junit, name, _context.getUser()); StudyImpl s = new StudyImpl(c, "Junit Study"); s.setTimepointType(TimepointType.DATE); - s.setStartDate(new Date(DateUtil.parseISODateTime("2001-01-01"))); - s.setSubjectColumnName("SubjectID"); - s.setSubjectNounPlural("Subjects"); - s.setSubjectNounSingular("Subject"); s.setSecurityType(SecurityType.BASIC_WRITE); - s.setStartDate(new Date(DateUtil.parseDateTime(c, "1 Jan 2000"))); - _testStudy = StudyManager.getInstance().createStudy(_context.getUser(), s); + s.setStartDate(new Date(DateUtil.parseDateTime("1 Jan 2000"))); + _testStudy = StudyManager.getInstance().createTestStudy(_context.getUser(), s); MvUtil.assignMvIndicators(c, new String[]{"X", "Y", "Z"}, diff --git a/study/src/org/labkey/study/model/StudyManager.java b/study/src/org/labkey/study/model/StudyManager.java index 2b38596364c..246a7a9e733 100644 --- a/study/src/org/labkey/study/model/StudyManager.java +++ b/study/src/org/labkey/study/model/StudyManager.java @@ -570,6 +570,19 @@ public Set getAllStudies(@NotNull Container root, @NotNull return Collections.unmodifiableSet(result); } + /** Helper to populate a test study with reasonable defaults */ + public StudyImpl createTestStudy(User user, StudyImpl study) + { + if (StringUtils.isBlank(study.getSubjectNounSingular())) + study.setSubjectNounSingular("Subject"); + if (StringUtils.isBlank(study.getSubjectNounPlural())) + study.setSubjectNounPlural("Subjects"); + if (StringUtils.isBlank(study.getSubjectColumnName())) + study.setSubjectColumnName("SubjectID"); + + return createStudy(user, study); + } + public StudyImpl createStudy(User user, StudyImpl study) { Container container = study.getContainer(); diff --git a/study/src/org/labkey/study/query/DatasetUpdateService.java b/study/src/org/labkey/study/query/DatasetUpdateService.java index 496212b30f0..50b2a6056d9 100644 --- a/study/src/org/labkey/study/query/DatasetUpdateService.java +++ b/study/src/org/labkey/study/query/DatasetUpdateService.java @@ -1113,12 +1113,9 @@ public void createStudy() MvUtil.assignMvIndicators(c, new String[] {"NA","QA"}, new String[] {"NA","QA"}); StudyImpl s = new StudyImpl(c, "Junit Study"); s.setTimepointType(TimepointType.VISIT); - s.setStartDate(new Date(DateUtil.parseDateTime(c, "2014-01-01"))); - s.setSubjectColumnName(SUBJECT_COLUMN_NAME); - s.setSubjectNounPlural("Subjects"); - s.setSubjectNounSingular("Subject"); + s.setStartDate(new Date(DateUtil.parseDateTime("2014-01-01"))); s.setSecurityType(SecurityType.BASIC_WRITE); - _junitStudy = StudyManager.getInstance().createStudy(_context.getUser(), s); + _junitStudy = StudyManager.getInstance().createTestStudy(_context.getUser(), s); _user = _context.getUser(); _container = _junitStudy.getContainer(); } diff --git a/survey/src/org/labkey/survey/SurveyController.java b/survey/src/org/labkey/survey/SurveyController.java index 3f78d0406ea..121d1a28ecf 100644 --- a/survey/src/org/labkey/survey/SurveyController.java +++ b/survey/src/org/labkey/survey/SurveyController.java @@ -70,6 +70,7 @@ import org.labkey.api.view.ActionURL; import org.labkey.api.view.JspView; import org.labkey.api.view.NavTree; +import org.labkey.api.view.NotFoundException; import org.springframework.validation.BindException; import org.springframework.validation.Errors; import org.springframework.web.servlet.ModelAndView; @@ -420,12 +421,19 @@ private SurveyDesign getSurveyDesign(SurveyDesignForm form) { SurveyDesign survey = new SurveyDesign(); if (form.getRowId() != 0) + { survey = SurveyManager.get().getSurveyDesign(getContainer(), getUser(), form.getRowId()); + // getSurveyDesign is container-scoped; null here means the rowId doesn't belong to this folder + if (survey == null) + throw new NotFoundException("No survey design found for rowId " + form.getRowId() + " in this folder"); + } else if (form.getDesignId() != null) { if (NumberUtils.isDigits(form.getDesignId())) { survey = SurveyManager.get().getSurveyDesign(getContainer(), getUser(), NumberUtils.toInt(form.getDesignId())); + if (survey == null) + throw new NotFoundException("No survey design found for designId " + form.getDesignId() + " in this folder"); } else { @@ -455,7 +463,12 @@ private Survey getSurvey(SurveyForm form) { Survey survey = new Survey(); if (form.getRowId() != null) + { survey = SurveyManager.get().getSurvey(getContainer(), getUser(), form.getRowId()); + // getSurvey is container-scoped; null here means the rowId doesn't belong to this folder + if (survey == null) + throw new NotFoundException("No survey found for rowId " + form.getRowId() + " in this folder"); + } if (survey != null) { diff --git a/survey/src/org/labkey/survey/SurveyManager.java b/survey/src/org/labkey/survey/SurveyManager.java index 2d35c13b839..66be29ef8b0 100644 --- a/survey/src/org/labkey/survey/SurveyManager.java +++ b/survey/src/org/labkey/survey/SurveyManager.java @@ -24,6 +24,7 @@ import org.json.JSONException; import org.json.JSONObject; import org.junit.Assert; +import org.junit.Before; import org.junit.Test; import org.labkey.api.action.NullSafeBindException; import org.labkey.api.collections.CaseInsensitiveHashMap; @@ -61,6 +62,7 @@ import org.labkey.api.query.UserSchema; import org.labkey.api.resource.Resource; import org.labkey.api.security.User; +import org.labkey.api.security.permissions.AbstractContainerScopingTest; import org.labkey.api.survey.model.Survey; import org.labkey.api.survey.model.SurveyDesign; import org.labkey.api.survey.model.SurveyListener; @@ -264,9 +266,13 @@ public Survey saveSurvey(Container container, User user, Survey survey) } } + @Nullable public SurveyDesign getSurveyDesign(Container container, User user, int surveyId) { - return new TableSelector(SurveySchema.getInstance().getSurveyDesignsTable(), new SimpleFilter(FieldKey.fromParts("rowId"), surveyId), null).getObject(SurveyDesign.class); + // Scope by container + SimpleFilter filter = new SimpleFilter(FieldKey.fromParts("rowId"), surveyId); + filter.addCondition(FieldKey.fromParts("Container"), container); + return new TableSelector(SurveySchema.getInstance().getSurveyDesignsTable(), filter, null).getObject(SurveyDesign.class); } /** @@ -313,8 +319,10 @@ public SurveyDesign[] getSurveyDesigns(SimpleFilter filter) public Survey getSurvey(Container container, User user, int rowId) { + // Scope by container so a global rowId can't read/modify a survey in another folder SimpleFilter filter = new SimpleFilter(); filter.addCondition(FieldKey.fromParts("rowId"), rowId); + filter.addCondition(FieldKey.fromParts("Container"), container); return new TableSelector(SurveySchema.getInstance().getSurveysTable(), filter, null).getObject(Survey.class); } @@ -381,7 +389,7 @@ public void deleteSurveyDesign(Container c, User user, int surveyDesignId, boole deleteSurvey(c, user, survey.getRowId()); } SQLFragment deleteSurveyDesignsSql = new SQLFragment("DELETE FROM "); - deleteSurveyDesignsSql.append(s.getSurveyDesignsTable()).append(" WHERE RowId = ?").add(surveyDesignId); + deleteSurveyDesignsSql.append(s.getSurveyDesignsTable()).append(" WHERE RowId = ? AND Container = ?").add(surveyDesignId).add(c); executor.execute(deleteSurveyDesignsSql); transaction.commit(); @@ -390,8 +398,10 @@ public void deleteSurveyDesign(Container c, User user, int surveyDesignId, boole public Survey[] getSurveys(Container c, User user, int surveyDesignId) { + // Scope by container so the delete cascade and lookups can't reach surveys in another folder SimpleFilter filter = new SimpleFilter(); filter.addCondition(FieldKey.fromParts("surveyDesignId"), surveyDesignId); + filter.addCondition(FieldKey.fromParts("Container"), c); return new TableSelector(SurveySchema.getInstance().getSurveysTable(), filter, null).getArray(Survey.class); } @@ -782,4 +792,69 @@ public void testGetMetaDataForColumn() assertTrue("Unexpected property value", trimmedMap.get("required").equals(true)); } } + + public static class ContainerScopingTestCase extends AbstractContainerScopingTest + { + private Container _projectA; + private Container _projectB; + private User _user; + + @Before + public void setUp() + { + _user = getAdmin(); + _projectA = createContainer("A"); + _projectB = createContainer("B"); + } + + @Test + public void testSurveyDesignContainerScoping() + { + SurveyManager sm = SurveyManager.get(); + + SurveyDesign design = new SurveyDesign(); + design.setLabel("Scoping test design"); + design = sm.saveSurveyDesign(_projectA, _user, design); + int designId = design.getRowId(); + + // Same-container lookup succeeds; cross-container lookup must return null + assertNotNull("Design should be visible from its own container", sm.getSurveyDesign(_projectA, _user, designId)); + assertNull("Design must NOT be visible from another container", sm.getSurveyDesign(_projectB, _user, designId)); + + // A delete issued from the wrong container must not remove the design + sm.deleteSurveyDesign(_projectB, _user, designId, true); + assertNotNull("Cross-container delete must be a no-op", sm.getSurveyDesign(_projectA, _user, designId)); + + // A delete from the correct container removes it + sm.deleteSurveyDesign(_projectA, _user, designId, true); + assertNull("Same-container delete should remove the design", sm.getSurveyDesign(_projectA, _user, designId)); + } + + @Test + public void testSurveyContainerScoping() + { + SurveyManager sm = SurveyManager.get(); + + SurveyDesign design = new SurveyDesign(); + design.setLabel("Scoping test design for survey"); + design = sm.saveSurveyDesign(_projectA, _user, design); + + Survey survey = new Survey(); + survey.setLabel("Scoping test survey"); + survey.setSurveyDesignId(design.getRowId()); + survey = sm.saveSurvey(_projectA, _user, survey); + int surveyRowId = survey.getRowId(); + + assertNotNull("Survey should be visible from its own container", sm.getSurvey(_projectA, _user, surveyRowId)); + assertNull("Survey must NOT be visible from another container", sm.getSurvey(_projectB, _user, surveyRowId)); + + // A delete issued from the wrong container must not remove the survey + sm.deleteSurvey(_projectB, _user, surveyRowId); + assertNotNull("Cross-container delete must be a no-op", sm.getSurvey(_projectA, _user, surveyRowId)); + + // A delete from the correct container removes it + sm.deleteSurvey(_projectA, _user, surveyRowId); + assertNull("Same-container delete should remove the survey", sm.getSurvey(_projectA, _user, surveyRowId)); + } + } } diff --git a/survey/src/org/labkey/survey/SurveyModule.java b/survey/src/org/labkey/survey/SurveyModule.java index a55dd4d8cf9..96ef0d24dc4 100644 --- a/survey/src/org/labkey/survey/SurveyModule.java +++ b/survey/src/org/labkey/survey/SurveyModule.java @@ -270,4 +270,12 @@ public Set getUnitTests() SurveyManager.TestCase.class ); } + + @Override + public @NotNull Set getIntegrationTests() + { + return Set.of( + SurveyManager.ContainerScopingTestCase.class + ); + } } diff --git a/wiki/src/org/labkey/wiki/WikiController.java b/wiki/src/org/labkey/wiki/WikiController.java index 850acac6852..164f1a37ce4 100644 --- a/wiki/src/org/labkey/wiki/WikiController.java +++ b/wiki/src/org/labkey/wiki/WikiController.java @@ -17,6 +17,7 @@ package org.labkey.wiki; import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServletResponse; import org.apache.commons.collections4.MultiValuedMap; import org.apache.commons.collections4.multimap.ArrayListValuedHashMap; import org.apache.commons.lang3.StringUtils; @@ -24,6 +25,7 @@ import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import org.junit.Test; import org.labkey.api.action.ApiResponse; import org.labkey.api.action.ApiSimpleResponse; import org.labkey.api.action.ConfirmAction; @@ -56,8 +58,10 @@ import org.labkey.api.security.User; import org.labkey.api.security.UserManager; import org.labkey.api.security.WikiTermsOfUseProvider; +import org.labkey.api.security.permissions.AbstractContainerScopingTest; import org.labkey.api.security.permissions.AdminPermission; import org.labkey.api.security.permissions.ReadPermission; +import org.labkey.api.security.roles.FolderAdminRole; import org.labkey.api.settings.AdminConsole; import org.labkey.api.settings.AppProps; import org.labkey.api.util.GUID; @@ -989,6 +993,9 @@ public boolean handlePost(CopyWikiForm form, BindException errors) throws Except if (errors.hasErrors()) return false; + if (cSrc == null || !cSrc.hasPermission(getUser(), AdminPermission.class)) + throw new NotFoundException("No source container found, or you do not have permission to copy from it."); + if (cSrc.equals(_cDest)) { throw new NotFoundException("Cannot copy a wiki into the folder it is being copied from."); @@ -2922,4 +2929,32 @@ private WikiManager getWikiManager() { return WikiManager.get(); } + + public static class CopyWikiContainerScopingTestCase extends AbstractContainerScopingTest + { + @Test + public void testCopyWikiRequiresSourceAdmin() throws Exception + { + Container dest = createContainer("Dest"); + Container source = createContainer("Source"); + + // A user who is a folder admin in the destination only (no rights in the source) + User destAdminOnly = createUserInRole(dest, FolderAdminRole.class); + + // A wiki page that lives in the source folder + WikiManager.get().insertWiki(getAdmin(), source, "secretPage", "secret body", WikiRendererType.HTML, "Secret Page"); + + ActionURL url = new ActionURL(CopyWikiAction.class, dest) + .addParameter("sourceContainer", source.getPath()) + .addParameter("destContainer", dest.getPath()); + assertStatus(HttpServletResponse.SC_NOT_FOUND, post(url, destAdminOnly)); + assertTrue("No pages should have been copied into the destination", WikiSelectManager.getPageNames(dest).isEmpty()); + + // Positive control: the same copy by a user who is admin on BOTH folders is accepted (redirect to the + // destination) and actually copies the page, proving the guard rejects only the cross-container case + // rather than every copy. + assertStatus(HttpServletResponse.SC_FOUND, post(url, getAdmin())); + assertFalse("Admin copy from a readable source should have copied the wiki page", WikiSelectManager.getPageNames(dest).isEmpty()); + } + } } diff --git a/wiki/src/org/labkey/wiki/WikiModule.java b/wiki/src/org/labkey/wiki/WikiModule.java index 096fd62a081..244815c51ea 100644 --- a/wiki/src/org/labkey/wiki/WikiModule.java +++ b/wiki/src/org/labkey/wiki/WikiModule.java @@ -199,7 +199,8 @@ private void loadWikiContent(@Nullable Container c, User user, String name, Stri public Set getIntegrationTests() { return Set.of( - WikiManager.TestCase.class + WikiManager.TestCase.class, + WikiController.CopyWikiContainerScopingTestCase.class ); }