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 5461566888c..7e36707eadb 100644 --- a/core/src/org/labkey/core/CoreModule.java +++ b/core/src/org/labkey/core/CoreModule.java @@ -1423,11 +1423,13 @@ public TabDisplayMode getTabDisplayMode() 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 4ec26471185..b6985c1603c 100644 --- a/core/src/org/labkey/core/admin/AdminController.java +++ b/core/src/org/labkey/core/admin/AdminController.java @@ -210,6 +210,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; @@ -5374,6 +5375,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 { @@ -5468,10 +5473,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, @@ -12269,4 +12280,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 6c680148437..1cbd0da8bb2 100644 --- a/core/src/org/labkey/core/webdav/DavController.java +++ b/core/src/org/labkey/core/webdav/DavController.java @@ -39,6 +39,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; @@ -73,8 +74,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; @@ -127,6 +132,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; @@ -146,7 +153,6 @@ import javax.xml.parsers.DocumentBuilder; 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; @@ -156,6 +162,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; @@ -3737,6 +3744,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); @@ -6643,4 +6653,134 @@ public void testElementName() } } + + public static class MoveActionContainerScopingTestCase extends AbstractContainerScopingTest + { + private Container _folder; + private User _mover; + + @Before + public void setUp() throws Exception + { + _folder = createContainer("Folder"); + // Author = Read + Insert but NOT Delete. This lets the caller pass the source-read and destination-create + // checks so a same-folder MOVE's outcome turns solely on the source-delete guard under test. + _mover = createUserInRole(_folder, AuthorRole.class); + } + + // Documents the precondition the MOVE fix relies on: MOVE removes the source, so it requires Delete there. + // An Author can read and create but, lacking Delete, must not be able to rename/move. + @Test + public void testRenamePermissionInvariant() + { + WebdavResource files = WebdavService.get().lookup(filesPath(_folder)); + assertNotNull("Expected a @files webdav node for the test folder", files); + assertTrue("Author should be able to read @files", files.canRead(_mover, true)); + assertTrue("Author should be able to create in @files (so the destination check passes)", files.canCreate(_mover, true)); + assertFalse("Author must NOT be able to rename/move @files (lacks Delete)", files.canRename(_mover, true)); + assertTrue("Admin should be able to rename @files", files.canRename(getAdmin(), true)); + } + + // Drives an actual MOVE through WebdavServlet/DavController. The caller can read and create in the (single) + // folder but cannot delete, so only the source-delete guard can forbid the move. Without the fix the move + // succeeds (SC_CREATED), removing a file the caller has no right to delete; with the fix it is forbidden. + @Test + public void testMoveActionRequiresSourceDelete() throws Exception + { + File dir = ensureFilesDir(_folder); + File srcFile = writeFile(dir); + assertTrue("Source file should resolve through the resolver", WebdavService.get().lookup(filesPath(_folder).append("secret.txt")).exists()); + + Path src = filesPath(_folder).append("secret.txt"); + Path dest = filesPath(_folder).append("moved.txt"); + + MockHttpServletResponse resp = doMove(_folder, src, dest, _mover); + assertEquals("A caller without Delete on the source must be forbidden from MOVE", HttpServletResponse.SC_FORBIDDEN, resp.getStatus()); + assertTrue("Source file must still exist after a forbidden move", srcFile.exists()); + + // Positive control: the same MOVE driven by an admin (who has Delete, hence rename) succeeds, proving the + // guard forbids only the under-privileged caller rather than every MOVE. + MockHttpServletResponse adminResp = doMove(_folder, src, dest, getAdmin()); + assertEquals("An admin must be allowed to MOVE", HttpServletResponse.SC_CREATED, adminResp.getStatus()); + assertFalse("Source file should no longer exist after a successful move", srcFile.exists()); + assertTrue("Destination file should exist after a successful move", new File(dir, "moved.txt").exists()); + } + + // Ensure moves by a caller who CAN delete in the source but lacks Insert in the target are forbidden + @Test + public void testMoveActionRequiresTargetCreate() throws Exception + { + Container source = createContainer("DeleteSource"); + Container target = createContainer("NoInsertTarget"); + + // Editor in the source (Read+Insert+Update+Delete) -> passes the source read and delete checks. + // Reader in the target (no Insert) -> proves read access to the target is not enough; Insert is required. + User editorInSourceOnly = createUserInRole(source, EditorRole.class); + grantRole(editorInSourceOnly, target, ReaderRole.class); + + File srcDir = ensureFilesDir(source); + File targetDir = ensureFilesDir(target); + File srcFile = writeFile(srcDir); + + Path src = filesPath(source).append("secret.txt"); + Path dest = filesPath(target).append("moved.txt"); + + MockHttpServletResponse resp = doMove(source, src, dest, editorInSourceOnly); + assertEquals("A caller without Insert on the target must be forbidden from MOVE", HttpServletResponse.SC_FORBIDDEN, resp.getStatus()); + assertTrue("Source file must still exist after a forbidden move", srcFile.exists()); + assertFalse("Destination file must not have been created", FileUtil.appendName(targetDir, "moved.txt").exists()); + + // Positive control: a caller who is also Editor (hence Insert) in the target can complete the same + // cross-container MOVE, proving the destination guard forbids only the under-privileged caller. + User editorInBoth = createUserInRole(source, EditorRole.class); + grantRole(editorInBoth, target, EditorRole.class); + MockHttpServletResponse okResp = doMove(source, src, dest, editorInBoth); + assertEquals("With Insert on the target the MOVE must succeed", HttpServletResponse.SC_CREATED, okResp.getStatus()); + assertFalse("Source file should no longer exist after a successful move", srcFile.exists()); + assertTrue("Destination file should exist after a successful move", FileUtil.appendName(targetDir, "moved.txt").exists()); + } + + private static Path filesPath(Container c) + { + return WebdavService.getPath().append(c.getParsedPath()).append(FileContentService.FILES_LINK); + } + + private static File ensureFilesDir(Container c) + { + WebdavResource filesNode = WebdavService.get().lookup(filesPath(c)); + assertNotNull("Test requires a @files node for " + c.getName(), filesNode); + File dir = filesNode.getFile(); + assertNotNull("Test requires a file root for " + c.getName(), dir); + if (!dir.exists()) + dir.mkdirs(); + return dir; + } + + private static File writeFile(File dir) throws IOException + { + File f = FileUtil.appendName(dir, "secret.txt"); + try (FileOutputStream os = new FileOutputStream(f)) + { + os.write("secret".getBytes(StandardCharsets.UTF_8)); + } + return f; + } + + private static MockHttpServletResponse doMove(Container sourceContainer, Path srcResource, Path destResource, User user) throws Exception + { + String srcWebdav = srcResource.toString(); + String destWebdav = destResource.toString(); + String servletPath = "/" + WebdavService.getServletPath(); + + HttpServletRequest base = ViewServlet.mockRequest("MOVE", new ActionURL(name, "move", sourceContainer), user, Map.of("Destination", destWebdav), null); + MockHttpServletRequest req = (MockHttpServletRequest) base; + req.setServletPath(servletPath); + req.setPathInfo(srcWebdav.substring(servletPath.length())); + req.setRequestURI(srcWebdav); + + MockHttpServletResponse resp = new MockHttpServletResponse(); + new WebdavServlet(false).service(req, resp); + return resp; + } + } } diff --git a/experiment/src/org/labkey/experiment/ExperimentModule.java b/experiment/src/org/labkey/experiment/ExperimentModule.java index 04c6ee053f7..57860b4ce0e 100644 --- a/experiment/src/org/labkey/experiment/ExperimentModule.java +++ b/experiment/src/org/labkey/experiment/ExperimentModule.java @@ -1074,6 +1074,7 @@ public Collection getSummary(Container c) public @NotNull Set> getIntegrationTests() { return Set.of( + ExperimentController.ContainerScopingTestCase.class, DomainImpl.TestCase.class, DomainPropertyImpl.TestCase.class, ExpDataTableImpl.TestCase.class, diff --git a/experiment/src/org/labkey/experiment/ExternalDocsURLCustomPropertyRenderer.java b/experiment/src/org/labkey/experiment/ExternalDocsURLCustomPropertyRenderer.java index e6544c8755a..604b1cf6768 100644 --- a/experiment/src/org/labkey/experiment/ExternalDocsURLCustomPropertyRenderer.java +++ b/experiment/src/org/labkey/experiment/ExternalDocsURLCustomPropertyRenderer.java @@ -78,6 +78,6 @@ public String getValue(ObjectProperty prop, List siblingProperti { // That's OK, we won't try to do anything with the link } - return "" + PageFlowUtil.filter(label) + ""; + return "" + PageFlowUtil.filter(label) + ""; } } diff --git a/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java b/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java index 087cd82d1d2..5d96040a75f 100644 --- a/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java +++ b/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java @@ -30,6 +30,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; @@ -58,6 +59,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; @@ -188,6 +190,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; @@ -199,6 +202,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; @@ -301,6 +305,7 @@ import org.labkey.vfs.FileSystemLike; 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; @@ -319,6 +324,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; @@ -389,6 +395,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); @@ -1800,6 +1810,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()); @@ -8358,4 +8370,47 @@ public Object execute(Object o, BindException errors) throws Exception } } + public static class ContainerScopingTestCase extends AbstractContainerScopingTest + { + @Test + public void testDataClassAttachmentContainerScoping() throws Exception + { + User admin = getAdmin(); + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + User readerA = createUserInRole(folderA, ReaderRole.class); + + // A data object that lives in folder B, with a real attachment so the legitimate download path can be exercised. + String attachmentName = "attachment.txt"; + String lsid = ExperimentService.get().generateGuidLSID(folderB, ExpData.class); + ExpData data = ExperimentService.get().createData(folderB, "exp1-scope-test", lsid); + data.save(admin); + AttachmentParent parent = new ExpDataClassAttachmentParent(folderB, new Lsid(lsid)); + AttachmentService.get().addAttachments(parent, List.of(new ByteArrayAttachmentFile(attachmentName, "scope test".getBytes(StandardCharsets.UTF_8), "text/plain")), admin); + + ActionURL foreignUrl = new ActionURL(DataClassAttachmentDownloadAction.class, folderA) + .addParameter("lsid", lsid) + .addParameter("name", attachmentName); + + // Deny branch: a caller who can read folder A but NOT folder B must not learn that B's data exists -> 404, + // rather than being redirected (which would leak B's existence and path). + assertStatus(HttpServletResponse.SC_NOT_FOUND, get(foreignUrl, readerA)); + + // Redirect branch: a caller who CAN read folder B is redirected to its own container (where Read is + // re-enforced) rather than served from folder A. Fails without the ensureCorrectContainer call. + MockHttpServletResponse resp = get(foreignUrl, admin); + assertStatus(HttpServletResponse.SC_FOUND, resp); + String location = resp.getRedirectedUrl(); + assertNotNull("Redirect must have a Location", location); + assertTrue("Redirect should target the data's own container, was: " + location, location.contains(folderB.getPath())); + + // Positive control: addressing the attachment through its own container serves the file (200), proving the + // action isn't simply rejecting every request. + ActionURL ownUrl = new ActionURL(DataClassAttachmentDownloadAction.class, folderB) + .addParameter("lsid", lsid) + .addParameter("name", attachmentName); + assertStatus(HttpServletResponse.SC_OK, get(ownUrl, admin)); + } + } + } diff --git a/issues/src/org/labkey/issue/IssuesController.java b/issues/src/org/labkey/issue/IssuesController.java index 5e652fe5c0b..7d286d9ea94 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; @@ -80,6 +81,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; @@ -100,11 +102,14 @@ import org.labkey.api.security.SecurityManager; import org.labkey.api.security.User; import org.labkey.api.security.UserManager; +import org.labkey.api.security.permissions.AbstractContainerScopingTest; import org.labkey.api.security.permissions.AdminPermission; import org.labkey.api.security.permissions.InsertPermission; import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.security.permissions.UpdatePermission; +import org.labkey.api.security.roles.FolderAdminRole; import org.labkey.api.security.roles.OwnerRole; +import org.labkey.api.security.roles.ReaderRole; import org.labkey.api.security.roles.RoleManager; import org.labkey.api.util.ButtonBuilder; import org.labkey.api.util.CSRFUtil; @@ -114,6 +119,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; @@ -1651,9 +1657,41 @@ public static class MoveAction extends MutatingApiAction @Override public ApiResponse execute(MoveIssueForm form, BindException errors) { + if (form.getIssueIds() == null || form.getIssueIds().length == 0) + throw new NotFoundException("No issues specified to move"); + + // The client supplies the destination; resolve it and validate it per issue below + Container dest = form.getTargetContainerId() != null ? ContainerManager.getForId(form.getTargetContainerId()) : null; + if (dest == null) + throw new NotFoundException("Target container not found"); + + if (!dest.hasPermission(getUser(), AdminPermission.class)) + throw new UnauthorizedException(); + + List issueIds = Arrays.asList(form.getIssueIds()); + for (Integer issueId : issueIds) + { + // getIssue(null, ...) resolves by global id, so each issue and the chosen destination must be + // authorized explicitly rather than relying on the current container's Admin grant. + IssueObject issue = IssueManager.getIssue(null, getUser(), issueId); + if (issue == null) + throw new NotFoundException("Issue not found: " + issueId); + + Container source = issue.getContainerFromId(); + // Moving an issue removes it from its source folder, so require Admin there. + if (source == null || !source.hasPermission(getUser(), AdminPermission.class)) + throw new UnauthorizedException(); + + // The destination must be a legitimate move target for this issue's list definition. This also + // confirms the user can access the matching issue list in the destination. + IssueListDef issueListDef = IssueManager.getIssueListDef(issue); + if (issueListDef == null || !IssueManager.getMoveDestinationContainers(source, getUser(), issueListDef.getName()).contains(dest)) + throw new UnauthorizedException(); + } + try { - IssueManager.moveIssues(getUser(), Arrays.asList(form.getIssueIds()), ContainerManager.getForId(form.getTargetContainerId())); + IssueManager.moveIssues(getUser(), issueIds, dest); } catch (IOException x) { @@ -2358,4 +2396,84 @@ public void setIssueId(int issueId) this.issueId = issueId; } } + + public static class MoveActionContainerScopingTestCase extends AbstractContainerScopingTest + { + @Test + public void testMoveRequiresSourceAdmin() throws Exception + { + // Admin in the destination only + // Positive control is in IssuesTest.moveIssueTest(). + assertCrossContainerMoveRejected(MoverScope.DESTINATION); + } + + @Test + public void testMoveRequiresDestinationAdmin() throws Exception + { + // Admin in the source only: driving the action through the source folder gets past @RequiresPermission and + // the source-admin guard, so the move's outcome turns solely on the destination-admin guard -> 403. + assertCrossContainerMoveRejected(MoverScope.SOURCE); + } + + private enum MoverScope { SOURCE, DESTINATION } + + /** + * Create a source and destination folder, put an issue in the source (owned by the site admin), then attempt to + * move it as a caller who is a folder admin in exactly one of the two folders ({@code moverScope}). + */ + private void assertCrossContainerMoveRejected(MoverScope moverScope) throws Exception + { + User admin = getAdmin(); + Container source = createContainer("Source"); // the issue lives here + Container dest = createContainer("Dest"); + + ensureIssuesEnabled(source); + + // Create an issue in the source folder (as the site admin) + IssueObject issue = new IssueObject(); + issue.open(source, admin); + issue.setAssignedTo(admin.getUserId()); + issue.setTitle("Scoping test issue"); + issue.setPriority("3"); + issue.setIssueDefName(IssueListDef.DEFAULT_ISSUE_LIST_NAME); + ObjectFactory.Registry.getFactory(IssueObject.class).toMap(issue, issue.getProperties()); + IssueManager.saveIssue(admin, source, issue); + int issueId = issue.getIssueId(); + + // A folder admin in exactly one of the two folders; drive the action through that folder + Container moverFolder = moverScope == MoverScope.SOURCE ? source : dest; + User mover = createUserInRole(moverFolder, FolderAdminRole.class); + // Make them a reader in the other folder + grantRole(mover, moverScope == MoverScope.SOURCE ? dest : source, ReaderRole.class); + + ActionURL url = new ActionURL(MoveAction.class, moverFolder) + .addParameter("issueIds", String.valueOf(issueId)) + .addParameter("targetContainerId", dest.getId()); + assertStatus(HttpServletResponse.SC_FORBIDDEN, post(url, mover)); + + // The issue must remain in its source container + assertNotNull("Issue should still exist in its source folder", IssueManager.getIssue(source, admin, issueId)); + } + + private static void ensureIssuesEnabled(Container c) + { + Module issueModule = ModuleLoader.getInstance().getModule(IssuesModule.NAME); + Set activeModules = c.getActiveModules(); + if (!activeModules.contains(issueModule)) + { + Set newActiveModules = new HashSet<>(activeModules); + newActiveModules.add(issueModule); + c.setActiveModules(newActiveModules); + } + if (IssueManager.getIssueListDef(c, IssueListDef.DEFAULT_ISSUE_LIST_NAME) == null) + { + IssueListDef def = new IssueListDef(); + def.setName(IssueListDef.DEFAULT_ISSUE_LIST_NAME); + def.setLabel(IssueListDef.DEFAULT_ISSUE_LIST_NAME); + def.setKind(IssueDefDomainKind.NAME); + def.beforeInsert(TestContext.get().getUser(), c.getId()); + def.save(TestContext.get().getUser()); + } + } + } } diff --git a/issues/src/org/labkey/issue/IssuesModule.java b/issues/src/org/labkey/issue/IssuesModule.java index 51ad08c0d1a..74fbd0fbe1d 100644 --- a/issues/src/org/labkey/issue/IssuesModule.java +++ b/issues/src/org/labkey/issue/IssuesModule.java @@ -186,7 +186,10 @@ public ActionURL getTabURL(Container c, User user) @Override public @NotNull Set> getIntegrationTests() { - return Collections.singleton(org.labkey.issue.model.IssueManager.TestCase.class); + return Set.of( + org.labkey.issue.model.IssueManager.TestCase.class, + org.labkey.issue.IssuesController.MoveActionContainerScopingTestCase.class + ); } @Override diff --git a/mothership/src/org/labkey/mothership/MothershipController.java b/mothership/src/org/labkey/mothership/MothershipController.java index cce8cc007db..7db3c2c9c2c 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; @@ -48,6 +50,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; @@ -63,6 +66,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; @@ -471,6 +475,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; } @@ -1847,5 +1867,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 f62c0ef3801..e88c6b84a91 100644 --- a/mothership/src/org/labkey/mothership/MothershipModule.java +++ b/mothership/src/org/labkey/mothership/MothershipModule.java @@ -116,6 +116,12 @@ public Set getSchemaNames() return PageFlowUtil.set(ExceptionStackTrace.TestCase.class); } + @Override + public @NotNull Set> getIntegrationTests() + { + return PageFlowUtil.set(MothershipController.ContainerScopingTestCase.class); + } + @Override public void doStartup(ModuleContext moduleContext) { diff --git a/pipeline/src/org/labkey/pipeline/PipelineModule.java b/pipeline/src/org/labkey/pipeline/PipelineModule.java index bc3fcda9dcc..9814c5508fa 100644 --- a/pipeline/src/org/labkey/pipeline/PipelineModule.java +++ b/pipeline/src/org/labkey/pipeline/PipelineModule.java @@ -304,6 +304,7 @@ public void containerDeleted(Container c, User user) 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 a34063257bb..e3e94577d63 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/query/api-src/org/labkey/remoteapi/RemoteConnections.java b/query/api-src/org/labkey/remoteapi/RemoteConnections.java index 00683718a7e..fc76277bdbc 100644 --- a/query/api-src/org/labkey/remoteapi/RemoteConnections.java +++ b/query/api-src/org/labkey/remoteapi/RemoteConnections.java @@ -16,19 +16,29 @@ package org.labkey.remoteapi; import org.apache.commons.lang3.StringUtils; +import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; +import org.junit.Assert; +import org.junit.Test; import org.labkey.api.action.LabKeyError; import org.labkey.api.data.Container; import org.labkey.api.data.PropertyManager; import org.labkey.api.data.PropertyManager.WritablePropertyMap; import org.labkey.api.security.ValidEmail; +import org.labkey.api.util.logging.LogHelper; import org.springframework.validation.BindException; +import javax.net.ssl.SSLException; import java.io.IOException; +import java.io.InputStream; import java.net.MalformedURLException; +import java.net.ServerSocket; +import java.net.Socket; import java.net.URL; import java.net.URLConnection; +import java.nio.charset.StandardCharsets; import java.util.Map; +import java.util.concurrent.TimeUnit; /** * User: gktaylor @@ -36,6 +46,8 @@ */ public class RemoteConnections { + private static final Logger LOG = LogHelper.getLogger(RemoteConnections.class, "Remote server connection management for ETLs"); + public static String REMOTE_QUERY_CONNECTIONS_CATEGORY = "remote-connections"; public static String REMOTE_FILE_CONNECTIONS_CATEGORY = "remote-file-connections"; public static String FIELD_URL = "URL"; @@ -91,9 +103,16 @@ public static boolean createOrEditRemoteConnection(RemoteConnectionForm remoteCo errors.addError(new LabKeyError("The entered URL is not valid.")); return false; } + catch (SSLException e) + { + LOG.warn("TLS error connecting to remote connection URL: {}", url, e); + errors.addError(new LabKeyError("A secure (TLS) connection to the entered URL could not be established. This is often caused by an untrusted, self-signed, or expired certificate. " + getBriefMessage(e))); + return false; + } catch (IOException e) { - errors.addError(new LabKeyError("A connection to the entered URL could not be established.")); + LOG.warn("Error connecting to remote connection URL: {}", url, e); + errors.addError(new LabKeyError("A connection to the entered URL could not be established. " + getBriefMessage(e))); return false; } @@ -139,6 +158,12 @@ public static boolean createOrEditRemoteConnection(RemoteConnectionForm remoteCo return true; } + /** @return a brief, user-facing description of the failure, suitable for appending to an error message. Full details should be logged separately. */ + public static String getBriefMessage(Throwable t) + { + return t.getMessage() == null ? t.getClass().getSimpleName() : t.getMessage(); + } + public static boolean deleteRemoteConnection(RemoteConnectionForm remoteConnectionForm, Container container) { String name = remoteConnectionForm.getConnectionName(); @@ -244,4 +269,82 @@ private static String makeRemoteConnectionKey(String connectionCategory, String { return connectionCategory + ":" + name; } + + public static class TestCase extends Assert + { + /** All URL validation failures return before touching the property store, so no container is needed */ + private BindException validate(String url) + { + RemoteConnectionForm form = new RemoteConnectionForm(); + form.setNewConnectionName("RemoteConnectionsTestCase"); + form.setUrl(url); + form.setUserEmail("remoteconnections_testcase@validation.test"); + form.setPassword("password"); + // A file connection doesn't require a folder path + form.setConnectionKind(CONNECTION_KIND_FILE); + + BindException errors = new BindException(form, "form"); + assertFalse("Expected validation to fail for URL: " + url, createOrEditRemoteConnection(form, null, errors)); + assertEquals("Expected a single validation error for URL: " + url, 1, errors.getErrorCount()); + return errors; + } + + private void assertErrorStartsWith(BindException errors, String expectedPrefix) + { + String message = errors.getAllErrors().get(0).getDefaultMessage(); + assertNotNull("Expected an error message", message); + assertTrue("Expected error message to start with '" + expectedPrefix + "' but was: " + message, message.startsWith(expectedPrefix)); + } + + @Test + public void testMalformedUrl() + { + assertErrorStartsWith(validate("hptt://localhost/bogus"), "The entered URL is not valid."); + } + + @Test + public void testConnectionRefused() throws IOException + { + // Bind an ephemeral port, then release it so nothing is listening when we connect + int port; + try (ServerSocket socket = new ServerSocket(0)) + { + port = socket.getLocalPort(); + } + assertErrorStartsWith(validate("http://localhost:" + port + "/"), "A connection to the entered URL could not be established."); + } + + @Test + public void testTlsFailure() throws Exception + { + // Answer the TLS handshake with plain text, which fails the https client connection with an SSLException + try (ServerSocket socket = new ServerSocket(0)) + { + Thread responder = new Thread(() -> + { + try (Socket client = socket.accept()) + { + client.getOutputStream().write("This is not a TLS handshake".getBytes(StandardCharsets.UTF_8)); + client.getOutputStream().flush(); + // Drain the client's handshake bytes until it disconnects + InputStream in = client.getInputStream(); + byte[] buffer = new byte[1024]; + while (in.read(buffer) != -1) { /* keep draining */ } + } + catch (IOException ignored) {} + }, "RemoteConnections.TestCase non-TLS responder"); + responder.start(); + + assertErrorStartsWith(validate("https://localhost:" + socket.getLocalPort() + "/"), "A secure (TLS) connection to the entered URL could not be established."); + responder.join(TimeUnit.SECONDS.toMillis(10)); + } + } + + @Test + public void testGetBriefMessage() + { + assertEquals("boom", getBriefMessage(new IOException("boom"))); + assertEquals("IOException", getBriefMessage(new IOException())); + } + } } diff --git a/query/src/org/labkey/query/QueryModule.java b/query/src/org/labkey/query/QueryModule.java index 7485cfe98ca..2231bf92a55 100644 --- a/query/src/org/labkey/query/QueryModule.java +++ b/query/src/org/labkey/query/QueryModule.java @@ -1,423 +1,425 @@ -/* - * 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.query; - -import org.jetbrains.annotations.NotNull; -import org.json.JSONObject; -import org.labkey.api.admin.FolderSerializationRegistry; -import org.labkey.api.audit.AuditLogService; -import org.labkey.api.audit.DefaultAuditProvider; -import org.labkey.api.cache.CacheManager; -import org.labkey.api.data.Aggregate; -import org.labkey.api.data.Container; -import org.labkey.api.data.ContainerManager; -import org.labkey.api.data.DataRegionSelection; -import org.labkey.api.data.JdbcType; -import org.labkey.api.data.views.DataViewService; -import org.labkey.api.exp.property.PropertyService; -import org.labkey.api.message.digest.DailyMessageDigest; -import org.labkey.api.message.digest.ReportAndDatasetChangeDigestProvider; -import org.labkey.api.module.AdminLinkManager; -import org.labkey.api.module.DefaultModule; -import org.labkey.api.module.Module; -import org.labkey.api.module.ModuleContext; -import org.labkey.api.pipeline.PipelineService; -import org.labkey.api.query.DefaultSchema; -import org.labkey.api.query.JavaExportScriptFactory; -import org.labkey.api.query.JavaScriptExportScriptFactory; -import org.labkey.api.query.PerlExportScriptFactory; -import org.labkey.api.query.PythonExportScriptFactory; -import org.labkey.api.query.QuerySchema; -import org.labkey.api.query.QueryService; -import org.labkey.api.query.QueryView; -import org.labkey.api.query.RExportScriptFactory; -import org.labkey.api.query.SasExportScriptFactory; -import org.labkey.api.query.SimpleTableDomainKind; -import org.labkey.api.query.URLExportScriptFactory; -import org.labkey.api.query.column.BuiltInColumnTypes; -import org.labkey.api.query.snapshot.QuerySnapshotService; -import org.labkey.api.reports.ReportService; -import org.labkey.api.reports.report.ExternalScriptEngineReport; -import org.labkey.api.reports.report.InternalScriptEngineReport; -import org.labkey.api.reports.report.JavaScriptReport; -import org.labkey.api.reports.report.JavaScriptReportDescriptor; -import org.labkey.api.reports.report.QueryReport; -import org.labkey.api.reports.report.QueryReportDescriptor; -import org.labkey.api.reports.report.ReportDescriptor; -import org.labkey.api.reports.report.ReportUrls; -import org.labkey.api.reports.report.python.IpynbReport; -import org.labkey.api.reports.report.python.IpynbReportDescriptor; -import org.labkey.api.reports.report.r.RReport; -import org.labkey.api.reports.report.r.RReportDescriptor; -import org.labkey.api.search.SearchService; -import org.labkey.api.security.User; -import org.labkey.api.security.permissions.AdminPermission; -import org.labkey.api.security.permissions.PlatformDeveloperPermission; -import org.labkey.api.security.permissions.ReadPermission; -import org.labkey.api.security.roles.PlatformDeveloperRole; -import org.labkey.api.security.roles.Role; -import org.labkey.api.security.roles.RoleManager; -import org.labkey.api.settings.OptionalFeatureService; -import org.labkey.api.stats.AnalyticsProviderRegistry; -import org.labkey.api.stats.SummaryStatisticRegistry; -import org.labkey.api.util.JspTestCase; -import org.labkey.api.util.JunitUtil; -import org.labkey.api.util.PageFlowUtil; -import org.labkey.api.util.emailTemplate.EmailTemplateService; -import org.labkey.api.view.ActionURL; -import org.labkey.api.view.NavTree; -import org.labkey.api.view.WebPartFactory; -import org.labkey.api.writer.ContainerUser; -import org.labkey.query.analytics.AggregatesCountNonBlankAnalyticsProvider; -import org.labkey.query.analytics.AggregatesMaxAnalyticsProvider; -import org.labkey.query.analytics.AggregatesMeanAnalyticsProvider; -import org.labkey.query.analytics.AggregatesMinAnalyticsProvider; -import org.labkey.query.analytics.AggregatesSumAnalyticsProvider; -import org.labkey.query.analytics.RemoveColumnAnalyticsProvider; -import org.labkey.query.analytics.SummaryStatisticsAnalyticsProvider; -import org.labkey.query.audit.QueryExportAuditProvider; -import org.labkey.query.audit.QueryUpdateAuditProvider; -import org.labkey.query.controllers.OlapController; -import org.labkey.query.controllers.QueryController; -import org.labkey.query.controllers.SqlController; -import org.labkey.query.jdbc.QueryDriver; -import org.labkey.query.olap.MemberSet; -import org.labkey.query.olap.ServerManager; -import org.labkey.query.olap.metadata.MetadataElementBase; -import org.labkey.query.olap.rolap.RolapReader; -import org.labkey.query.olap.rolap.RolapTestCase; -import org.labkey.query.olap.rolap.RolapTestSchema; -import org.labkey.query.persist.QueryManager; -import org.labkey.query.reports.AttachmentReport; -import org.labkey.query.reports.LinkReport; -import org.labkey.query.reports.ModuleReportCache; -import org.labkey.query.reports.ReportAndDatasetChangeDigestProviderImpl; -import org.labkey.query.reports.ReportAuditProvider; -import org.labkey.query.reports.ReportImporter; -import org.labkey.query.reports.ReportNotificationInfoProvider; -import org.labkey.query.reports.ReportServiceImpl; -import org.labkey.query.reports.ReportViewProvider; -import org.labkey.query.reports.ReportWriter; -import org.labkey.query.reports.ReportsController; -import org.labkey.query.reports.ReportsPipelineProvider; -import org.labkey.query.reports.ReportsWebPartFactory; -import org.labkey.query.reports.ViewCategoryImporter; -import org.labkey.query.reports.ViewCategoryWriter; -import org.labkey.query.reports.getdata.AggregateQueryDataTransform; -import org.labkey.query.reports.getdata.FilterClauseBuilder; -import org.labkey.query.reports.view.ReportAndDatasetChangeDigestEmailTemplate; -import org.labkey.query.reports.view.ReportUIProvider; -import org.labkey.query.sql.Method; -import org.labkey.query.sql.QNode; -import org.labkey.query.sql.Query; -import org.labkey.query.sql.SqlParser; -import org.labkey.query.view.InheritedQueryDataViewProvider; -import org.labkey.query.view.QueryDataViewProvider; -import org.labkey.query.view.QueryWebPartFactory; -import org.labkey.remoteapi.SelectRowsStreamHack; - -import java.util.ArrayList; -import java.util.Collection; -import java.util.List; -import java.util.Set; -import java.util.function.Supplier; - -import static org.labkey.api.query.QueryService.USE_ROW_BY_ROW_UPDATE; - -public class QueryModule extends DefaultModule -{ - public QueryModule() - { - QueryService.setInstance(new QueryServiceImpl()); - BuiltInColumnTypes.registerStandardColumnTransformers(); - - QueryDriver.register(); - ReportAndDatasetChangeDigestProvider.set(new ReportAndDatasetChangeDigestProviderImpl()); - } - - @Override - public String getName() - { - return "Query"; - } - - @Override - public Double getSchemaVersion() - { - return 25.001; - } - - @Override - protected void init() - { - DefaultSchema.registerProvider("rolap_test", new DefaultSchema.SchemaProvider(this) - { - @Override - public boolean isAvailable(DefaultSchema schema, Module module) - { - return schema.getContainer().getParsedPath().equals(JunitUtil.getTestContainerPath()); - } - - @Override - public QuerySchema createSchema(DefaultSchema schema, Module module) - { - return new RolapTestSchema(schema.getUser(), schema.getContainer()); - } - }); - - addController("query", QueryController.class); - addController("sql", SqlController.class); - addController("reports", ReportsController.class); - addController("olap", OlapController.class); - - ExternalSchema.register(); - LinkedSchema.register(); - - QueryService.get().addQueryListener(new CustomViewQueryChangeListener()); - QueryService.get().addQueryListener(new QuerySnapshotQueryChangeListener()); - QueryService.get().addQueryListener(new QueryDefQueryChangeListener()); - - ReportService.registerProvider(ReportServiceImpl.getInstance()); - ReportService.get().addUIProvider(new ReportUIProvider()); - ReportService.get().addGlobalItemFilterType(JavaScriptReport.TYPE); - ReportService.get().addGlobalItemFilterType(QuerySnapshotService.TYPE); - ReportService.get().addGlobalItemFilterType(IpynbReport.TYPE); - - ReportService.get().registerDescriptor(new IpynbReportDescriptor()); - ReportService.get().registerDescriptor(new ReportDescriptor()); - ReportService.get().registerDescriptor(new QueryReportDescriptor()); - ReportService.get().registerDescriptor(new RReportDescriptor()); - ReportService.get().registerDescriptor(new JavaScriptReportDescriptor()); - - ReportService.get().registerReport(new IpynbReport()); - ReportService.get().registerReport(new QueryReport()); - ReportService.get().registerReport(new RReport()); - ReportService.get().registerReport(new ExternalScriptEngineReport()); - ReportService.get().registerReport(new InternalScriptEngineReport()); - ReportService.get().registerReport(new JavaScriptReport()); - ReportService.get().registerReport(new AttachmentReport()); - ReportService.get().registerReport(new LinkReport()); - EmailTemplateService.get().registerTemplate(ReportAndDatasetChangeDigestEmailTemplate.class); - - QueryView.register(new RExportScriptFactory()); - QueryView.register(new JavaScriptExportScriptFactory()); - QueryView.register(new PerlExportScriptFactory()); - QueryView.register(new JavaExportScriptFactory()); - QueryView.register(new URLExportScriptFactory()); - QueryView.register(new PythonExportScriptFactory()); - QueryView.register(new SasExportScriptFactory()); - - DataViewService.get().registerProvider(ReportViewProvider.TYPE, new ReportViewProvider()); - - DataViewService.get().registerProvider(QueryDataViewProvider.TYPE, new QueryDataViewProvider()); - DataViewService.get().registerProvider(InheritedQueryDataViewProvider.TYPE, new InheritedQueryDataViewProvider()); - - OptionalFeatureService.get().addExperimentalFeatureFlag(QueryView.EXPERIMENTAL_GENERIC_DETAILS_URL, "Generic [details] link in grids/queries", - "This feature will turn on generating a generic [details] URL link in most grids.", false); - OptionalFeatureService.get().addExperimentalFeatureFlag(QueryServiceImpl.EXPERIMENTAL_LAST_MODIFIED, "Include Last-Modified header on query metadata requests", - "For schema, query, and view metadata requests include a Last-Modified header such that the browser can cache the response. " + - "The metadata is invalidated when performing actions such as creating a new List or modifying the columns on a custom view", false); - OptionalFeatureService.get().addExperimentalFeatureFlag(USE_ROW_BY_ROW_UPDATE, "Use row-by-row update", - "For Query.updateRows api, do row-by-row update, instead of using a prepared statement that updates rows in batches.", false); - OptionalFeatureService.get().addExperimentalFeatureFlag(QueryServiceImpl.EXPERIMENTAL_PRODUCT_ALL_FOLDER_LOOKUPS, "Less restrictive product folder lookups", - "Allow for lookup fields in product folders to query across all folders within the top-level folder.", false); - OptionalFeatureService.get().addExperimentalFeatureFlag(QueryServiceImpl.EXPERIMENTAL_PRODUCT_PROJECT_DATA_LISTING_SCOPED, "Product folders display folder-specific data", - "Only list folder-specific data within product folders.", false); - } - - - @Override - @NotNull - protected Collection createWebPartFactories() - { - return List.of( - new DataViewsWebPartFactory(), - new QueryWebPartFactory(), - new ReportsWebPartFactory() -// new QueryBrowserWebPartFactory() - ); - } - - @Override - public boolean hasScripts() - { - return true; - } - - @Override - public void doStartup(ModuleContext moduleContext) - { - ContainerManager.addContainerListener(QueryManager.CONTAINER_LISTENER, ContainerManager.ContainerListener.Order.Last); - - if (null != PipelineService.get()) - PipelineService.get().registerPipelineProvider(new ReportsPipelineProvider(this)); - QueryController.registerAdminConsoleLinks(); - - FolderSerializationRegistry folderRegistry = FolderSerializationRegistry.get(); - if (null != folderRegistry) - { - folderRegistry.addFactories(new QueryWriter.Factory(), new QueryImporter.Factory()); - folderRegistry.addFactories(new CustomViewWriter.Factory(), new CustomViewImporter.Factory()); - folderRegistry.addFactories(new ReportWriter.Factory(), new ReportImporter.Factory()); - folderRegistry.addFactories(new ViewCategoryWriter.Factory(), new ViewCategoryImporter.Factory()); - folderRegistry.addFactories(new ExternalSchemaDefWriterFactory(), new ExternalSchemaDefImporterFactory()); - } - - SearchService ss = SearchService.get(); - ss.addDocumentProvider(ExternalSchemaDocumentProvider.getInstance()); - ss.addSearchCategory(ExternalSchemaDocumentProvider.externalTableCategory); - - if (null != PropertyService.get()) - PropertyService.get().registerDomainKind(new SimpleTableDomainKind()); - - if (null != AuditLogService.get() && AuditLogService.get().getClass() != DefaultAuditProvider.class) - { - AuditLogService.get().registerAuditType(new QueryExportAuditProvider()); - AuditLogService.get().registerAuditType(new QueryUpdateAuditProvider()); - } - AuditLogService.get().registerAuditType(new ReportAuditProvider()); - - ReportAndDatasetChangeDigestProvider.get().addNotificationInfoProvider(new ReportNotificationInfoProvider()); - DailyMessageDigest.getInstance().addProvider(ReportAndDatasetChangeDigestProvider.get()); - // Note: DailyMessageDigest timer is initialized by the AnnouncementModule - - CacheManager.addListener(new ServerManager.CacheListener()); - CacheManager.addListener(new QueryServiceImpl.CacheListener()); - - AdminLinkManager.getInstance().addListener((adminNavTree, container, user) -> { - if (container.hasPermission(user, ReadPermission.class)) - adminNavTree.addChild(new NavTree("Manage Views", PageFlowUtil.urlProvider(ReportUrls.class).urlManageViews(container))); - }); - - AnalyticsProviderRegistry analyticsProviderRegistry = AnalyticsProviderRegistry.get(); - if (null != analyticsProviderRegistry) - { - analyticsProviderRegistry.registerProvider(new AggregatesCountNonBlankAnalyticsProvider()); - analyticsProviderRegistry.registerProvider(new AggregatesSumAnalyticsProvider()); - analyticsProviderRegistry.registerProvider(new AggregatesMeanAnalyticsProvider()); - analyticsProviderRegistry.registerProvider(new AggregatesMinAnalyticsProvider()); - analyticsProviderRegistry.registerProvider(new AggregatesMaxAnalyticsProvider()); - analyticsProviderRegistry.registerProvider(new SummaryStatisticsAnalyticsProvider()); - analyticsProviderRegistry.registerProvider(new RemoveColumnAnalyticsProvider()); - } - - SummaryStatisticRegistry summaryStatisticRegistry = SummaryStatisticRegistry.get(); - if (null != summaryStatisticRegistry) - { - summaryStatisticRegistry.register(Aggregate.BaseType.SUM); - summaryStatisticRegistry.register(Aggregate.BaseType.MEAN); - summaryStatisticRegistry.register(Aggregate.BaseType.COUNT); - summaryStatisticRegistry.register(Aggregate.BaseType.MIN); - summaryStatisticRegistry.register(Aggregate.BaseType.MAX); - } - - QueryManager.registerUsageMetrics(getName()); - ReportServiceImpl.registerUsageMetrics(getName()); - - // Administrators, Platform Developers, and Trusted Analysts can edit queries, if they also have edit permissions in the current folder - RoleManager.registerPermission(new EditQueriesPermission()); - Role platformDeveloperRole = RoleManager.getRole(PlatformDeveloperRole.class); - platformDeveloperRole.addPermission(EditQueriesPermission.class); - Role trustedAnalystRole = RoleManager.getRole("org.labkey.api.security.roles.TrustedAnalystRole"); - if (null != trustedAnalystRole) - trustedAnalystRole.addPermission(EditQueriesPermission.class); - } - - @Override - @NotNull - public Set getSchemaNames() - { - return PageFlowUtil.set(QueryManager.get().getDbSchemaName(), "junit"); - } - - @Override - public @NotNull Set> getIntegrationTests() - { - return Set.of( - ModuleReportCache.TestCase.class, - OlapController.TestCase.class, - QueryController.TestCase.class, - QueryController.SaveRowsTestCase.class, - QueryServiceImpl.TestCase.class, - RolapReader.RolapTest.class, - RolapTestCase.class, - ServerManager.TestCase.class, - SelectRowsStreamHack.TestCase.class - ); - } - - @Override - public @NotNull Collection>> getIntegrationTestFactories() - { - List>> ret = new ArrayList<>(super.getIntegrationTestFactories()); - ret.add(new JspTestCase("/org/labkey/query/MultiValueTest.jsp")); - ret.add(new JspTestCase("/org/labkey/query/olap/OlapTestCase.jsp")); - ret.add(new JspTestCase("/org/labkey/query/QueryServiceImplTestCase.jsp")); - ret.add(new JspTestCase("/org/labkey/query/QueryTestCase.jsp")); - ret.add(new JspTestCase("/org/labkey/query/sql/CalculatedColumnTestCase.jsp")); - - return ret; - } - - - @Override - public @NotNull Set> getUnitTests() - { - return Set.of( - AggregateQueryDataTransform.TestCase.class, - AttachmentReport.TestCase.class, - FilterClauseBuilder.TestCase.class, - JdbcType.TestCase.class, - MemberSet.TestCase.class, - MetadataElementBase.TestCase.class, - Method.TestCase.class, - QNode.TestCase.class, - Query.TestCase.class, - ReportsController.SerializationTest.class, - SqlParser.SqlParserTestCase.class, - TableWriter.TestCase.class - ); - } - - @Override - public ActionURL getTabURL(Container c, User user) - { - // Don't show Query nav trails to users who aren't admins or developers since they almost certainly don't want - // to go to those links - if (c.hasOneOf(user, AdminPermission.class, PlatformDeveloperPermission.class)) - { - return super.getTabURL(c, user); - } - return null; - } - - @Override - public JSONObject getPageContextJson(ContainerUser context) - { - JSONObject json = super.getPageContextJson(context); - boolean hasEditQueriesPermission = context.getContainer().hasPermission(context.getUser(), EditQueriesPermission.class); - json.put("hasEditQueriesPermission", hasEditQueriesPermission); - Container container = context.getContainer(); - boolean isProductFoldersEnabled = container != null && container.isProductFoldersEnabled(); // TODO: should these be moved to CoreModule? - json.put(QueryService.PRODUCT_FOLDERS_ENABLED, isProductFoldersEnabled); - json.put(QueryService.PRODUCT_FOLDERS_EXIST, isProductFoldersEnabled && container.hasProductFolders()); - json.put(QueryService.EXPERIMENTAL_PRODUCT_ALL_FOLDER_LOOKUPS, QueryService.get().isProductFoldersAllFolderScopeEnabled()); - json.put(QueryService.EXPERIMENTAL_PRODUCT_PROJECT_DATA_LISTING_SCOPED, QueryService.get().isProductFoldersDataListingScopedToProject()); - json.put(QueryService.MAX_QUERY_SELECTION, DataRegionSelection.MAX_QUERY_SELECTION_SIZE); - return json; - } -} +/* + * 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.query; + +import org.jetbrains.annotations.NotNull; +import org.json.JSONObject; +import org.labkey.api.admin.FolderSerializationRegistry; +import org.labkey.api.audit.AuditLogService; +import org.labkey.api.audit.DefaultAuditProvider; +import org.labkey.api.cache.CacheManager; +import org.labkey.api.data.Aggregate; +import org.labkey.api.data.Container; +import org.labkey.api.data.ContainerManager; +import org.labkey.api.data.DataRegionSelection; +import org.labkey.api.data.JdbcType; +import org.labkey.api.data.views.DataViewService; +import org.labkey.api.exp.property.PropertyService; +import org.labkey.api.message.digest.DailyMessageDigest; +import org.labkey.api.message.digest.ReportAndDatasetChangeDigestProvider; +import org.labkey.api.module.AdminLinkManager; +import org.labkey.api.module.DefaultModule; +import org.labkey.api.module.Module; +import org.labkey.api.module.ModuleContext; +import org.labkey.api.pipeline.PipelineService; +import org.labkey.api.query.DefaultSchema; +import org.labkey.api.query.JavaExportScriptFactory; +import org.labkey.api.query.JavaScriptExportScriptFactory; +import org.labkey.api.query.PerlExportScriptFactory; +import org.labkey.api.query.PythonExportScriptFactory; +import org.labkey.api.query.QuerySchema; +import org.labkey.api.query.QueryService; +import org.labkey.api.query.QueryView; +import org.labkey.api.query.RExportScriptFactory; +import org.labkey.api.query.SasExportScriptFactory; +import org.labkey.api.query.SimpleTableDomainKind; +import org.labkey.api.query.URLExportScriptFactory; +import org.labkey.api.query.column.BuiltInColumnTypes; +import org.labkey.api.query.snapshot.QuerySnapshotService; +import org.labkey.api.reports.ReportService; +import org.labkey.api.reports.report.ExternalScriptEngineReport; +import org.labkey.api.reports.report.InternalScriptEngineReport; +import org.labkey.api.reports.report.JavaScriptReport; +import org.labkey.api.reports.report.JavaScriptReportDescriptor; +import org.labkey.api.reports.report.QueryReport; +import org.labkey.api.reports.report.QueryReportDescriptor; +import org.labkey.api.reports.report.ReportDescriptor; +import org.labkey.api.reports.report.ReportUrls; +import org.labkey.api.reports.report.python.IpynbReport; +import org.labkey.api.reports.report.python.IpynbReportDescriptor; +import org.labkey.api.reports.report.r.RReport; +import org.labkey.api.reports.report.r.RReportDescriptor; +import org.labkey.api.search.SearchService; +import org.labkey.api.security.User; +import org.labkey.api.security.permissions.AdminPermission; +import org.labkey.api.security.permissions.PlatformDeveloperPermission; +import org.labkey.api.security.permissions.ReadPermission; +import org.labkey.api.security.roles.PlatformDeveloperRole; +import org.labkey.api.security.roles.Role; +import org.labkey.api.security.roles.RoleManager; +import org.labkey.api.settings.OptionalFeatureService; +import org.labkey.api.stats.AnalyticsProviderRegistry; +import org.labkey.api.stats.SummaryStatisticRegistry; +import org.labkey.api.util.JspTestCase; +import org.labkey.api.util.JunitUtil; +import org.labkey.api.util.PageFlowUtil; +import org.labkey.api.util.emailTemplate.EmailTemplateService; +import org.labkey.api.view.ActionURL; +import org.labkey.api.view.NavTree; +import org.labkey.api.view.WebPartFactory; +import org.labkey.api.writer.ContainerUser; +import org.labkey.query.analytics.AggregatesCountNonBlankAnalyticsProvider; +import org.labkey.query.analytics.AggregatesMaxAnalyticsProvider; +import org.labkey.query.analytics.AggregatesMeanAnalyticsProvider; +import org.labkey.query.analytics.AggregatesMinAnalyticsProvider; +import org.labkey.query.analytics.AggregatesSumAnalyticsProvider; +import org.labkey.query.analytics.RemoveColumnAnalyticsProvider; +import org.labkey.query.analytics.SummaryStatisticsAnalyticsProvider; +import org.labkey.query.audit.QueryExportAuditProvider; +import org.labkey.query.audit.QueryUpdateAuditProvider; +import org.labkey.query.controllers.OlapController; +import org.labkey.query.controllers.QueryController; +import org.labkey.query.controllers.SqlController; +import org.labkey.query.jdbc.QueryDriver; +import org.labkey.query.olap.MemberSet; +import org.labkey.query.olap.ServerManager; +import org.labkey.query.olap.metadata.MetadataElementBase; +import org.labkey.query.olap.rolap.RolapReader; +import org.labkey.query.olap.rolap.RolapTestCase; +import org.labkey.query.olap.rolap.RolapTestSchema; +import org.labkey.query.persist.QueryManager; +import org.labkey.query.reports.AttachmentReport; +import org.labkey.query.reports.LinkReport; +import org.labkey.query.reports.ModuleReportCache; +import org.labkey.query.reports.ReportAndDatasetChangeDigestProviderImpl; +import org.labkey.query.reports.ReportAuditProvider; +import org.labkey.query.reports.ReportImporter; +import org.labkey.query.reports.ReportNotificationInfoProvider; +import org.labkey.query.reports.ReportServiceImpl; +import org.labkey.query.reports.ReportViewProvider; +import org.labkey.query.reports.ReportWriter; +import org.labkey.query.reports.ReportsController; +import org.labkey.query.reports.ReportsPipelineProvider; +import org.labkey.query.reports.ReportsWebPartFactory; +import org.labkey.query.reports.ViewCategoryImporter; +import org.labkey.query.reports.ViewCategoryWriter; +import org.labkey.query.reports.getdata.AggregateQueryDataTransform; +import org.labkey.query.reports.getdata.FilterClauseBuilder; +import org.labkey.query.reports.view.ReportAndDatasetChangeDigestEmailTemplate; +import org.labkey.query.reports.view.ReportUIProvider; +import org.labkey.query.sql.Method; +import org.labkey.query.sql.QNode; +import org.labkey.query.sql.Query; +import org.labkey.query.sql.SqlParser; +import org.labkey.query.view.InheritedQueryDataViewProvider; +import org.labkey.query.view.QueryDataViewProvider; +import org.labkey.query.view.QueryWebPartFactory; +import org.labkey.remoteapi.RemoteConnections; +import org.labkey.remoteapi.SelectRowsStreamHack; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; +import java.util.Set; +import java.util.function.Supplier; + +import static org.labkey.api.query.QueryService.USE_ROW_BY_ROW_UPDATE; + +public class QueryModule extends DefaultModule +{ + public QueryModule() + { + QueryService.setInstance(new QueryServiceImpl()); + BuiltInColumnTypes.registerStandardColumnTransformers(); + + QueryDriver.register(); + ReportAndDatasetChangeDigestProvider.set(new ReportAndDatasetChangeDigestProviderImpl()); + } + + @Override + public String getName() + { + return "Query"; + } + + @Override + public Double getSchemaVersion() + { + return 25.001; + } + + @Override + protected void init() + { + DefaultSchema.registerProvider("rolap_test", new DefaultSchema.SchemaProvider(this) + { + @Override + public boolean isAvailable(DefaultSchema schema, Module module) + { + return schema.getContainer().getParsedPath().equals(JunitUtil.getTestContainerPath()); + } + + @Override + public QuerySchema createSchema(DefaultSchema schema, Module module) + { + return new RolapTestSchema(schema.getUser(), schema.getContainer()); + } + }); + + addController("query", QueryController.class); + addController("sql", SqlController.class); + addController("reports", ReportsController.class); + addController("olap", OlapController.class); + + ExternalSchema.register(); + LinkedSchema.register(); + + QueryService.get().addQueryListener(new CustomViewQueryChangeListener()); + QueryService.get().addQueryListener(new QuerySnapshotQueryChangeListener()); + QueryService.get().addQueryListener(new QueryDefQueryChangeListener()); + + ReportService.registerProvider(ReportServiceImpl.getInstance()); + ReportService.get().addUIProvider(new ReportUIProvider()); + ReportService.get().addGlobalItemFilterType(JavaScriptReport.TYPE); + ReportService.get().addGlobalItemFilterType(QuerySnapshotService.TYPE); + ReportService.get().addGlobalItemFilterType(IpynbReport.TYPE); + + ReportService.get().registerDescriptor(new IpynbReportDescriptor()); + ReportService.get().registerDescriptor(new ReportDescriptor()); + ReportService.get().registerDescriptor(new QueryReportDescriptor()); + ReportService.get().registerDescriptor(new RReportDescriptor()); + ReportService.get().registerDescriptor(new JavaScriptReportDescriptor()); + + ReportService.get().registerReport(new IpynbReport()); + ReportService.get().registerReport(new QueryReport()); + ReportService.get().registerReport(new RReport()); + ReportService.get().registerReport(new ExternalScriptEngineReport()); + ReportService.get().registerReport(new InternalScriptEngineReport()); + ReportService.get().registerReport(new JavaScriptReport()); + ReportService.get().registerReport(new AttachmentReport()); + ReportService.get().registerReport(new LinkReport()); + EmailTemplateService.get().registerTemplate(ReportAndDatasetChangeDigestEmailTemplate.class); + + QueryView.register(new RExportScriptFactory()); + QueryView.register(new JavaScriptExportScriptFactory()); + QueryView.register(new PerlExportScriptFactory()); + QueryView.register(new JavaExportScriptFactory()); + QueryView.register(new URLExportScriptFactory()); + QueryView.register(new PythonExportScriptFactory()); + QueryView.register(new SasExportScriptFactory()); + + DataViewService.get().registerProvider(ReportViewProvider.TYPE, new ReportViewProvider()); + + DataViewService.get().registerProvider(QueryDataViewProvider.TYPE, new QueryDataViewProvider()); + DataViewService.get().registerProvider(InheritedQueryDataViewProvider.TYPE, new InheritedQueryDataViewProvider()); + + OptionalFeatureService.get().addExperimentalFeatureFlag(QueryView.EXPERIMENTAL_GENERIC_DETAILS_URL, "Generic [details] link in grids/queries", + "This feature will turn on generating a generic [details] URL link in most grids.", false); + OptionalFeatureService.get().addExperimentalFeatureFlag(QueryServiceImpl.EXPERIMENTAL_LAST_MODIFIED, "Include Last-Modified header on query metadata requests", + "For schema, query, and view metadata requests include a Last-Modified header such that the browser can cache the response. " + + "The metadata is invalidated when performing actions such as creating a new List or modifying the columns on a custom view", false); + OptionalFeatureService.get().addExperimentalFeatureFlag(USE_ROW_BY_ROW_UPDATE, "Use row-by-row update", + "For Query.updateRows api, do row-by-row update, instead of using a prepared statement that updates rows in batches.", false); + OptionalFeatureService.get().addExperimentalFeatureFlag(QueryServiceImpl.EXPERIMENTAL_PRODUCT_ALL_FOLDER_LOOKUPS, "Less restrictive product folder lookups", + "Allow for lookup fields in product folders to query across all folders within the top-level folder.", false); + OptionalFeatureService.get().addExperimentalFeatureFlag(QueryServiceImpl.EXPERIMENTAL_PRODUCT_PROJECT_DATA_LISTING_SCOPED, "Product folders display folder-specific data", + "Only list folder-specific data within product folders.", false); + } + + + @Override + @NotNull + protected Collection createWebPartFactories() + { + return List.of( + new DataViewsWebPartFactory(), + new QueryWebPartFactory(), + new ReportsWebPartFactory() +// new QueryBrowserWebPartFactory() + ); + } + + @Override + public boolean hasScripts() + { + return true; + } + + @Override + public void doStartup(ModuleContext moduleContext) + { + ContainerManager.addContainerListener(QueryManager.CONTAINER_LISTENER, ContainerManager.ContainerListener.Order.Last); + + if (null != PipelineService.get()) + PipelineService.get().registerPipelineProvider(new ReportsPipelineProvider(this)); + QueryController.registerAdminConsoleLinks(); + + FolderSerializationRegistry folderRegistry = FolderSerializationRegistry.get(); + if (null != folderRegistry) + { + folderRegistry.addFactories(new QueryWriter.Factory(), new QueryImporter.Factory()); + folderRegistry.addFactories(new CustomViewWriter.Factory(), new CustomViewImporter.Factory()); + folderRegistry.addFactories(new ReportWriter.Factory(), new ReportImporter.Factory()); + folderRegistry.addFactories(new ViewCategoryWriter.Factory(), new ViewCategoryImporter.Factory()); + folderRegistry.addFactories(new ExternalSchemaDefWriterFactory(), new ExternalSchemaDefImporterFactory()); + } + + SearchService ss = SearchService.get(); + ss.addDocumentProvider(ExternalSchemaDocumentProvider.getInstance()); + ss.addSearchCategory(ExternalSchemaDocumentProvider.externalTableCategory); + + if (null != PropertyService.get()) + PropertyService.get().registerDomainKind(new SimpleTableDomainKind()); + + if (null != AuditLogService.get() && AuditLogService.get().getClass() != DefaultAuditProvider.class) + { + AuditLogService.get().registerAuditType(new QueryExportAuditProvider()); + AuditLogService.get().registerAuditType(new QueryUpdateAuditProvider()); + } + AuditLogService.get().registerAuditType(new ReportAuditProvider()); + + ReportAndDatasetChangeDigestProvider.get().addNotificationInfoProvider(new ReportNotificationInfoProvider()); + DailyMessageDigest.getInstance().addProvider(ReportAndDatasetChangeDigestProvider.get()); + // Note: DailyMessageDigest timer is initialized by the AnnouncementModule + + CacheManager.addListener(new ServerManager.CacheListener()); + CacheManager.addListener(new QueryServiceImpl.CacheListener()); + + AdminLinkManager.getInstance().addListener((adminNavTree, container, user) -> { + if (container.hasPermission(user, ReadPermission.class)) + adminNavTree.addChild(new NavTree("Manage Views", PageFlowUtil.urlProvider(ReportUrls.class).urlManageViews(container))); + }); + + AnalyticsProviderRegistry analyticsProviderRegistry = AnalyticsProviderRegistry.get(); + if (null != analyticsProviderRegistry) + { + analyticsProviderRegistry.registerProvider(new AggregatesCountNonBlankAnalyticsProvider()); + analyticsProviderRegistry.registerProvider(new AggregatesSumAnalyticsProvider()); + analyticsProviderRegistry.registerProvider(new AggregatesMeanAnalyticsProvider()); + analyticsProviderRegistry.registerProvider(new AggregatesMinAnalyticsProvider()); + analyticsProviderRegistry.registerProvider(new AggregatesMaxAnalyticsProvider()); + analyticsProviderRegistry.registerProvider(new SummaryStatisticsAnalyticsProvider()); + analyticsProviderRegistry.registerProvider(new RemoveColumnAnalyticsProvider()); + } + + SummaryStatisticRegistry summaryStatisticRegistry = SummaryStatisticRegistry.get(); + if (null != summaryStatisticRegistry) + { + summaryStatisticRegistry.register(Aggregate.BaseType.SUM); + summaryStatisticRegistry.register(Aggregate.BaseType.MEAN); + summaryStatisticRegistry.register(Aggregate.BaseType.COUNT); + summaryStatisticRegistry.register(Aggregate.BaseType.MIN); + summaryStatisticRegistry.register(Aggregate.BaseType.MAX); + } + + QueryManager.registerUsageMetrics(getName()); + ReportServiceImpl.registerUsageMetrics(getName()); + + // Administrators, Platform Developers, and Trusted Analysts can edit queries, if they also have edit permissions in the current folder + RoleManager.registerPermission(new EditQueriesPermission()); + Role platformDeveloperRole = RoleManager.getRole(PlatformDeveloperRole.class); + platformDeveloperRole.addPermission(EditQueriesPermission.class); + Role trustedAnalystRole = RoleManager.getRole("org.labkey.api.security.roles.TrustedAnalystRole"); + if (null != trustedAnalystRole) + trustedAnalystRole.addPermission(EditQueriesPermission.class); + } + + @Override + @NotNull + public Set getSchemaNames() + { + return PageFlowUtil.set(QueryManager.get().getDbSchemaName(), "junit"); + } + + @Override + public @NotNull Set> getIntegrationTests() + { + return Set.of( + ModuleReportCache.TestCase.class, + OlapController.TestCase.class, + QueryController.TestCase.class, + QueryController.SaveRowsTestCase.class, + QueryServiceImpl.TestCase.class, + RolapReader.RolapTest.class, + RolapTestCase.class, + ServerManager.TestCase.class, + SelectRowsStreamHack.TestCase.class + ); + } + + @Override + public @NotNull Collection>> getIntegrationTestFactories() + { + List>> ret = new ArrayList<>(super.getIntegrationTestFactories()); + ret.add(new JspTestCase("/org/labkey/query/MultiValueTest.jsp")); + ret.add(new JspTestCase("/org/labkey/query/olap/OlapTestCase.jsp")); + ret.add(new JspTestCase("/org/labkey/query/QueryServiceImplTestCase.jsp")); + ret.add(new JspTestCase("/org/labkey/query/QueryTestCase.jsp")); + ret.add(new JspTestCase("/org/labkey/query/sql/CalculatedColumnTestCase.jsp")); + + return ret; + } + + + @Override + public @NotNull Set> getUnitTests() + { + return Set.of( + AggregateQueryDataTransform.TestCase.class, + AttachmentReport.TestCase.class, + FilterClauseBuilder.TestCase.class, + JdbcType.TestCase.class, + MemberSet.TestCase.class, + MetadataElementBase.TestCase.class, + Method.TestCase.class, + QNode.TestCase.class, + Query.TestCase.class, + RemoteConnections.TestCase.class, + ReportsController.SerializationTest.class, + SqlParser.SqlParserTestCase.class, + TableWriter.TestCase.class + ); + } + + @Override + public ActionURL getTabURL(Container c, User user) + { + // Don't show Query nav trails to users who aren't admins or developers since they almost certainly don't want + // to go to those links + if (c.hasOneOf(user, AdminPermission.class, PlatformDeveloperPermission.class)) + { + return super.getTabURL(c, user); + } + return null; + } + + @Override + public JSONObject getPageContextJson(ContainerUser context) + { + JSONObject json = super.getPageContextJson(context); + boolean hasEditQueriesPermission = context.getContainer().hasPermission(context.getUser(), EditQueriesPermission.class); + json.put("hasEditQueriesPermission", hasEditQueriesPermission); + Container container = context.getContainer(); + boolean isProductFoldersEnabled = container != null && container.isProductFoldersEnabled(); // TODO: should these be moved to CoreModule? + json.put(QueryService.PRODUCT_FOLDERS_ENABLED, isProductFoldersEnabled); + json.put(QueryService.PRODUCT_FOLDERS_EXIST, isProductFoldersEnabled && container.hasProductFolders()); + json.put(QueryService.EXPERIMENTAL_PRODUCT_ALL_FOLDER_LOOKUPS, QueryService.get().isProductFoldersAllFolderScopeEnabled()); + json.put(QueryService.EXPERIMENTAL_PRODUCT_PROJECT_DATA_LISTING_SCOPED, QueryService.get().isProductFoldersDataListingScopedToProject()); + json.put(QueryService.MAX_QUERY_SELECTION, DataRegionSelection.MAX_QUERY_SELECTION_SIZE); + return json; + } +} diff --git a/query/src/org/labkey/query/controllers/QueryController.java b/query/src/org/labkey/query/controllers/QueryController.java index 0ed4aa66be9..6fa94693f4f 100644 --- a/query/src/org/labkey/query/controllers/QueryController.java +++ b/query/src/org/labkey/query/controllers/QueryController.java @@ -309,6 +309,7 @@ import org.springframework.web.multipart.MultipartFile; import org.springframework.web.servlet.ModelAndView; +import javax.net.ssl.SSLException; import java.io.BufferedOutputStream; import java.io.ByteArrayOutputStream; import java.io.File; @@ -544,8 +545,18 @@ public ModelAndView getView(RemoteConnections.RemoteConnectionForm remoteConnect } catch (Exception e) { - errors.addError(new LabKeyError("The listed credentials for this remote connection failed to connect.")); - return new JspView<>("/org/labkey/query/view/testRemoteConnectionsFailure.jsp", remoteConnectionForm); + LOG.warn("Failed to connect for remote connection '{}' to {}", name, url, e); + // SelectRowsStreamHack wraps the underlying failure in a RuntimeException; unwrap to categorize it + Throwable cause = ExceptionUtil.unwrapException(e); + String message; + if (cause instanceof SSLException) + message = "A secure (TLS) connection to the remote server could not be established. This is often caused by an untrusted, self-signed, or expired certificate. "; + else if (cause instanceof IOException) + message = "A connection to the remote server could not be established. "; + else + message = "The listed credentials for this remote connection failed to connect. "; + errors.addError(new LabKeyError(message + RemoteConnections.getBriefMessage(cause))); + return new JspView<>("/org/labkey/query/view/testRemoteConnectionsFailure.jsp", remoteConnectionForm, errors); } return new JspView<>("/org/labkey/query/view/testRemoteConnectionsSuccess.jsp", remoteConnectionForm); diff --git a/query/src/org/labkey/query/view/testRemoteConnectionsFailure.jsp b/query/src/org/labkey/query/view/testRemoteConnectionsFailure.jsp index d40c50d2f8f..323a26eb5d2 100644 --- a/query/src/org/labkey/query/view/testRemoteConnectionsFailure.jsp +++ b/query/src/org/labkey/query/view/testRemoteConnectionsFailure.jsp @@ -21,4 +21,5 @@

The connection using the supplied credentials was not successful.

+ <%=link("manage remote connections", QueryController.RemoteQueryConnectionUrls.urlManageRemoteConnection(getContainer()))%> diff --git a/specimen/src/org/labkey/specimen/SpecimenModule.java b/specimen/src/org/labkey/specimen/SpecimenModule.java index 02ad4f3b1b0..e5c70eed025 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; @@ -332,7 +333,9 @@ protected void startupAfterSpringConfig(ModuleContext moduleContext) 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 d3d180977fb..a6628440394 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; @@ -83,6 +84,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; @@ -4612,7 +4614,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; @@ -5178,6 +5181,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]); @@ -5758,4 +5764,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/importer/ImportableColumn.java b/specimen/src/org/labkey/specimen/importer/ImportableColumn.java index c556a687278..ee71e0fb59a 100644 --- a/specimen/src/org/labkey/specimen/importer/ImportableColumn.java +++ b/specimen/src/org/labkey/specimen/importer/ImportableColumn.java @@ -24,7 +24,7 @@ public class ImportableColumn private final boolean _maskOnExport; private final boolean _unique; private final int _size; - private final Class _javaClass; + private final Class _javaClass; private final JdbcType _jdbcType; private Object _defaultValue = null; @@ -88,7 +88,7 @@ public ImportableColumn(String tsvColumnName, Collection tsvColumnAliase } // Can't use standard JdbcType.valueOf() method since this uses contains() - private static Class determineJavaType(String dbType) + private static Class determineJavaType(String dbType) { if (dbType.contains(ImportTypes.DATETIME_TYPE)) throw new IllegalStateException("Java types for DateTime/Timestamp columns should be previously initialized."); @@ -148,7 +148,7 @@ public boolean isUnique() return _unique; } - public Class getJavaClass() + public Class getJavaClass() { return _javaClass; } diff --git a/specimen/src/org/labkey/specimen/importer/RequestabilityManager.java b/specimen/src/org/labkey/specimen/importer/RequestabilityManager.java index cc6ccdc0045..3b718aca949 100644 --- a/specimen/src/org/labkey/specimen/importer/RequestabilityManager.java +++ b/specimen/src/org/labkey/specimen/importer/RequestabilityManager.java @@ -395,8 +395,6 @@ public RequestableRule(Container container) public int updateRequestability(User user, List vials) throws InvalidRuleException { TableInfo tableInfoVial = SpecimenSchema.get().getTableInfoVial(_container); - if (null == tableInfoVial) - throw new IllegalStateException("Expected Vial table to exist."); SQLFragment reason = getAvailabilityReason(); SQLFragment updateSQL = new SQLFragment("UPDATE "); @@ -573,7 +571,7 @@ public SQLFragment getFilterSQL(Container container, User user, List vials @Override public SQLFragment getAvailabilityReason() { - return new SQLFragment("'This vial is " + getMarkType().getLabel().toLowerCase() + " because it was found in the set called \"" + _queryName + "\".'"); + return new SQLFragment("?", "This vial is " + getMarkType().getLabel().toLowerCase() + " because it was found in the set called \"" + _queryName + "\"."); } @Override @@ -695,8 +693,6 @@ public LockedWhileProcessingRule(Container container) public SQLFragment getFilterSQL(Container container, User user, List vials) { TableInfo tableInfoVial = SpecimenSchema.get().getTableInfoVial(_container); - if (null == tableInfoVial) - throw new IllegalStateException("Expected Vial table to exist."); SQLFragment sql = new SQLFragment(); if (vials != null && !vials.isEmpty()) @@ -792,8 +788,6 @@ private void updateRequestability(Container container, User user, boolean resetT if (resetToAvailable) { TableInfo tableInfoVial = SpecimenSchema.get().getTableInfoVial(container); - if (null == tableInfoVial) - throw new IllegalStateException("Expected Vial table to exist."); if (logger != null) logger.info("\tResetting vials to default available state."); @@ -847,10 +841,9 @@ public void updateRequestability(Container container, User user, boolean resetTo public static String makeSpecimenUnavailableMessage(Vial vial, @Nullable String additionalText) { - String message = String.format("Specimen %s%s%s", + return String.format("Specimen %s%s%s", vial.getGlobalUniqueId(), null != vial.getAvailabilityReason() ? vial.getAvailabilityReason().replaceFirst("This vial", "") : " is not available.", null != additionalText ? " " + additionalText : ""); - return message; } } diff --git a/specimen/src/org/labkey/specimen/importer/SimpleSpecimenImporter.java b/specimen/src/org/labkey/specimen/importer/SimpleSpecimenImporter.java index 09d4f5f5b1d..13a5cfd54bc 100644 --- a/specimen/src/org/labkey/specimen/importer/SimpleSpecimenImporter.java +++ b/specimen/src/org/labkey/specimen/importer/SimpleSpecimenImporter.java @@ -84,11 +84,6 @@ public class SimpleSpecimenImporter extends SpecimenImporter private final Map _columnLabels; private final TimepointType _timepointType; - public SimpleSpecimenImporter(Container container, User user) - { - this(container, user, TimepointType.DATE, "Subject"); - } - public SimpleSpecimenImporter(Container container, User user, TimepointType timepointType, String participantIdLabel) { super(container, user); @@ -130,15 +125,6 @@ public void fixupSpecimenColumns(TabLoader tl) throws IOException tl.setColumns(mappedCols); } - // UNDONE: Converting values belongs in _process - public List> fixupSpecimenRows(List> rows) - { - List> result = new ArrayList<>(); - for (Map row : rows) - result.add(fixupSpecimenRow(row)); - return result; - } - private Map fixupSpecimenRow(Map row) { Map result = new HashMap<>(); diff --git a/specimen/src/org/labkey/specimen/requirements/DefaultRequirementProvider.java b/specimen/src/org/labkey/specimen/requirements/DefaultRequirementProvider.java index 803e98fd4b5..2111948c60c 100644 --- a/specimen/src/org/labkey/specimen/requirements/DefaultRequirementProvider.java +++ b/specimen/src/org/labkey/specimen/requirements/DefaultRequirementProvider.java @@ -100,7 +100,11 @@ private synchronized String getDefaultRequirementPlaceholder(final Container con @Override public R getRequirement(Container container, Object requirementPrimaryKey) { - return new TableSelector(getRequirementTableInfo()).getObject(requirementPrimaryKey, _requirementClass); + R requirement = new TableSelector(getRequirementTableInfo()).getObject(requirementPrimaryKey, _requirementClass); + // The lookup is by global primary key; reject rows that don't belong to the requested container + if (requirement != null && !container.equals(requirement.getContainer())) + return null; + return requirement; } public R[] getRequirements(Container container, String ownerEntityId) @@ -133,7 +137,11 @@ public A[] getActors(Container c) @Override public A getActor(Container c, Object primaryKey) { - return new TableSelector(getActorTableInfo()).getObject(primaryKey, _actorClass); + A actor = new TableSelector(getActorTableInfo()).getObject(primaryKey, _actorClass); + // The lookup is by global primary key; reject rows that don't belong to the requested container + if (actor != null && !c.equals(actor.getContainer())) + return null; + return actor; } @Override diff --git a/specimen/src/org/labkey/specimen/requirements/SpecimenRequestRequirementProvider.java b/specimen/src/org/labkey/specimen/requirements/SpecimenRequestRequirementProvider.java index 4af4b5ab4d2..c7be11bb57c 100644 --- a/specimen/src/org/labkey/specimen/requirements/SpecimenRequestRequirementProvider.java +++ b/specimen/src/org/labkey/specimen/requirements/SpecimenRequestRequirementProvider.java @@ -16,9 +16,12 @@ package org.labkey.specimen.requirements; +import org.junit.Test; import org.labkey.api.data.Container; import org.labkey.api.data.TableInfo; +import org.labkey.api.security.permissions.AbstractContainerScopingTest; import org.labkey.api.specimen.SpecimenSchema; +import org.labkey.api.util.GUID; import org.labkey.specimen.model.SpecimenRequestActor; import java.util.Collection; @@ -99,4 +102,49 @@ public Set getActorsInUseSet(Container container) ids.add(actor.getRowId()); return ids; } + + public static class ContainerScopingTestCase extends AbstractContainerScopingTest + { + @Test + public void testGetActorContainerScoping() + { + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + SpecimenRequestRequirementProvider provider = SpecimenRequestRequirementProvider.get(); + + SpecimenRequestActor actor = new SpecimenRequestActor(); + actor.setContainer(folderA); + actor.setLabel("Scoping test actor"); + actor = actor.create(getAdmin()); + int rowId = actor.getRowId(); + + assertNotNull("Actor should be visible from its own container", provider.getActor(folderA, rowId)); + assertNull("Actor must NOT be visible from another container", provider.getActor(folderB, rowId)); + } + + @Test + public void testGetRequirementContainerScoping() + { + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + SpecimenRequestRequirementProvider provider = SpecimenRequestRequirementProvider.get(); + + // A requirement references an actor through a NOT NULL foreign key, so create one in the same folder first + SpecimenRequestActor actor = new SpecimenRequestActor(); + actor.setContainer(folderA); + actor.setLabel("Requirement scoping test actor"); + actor = actor.create(getAdmin()); + + SpecimenRequestRequirement requirement = new SpecimenRequestRequirement(); + requirement.setContainer(folderA); + requirement.setRequestId(-1); // no real request row is needed for a primary-key lookup + requirement.setActorId(actor.getRowId()); + requirement.setDescription("Requirement scoping test"); + requirement = requirement.persist(getAdmin(), GUID.makeGUID()); + int rowId = requirement.getRowId(); + + assertNotNull("Requirement should be visible from its own container", provider.getRequirement(folderA, rowId)); + assertNull("Requirement must NOT be visible from another container", provider.getRequirement(folderB, rowId)); + } + } } diff --git a/study/src/org/labkey/study/StudyModule.java b/study/src/org/labkey/study/StudyModule.java index fbb6ab9dc93..2bcb3bf00cf 100644 --- a/study/src/org/labkey/study/StudyModule.java +++ b/study/src/org/labkey/study/StudyModule.java @@ -757,15 +757,16 @@ public WebPartView getWebPartView(@NotNull ViewContext portalCtx, @NotNull Po public @NotNull Set> getIntegrationTests() { return Set.of( - DatasetDefinition.TestCleanupOrphanedDatasetDomains.class, - ParticipantGroupManager.ParticipantGroupTestCase.class, - StudyImpl.ProtocolDocumentTestCase.class, - StudyManager.StudySnapshotTestCase.class, - StudyManager.VisitCreationTestCase.class, - StudyModule.TestCase.class, - VisitImpl.TestCase.class, - DatasetUpdateService.TestCase.class, - DatasetLsidImportHelper.TestCase.class + DatasetDefinition.TestCleanupOrphanedDatasetDomains.class, + ParticipantGroupManager.ParticipantGroupTestCase.class, + StudyImpl.ProtocolDocumentTestCase.class, + StudyManager.StudySnapshotTestCase.class, + StudyManager.VisitCreationTestCase.class, + StudyModule.TestCase.class, + VisitImpl.TestCase.class, + DatasetUpdateService.TestCase.class, + DatasetLsidImportHelper.TestCase.class, + org.labkey.study.controllers.CreateChildStudyAction.ContainerScopingTestCase.class ); } diff --git a/study/src/org/labkey/study/controllers/CreateChildStudyAction.java b/study/src/org/labkey/study/controllers/CreateChildStudyAction.java index 9e502bf6a30..4f626b5d595 100644 --- a/study/src/org/labkey/study/controllers/CreateChildStudyAction.java +++ b/study/src/org/labkey/study/controllers/CreateChildStudyAction.java @@ -15,7 +15,10 @@ */ package org.labkey.study.controllers; +import jakarta.servlet.http.HttpServletResponse; import org.jetbrains.annotations.NotNull; +import org.junit.Before; +import org.junit.Test; import org.labkey.api.action.ApiResponse; import org.labkey.api.action.ApiSimpleResponse; import org.labkey.api.action.MutatingApiAction; @@ -31,19 +34,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 +110,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; - // make sure the folder, if already existing doesn't already contain a study - _dstContainer = ContainerManager.ensureContainer(Path.parse(form.getDstPath()), getUser()); - if (_dstContainer != 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; + } + + 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 +214,72 @@ private StudyImpl createNewStudy(ChildStudyDefinition form, BindException errors return study; } + + public static class ContainerScopingTestCase extends AbstractContainerScopingTest + { + private Container _source; + + @Before + public void createSourceStudy() + { + // A real study in the source folder, owned by the site admin + _source = createContainer("Source"); + StudyImpl study = new StudyImpl(_source, "Source Study"); + study.setTimepointType(TimepointType.VISIT); + StudyManager.getInstance().createTestStudy(getAdmin(), study); + } + + @Test + public void testPublishRequiresSourceRead() throws Exception + { + // A user who is a folder admin in the destination only (no rights in the source) + Container dest = createContainer("Dest"); + User destAdminOnly = createUserInRole(dest, FolderAdminRole.class); + + MockHttpServletResponse resp = publish(dest, dest.getPath(), 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()); + assertNull("No study should have been created in the destination", StudyManager.getInstance().getStudy(dest)); + assertSourceUntouched(); + + // Positive scenario covered by StudyPublishTest + } + + @Test + public void testPublishToNewFolderRequiresParentAdmin() throws Exception + { + // A user who is a folder admin in the source only + Container parent = createContainer("Parent"); + User sourceAdminOnly = createUserInRole(_source, FolderAdminRole.class); + + // The destination folder does not exist yet, so authorization falls to the dstParent branch + String newDstPath = parent.getPath() + "/PublishedChild"; + MockHttpServletResponse resp = publish(_source, newDstPath, sourceAdminOnly); + + // The publish must not succeed because the user can't administer the destination's parent + assertNotEquals("Publishing into a new folder under an unadministered parent must not succeed", HttpServletResponse.SC_OK, resp.getStatus()); + // The guard runs before ensureContainer(), so no destination folder must have been grafted in + assertNull("No destination folder should have been created under the parent", ContainerManager.getForPath(Path.parse(newDstPath))); + assertSourceUntouched(); + + // Positive scenario (a parent admin creating the child folder) covered by StudyPublishTest + } + + // Posts a publish of _source into dstPath, dispatched through requestContainer (which is the container the + // action's @RequiresPermission is evaluated against). + private MockHttpServletResponse publish(Container requestContainer, String dstPath, User user) throws Exception + { + ActionURL url = new ActionURL(CreateChildStudyAction.class, requestContainer) + .addParameter("srcPath", _source.getPath()) + .addParameter("dstPath", dstPath) + .addParameter("mode", StudySnapshotType.publish.name()); + return post(url, user); + } + + private void assertSourceUntouched() + { + assertNotNull("The source study must be untouched", StudyManager.getInstance().getStudy(_source)); + } + } } 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 1f18609dff5..5ca9bf98186 100644 --- a/study/src/org/labkey/study/model/StudyImpl.java +++ b/study/src/org/labkey/study/model/StudyImpl.java @@ -1258,13 +1258,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 2cdcd6926f4..d1f7641d839 100644 --- a/study/src/org/labkey/study/model/StudyManager.java +++ b/study/src/org/labkey/study/model/StudyManager.java @@ -575,6 +575,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 729c110246c..8c86f1e44bc 100644 --- a/study/src/org/labkey/study/query/DatasetUpdateService.java +++ b/study/src/org/labkey/study/query/DatasetUpdateService.java @@ -1119,12 +1119,9 @@ public void createStudy() MvUtil.assignMvIndicators(c, new String[] {"NA","QA"}, new String[] {"NA","QA"}); StudyImpl s = new StudyImpl(c, "Junit Study"); s.setTimepointType(TimepointType.VISIT); - s.setStartDate(new Date(DateUtil.parseDateTime(c, "2014-01-01"))); - s.setSubjectColumnName(SUBJECT_COLUMN_NAME); - s.setSubjectNounPlural("Subjects"); - s.setSubjectNounSingular("Subject"); + s.setStartDate(new Date(DateUtil.parseDateTime("2014-01-01"))); s.setSecurityType(SecurityType.BASIC_WRITE); - _junitStudy = StudyManager.getInstance().createStudy(_context.getUser(), s); + _junitStudy = StudyManager.getInstance().createTestStudy(_context.getUser(), s); _user = _context.getUser(); _container = _junitStudy.getContainer(); } diff --git a/survey/src/org/labkey/survey/SurveyController.java b/survey/src/org/labkey/survey/SurveyController.java index 3f78d0406ea..121d1a28ecf 100644 --- a/survey/src/org/labkey/survey/SurveyController.java +++ b/survey/src/org/labkey/survey/SurveyController.java @@ -70,6 +70,7 @@ import org.labkey.api.view.ActionURL; import org.labkey.api.view.JspView; import org.labkey.api.view.NavTree; +import org.labkey.api.view.NotFoundException; import org.springframework.validation.BindException; import org.springframework.validation.Errors; import org.springframework.web.servlet.ModelAndView; @@ -420,12 +421,19 @@ private SurveyDesign getSurveyDesign(SurveyDesignForm form) { SurveyDesign survey = new SurveyDesign(); if (form.getRowId() != 0) + { survey = SurveyManager.get().getSurveyDesign(getContainer(), getUser(), form.getRowId()); + // getSurveyDesign is container-scoped; null here means the rowId doesn't belong to this folder + if (survey == null) + throw new NotFoundException("No survey design found for rowId " + form.getRowId() + " in this folder"); + } else if (form.getDesignId() != null) { if (NumberUtils.isDigits(form.getDesignId())) { survey = SurveyManager.get().getSurveyDesign(getContainer(), getUser(), NumberUtils.toInt(form.getDesignId())); + if (survey == null) + throw new NotFoundException("No survey design found for designId " + form.getDesignId() + " in this folder"); } else { @@ -455,7 +463,12 @@ private Survey getSurvey(SurveyForm form) { Survey survey = new Survey(); if (form.getRowId() != null) + { survey = SurveyManager.get().getSurvey(getContainer(), getUser(), form.getRowId()); + // getSurvey is container-scoped; null here means the rowId doesn't belong to this folder + if (survey == null) + throw new NotFoundException("No survey found for rowId " + form.getRowId() + " in this folder"); + } if (survey != null) { diff --git a/survey/src/org/labkey/survey/SurveyManager.java b/survey/src/org/labkey/survey/SurveyManager.java index 2d35c13b839..66be29ef8b0 100644 --- a/survey/src/org/labkey/survey/SurveyManager.java +++ b/survey/src/org/labkey/survey/SurveyManager.java @@ -24,6 +24,7 @@ import org.json.JSONException; import org.json.JSONObject; import org.junit.Assert; +import org.junit.Before; import org.junit.Test; import org.labkey.api.action.NullSafeBindException; import org.labkey.api.collections.CaseInsensitiveHashMap; @@ -61,6 +62,7 @@ import org.labkey.api.query.UserSchema; import org.labkey.api.resource.Resource; import org.labkey.api.security.User; +import org.labkey.api.security.permissions.AbstractContainerScopingTest; import org.labkey.api.survey.model.Survey; import org.labkey.api.survey.model.SurveyDesign; import org.labkey.api.survey.model.SurveyListener; @@ -264,9 +266,13 @@ public Survey saveSurvey(Container container, User user, Survey survey) } } + @Nullable public SurveyDesign getSurveyDesign(Container container, User user, int surveyId) { - return new TableSelector(SurveySchema.getInstance().getSurveyDesignsTable(), new SimpleFilter(FieldKey.fromParts("rowId"), surveyId), null).getObject(SurveyDesign.class); + // Scope by container + SimpleFilter filter = new SimpleFilter(FieldKey.fromParts("rowId"), surveyId); + filter.addCondition(FieldKey.fromParts("Container"), container); + return new TableSelector(SurveySchema.getInstance().getSurveyDesignsTable(), filter, null).getObject(SurveyDesign.class); } /** @@ -313,8 +319,10 @@ public SurveyDesign[] getSurveyDesigns(SimpleFilter filter) public Survey getSurvey(Container container, User user, int rowId) { + // Scope by container so a global rowId can't read/modify a survey in another folder SimpleFilter filter = new SimpleFilter(); filter.addCondition(FieldKey.fromParts("rowId"), rowId); + filter.addCondition(FieldKey.fromParts("Container"), container); return new TableSelector(SurveySchema.getInstance().getSurveysTable(), filter, null).getObject(Survey.class); } @@ -381,7 +389,7 @@ public void deleteSurveyDesign(Container c, User user, int surveyDesignId, boole deleteSurvey(c, user, survey.getRowId()); } SQLFragment deleteSurveyDesignsSql = new SQLFragment("DELETE FROM "); - deleteSurveyDesignsSql.append(s.getSurveyDesignsTable()).append(" WHERE RowId = ?").add(surveyDesignId); + deleteSurveyDesignsSql.append(s.getSurveyDesignsTable()).append(" WHERE RowId = ? AND Container = ?").add(surveyDesignId).add(c); executor.execute(deleteSurveyDesignsSql); transaction.commit(); @@ -390,8 +398,10 @@ public void deleteSurveyDesign(Container c, User user, int surveyDesignId, boole public Survey[] getSurveys(Container c, User user, int surveyDesignId) { + // Scope by container so the delete cascade and lookups can't reach surveys in another folder SimpleFilter filter = new SimpleFilter(); filter.addCondition(FieldKey.fromParts("surveyDesignId"), surveyDesignId); + filter.addCondition(FieldKey.fromParts("Container"), c); return new TableSelector(SurveySchema.getInstance().getSurveysTable(), filter, null).getArray(Survey.class); } @@ -782,4 +792,69 @@ public void testGetMetaDataForColumn() assertTrue("Unexpected property value", trimmedMap.get("required").equals(true)); } } + + public static class ContainerScopingTestCase extends AbstractContainerScopingTest + { + private Container _projectA; + private Container _projectB; + private User _user; + + @Before + public void setUp() + { + _user = getAdmin(); + _projectA = createContainer("A"); + _projectB = createContainer("B"); + } + + @Test + public void testSurveyDesignContainerScoping() + { + SurveyManager sm = SurveyManager.get(); + + SurveyDesign design = new SurveyDesign(); + design.setLabel("Scoping test design"); + design = sm.saveSurveyDesign(_projectA, _user, design); + int designId = design.getRowId(); + + // Same-container lookup succeeds; cross-container lookup must return null + assertNotNull("Design should be visible from its own container", sm.getSurveyDesign(_projectA, _user, designId)); + assertNull("Design must NOT be visible from another container", sm.getSurveyDesign(_projectB, _user, designId)); + + // A delete issued from the wrong container must not remove the design + sm.deleteSurveyDesign(_projectB, _user, designId, true); + assertNotNull("Cross-container delete must be a no-op", sm.getSurveyDesign(_projectA, _user, designId)); + + // A delete from the correct container removes it + sm.deleteSurveyDesign(_projectA, _user, designId, true); + assertNull("Same-container delete should remove the design", sm.getSurveyDesign(_projectA, _user, designId)); + } + + @Test + public void testSurveyContainerScoping() + { + SurveyManager sm = SurveyManager.get(); + + SurveyDesign design = new SurveyDesign(); + design.setLabel("Scoping test design for survey"); + design = sm.saveSurveyDesign(_projectA, _user, design); + + Survey survey = new Survey(); + survey.setLabel("Scoping test survey"); + survey.setSurveyDesignId(design.getRowId()); + survey = sm.saveSurvey(_projectA, _user, survey); + int surveyRowId = survey.getRowId(); + + assertNotNull("Survey should be visible from its own container", sm.getSurvey(_projectA, _user, surveyRowId)); + assertNull("Survey must NOT be visible from another container", sm.getSurvey(_projectB, _user, surveyRowId)); + + // A delete issued from the wrong container must not remove the survey + sm.deleteSurvey(_projectB, _user, surveyRowId); + assertNotNull("Cross-container delete must be a no-op", sm.getSurvey(_projectA, _user, surveyRowId)); + + // A delete from the correct container removes it + sm.deleteSurvey(_projectA, _user, surveyRowId); + assertNull("Same-container delete should remove the survey", sm.getSurvey(_projectA, _user, surveyRowId)); + } + } } diff --git a/survey/src/org/labkey/survey/SurveyModule.java b/survey/src/org/labkey/survey/SurveyModule.java index f30de7ddf14..883d9a3cec7 100644 --- a/survey/src/org/labkey/survey/SurveyModule.java +++ b/survey/src/org/labkey/survey/SurveyModule.java @@ -269,4 +269,12 @@ public WebPartView getWebPartView(@NotNull ViewContext context, @NotNull Port SurveyManager.TestCase.class ); } + + @Override + public @NotNull Set> getIntegrationTests() + { + return Set.of( + SurveyManager.ContainerScopingTestCase.class + ); + } } diff --git a/wiki/src/org/labkey/wiki/WikiController.java b/wiki/src/org/labkey/wiki/WikiController.java index a9c9c35cc7a..e1dd90d0f6b 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; @@ -25,6 +26,8 @@ import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import org.junit.Before; +import org.junit.Test; import org.labkey.api.action.ApiResponse; import org.labkey.api.action.ApiSimpleResponse; import org.labkey.api.action.ConfirmAction; @@ -57,8 +60,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; @@ -988,6 +993,11 @@ 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 (_cDest == null || !_cDest.hasPermission(getUser(), AdminPermission.class)) + throw new NotFoundException("No destination container found, or you do not have permission to copy to it."); + if (cSrc.equals(_cDest)) { throw new NotFoundException("Cannot copy a wiki into the folder it is being copied from."); @@ -1006,37 +1016,34 @@ public boolean handlePost(CopyWikiForm form, BindException errors) throws Except throw new NotFoundException("No page named '" + pageName + "' exists in the source container."); } - if (_cDest != null && _cDest.hasPermission(getUser(), AdminPermission.class)) - { - //get source wiki pages - List srcPageNames; + //get source wiki pages + List srcPageNames; - if (parentPage != null) - // TODO: make subtrees work; previously getWikiManager().getSubTreePageList(cSrc, parentPage), now - // something like WikiSelectManager.getDescendents(cSrc, name) - srcPageNames = WikiSelectManager.getPageNames(cSrc); - else - srcPageNames = WikiSelectManager.getPageNames(cSrc); + if (parentPage != null) + // TODO: make subtrees work; previously getWikiManager().getSubTreePageList(cSrc, parentPage), now + // something like WikiSelectManager.getDescendents(cSrc, name) + srcPageNames = WikiSelectManager.getPageNames(cSrc); + else + srcPageNames = WikiSelectManager.getPageNames(cSrc); - //get existing destination wiki page names - List destPageNames = WikiSelectManager.getPageNames(_cDest); + //get existing destination wiki page names + List destPageNames = WikiSelectManager.getPageNames(_cDest); - //map source page row ids to new page row ids - Map pageIdMap = new IntHashMap<>(); - //shortcut for root topics - pageIdMap.put(null, null); + //map source page row ids to new page row ids + Map pageIdMap = new IntHashMap<>(); + //shortcut for root topics + pageIdMap.put(null, null); - //copy each page in the list - for (String name : srcPageNames) - { - Wiki srcWikiPage = WikiSelectManager.getWiki(cSrc, name); - getWikiManager().copyPage(getUser(), cSrc, srcWikiPage, _cDest, destPageNames, pageIdMap, form.getIsCopyingHistory()); - } - - //display the wiki module in the destination container - displayWikiModuleInDestContainer(_cDest); + //copy each page in the list + for (String name : srcPageNames) + { + Wiki srcWikiPage = WikiSelectManager.getWiki(cSrc, name); + getWikiManager().copyPage(getUser(), cSrc, srcWikiPage, _cDest, destPageNames, pageIdMap, form.getIsCopyingHistory()); } + //display the wiki module in the destination container + displayWikiModuleInDestContainer(_cDest); + return true; } } @@ -2897,4 +2904,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 6e6f4c4e18e..806589e81b4 100644 --- a/wiki/src/org/labkey/wiki/WikiModule.java +++ b/wiki/src/org/labkey/wiki/WikiModule.java @@ -198,7 +198,8 @@ private void loadWikiContent(@Nullable Container c, User user, String name, Stri public @NotNull Set> getIntegrationTests() { return Set.of( - WikiManager.TestCase.class + WikiManager.TestCase.class, + WikiController.CopyWikiContainerScopingTestCase.class ); }