From 714a471f1e491e3ce968653d5c92cd4d52319356 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Mon, 8 Jun 2026 21:35:16 -0700 Subject: [PATCH 1/6] Improve scoping checks --- .../AbstractContainerScopingTest.java | 161 ++++++++++++++++++ core/src/org/labkey/core/CoreModule.java | 2 + .../labkey/core/admin/AdminController.java | 39 ++++- .../org/labkey/core/webdav/DavController.java | 143 +++++++++++++++- .../labkey/experiment/CustomProperties.java | 64 ++++++- .../labkey/experiment/CustomProperties.jsp | 2 +- .../experiment/CustomPropertiesView.java | 32 +--- .../experiment/CustomPropertyRenderer.java | 35 ---- .../DefaultCustomPropertyRenderer.java | 92 ---------- .../labkey/experiment/ExperimentModule.java | 1 + ...ternalDocsLabelCustomPropertyRenderer.java | 49 ------ ...ExternalDocsURLCustomPropertyRenderer.java | 83 --------- .../org/labkey/experiment/XarExporter.java | 20 +-- .../src/org/labkey/experiment/XarReader.java | 21 --- .../controllers/exp/ExperimentController.java | 55 ++++++ .../org/labkey/issue/IssuesController.java | 95 ++++++++++- issues/src/org/labkey/issue/IssuesModule.java | 5 +- .../org/labkey/pipeline/PipelineModule.java | 1 + .../pipeline/status/StatusController.java | 44 ++++- study/src/org/labkey/study/StudyModule.java | 3 +- .../controllers/CreateChildStudyAction.java | 100 +++++++++-- .../study/model/DatasetImportTestCase.jsp | 15 +- .../study/model/DatasetLsidImportHelper.java | 7 +- .../src/org/labkey/study/model/StudyImpl.java | 8 +- .../org/labkey/study/model/StudyManager.java | 13 ++ .../study/query/DatasetUpdateService.java | 7 +- wiki/src/org/labkey/wiki/WikiController.java | 35 ++++ wiki/src/org/labkey/wiki/WikiModule.java | 3 +- 28 files changed, 748 insertions(+), 387 deletions(-) create mode 100644 api/src/org/labkey/api/security/permissions/AbstractContainerScopingTest.java delete mode 100644 experiment/src/org/labkey/experiment/CustomPropertyRenderer.java delete mode 100644 experiment/src/org/labkey/experiment/DefaultCustomPropertyRenderer.java delete mode 100644 experiment/src/org/labkey/experiment/ExternalDocsLabelCustomPropertyRenderer.java delete mode 100644 experiment/src/org/labkey/experiment/ExternalDocsURLCustomPropertyRenderer.java 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/CustomProperties.java b/experiment/src/org/labkey/experiment/CustomProperties.java index 1fb6f08e96b..38091f99a5a 100644 --- a/experiment/src/org/labkey/experiment/CustomProperties.java +++ b/experiment/src/org/labkey/experiment/CustomProperties.java @@ -17,18 +17,28 @@ import org.labkey.api.data.Container; import org.labkey.api.exp.ObjectProperty; +import org.labkey.api.exp.OntologyManager; +import org.labkey.api.exp.PropertyDescriptor; +import org.labkey.api.exp.PropertyType; +import org.labkey.api.files.FileContentService; +import org.labkey.api.study.assay.FileLinkDisplayColumn; +import org.labkey.api.util.DateUtil; +import org.labkey.api.util.FileUtil; +import org.labkey.api.util.Formats; +import org.labkey.api.util.PageFlowUtil; +import java.io.File; import java.util.ArrayList; import java.util.Collection; +import java.util.Date; import java.util.List; -import java.util.Map; /** * Created by adam on 9/4/2016. */ public class CustomProperties { - public static void iterate(Container c, Collection properties, Map rendererMap, PropertyHandler handler) + public static void iterate(Container c, Collection properties, PropertyHandler handler) { List> stack = new ArrayList<>(); stack.add(new ArrayList<>(properties)); @@ -49,11 +59,7 @@ public static void iterate(Container c, Collection properties, M else { ObjectProperty value = values.get(currentIndex); - CustomPropertyRenderer renderer = rendererMap.get(value.getPropertyURI()); - if (renderer.shouldRender(value, values)) - { - handler.handle(stack.size() - 1, renderer.getDescription(value, values), renderer.getValue(value, values, c)); - } + handler.handle(stack.size() - 1, getDescription(value), getValue(value, c)); if (!value.retrieveChildProperties().isEmpty()) { stack.add(new ArrayList<>(value.retrieveChildProperties().values())); @@ -63,6 +69,50 @@ public static void iterate(Container c, Collection properties, M } } + private static String getValue(ObjectProperty prop, Container c) + { + Object o = prop.value(); + if (o == null) + { + return ""; + } + if (prop.getPropertyType() == PropertyType.FILE_LINK) + { + File f = FileUtil.getAbsoluteCaseSensitiveFile(new File(o.toString())); + o = FileLinkDisplayColumn.relativize(f, FileContentService.get().getFileRoot(c, FileContentService.ContentType.files)); + if (o == null) + { + o = FileLinkDisplayColumn.relativize(f, FileContentService.get().getFileRoot(c, FileContentService.ContentType.pipeline)); + } + if (o == null) + { + o = f.toString(); + } + } + + String value; + + // TODO: Should have a standard method that does this + if (o instanceof Date d) + value = DateUtil.formatDateInfer(c, d); + else if (o instanceof Number n) + value = Formats.formatNumber(c, n); + else + value = o.toString(); + + return PageFlowUtil.filter(value); + } + + private static String getDescription(ObjectProperty prop) + { + PropertyDescriptor pd = OntologyManager.getPropertyDescriptor(prop.getPropertyURI(), prop.getContainer()); + String name = prop.getName(); + if (pd != null) + name = pd.getLabel() != null ? pd.getLabel() : pd.getName(); + return PageFlowUtil.filter(name); + } + + public interface PropertyHandler { void handle(int indent, String description, String value); diff --git a/experiment/src/org/labkey/experiment/CustomProperties.jsp b/experiment/src/org/labkey/experiment/CustomProperties.jsp index 779bb3d4b34..74632d8a8db 100644 --- a/experiment/src/org/labkey/experiment/CustomProperties.jsp +++ b/experiment/src/org/labkey/experiment/CustomProperties.jsp @@ -32,7 +32,7 @@ <% final JspWriter fout = out; - CustomProperties.iterate(getContainer(), form.getCustomProperties().values(), form.getRenderers(), (indent, description, value) -> + CustomProperties.iterate(getContainer(), form.getCustomProperties().values(), (indent, description, value) -> { try { diff --git a/experiment/src/org/labkey/experiment/CustomPropertiesView.java b/experiment/src/org/labkey/experiment/CustomPropertiesView.java index 8567da3900b..bd883c952f3 100644 --- a/experiment/src/org/labkey/experiment/CustomPropertiesView.java +++ b/experiment/src/org/labkey/experiment/CustomPropertiesView.java @@ -38,11 +38,9 @@ import org.labkey.experiment.api.ExpMaterialImpl; import org.labkey.experiment.api.ExpSampleTypeImpl; -import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Objects; import java.util.Set; import java.util.TreeMap; @@ -54,33 +52,14 @@ */ public class CustomPropertiesView extends JspView { - private static final CustomPropertyRenderer DEFAULT_RENDERER = new DefaultCustomPropertyRenderer(); - private static final Map _renderers = new HashMap<>() - { - @Override - public CustomPropertyRenderer get(Object key) - { - CustomPropertyRenderer result = super.get(key); - return Objects.requireNonNullElse(result, DEFAULT_RENDERER); - } - }; - - static - { - _renderers.put(ExternalDocsURLCustomPropertyRenderer.URI, new ExternalDocsURLCustomPropertyRenderer()); - _renderers.put(ExternalDocsLabelCustomPropertyRenderer.URI, new ExternalDocsLabelCustomPropertyRenderer()); - } - public static class CustomPropertiesBean { private final Map _customProperties; - private final Map _renderers; private final List> _attachments; - public CustomPropertiesBean(Map customProperties, Map renderers, List> attachments) + public CustomPropertiesBean(Map customProperties, List> attachments) { _customProperties = customProperties; - _renderers = renderers; _attachments = attachments; } @@ -89,11 +68,6 @@ public Map getCustomProperties() return _customProperties; } - public Map getRenderers() - { - return _renderers; - } - public List> getAttachments() { return _attachments; @@ -120,7 +94,7 @@ public CustomPropertiesView(String parentLSID, Container c, List siblingProperties); - - String getDescription(ObjectProperty prop, List siblingProperties); - - String getValue(ObjectProperty prop, List siblingProperties, Container c); -} diff --git a/experiment/src/org/labkey/experiment/DefaultCustomPropertyRenderer.java b/experiment/src/org/labkey/experiment/DefaultCustomPropertyRenderer.java deleted file mode 100644 index 0186d9b1e72..00000000000 --- a/experiment/src/org/labkey/experiment/DefaultCustomPropertyRenderer.java +++ /dev/null @@ -1,92 +0,0 @@ -/* - * Copyright (c) 2008-2019 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.experiment; - -import org.labkey.api.data.Container; -import org.labkey.api.exp.ObjectProperty; -import org.labkey.api.exp.OntologyManager; -import org.labkey.api.exp.PropertyDescriptor; -import org.labkey.api.exp.PropertyType; -import org.labkey.api.files.FileContentService; -import org.labkey.api.study.assay.FileLinkDisplayColumn; -import org.labkey.api.util.DateUtil; -import org.labkey.api.util.FileUtil; -import org.labkey.api.util.Formats; -import org.labkey.api.util.PageFlowUtil; - -import java.io.File; -import java.util.Date; -import java.util.List; - -/** - * Responsible for showing custom field values (like assay run properties or sample type columns) in experiment module detail pages. - * User: jeckels - * Date: Jan 23, 2006 - */ -public class DefaultCustomPropertyRenderer implements CustomPropertyRenderer -{ - @Override - public String getValue(ObjectProperty prop, List siblingProperties, Container c) - { - Object o = prop.value(); - if (o == null) - { - return ""; - } - if (prop.getPropertyType() == PropertyType.FILE_LINK) - { - File f = FileUtil.getAbsoluteCaseSensitiveFile(new File(o.toString())); - o = FileLinkDisplayColumn.relativize(f, FileContentService.get().getFileRoot(c, FileContentService.ContentType.files)); - if (o == null) - { - o = FileLinkDisplayColumn.relativize(f, FileContentService.get().getFileRoot(c, FileContentService.ContentType.pipeline)); - } - if (o == null) - { - o = f.toString(); - } - } - - String value; - - // TODO: Should have a standard method that does this - if (o instanceof Date) - value = DateUtil.formatDateInfer(c, (Date)o); - else if (o instanceof Number) - value = Formats.formatNumber(c, (Number) o); - else - value = o.toString(); - - return PageFlowUtil.filter(value); - } - - @Override - public boolean shouldRender(ObjectProperty prop, List siblingProperties) - { - return true; - } - - @Override - public String getDescription(ObjectProperty prop, List siblingProperties) - { - PropertyDescriptor pd = OntologyManager.getPropertyDescriptor(prop.getPropertyURI(), prop.getContainer()); - String name = prop.getName(); - if (pd != null) - name = pd.getLabel() != null ? pd.getLabel() : pd.getName(); - return PageFlowUtil.filter(name); - } -} 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/ExternalDocsLabelCustomPropertyRenderer.java b/experiment/src/org/labkey/experiment/ExternalDocsLabelCustomPropertyRenderer.java deleted file mode 100644 index 854ad65fd38..00000000000 --- a/experiment/src/org/labkey/experiment/ExternalDocsLabelCustomPropertyRenderer.java +++ /dev/null @@ -1,49 +0,0 @@ -/* - * Copyright (c) 2008-2019 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.experiment; - -import org.labkey.api.data.Container; -import org.labkey.api.exp.ObjectProperty; - -import java.util.List; - -/** - * User: jeckels - * Date: Jan 23, 2006 - */ -public class ExternalDocsLabelCustomPropertyRenderer implements CustomPropertyRenderer -{ - public static final String URI = "terms.fhcrc.org#ExternalDocumentation.label"; - - @Override - public String getDescription(ObjectProperty prop, List siblingProperties) - { - throw new UnsupportedOperationException(); - } - - @Override - public String getValue(ObjectProperty prop, List siblingProperties, Container c) - { - throw new UnsupportedOperationException(); - } - - @Override - public boolean shouldRender(ObjectProperty prop, List siblingProperties) - { - return false; - } -} diff --git a/experiment/src/org/labkey/experiment/ExternalDocsURLCustomPropertyRenderer.java b/experiment/src/org/labkey/experiment/ExternalDocsURLCustomPropertyRenderer.java deleted file mode 100644 index e6544c8755a..00000000000 --- a/experiment/src/org/labkey/experiment/ExternalDocsURLCustomPropertyRenderer.java +++ /dev/null @@ -1,83 +0,0 @@ -/* - * Copyright (c) 2008-2019 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.experiment; - -import org.labkey.api.data.Container; -import org.labkey.api.exp.ObjectProperty; -import org.labkey.api.util.FileUtil; -import org.labkey.api.util.PageFlowUtil; -import org.labkey.api.view.ActionURL; -import org.labkey.experiment.controllers.exp.ExperimentController; - -import java.net.MalformedURLException; -import java.net.URL; -import java.util.List; - -/** - * User: jeckels - * Date: Jan 23, 2006 - */ -public class ExternalDocsURLCustomPropertyRenderer implements CustomPropertyRenderer -{ - public static final String URI = "terms.fhcrc.org#ExternalDocumentation.href"; - - @Override - public boolean shouldRender(ObjectProperty prop, List siblingProperties) - { - return true; - } - - @Override - public String getDescription(ObjectProperty prop, List siblingProperties) - { - return "External documentation"; - } - - @Override - public String getValue(ObjectProperty prop, List siblingProperties, Container c) - { - String label = null; - for (ObjectProperty p : siblingProperties) - { - if (ExternalDocsLabelCustomPropertyRenderer.URI.equals(p.getPropertyURI())) - { - label = p.getStringValue(); - } - } - if (label == null) - { - label = prop.getStringValue(); - } - String link = prop.getStringValue(); - try - { - URL url = new URL(prop.getStringValue()); - if (url.getProtocol().equals(FileUtil.FILE_SCHEME)) - { - ActionURL h = new ActionURL(ExperimentController.ShowExternalDocsAction.class, c); - h.addParameter("objectURI", prop.getObjectURI()); - h.addParameter("propertyURI", prop.getPropertyURI()); - link = h.toString(); - } - } - catch (MalformedURLException e) - { - // That's OK, we won't try to do anything with the link - } - return "" + PageFlowUtil.filter(label) + ""; - } -} diff --git a/experiment/src/org/labkey/experiment/XarExporter.java b/experiment/src/org/labkey/experiment/XarExporter.java index aad4fa287b3..adce9dca0f4 100644 --- a/experiment/src/org/labkey/experiment/XarExporter.java +++ b/experiment/src/org/labkey/experiment/XarExporter.java @@ -1387,27 +1387,9 @@ private PropertyCollectionType getProperties(Map propert case MULTI_LINE: case XML_TEXT: simpleValue.setValueType(SimpleTypeNames.STRING); - if (ExternalDocsURLCustomPropertyRenderer.URI.equals(value.getPropertyURI())) - { - String link = value.getStringValue(); - try - { - URI uri = new URI(link); - if (FileUtil.FILE_SCHEME.equals(uri.getScheme()) || FileUtil.hasCloudScheme(uri)) - { - Path path = FileUtil.getPath(parentContainer, uri); - if (Files.exists(path)) - { - link = _urlRewriter.rewriteURL(path, null, null, null, _user, _fileRootPath); - } - } - } - catch (URISyntaxException ignored) {} - simpleValue.setStringValue(link); - } // This property stores rowIds of assay designs; we need to translate them to LSIDs for export // TODO perhaps this property should hold protocol strings instead of rowIds - else if (value.getPropertyURI().endsWith(":WorkflowTask#AssayTypes")) + if (value.getPropertyURI().endsWith(":WorkflowTask#AssayTypes")) { String assayIdsString = value.getStringValue(); if (!StringUtils.isEmpty(assayIdsString)) diff --git a/experiment/src/org/labkey/experiment/XarReader.java b/experiment/src/org/labkey/experiment/XarReader.java index 341ea7a210e..c771927a732 100644 --- a/experiment/src/org/labkey/experiment/XarReader.java +++ b/experiment/src/org/labkey/experiment/XarReader.java @@ -1988,27 +1988,6 @@ else if (propType == PropertyType.FILE_LINK && !StringUtils.isEmpty(value) && va ObjectProperty objectProp = new ObjectProperty(parentLSID, getContainer(), ontologyEntryURI, value, propType, simpleProp.getName()); setPropertyId(objectProp); - if (ExternalDocsURLCustomPropertyRenderer.URI.equals(trimString(objectProp.getPropertyURI()))) - { - String relativePath = trimString(objectProp.getStringValue()); - if (relativePath != null) - { - try - { - String fullPath = _xarSource.getCanonicalDataFileURL(relativePath); - Path file = FileUtil.stringToPath(getContainer(), fullPath); - if (Files.exists(file)) - { - objectProp.setStringValue(fullPath); - } - } - catch (XarFormatException ignored) - { - // That's OK, don't treat the value as a relative path to the file - } - } - } - if (checkForDuplicates && existingProps.containsKey(ontologyEntryURI)) { ObjectProperty existingProp = existingProps.get(ontologyEntryURI); diff --git a/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java b/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java index 67c15cdc4cf..682da96e31b 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().generateLSID(folderB, ExpData.class, "exp1-scope-test"); + 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..c858247f477 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,10 +101,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.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.RoleManager; import org.labkey.api.util.ButtonBuilder; @@ -113,6 +117,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 +1638,38 @@ 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"); + + 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 +2374,63 @@ public void setIssueId(int issueId) this.issueId = issueId; } } + + public static class MoveActionContainerScopingTestCase extends AbstractContainerScopingTest + { + @Test + public void testMoveRequiresSourceAdmin() throws Exception + { + User admin = getAdmin(); + Container dest = createContainer("Dest"); // the limited user is admin here + Container source = createContainer("Source"); // the issue lives here; the limited user has no rights + + 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 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(MoveAction.class, dest) + .addParameter("issueIds", String.valueOf(issueId)) + .addParameter("targetContainerId", dest.getId()); + assertStatus(HttpServletResponse.SC_NOT_FOUND, post(url, destAdminOnly)); + + // The issue must remain in its source container + IssueObject reloaded = IssueManager.getIssue(source, admin, issueId); + assertNotNull("Issue should still exist in its source folder", reloaded); + + // Positive control is in IssuesTest.moveIssueTest() + } + + 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/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/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/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 ); } From 4134cd1539ccce2d9c6060cc38da03e5e56581b2 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Mon, 8 Jun 2026 21:53:46 -0700 Subject: [PATCH 2/6] Survey and mothership --- .../mothership/MothershipController.java | 58 ++++++++++++++ .../labkey/mothership/MothershipModule.java | 6 ++ .../org/labkey/survey/SurveyController.java | 13 +++ .../src/org/labkey/survey/SurveyManager.java | 79 ++++++++++++++++++- .../src/org/labkey/survey/SurveyModule.java | 8 ++ 5 files changed, 162 insertions(+), 2 deletions(-) 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/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 + ); + } } From 873af9b16b1537d86232a220d1b0332d20cb9485 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Mon, 8 Jun 2026 22:13:06 -0700 Subject: [PATCH 3/6] Specimen --- .../org/labkey/specimen/SpecimenModule.java | 5 +- .../actions/ShowGroupMembersAction.java | 4 + .../specimen/actions/SpecimenController.java | 93 ++++++++++++++++++- .../DefaultRequirementProvider.java | 12 ++- .../SpecimenRequestRequirementProvider.java | 48 ++++++++++ 5 files changed, 158 insertions(+), 4 deletions(-) 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)); + } + } } From 9c08dbea7f3a821181c1116addf35025afb77b62 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Tue, 9 Jun 2026 07:35:00 -0700 Subject: [PATCH 4/6] Restore with encoding --- .../labkey/experiment/CustomProperties.java | 64 ++----------- .../labkey/experiment/CustomProperties.jsp | 2 +- .../experiment/CustomPropertiesView.java | 32 ++++++- .../experiment/CustomPropertyRenderer.java | 35 +++++++ .../DefaultCustomPropertyRenderer.java | 92 +++++++++++++++++++ ...ternalDocsLabelCustomPropertyRenderer.java | 49 ++++++++++ ...ExternalDocsURLCustomPropertyRenderer.java | 83 +++++++++++++++++ .../org/labkey/experiment/XarExporter.java | 20 +++- .../src/org/labkey/experiment/XarReader.java | 21 +++++ 9 files changed, 336 insertions(+), 62 deletions(-) create mode 100644 experiment/src/org/labkey/experiment/CustomPropertyRenderer.java create mode 100644 experiment/src/org/labkey/experiment/DefaultCustomPropertyRenderer.java create mode 100644 experiment/src/org/labkey/experiment/ExternalDocsLabelCustomPropertyRenderer.java create mode 100644 experiment/src/org/labkey/experiment/ExternalDocsURLCustomPropertyRenderer.java diff --git a/experiment/src/org/labkey/experiment/CustomProperties.java b/experiment/src/org/labkey/experiment/CustomProperties.java index 38091f99a5a..1fb6f08e96b 100644 --- a/experiment/src/org/labkey/experiment/CustomProperties.java +++ b/experiment/src/org/labkey/experiment/CustomProperties.java @@ -17,28 +17,18 @@ import org.labkey.api.data.Container; import org.labkey.api.exp.ObjectProperty; -import org.labkey.api.exp.OntologyManager; -import org.labkey.api.exp.PropertyDescriptor; -import org.labkey.api.exp.PropertyType; -import org.labkey.api.files.FileContentService; -import org.labkey.api.study.assay.FileLinkDisplayColumn; -import org.labkey.api.util.DateUtil; -import org.labkey.api.util.FileUtil; -import org.labkey.api.util.Formats; -import org.labkey.api.util.PageFlowUtil; -import java.io.File; import java.util.ArrayList; import java.util.Collection; -import java.util.Date; import java.util.List; +import java.util.Map; /** * Created by adam on 9/4/2016. */ public class CustomProperties { - public static void iterate(Container c, Collection properties, PropertyHandler handler) + public static void iterate(Container c, Collection properties, Map rendererMap, PropertyHandler handler) { List> stack = new ArrayList<>(); stack.add(new ArrayList<>(properties)); @@ -59,7 +49,11 @@ public static void iterate(Container c, Collection properties, P else { ObjectProperty value = values.get(currentIndex); - handler.handle(stack.size() - 1, getDescription(value), getValue(value, c)); + CustomPropertyRenderer renderer = rendererMap.get(value.getPropertyURI()); + if (renderer.shouldRender(value, values)) + { + handler.handle(stack.size() - 1, renderer.getDescription(value, values), renderer.getValue(value, values, c)); + } if (!value.retrieveChildProperties().isEmpty()) { stack.add(new ArrayList<>(value.retrieveChildProperties().values())); @@ -69,50 +63,6 @@ public static void iterate(Container c, Collection properties, P } } - private static String getValue(ObjectProperty prop, Container c) - { - Object o = prop.value(); - if (o == null) - { - return ""; - } - if (prop.getPropertyType() == PropertyType.FILE_LINK) - { - File f = FileUtil.getAbsoluteCaseSensitiveFile(new File(o.toString())); - o = FileLinkDisplayColumn.relativize(f, FileContentService.get().getFileRoot(c, FileContentService.ContentType.files)); - if (o == null) - { - o = FileLinkDisplayColumn.relativize(f, FileContentService.get().getFileRoot(c, FileContentService.ContentType.pipeline)); - } - if (o == null) - { - o = f.toString(); - } - } - - String value; - - // TODO: Should have a standard method that does this - if (o instanceof Date d) - value = DateUtil.formatDateInfer(c, d); - else if (o instanceof Number n) - value = Formats.formatNumber(c, n); - else - value = o.toString(); - - return PageFlowUtil.filter(value); - } - - private static String getDescription(ObjectProperty prop) - { - PropertyDescriptor pd = OntologyManager.getPropertyDescriptor(prop.getPropertyURI(), prop.getContainer()); - String name = prop.getName(); - if (pd != null) - name = pd.getLabel() != null ? pd.getLabel() : pd.getName(); - return PageFlowUtil.filter(name); - } - - public interface PropertyHandler { void handle(int indent, String description, String value); diff --git a/experiment/src/org/labkey/experiment/CustomProperties.jsp b/experiment/src/org/labkey/experiment/CustomProperties.jsp index 74632d8a8db..779bb3d4b34 100644 --- a/experiment/src/org/labkey/experiment/CustomProperties.jsp +++ b/experiment/src/org/labkey/experiment/CustomProperties.jsp @@ -32,7 +32,7 @@ <% final JspWriter fout = out; - CustomProperties.iterate(getContainer(), form.getCustomProperties().values(), (indent, description, value) -> + CustomProperties.iterate(getContainer(), form.getCustomProperties().values(), form.getRenderers(), (indent, description, value) -> { try { diff --git a/experiment/src/org/labkey/experiment/CustomPropertiesView.java b/experiment/src/org/labkey/experiment/CustomPropertiesView.java index bd883c952f3..8567da3900b 100644 --- a/experiment/src/org/labkey/experiment/CustomPropertiesView.java +++ b/experiment/src/org/labkey/experiment/CustomPropertiesView.java @@ -38,9 +38,11 @@ import org.labkey.experiment.api.ExpMaterialImpl; import org.labkey.experiment.api.ExpSampleTypeImpl; +import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.TreeMap; @@ -52,14 +54,33 @@ */ public class CustomPropertiesView extends JspView { + private static final CustomPropertyRenderer DEFAULT_RENDERER = new DefaultCustomPropertyRenderer(); + private static final Map _renderers = new HashMap<>() + { + @Override + public CustomPropertyRenderer get(Object key) + { + CustomPropertyRenderer result = super.get(key); + return Objects.requireNonNullElse(result, DEFAULT_RENDERER); + } + }; + + static + { + _renderers.put(ExternalDocsURLCustomPropertyRenderer.URI, new ExternalDocsURLCustomPropertyRenderer()); + _renderers.put(ExternalDocsLabelCustomPropertyRenderer.URI, new ExternalDocsLabelCustomPropertyRenderer()); + } + public static class CustomPropertiesBean { private final Map _customProperties; + private final Map _renderers; private final List> _attachments; - public CustomPropertiesBean(Map customProperties, List> attachments) + public CustomPropertiesBean(Map customProperties, Map renderers, List> attachments) { _customProperties = customProperties; + _renderers = renderers; _attachments = attachments; } @@ -68,6 +89,11 @@ public Map getCustomProperties() return _customProperties; } + public Map getRenderers() + { + return _renderers; + } + public List> getAttachments() { return _attachments; @@ -94,7 +120,7 @@ public CustomPropertiesView(String parentLSID, Container c, List siblingProperties); + + String getDescription(ObjectProperty prop, List siblingProperties); + + String getValue(ObjectProperty prop, List siblingProperties, Container c); +} diff --git a/experiment/src/org/labkey/experiment/DefaultCustomPropertyRenderer.java b/experiment/src/org/labkey/experiment/DefaultCustomPropertyRenderer.java new file mode 100644 index 00000000000..0186d9b1e72 --- /dev/null +++ b/experiment/src/org/labkey/experiment/DefaultCustomPropertyRenderer.java @@ -0,0 +1,92 @@ +/* + * Copyright (c) 2008-2019 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.experiment; + +import org.labkey.api.data.Container; +import org.labkey.api.exp.ObjectProperty; +import org.labkey.api.exp.OntologyManager; +import org.labkey.api.exp.PropertyDescriptor; +import org.labkey.api.exp.PropertyType; +import org.labkey.api.files.FileContentService; +import org.labkey.api.study.assay.FileLinkDisplayColumn; +import org.labkey.api.util.DateUtil; +import org.labkey.api.util.FileUtil; +import org.labkey.api.util.Formats; +import org.labkey.api.util.PageFlowUtil; + +import java.io.File; +import java.util.Date; +import java.util.List; + +/** + * Responsible for showing custom field values (like assay run properties or sample type columns) in experiment module detail pages. + * User: jeckels + * Date: Jan 23, 2006 + */ +public class DefaultCustomPropertyRenderer implements CustomPropertyRenderer +{ + @Override + public String getValue(ObjectProperty prop, List siblingProperties, Container c) + { + Object o = prop.value(); + if (o == null) + { + return ""; + } + if (prop.getPropertyType() == PropertyType.FILE_LINK) + { + File f = FileUtil.getAbsoluteCaseSensitiveFile(new File(o.toString())); + o = FileLinkDisplayColumn.relativize(f, FileContentService.get().getFileRoot(c, FileContentService.ContentType.files)); + if (o == null) + { + o = FileLinkDisplayColumn.relativize(f, FileContentService.get().getFileRoot(c, FileContentService.ContentType.pipeline)); + } + if (o == null) + { + o = f.toString(); + } + } + + String value; + + // TODO: Should have a standard method that does this + if (o instanceof Date) + value = DateUtil.formatDateInfer(c, (Date)o); + else if (o instanceof Number) + value = Formats.formatNumber(c, (Number) o); + else + value = o.toString(); + + return PageFlowUtil.filter(value); + } + + @Override + public boolean shouldRender(ObjectProperty prop, List siblingProperties) + { + return true; + } + + @Override + public String getDescription(ObjectProperty prop, List siblingProperties) + { + PropertyDescriptor pd = OntologyManager.getPropertyDescriptor(prop.getPropertyURI(), prop.getContainer()); + String name = prop.getName(); + if (pd != null) + name = pd.getLabel() != null ? pd.getLabel() : pd.getName(); + return PageFlowUtil.filter(name); + } +} diff --git a/experiment/src/org/labkey/experiment/ExternalDocsLabelCustomPropertyRenderer.java b/experiment/src/org/labkey/experiment/ExternalDocsLabelCustomPropertyRenderer.java new file mode 100644 index 00000000000..854ad65fd38 --- /dev/null +++ b/experiment/src/org/labkey/experiment/ExternalDocsLabelCustomPropertyRenderer.java @@ -0,0 +1,49 @@ +/* + * Copyright (c) 2008-2019 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.experiment; + +import org.labkey.api.data.Container; +import org.labkey.api.exp.ObjectProperty; + +import java.util.List; + +/** + * User: jeckels + * Date: Jan 23, 2006 + */ +public class ExternalDocsLabelCustomPropertyRenderer implements CustomPropertyRenderer +{ + public static final String URI = "terms.fhcrc.org#ExternalDocumentation.label"; + + @Override + public String getDescription(ObjectProperty prop, List siblingProperties) + { + throw new UnsupportedOperationException(); + } + + @Override + public String getValue(ObjectProperty prop, List siblingProperties, Container c) + { + throw new UnsupportedOperationException(); + } + + @Override + public boolean shouldRender(ObjectProperty prop, List siblingProperties) + { + return false; + } +} diff --git a/experiment/src/org/labkey/experiment/ExternalDocsURLCustomPropertyRenderer.java b/experiment/src/org/labkey/experiment/ExternalDocsURLCustomPropertyRenderer.java new file mode 100644 index 00000000000..604b1cf6768 --- /dev/null +++ b/experiment/src/org/labkey/experiment/ExternalDocsURLCustomPropertyRenderer.java @@ -0,0 +1,83 @@ +/* + * Copyright (c) 2008-2019 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.experiment; + +import org.labkey.api.data.Container; +import org.labkey.api.exp.ObjectProperty; +import org.labkey.api.util.FileUtil; +import org.labkey.api.util.PageFlowUtil; +import org.labkey.api.view.ActionURL; +import org.labkey.experiment.controllers.exp.ExperimentController; + +import java.net.MalformedURLException; +import java.net.URL; +import java.util.List; + +/** + * User: jeckels + * Date: Jan 23, 2006 + */ +public class ExternalDocsURLCustomPropertyRenderer implements CustomPropertyRenderer +{ + public static final String URI = "terms.fhcrc.org#ExternalDocumentation.href"; + + @Override + public boolean shouldRender(ObjectProperty prop, List siblingProperties) + { + return true; + } + + @Override + public String getDescription(ObjectProperty prop, List siblingProperties) + { + return "External documentation"; + } + + @Override + public String getValue(ObjectProperty prop, List siblingProperties, Container c) + { + String label = null; + for (ObjectProperty p : siblingProperties) + { + if (ExternalDocsLabelCustomPropertyRenderer.URI.equals(p.getPropertyURI())) + { + label = p.getStringValue(); + } + } + if (label == null) + { + label = prop.getStringValue(); + } + String link = prop.getStringValue(); + try + { + URL url = new URL(prop.getStringValue()); + if (url.getProtocol().equals(FileUtil.FILE_SCHEME)) + { + ActionURL h = new ActionURL(ExperimentController.ShowExternalDocsAction.class, c); + h.addParameter("objectURI", prop.getObjectURI()); + h.addParameter("propertyURI", prop.getPropertyURI()); + link = h.toString(); + } + } + catch (MalformedURLException e) + { + // That's OK, we won't try to do anything with the link + } + return "" + PageFlowUtil.filter(label) + ""; + } +} diff --git a/experiment/src/org/labkey/experiment/XarExporter.java b/experiment/src/org/labkey/experiment/XarExporter.java index adce9dca0f4..aad4fa287b3 100644 --- a/experiment/src/org/labkey/experiment/XarExporter.java +++ b/experiment/src/org/labkey/experiment/XarExporter.java @@ -1387,9 +1387,27 @@ private PropertyCollectionType getProperties(Map propert case MULTI_LINE: case XML_TEXT: simpleValue.setValueType(SimpleTypeNames.STRING); + if (ExternalDocsURLCustomPropertyRenderer.URI.equals(value.getPropertyURI())) + { + String link = value.getStringValue(); + try + { + URI uri = new URI(link); + if (FileUtil.FILE_SCHEME.equals(uri.getScheme()) || FileUtil.hasCloudScheme(uri)) + { + Path path = FileUtil.getPath(parentContainer, uri); + if (Files.exists(path)) + { + link = _urlRewriter.rewriteURL(path, null, null, null, _user, _fileRootPath); + } + } + } + catch (URISyntaxException ignored) {} + simpleValue.setStringValue(link); + } // This property stores rowIds of assay designs; we need to translate them to LSIDs for export // TODO perhaps this property should hold protocol strings instead of rowIds - if (value.getPropertyURI().endsWith(":WorkflowTask#AssayTypes")) + else if (value.getPropertyURI().endsWith(":WorkflowTask#AssayTypes")) { String assayIdsString = value.getStringValue(); if (!StringUtils.isEmpty(assayIdsString)) diff --git a/experiment/src/org/labkey/experiment/XarReader.java b/experiment/src/org/labkey/experiment/XarReader.java index c771927a732..341ea7a210e 100644 --- a/experiment/src/org/labkey/experiment/XarReader.java +++ b/experiment/src/org/labkey/experiment/XarReader.java @@ -1988,6 +1988,27 @@ else if (propType == PropertyType.FILE_LINK && !StringUtils.isEmpty(value) && va ObjectProperty objectProp = new ObjectProperty(parentLSID, getContainer(), ontologyEntryURI, value, propType, simpleProp.getName()); setPropertyId(objectProp); + if (ExternalDocsURLCustomPropertyRenderer.URI.equals(trimString(objectProp.getPropertyURI()))) + { + String relativePath = trimString(objectProp.getStringValue()); + if (relativePath != null) + { + try + { + String fullPath = _xarSource.getCanonicalDataFileURL(relativePath); + Path file = FileUtil.stringToPath(getContainer(), fullPath); + if (Files.exists(file)) + { + objectProp.setStringValue(fullPath); + } + } + catch (XarFormatException ignored) + { + // That's OK, don't treat the value as a relative path to the file + } + } + } + if (checkForDuplicates && existingProps.containsKey(ontologyEntryURI)) { ObjectProperty existingProp = existingProps.get(ontologyEntryURI); From b2b604461f5171ea8f7cc74b9f1149ab1cb1ca5c Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Tue, 9 Jun 2026 07:36:16 -0700 Subject: [PATCH 5/6] Admin on both sides --- .../org/labkey/issue/IssuesController.java | 45 ++++++++++++++----- 1 file changed, 35 insertions(+), 10 deletions(-) diff --git a/issues/src/org/labkey/issue/IssuesController.java b/issues/src/org/labkey/issue/IssuesController.java index c858247f477..593a006f9c8 100644 --- a/issues/src/org/labkey/issue/IssuesController.java +++ b/issues/src/org/labkey/issue/IssuesController.java @@ -108,6 +108,7 @@ 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; @@ -1646,6 +1647,9 @@ public ApiResponse execute(MoveIssueForm form, BindException errors) 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) { @@ -2379,10 +2383,31 @@ public static class MoveActionContainerScopingTestCase extends AbstractContainer { @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 dest = createContainer("Dest"); // the limited user is admin here - Container source = createContainer("Source"); // the issue lives here; the limited user has no rights + Container source = createContainer("Source"); // the issue lives here + Container dest = createContainer("Dest"); ensureIssuesEnabled(source); @@ -2397,19 +2422,19 @@ public void testMoveRequiresSourceAdmin() throws Exception IssueManager.saveIssue(admin, source, issue); int issueId = issue.getIssueId(); - // A user who is a folder admin in the destination only (no rights in the source) - User destAdminOnly = createUserInRole(dest, FolderAdminRole.class); + // 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, dest) + ActionURL url = new ActionURL(MoveAction.class, moverFolder) .addParameter("issueIds", String.valueOf(issueId)) .addParameter("targetContainerId", dest.getId()); - assertStatus(HttpServletResponse.SC_NOT_FOUND, post(url, destAdminOnly)); + assertStatus(HttpServletResponse.SC_UNAUTHORIZED, post(url, mover)); // The issue must remain in its source container - IssueObject reloaded = IssueManager.getIssue(source, admin, issueId); - assertNotNull("Issue should still exist in its source folder", reloaded); - - // Positive control is in IssuesTest.moveIssueTest() + assertNotNull("Issue should still exist in its source folder", IssueManager.getIssue(source, admin, issueId)); } private static void ensureIssuesEnabled(Container c) From 7bee8786c180e30354f9f2199b7951851c3c0b5e Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Tue, 9 Jun 2026 17:23:51 -0700 Subject: [PATCH 6/6] Fix new tests --- .../experiment/controllers/exp/ExperimentController.java | 4 ++-- issues/src/org/labkey/issue/IssuesController.java | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java b/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java index 682da96e31b..cc2ffd6445f 100644 --- a/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java +++ b/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java @@ -8357,9 +8357,9 @@ public void testDataClassAttachmentContainerScoping() throws Exception 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 + // 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().generateLSID(folderB, ExpData.class, "exp1-scope-test"); + 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)); diff --git a/issues/src/org/labkey/issue/IssuesController.java b/issues/src/org/labkey/issue/IssuesController.java index 593a006f9c8..ee2eb357a1a 100644 --- a/issues/src/org/labkey/issue/IssuesController.java +++ b/issues/src/org/labkey/issue/IssuesController.java @@ -2431,7 +2431,7 @@ private void assertCrossContainerMoveRejected(MoverScope moverScope) throws Exce ActionURL url = new ActionURL(MoveAction.class, moverFolder) .addParameter("issueIds", String.valueOf(issueId)) .addParameter("targetContainerId", dest.getId()); - assertStatus(HttpServletResponse.SC_UNAUTHORIZED, post(url, mover)); + 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));