diff --git a/announcements/src/org/labkey/announcements/AnnouncementModule.java b/announcements/src/org/labkey/announcements/AnnouncementModule.java index 0d1e6466ad9..f4888b00295 100644 --- a/announcements/src/org/labkey/announcements/AnnouncementModule.java +++ b/announcements/src/org/labkey/announcements/AnnouncementModule.java @@ -216,7 +216,8 @@ public void startBackgroundThreads() public Set getIntegrationTests() { return Set.of( - AnnouncementManager.TestCase.class + AnnouncementManager.TestCase.class, + ToursController.ContainerScopingTestCase.class ); } diff --git a/announcements/src/org/labkey/announcements/ToursController.java b/announcements/src/org/labkey/announcements/ToursController.java index 7fa4054f84c..208fc490768 100644 --- a/announcements/src/org/labkey/announcements/ToursController.java +++ b/announcements/src/org/labkey/announcements/ToursController.java @@ -15,7 +15,9 @@ */ package org.labkey.announcements; +import jakarta.servlet.http.HttpServletResponse; import org.json.JSONObject; +import org.junit.Test; import org.labkey.announcements.model.TourManager; import org.labkey.announcements.model.TourModel; import org.labkey.announcements.query.AnnouncementSchema; @@ -30,14 +32,24 @@ import org.labkey.api.query.QueryView; import org.labkey.api.security.ActionNames; 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.security.roles.ReaderRole; import org.labkey.api.view.ActionURL; import org.labkey.api.view.JspView; import org.labkey.api.view.NavTree; +import org.labkey.api.view.UnauthorizedException; +import org.labkey.api.view.ViewServlet; +import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.validation.BindException; import org.springframework.validation.Errors; import org.springframework.web.servlet.ModelAndView; +import java.util.Map; + /** * Created by Marty on 1/19/2015. */ @@ -123,6 +135,12 @@ public static class SaveTourAction extends MutatingApiAction @Override public void validateForm(SimpleApiJsonForm form, Errors errors) { + // The "//will check below" gate on the annotation was never implemented: this action inserts/updates tour + // content (a folder-level configuration asset) but performed no insert/update/admin check, so a Read user + // could create or overwrite tours. Require folder admin to manage tours. + if (!getContainer().hasPermission(getUser(), AdminPermission.class)) + throw new UnauthorizedException("You do not have permission to modify tours in this folder."); + TourModel model; JSONObject json = form.getJsonObject(); int rowId = json.getInt("rowId"); @@ -203,4 +221,32 @@ public void setRowid(String rowid) _rowid = rowid; } } + + public static class ContainerScopingTestCase extends AbstractContainerScopingTest + { + @Test + public void testSaveTourRequiresAdmin() throws Exception + { + Container folder = createContainer("A"); + ActionURL url = new ActionURL(SaveTourAction.class, folder); + Map jsonHeaders = Map.of("Content-Type", "application/json"); + + // A Reader must not be able to create/modify tours + User reader = createUserInRole(folder, ReaderRole.class); + String body = new JSONObject().put("rowId", -1).toString(); + assertStatus(HttpServletResponse.SC_FORBIDDEN, ViewServlet.POST(url, reader, jsonHeaders, body)); + + // Positive control: a folder admin passes the permission gate and the tour is created (success, 200). + User folderAdmin = createUserInRole(folder, FolderAdminRole.class); + String adminBody = new JSONObject() + .put("rowId", -1) + .put("title", "scoping-test-tour") + .put("description", "d") + .put("mode", "0") + .put("tour", new JSONObject()) + .toString(); + MockHttpServletResponse resp = ViewServlet.POST(url, folderAdmin, jsonHeaders, adminBody); + assertStatus(HttpServletResponse.SC_OK, resp); + } + } } diff --git a/api/src/org/labkey/api/data/AbstractParticipantCategory.java b/api/src/org/labkey/api/data/AbstractParticipantCategory.java index 914e1a939ee..00ab7ad438b 100644 --- a/api/src/org/labkey/api/data/AbstractParticipantCategory.java +++ b/api/src/org/labkey/api/data/AbstractParticipantCategory.java @@ -229,7 +229,10 @@ public boolean canEdit(Container container, User user, List err else { User owner = UserManager.getUser(getCreatedBy()); - boolean allowed = (owner != null && !owner.isGuest()) ? owner.equals(user) : false; + boolean allowed = + container.hasPermission(user, SharedParticipantGroupPermission.class) || + container.hasPermission(user, AdminPermission.class) || + (owner != null && !owner.isGuest() && owner.equals(user)); if (!allowed) errors.add(new SimpleValidationError("You must be the owner to unshare this participant category")); diff --git a/api/src/org/labkey/api/data/TableViewForm.java b/api/src/org/labkey/api/data/TableViewForm.java index 7254abb1e69..6b4a2b62506 100644 --- a/api/src/org/labkey/api/data/TableViewForm.java +++ b/api/src/org/labkey/api/data/TableViewForm.java @@ -34,6 +34,7 @@ import org.labkey.api.action.NullSafeBindException; import org.labkey.api.action.SpringActionController; import org.labkey.api.collections.CaseInsensitiveHashMap; +import org.labkey.api.query.FieldKey; import org.labkey.api.query.SchemaKey; import org.labkey.api.security.permissions.DeletePermission; import org.labkey.api.security.permissions.InsertPermission; @@ -183,8 +184,21 @@ public void doUpdate() throws SQLException throw new UnauthorizedException(); } - if (null != _tinfo.getColumn("container")) + FieldKey containerFK = FieldKey.fromParts("Container"); + if (null != _tinfo.getColumn(containerFK)) + { + // The hasPermission() check above only proves the user can update the *current* container. The UPDATE below + // keys on the PK alone and stamps the row into the current container, so without this guard a user with + // update permission here could edit (and re-home) a row that actually lives in another container simply by + // POSTing its PK. Confirm the existing row is in this container; 404 otherwise. PkFilter validates and + // converts the PK as well, so a missing or malformed key likewise 404s here rather than later. + SimpleFilter filter = new PkFilter(_tinfo, getPkVals()); + filter.addCondition(containerFK, _c.getId()); + if (!new TableSelector(_tinfo, filter, null).exists()) + throw new NotFoundException("No matching row found in this folder"); + set("container", _c.getId()); + } Object[] pkVal = getPkVals(); Map newMap = Table.update(_user, _tinfo, getTypedValues(), pkVal); @@ -207,21 +221,50 @@ public void doDelete() throw new UnauthorizedException(); } - if (null != _selectedRows && _selectedRows.length > 0) - { - for (String selectedRow : _selectedRows) - Table.delete(_tinfo, selectedRow); - } - else + // Table.delete() keys on the PK alone. As with doUpdate(), the DeletePermission check only proves the user can + // delete in the *current* container, so for container-scoped tables we must confirm each target row lives here; + // otherwise a user could delete a row that belongs to another container by POSTing (or grid-selecting) its PK. + FieldKey containerFK = FieldKey.fromParts("Container"); + boolean scopeToContainer = null != _tinfo.getColumn(containerFK); + + try (DbScope.Transaction t = _tinfo.getSchema().getScope().ensureTransaction()) { - Object[] pkVal = getPkVals(); - if (null != pkVal && null != pkVal[0]) - Table.delete(_tinfo, pkVal); - else //Hmm, throw an exception here???? - _log.warn("Nothing to delete for table " + _tinfo.getName() + " on request " + _request.getRequestURI()); + if (null != _selectedRows && _selectedRows.length > 0) + { + for (String selectedRow : _selectedRows) + { + if (scopeToContainer) + deleteInContainer(selectedRow, containerFK); + else + Table.delete(_tinfo, selectedRow); + } + } + else + { + Object[] pkVal = getPkVals(); + if (null != pkVal && null != pkVal[0]) + { + if (scopeToContainer) + deleteInContainer(pkVal, containerFK); + else + Table.delete(_tinfo, pkVal); + } + else //Hmm, throw an exception here???? + _log.warn("Nothing to delete for table " + _tinfo.getName() + " on request " + _request.getRequestURI()); + } } } + // Deletes a single row only if it lives in the form's container, scoping the DELETE's WHERE clause to the PK and the + // container together. 404s on a miss (cross-container or already gone), mirroring doUpdate(). + private void deleteInContainer(Object pkVal, FieldKey containerFK) + { + SimpleFilter filter = new PkFilter(_tinfo, pkVal); + filter.addCondition(containerFK, _c.getId()); + if (Table.delete(_tinfo, filter) == 0) + throw new NotFoundException("No matching row found in this folder"); + } + /** * Pulls in the data from the current row of the database. */ diff --git a/api/src/org/labkey/api/security/permissions/AbstractContainerScopingTest.java b/api/src/org/labkey/api/security/permissions/AbstractContainerScopingTest.java index bb5db4d34a6..615856dbb9f 100644 --- a/api/src/org/labkey/api/security/permissions/AbstractContainerScopingTest.java +++ b/api/src/org/labkey/api/security/permissions/AbstractContainerScopingTest.java @@ -74,7 +74,17 @@ protected User getAdmin() protected Container createContainer(String name) { Container junit = JunitUtil.getTestContainer(); - Container c = ContainerManager.ensureContainer(junit.getParsedPath().append(getClass().getSimpleName() + "-" + name, true), getAdmin()); + // Use the fully-qualified class name, not getSimpleName(): the nested test class is named + // "ContainerScopingTestCase" in nearly every controller, so getSimpleName() would give them all the SAME + // /_junit child path and they would share fixtures (and collide on unique constraints across runs). Sanitize + // to a valid folder name, and force-delete any fixture an interrupted prior run left behind so each run starts + // from a clean container even when a previous @After could not complete. + String prefix = getClass().getName().replaceAll("[^A-Za-z0-9]", "_"); + var path = junit.getParsedPath().append(prefix + "-" + name, true); + Container existing = ContainerManager.getForPath(path); + if (existing != null) + ContainerManager.deleteAll(existing, getAdmin()); + Container c = ContainerManager.ensureContainer(path, getAdmin()); _containers.add(c); return c; } diff --git a/assay/src/org/labkey/assay/AssayController.java b/assay/src/org/labkey/assay/AssayController.java index 507630bae50..734c442267a 100644 --- a/assay/src/org/labkey/assay/AssayController.java +++ b/assay/src/org/labkey/assay/AssayController.java @@ -160,11 +160,31 @@ import org.springframework.validation.Errors; import org.springframework.web.servlet.ModelAndView; import org.springframework.web.servlet.mvc.Controller; +import org.springframework.mock.web.MockMultipartHttpServletRequest; +import jakarta.servlet.http.HttpSession; +import org.junit.After; +import org.junit.Test; +import org.labkey.api.assay.AssayDataCollector; +import org.labkey.api.assay.AssayRunCreator; +import org.labkey.api.assay.PipelineDataCollector; +import org.labkey.api.exp.api.ExpExperiment; +import org.labkey.api.gwt.client.assay.model.GWTProtocol; +import org.labkey.api.gwt.client.model.GWTDomain; +import org.labkey.api.gwt.client.model.GWTPropertyDescriptor; +import org.labkey.api.security.permissions.AbstractContainerScopingTest; +import org.labkey.api.security.roles.EditorRole; +import org.labkey.api.security.roles.ReaderRole; +import org.labkey.api.util.JunitUtil; +import org.labkey.api.util.TestContext; +import org.labkey.api.view.ViewBackgroundInfo; +import org.labkey.api.view.ViewContext; import java.io.File; import java.io.FileInputStream; import java.io.IOException; import java.io.InputStream; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -1419,6 +1439,24 @@ public ApiResponse execute(SetResultFlagForm form, BindException errors) if (!org.labkey.api.gwt.client.ui.PropertyType.expFlag.getURI().equals(flagCol.getConceptURI())) throw new NotFoundException(); + // Confirm every requested result row actually belongs to the request container by reading its run's container + Set requestedIds = new HashSet<>(form.getRowList()); + if (!requestedIds.isEmpty()) + { + SimpleFilter idFilter = new SimpleFilter(); + idFilter.addInClause(FieldKey.fromParts("RowId"), requestedIds); + Map[] rows = new TableSelector(assayResultTable, PageFlowUtil.set("RowId", "Folder"), idFilter, null).getMapArray(); + Set allowedIds = new HashSet<>(); + String currentContainerId = getContainer().getId(); + for (Map row : rows) + { + if (currentContainerId.equals(String.valueOf(row.get("Folder"))) && row.get("RowId") instanceof Number n) + allowedIds.add(n.intValue()); + } + if (!allowedIds.containsAll(requestedIds)) + throw new NotFoundException("One or more result rows were not found in this folder"); + } + DbScope scope = ti.getSchema().getScope(); int rowsAffected = 0 ; try (DbScope.Transaction transaction = scope.ensureTransaction()) @@ -1605,6 +1643,13 @@ public ApiSimpleResponse execute(UpdateQCStateForm form, BindException errors) t ApiSimpleResponse response = new ApiSimpleResponse(); if (form.getRuns() != null && _firstRun != null) { + for (int id : form.getRuns()) + { + ExpRun run = ExperimentService.get().getExpRun(id); + if (run == null || !run.getContainer().hasPermission(getUser(), QCAnalystPermission.class)) + throw new NotFoundException("Run " + id + " not found in this folder"); + } + DataState state = DataStateManager.getInstance().getStateForRowId(_firstRun.getProtocol().getContainer(), form.getState()); if (state != null) AssayQCService.getProvider().setQCStates(_firstRun.getProtocol(), getContainer(), getUser(), List.copyOf(form.getRuns()), state, form.getComment()); @@ -1957,4 +2002,128 @@ public Object execute(FilterCriteriaColumnsForm form, BindException errors) thro return AssayPlateMetadataService.get().previewFilterCriteriaColumns(protocol, form.getColumnNames()); } } + + /** + * Verifies that assay actions resolving result rows by global RowId reject rows that belong to a different + * container, even when the caller has the action's required permission in the request folder. The design is created + * in /Shared so its protocol resolves from any sibling folder (project + shared scope, per ProtocolIdForm), while + * the result rows live in a folder where the caller has no rights. + */ + public static class ContainerScopingTestCase extends AbstractContainerScopingTest + { + private ExpProtocol _sharedProtocol; // lives in /Shared; not auto-cleaned, so we delete it in @After + + @After + public void deleteSharedAssay() + { + if (_sharedProtocol != null) + { + try { _sharedProtocol.delete(getAdmin()); } catch (Exception ignored) {} + _sharedProtocol = null; + } + } + + // Build a General (GPAT) design in /Shared so its protocol resolves from any sibling folder, with a results + // "Flag" property (conceptURI expFlag) that SetResultFlagAction requires plus a plain int result field so a + // one-row file can be imported. + private Pair createSharedAssay() throws Exception + { + Container shared = ContainerManager.getSharedContainer(); + ViewContext ctx = new ViewContext(new ViewBackgroundInfo(shared, getAdmin(), null)); + AssayDomainServiceImpl svc = new AssayDomainServiceImpl(ctx); + GWTProtocol tmpl = svc.getAssayTemplate("General"); + tmpl.setName("scoping-assay-" + getClass().getSimpleName()); + tmpl.setEditableResults(true); + + for (GWTDomain d : tmpl.getDomains()) + { + if ("Batch Fields".equals(d.getName()) || "Run Fields".equals(d.getName())) + d.getFields(true).clear(); + else if ("Data Fields".equals(d.getName())) + { + d.getFields(true).clear(); + d.getFields(true).add(new GWTPropertyDescriptor("result", "int")); + GWTPropertyDescriptor flag = new GWTPropertyDescriptor("Flag", "string"); + // SetResultFlagAction requires the target column to carry the expFlag conceptURI + flag.setConceptURI(org.labkey.api.gwt.client.ui.PropertyType.expFlag.getURI()); + d.getFields(true).add(flag); + } + } + + GWTProtocol saved = svc.saveChanges(tmpl, true); + ExpProtocol protocol = ExperimentService.get().getExpProtocol(saved.getProtocolId()); + _sharedProtocol = protocol; + return Pair.of(AssayService.get().getProvider(protocol), protocol); + } + + // Import a one-row run into container c (as the site admin) and return its assay-result RowId. + private int importOneResult(Container c, AssayProvider provider, ExpProtocol protocol) throws Exception + { + var pipeRoot = PipelineService.get().findPipelineRoot(c); + assertNotNull("Test requires a pipeline root for " + c.getPath(), pipeRoot); + File file = FileUtil.createTempFile(getClass().getSimpleName(), ".tsv", pipeRoot.getRootPath()); + Files.writeString(file.toPath(), "result\n100\n", StandardCharsets.UTF_8); + + HttpSession session = TestContext.get().getRequest().getSession(); + PipelineDataCollector.setFileCollection(session, c, protocol, List.of(Map.of(AssayDataCollector.PRIMARY_FILE, file))); + + MockMultipartHttpServletRequest req = new MockMultipartHttpServletRequest(); + req.setUserPrincipal(getAdmin()); + req.setSession(session); + AssayRunUploadForm uploadForm = new AssayRunUploadForm<>(); + uploadForm.setViewContext(new ViewContext(req, null, null)); + uploadForm.setContainer(c); + uploadForm.setUser(getAdmin()); + uploadForm.setRowId(protocol.getRowId()); + uploadForm.setName("scoping-run-" + c.getName()); + uploadForm.setDataCollectorName("Pipeline"); + + AssayRunCreator runCreator = provider.getRunCreator(); + Pair pair = runCreator.saveExperimentRun(uploadForm, null); + ExpRun run = pair.second; + + AssayProtocolSchema schema = provider.createProtocolSchema(getAdmin(), c, protocol, null); + TableInfo results = schema.getTable("Data"); + FieldKey runRowIdFk = new FieldKey(FieldKey.fromParts("Run"), "RowId"); + Map row = new TableSelector(results, Set.of("RowId"), new SimpleFilter(runRowIdFk, run.getRowId()), null).getMap(); + return (Integer) row.get("RowId"); + } + + @Test + public void testSetResultFlagRejectsForeignResultRow() throws Exception + { + Container shared = ContainerManager.getSharedContainer(); + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + Pair assay = createSharedAssay(); + AssayProvider provider = assay.first; + ExpProtocol protocol = assay.second; + + int resultRowId = importOneResult(folderB, provider, protocol); // result row lives in folder B + + // Caller can Update folder A and Read the shared design, but has NO rights in folder B + User editorA = createUserInRole(folderA, EditorRole.class); + grantRole(editorA, shared, ReaderRole.class); + + // Flagging B's result row through folder A must 404 at the per-row Folder guard (not at getProtocol). + ActionURL foreignUrl = new ActionURL(SetResultFlagAction.class, folderA) + .addParameter("rowId", String.valueOf(protocol.getRowId())) + .addParameter("providerName", provider.getName()) + .addParameter("resultRowIds", String.valueOf(resultRowId)) + .addParameter("columnName", "Flag") + .addParameter("comment", "hacked"); + assertStatus(HttpServletResponse.SC_NOT_FOUND, post(foreignUrl, editorA)); + + // Positive control: an Editor in B (with Read on the shared design) flags the same row through folder B → 200. + User editorB = createUserInRole(folderB, EditorRole.class); + grantRole(editorB, shared, ReaderRole.class); + ActionURL ownUrl = new ActionURL(SetResultFlagAction.class, folderB) + .addParameter("rowId", String.valueOf(protocol.getRowId())) + .addParameter("providerName", provider.getName()) + .addParameter("resultRowIds", String.valueOf(resultRowId)) + .addParameter("columnName", "Flag") + .addParameter("comment", "ok"); + assertStatus(HttpServletResponse.SC_OK, post(ownUrl, editorB)); + } + } } diff --git a/assay/src/org/labkey/assay/AssayModule.java b/assay/src/org/labkey/assay/AssayModule.java index e03b6e1d23d..5978d7c0d5d 100644 --- a/assay/src/org/labkey/assay/AssayModule.java +++ b/assay/src/org/labkey/assay/AssayModule.java @@ -317,6 +317,7 @@ public Set getProvisionedSchemaNames() { return Set.of( ModuleAssayCache.TestCase.class, + AssayController.ContainerScopingTestCase.class, PlateManagerTest.class, PlateSchemaTest.class ); diff --git a/assay/src/org/labkey/assay/plate/PlateManager.java b/assay/src/org/labkey/assay/plate/PlateManager.java index c62458a3b65..d9c320787ed 100644 --- a/assay/src/org/labkey/assay/plate/PlateManager.java +++ b/assay/src/org/labkey/assay/plate/PlateManager.java @@ -3224,6 +3224,17 @@ public void markHits( } else { + // Verify the caller has UpdatePermission on the container of every hit row being removed + SimpleFilter hitFilter = new SimpleFilter(FieldKey.fromParts("ResultId"), rowIds, CompareType.IN); + hitFilter.addCondition(FieldKey.fromParts("ProtocolId"), protocol.getRowId()); + Set hitContainerIds = new HashSet<>(new TableSelector(hitTable, Collections.singleton("Container"), hitFilter, null).getArrayList(String.class)); + for (String hitContainerId : hitContainerIds) + { + Container hitContainer = ContainerManager.getForId(hitContainerId); + if (hitContainer == null || !hitContainer.hasPermission(user, UpdatePermission.class)) + throw new UnauthorizedException("Failed to unmark hits. You do not have permissions to update hits in this folder."); + } + deleteHits(protocolId, rowIds); } diff --git a/core/src/org/labkey/core/CoreModule.java b/core/src/org/labkey/core/CoreModule.java index bd342ae5afa..f935f0c57c6 100644 --- a/core/src/org/labkey/core/CoreModule.java +++ b/core/src/org/labkey/core/CoreModule.java @@ -1395,6 +1395,7 @@ public Set getIntegrationTests() AdminController.TestCase.class, AdminController.WorkbookDeleteTestCase.class, AdminController.ImportFolderSourceScopingTestCase.class, + AdminController.RevertFolderScopingTestCase.class, AllowListType.TestCase.class, AttachmentServiceImpl.TestCase.class, CoreController.TestCase.class, diff --git a/core/src/org/labkey/core/admin/AdminController.java b/core/src/org/labkey/core/admin/AdminController.java index 8868dcbf947..69cc3884501 100644 --- a/core/src/org/labkey/core/admin/AdminController.java +++ b/core/src/org/labkey/core/admin/AdminController.java @@ -8003,6 +8003,8 @@ public boolean handlePost(SetFolderPermissionsForm form, BindException errors) { throw new NotFoundException("An unknown project was specified to copy permissions from: " + targetProject); } + if (!source.hasPermission(getUser(), AdminPermission.class)) + throw new UnauthorizedException("You do not have permission to copy permissions from the specified project."); Map groupMap = GroupManager.copyGroupsToContainer(source, c, getUser()); //copy role assignments @@ -8425,6 +8427,9 @@ public ApiResponse execute(RevertFolderForm form, BindException errors) Container revertContainer = ContainerManager.getForPath(form.getContainerPath()); if (null != revertContainer) { + if (!revertContainer.hasPermission(getUser(), AdminPermission.class)) + throw new UnauthorizedException(); + if (revertContainer.isContainerTab()) { FolderTab tab = revertContainer.getParent().getFolderType().findTab(revertContainer.getName()); @@ -10205,6 +10210,10 @@ public ApiResponse execute(DeletedFoldersForm form, BindException errors) if (isBlank(form.getContainerPath())) throw new NotFoundException(); Container container = ContainerManager.getForPath(form.getContainerPath()); + if (container == null) + throw new NotFoundException(); + if (!container.hasPermission(getUser(), AdminPermission.class)) + throw new UnauthorizedException(); for (String tabName : form.getResurrectFolders()) { ContainerManager.clearContainerTabDeleted(container, tabName, form.getNewFolderType()); @@ -12321,4 +12330,65 @@ public void testImportFromTemplateRequiresSourceAdmin() throws Exception // Positive control performed in S3ImportTest.testS3Import(). Difficult to mock here due to pipeline job } } + + public static class RevertFolderScopingTestCase extends AbstractContainerScopingTest + { + @Test + public void testRevertFolderRequiresTargetAdmin() throws Exception + { + User admin = getAdmin(); + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + + User adminA = createUserInRole(folderA, FolderAdminRole.class); + + ActionURL foreignUrl = new ActionURL(RevertFolderAction.class, folderA) + .addParameter("containerPath", folderB.getPath()); + + // Cross-container attempt by a caller who is not an admin of the target -> 403, before any mutation. + assertStatus(HttpServletResponse.SC_FORBIDDEN, post(foreignUrl, adminA)); + + // Positive control: a site admin (who IS an admin of the target) is allowed through even cross-container, + // proving the fix re-checks AdminPermission on the target rather than locking to the request container. + // folderB is a plain folder with no container tabs, so the action makes no change and returns success:false + // at status 200 -- the point is that the guard does not reject an authorized caller. + assertStatus(HttpServletResponse.SC_OK, post(foreignUrl, admin)); + } + + @Test + public void testClearDeletedTabFoldersRequiresTargetAdmin() throws Exception + { + User admin = getAdmin(); + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + User adminA = createUserInRole(folderA, FolderAdminRole.class); + + ActionURL foreignUrl = new ActionURL(ClearDeletedTabFoldersAction.class, folderA) + .addParameter("containerPath", folderB.getPath()) + .addParameter("resurrectFolders", "anyTab"); + + // A folder admin in A only must not clear deleted-tab markers in folder B (resolved by containerPath) -> 403. + assertStatus(HttpServletResponse.SC_FORBIDDEN, post(foreignUrl, adminA)); + + // Positive control: a site admin (admin of the target) is allowed through -> 200. + assertStatus(HttpServletResponse.SC_OK, post(foreignUrl, admin)); + } + + @Test + public void testSetFolderPermissionsCopyRequiresSourceAdmin() throws Exception + { + Container dest = createContainer("Dest"); + Container source = createContainer("Source"); + User destAdmin = createUserInRole(dest, FolderAdminRole.class); + + // Copying groups/role assignments from a project the caller does not administer must be rejected. The + // action's @RequiresPermission(AdminPermission.class) only proves admin on the destination container, so a + // dest-only admin supplying another project's id as targetProject must get 403, not a copy of its security + // configuration. (Positive control omitted: a successful copy needs real project group/policy fixtures.) + ActionURL url = new ActionURL(SetFolderPermissionsAction.class, dest) + .addParameter("permissionType", "CopyExistingProject") + .addParameter("targetProject", source.getId()); + assertStatus(HttpServletResponse.SC_FORBIDDEN, post(url, destAdmin)); + } + } } diff --git a/core/src/org/labkey/core/admin/miniprofiler/MiniProfilerController.java b/core/src/org/labkey/core/admin/miniprofiler/MiniProfilerController.java index 75eae655108..05fa70c112b 100644 --- a/core/src/org/labkey/core/admin/miniprofiler/MiniProfilerController.java +++ b/core/src/org/labkey/core/admin/miniprofiler/MiniProfilerController.java @@ -243,17 +243,17 @@ public Object execute(RequestForm form, BindException errors) throw new UnauthorizedException(); RequestInfo req = MemTracker.getInstance().getRequest(form.getId()); + if (req != null && !getUser().equals(req.getUser()) && !getUser().hasApplicationAdminPermission()) + { + throw new UnauthorizedException(); + } + MemTracker.get().setViewed(getUser(), form.getId()); // Reset the X-MiniProfiler-Ids header to only include remaining unviewed (without the id we are returning) LinkedHashSet ids = new LinkedHashSet<>(MemTracker.get().getUnviewed(getUser())); getViewContext().getResponse().setHeader("X-MiniProfiler-Ids", ids.toString()); - if (req != null && !getUser().equals(req.getUser()) && !getUser().hasApplicationAdminPermission()) - { - throw new UnauthorizedException(); - } - return req; } } diff --git a/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java b/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java index cc2ffd6445f..a66ba02384b 100644 --- a/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java +++ b/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java @@ -150,6 +150,7 @@ import org.labkey.api.exp.xar.LsidUtils; import org.labkey.api.files.FileContentService; import org.labkey.api.gwt.client.AuditBehaviorType; +import org.labkey.api.gwt.client.model.GWTPropertyDescriptor; import org.labkey.api.inventory.InventoryService; import org.labkey.api.module.ModuleHtmlView; import org.labkey.api.module.ModuleLoader; @@ -201,7 +202,10 @@ 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.EditorRole; +import org.labkey.api.security.roles.FolderAdminRole; import org.labkey.api.security.roles.ReaderRole; +import org.labkey.api.security.roles.Role; import org.labkey.api.settings.AppProps; import org.labkey.api.settings.ConceptURIProperties; import org.labkey.api.sql.LabKeySql; @@ -3752,6 +3756,10 @@ protected void deleteObjects(DeleteForm form) { for (ExpProtocol protocol : getProtocolsForDeletion(form)) { + // Re-check here - cannot delete a run-less assay design owned by another container via a forged rowId. + if (!protocol.getContainer().hasPermission(getUser(), DesignAssayPermission.class)) + throw new UnauthorizedException("You do not have sufficient permissions to delete this assay design."); + protocol.delete(getUser(), form.getUserComment()); } } @@ -4713,6 +4721,12 @@ public void validateCommand(ExperimentForm target, Errors errors) @Override public boolean handlePost(ExperimentForm form, BindException errors) throws Exception { + // Confirm the run group actually lives in this container before updating, like the GET sibling ShowUpdateAction. + Experiment bean = form.getBean(); + ExpExperiment exp = bean == null ? null : ExperimentService.get().getExpExperiment(bean.getRowId()); + if (exp == null || !getContainer().equals(exp.getContainer())) + throw new NotFoundException("Run group not found in this folder"); + form.doUpdate(); form.refreshFromDb(); _exp = form.getBean(); @@ -5279,7 +5293,19 @@ public void validateCommand(ExperimentRunListForm target, Errors errors) @Override public boolean handlePost(ExperimentRunListForm form, BindException errors) { - addSelectedRunsToExperiment(form.lookupExperiment(), form.getDataRegionSelectionKey()); + ExpExperiment exp = form.lookupExperiment(); + if (exp == null || !exp.getContainer().hasPermission(getUser(), InsertPermission.class)) + throw new NotFoundException("Could not find run group with RowId " + form.getExpRowId()); + + List runs = new ArrayList<>(); + for (int runId : DataRegionSelection.getSelectedIntegers(getViewContext(), form.getDataRegionSelectionKey(), true)) + { + ExpRun run = ExperimentService.get().getExpRun(runId); + if (run == null || !run.getContainer().hasPermission(getUser(), InsertPermission.class)) + throw new NotFoundException("Could not find run with RowId " + runId); + runs.add(run); + } + exp.addRuns(getUser(), runs.toArray(new ExpRun[0])); return true; } @@ -6020,9 +6046,9 @@ else if (in.rowId > 0) errors.reject(ERROR_MSG, "Can't resolve sample '" + in.rowId + "'"); } - if (m == null) + if (m == null || !m.getContainer().hasPermission(getUser(), ReadPermission.class)) { - errors.reject(ERROR_MSG, "Material input lsid or rowId required"); + errors.reject(ERROR_MSG, "Material input couldn't be resolved"); continue; } @@ -6062,9 +6088,9 @@ else if (in.rowId > 0) errors.reject(ERROR_MSG, "Can't resolve data '" + in.rowId + "'"); } - if (d == null) + if (d == null || !d.getContainer().hasPermission(getUser(), ReadPermission.class)) { - errors.reject(ERROR_MSG, "Data input lsid or rowId required"); + errors.reject(ERROR_MSG, "Data input couldn't be resolved"); continue; } @@ -6087,9 +6113,8 @@ else if (in.rowId > 0) ExpSampleType outSampleType; if (form.targetSampleType != null) { - // TODO: check in scope and has permission outSampleType = SampleTypeService.get().getSampleType(form.targetSampleType.toString()); - if (outSampleType == null) + if (outSampleType == null || !outSampleType.getContainer().hasPermission(getUser(), ReadPermission.class)) errors.reject(ERROR_MSG, "Sample type not found: " + form.targetSampleType.toString()); } else @@ -6100,9 +6125,8 @@ else if (in.rowId > 0) ExpDataClass outDataClass; if (form.targetDataClass != null) { - // TODO: check in scope and has permission outDataClass = ExperimentServiceImpl.get().getDataClass(form.targetDataClass.toString()); - if (outDataClass == null) + if (outDataClass == null || !outDataClass.getContainer().hasPermission(getUser(), ReadPermission.class)) errors.reject(ERROR_MSG, "DataClass not found: " + form.targetDataClass.toString()); } else @@ -6583,10 +6607,11 @@ public boolean handlePost(MoveRunsForm form, BindException errors) for (Integer runId : runIds) { ExpRun run = ExperimentService.get().getExpRun(runId); - if (run != null) + if (run == null || !run.getContainer().equals(getContainer())) { - runs.add(run); + throw new NotFoundException("Could not find run with RowId " + runId + " in this folder"); } + runs.add(run); } ViewBackgroundInfo info = getViewBackgroundInfo(); @@ -8014,6 +8039,9 @@ public Object execute(EntitySequenceForm form, BindException errors) ExpSampleType sampleType = SampleTypeService.get().getSampleType(form.getRowId()); if (sampleType != null) { + if (!sampleType.getContainer().hasPermission(getUser(), DesignSampleTypePermission.class)) + throw new UnauthorizedException("Insufficient permissions."); + sampleType.ensureMinGenId(form.getNewValue()); domain = sampleType.getDomain(); } @@ -8388,6 +8416,335 @@ public void testDataClassAttachmentContainerScoping() throws Exception .addParameter("name", attachmentName); assertStatus(HttpServletResponse.SC_OK, get(ownUrl, admin)); } + + @Test + public void testUpdateRunGroupContainerScoping() throws Exception + { + User admin = getAdmin(); + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + + // A run group (Experiment) that lives in folder B + ExpExperiment runGroup = ExperimentService.get().createExpExperiment(folderB, "scoping-test-run-group"); + runGroup.save(admin); + int rowId = runGroup.getRowId(); + + // A caller who can Update folder A (Editor) but has no rights in folder B + User editorA = createUserInRole(folderA, EditorRole.class); + + // Updating B's run group through folder A must 404 rather than overwrite/re-home it. The action's + // @RequiresPermission(UpdatePermission.class) passes in folder A, so without the handlePost guard the + // unscoped doUpdate() would edit B's row and rewrite its container to A. + ActionURL foreignUrl = new ActionURL(UpdateAction.class, folderA) + .addParameter("RowId", String.valueOf(rowId)) + .addParameter("Name", "hacked"); + assertStatus(HttpServletResponse.SC_NOT_FOUND, post(foreignUrl, editorA)); + // A site admin, who CAN update folder B, still gets 404 through folder A (no cross-container write). + assertStatus(HttpServletResponse.SC_NOT_FOUND, post(foreignUrl, admin)); + + // The run group must be untouched in its own container + ExpExperiment after = ExperimentService.get().getExpExperiment(rowId); + assertNotNull("Run group must still exist", after); + assertEquals("Name must be unchanged after a cross-container update", "scoping-test-run-group", after.getName()); + assertEquals("Container must be unchanged after a cross-container update", folderB, after.getContainer()); + + // Positive control: updating through its own container succeeds (302 redirect to the success URL) and applies. + ActionURL ownUrl = new ActionURL(UpdateAction.class, folderB) + .addParameter("RowId", String.valueOf(rowId)) + .addParameter("Name", "renamed"); + assertStatus(HttpServletResponse.SC_FOUND, post(ownUrl, admin)); + assertEquals("Name should be updated by a same-container request", "renamed", + ExperimentService.get().getExpExperiment(rowId).getName()); + } + + @Test + public void testAddRunsToExperimentContainerScoping() throws Exception + { + User admin = getAdmin(); + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + + // A run group (Experiment) that lives in folder B + ExpExperiment runGroup = ExperimentService.get().createExpExperiment(folderB, "scoping-test-add-runs"); + runGroup.save(admin); + int expRowId = runGroup.getRowId(); + + // A caller who can Insert in folder A but has no rights in folder B + User editorA = createUserInRole(folderA, EditorRole.class); + + // Adding runs to B's run group through folder A must 404: the run group is resolved by a global RowId and + // ExpExperimentImpl.addRuns does a raw INSERT with no authorization, so without the handlePost guard a + // forged expRowId would let a folder-A user mutate a foreign run group. + ActionURL foreignUrl = new ActionURL(AddRunsToExperimentAction.class, folderA) + .addParameter("expRowId", String.valueOf(expRowId)); + assertStatus(HttpServletResponse.SC_NOT_FOUND, post(foreignUrl, editorA)); + + // Positive control: addressing the run group through its own container passes the guard. No runs are + // selected, so the action makes no change and redirects to the group's details page (302). + ActionURL ownUrl = new ActionURL(AddRunsToExperimentAction.class, folderB) + .addParameter("expRowId", String.valueOf(expRowId)); + assertStatus(HttpServletResponse.SC_FOUND, post(ownUrl, admin)); + } + + @Test + public void testMoveRunsContainerScoping() throws Exception + { + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + + // A run that lives in folder B + ExpRun run = createRun(folderB, "scoping-test-move-run"); + int runId = run.getRowId(); + + // A caller with Insert+Delete in folder A (Editor) but no rights in folder B + User editorA = createUserInRole(folderA, EditorRole.class); + + // MoveRuns is scoped to getContainer() as the source and only checks Insert on the target; runs are resolved + // from the client-supplied selection by global RowId. Moving B's run via folder A (as both source and + // target) must 404 because the run does not live in the source container the caller is operating in. + ActionURL foreignUrl = new ActionURL(MoveRunsAction.class, folderA) + .addParameter("targetContainerId", folderA.getId()) + .addParameter(DataRegion.SELECT_CHECKBOX_NAME, String.valueOf(runId)); + assertStatus(HttpServletResponse.SC_NOT_FOUND, post(foreignUrl, editorA)); + + // The run must remain in folder B + ExpRun after = ExperimentService.get().getExpRun(runId); + assertNotNull("Run must still exist", after); + assertEquals("Run must not have been moved out of its container", folderB, after.getContainer()); + + // Positive control: a successful move queues a MoveRunsPipelineJob, which is exercised by existing run-move + // tests; this case verifies only that the cross-container request is rejected before any job is queued. + } + + @Test + public void testDeleteProtocolByRowIdsContainerScoping() throws Exception + { + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + + // A run-less protocol (assay design) that lives in folder B. Run-less so deleteProtocolByRowIds skips its + // AdminPermission check, leaving the per-protocol DesignAssay guard in deleteObjects as the only gate. + ExpProtocol protocol = ExperimentService.get().createExpProtocol(folderB, ExpProtocol.ApplicationType.ExperimentRun, "scoping-test-protocol"); + protocol.save(getAdmin()); + int rowId = protocol.getRowId(); + + // A caller who can design assays in folder A only. DesignAssayPermission's role lives in the assay module, + // resolved by name here to avoid a compile-time dependency from the experiment module. + @SuppressWarnings("unchecked") + Class assayDesigner = (Class) Class.forName("org.labkey.assay.security.AssayDesignerRole"); + User designerA = createUserInRole(folderA, assayDesigner); + + // Force-deleting B's protocol through folder A must be rejected. The forceDelete POST path runs handlePost + // -> deleteObjects directly, bypassing the getView container check, so the deleteObjects guard is what stops + // it (403 for an authenticated caller lacking DesignAssay on the protocol's own container). + ActionURL foreignUrl = new ActionURL(DeleteProtocolByRowIdsAction.class, folderA) + .addParameter("forceDelete", "true") + .addParameter("singleObjectRowId", String.valueOf(rowId)); + assertStatus(HttpServletResponse.SC_FORBIDDEN, post(foreignUrl, designerA)); + + // The protocol must still exist in folder B + assertNotNull("Protocol must not have been deleted cross-container", ExperimentService.get().getExpProtocol(rowId)); + + // Positive control: once the caller is granted design rights in folder B, the same forceDelete through + // folder B succeeds (302) and removes the protocol -- proving the guard rejects only the cross-container case. + grantRole(designerA, folderB, assayDesigner); + ActionURL ownUrl = new ActionURL(DeleteProtocolByRowIdsAction.class, folderB) + .addParameter("forceDelete", "true") + .addParameter("singleObjectRowId", String.valueOf(rowId)); + assertStatus(HttpServletResponse.SC_FOUND, post(ownUrl, designerA)); + assertNull("Protocol should be deleted by a same-container request", ExperimentService.get().getExpProtocol(rowId)); + } + + @Test + public void testSetEntitySequenceContainerScoping() throws Exception + { + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + + // A sample type that lives in folder B + List props = List.of(new GWTPropertyDescriptor("name", "string")); + ExpSampleType sampleType = SampleTypeService.get().createSampleType(folderB, getAdmin(), "scoping-test-st", null, + props, Collections.emptyList(), -1, -1, -1, -1, null, null); + int rowId = sampleType.getRowId(); + + // A folder admin in A only (has DesignSampleType in A via FolderAdminRole, no rights in B) + User adminA = createUserInRole(folderA, FolderAdminRole.class); + + // Advancing the genId of B's sample type through folder A must 403. The request-container DesignSampleType + // check passes in A, but ensureMinGenId operates on the type's OWN container, so the per-object check rejects it. + ActionURL foreignUrl = new ActionURL(SetEntitySequenceAction.class, folderA) + .addParameter("kindName", SampleTypeDomainKind.NAME) + .addParameter("seqType", NameGenerator.EntityCounter.genId.name()) + .addParameter("rowId", String.valueOf(rowId)) + .addParameter("newValue", "100"); + assertStatus(HttpServletResponse.SC_FORBIDDEN, post(foreignUrl, adminA)); + + // Positive control: a folder admin in B can advance the sequence through folder B (success, 200). + User adminB = createUserInRole(folderB, FolderAdminRole.class); + ActionURL ownUrl = new ActionURL(SetEntitySequenceAction.class, folderB) + .addParameter("kindName", SampleTypeDomainKind.NAME) + .addParameter("seqType", NameGenerator.EntityCounter.genId.name()) + .addParameter("rowId", String.valueOf(rowId)) + .addParameter("newValue", "100"); + assertStatus(HttpServletResponse.SC_OK, post(ownUrl, adminB)); + } + + @Test + public void testDeriveActionMaterialContainerScoping() throws Exception + { + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + + // Editor in folder A only: holds the InsertPermission the action requires in A, but has no rights in folder B. + User editorA = createUserInRole(folderA, EditorRole.class); + + // A sample type with one sample in each folder. The editor can read its own folder A but not folder B. + ExpSampleType stA = createSampleType(folderA, "DeriveScopeStA"); + ExpSampleType stB = createSampleType(folderB, "DeriveScopeStB"); + ExpMaterial sampleA = createSample(folderA, stA, "srcA"); + ExpMaterial sampleB = createSample(folderB, stB, "srcB"); + + ActionURL url = new ActionURL(DeriveAction.class, folderA); + + // Negative (input): a material input resolved by global rowId that lives in folder B must not be usable as a + // derivation parent by a caller who cannot read B. Without the per-input Read check the foreign sample would + // be silently consumed (an IDOR). The output target is in folder A, so only the foreign input can be at fault. + JSONObject foreignInput = new JSONObject() + .put("materialInputs", new JSONArray().put(new JSONObject().put("rowId", sampleB.getRowId()))) + .put("targetSampleType", stA.getLSID()) + .put("materialOutputCount", 1); + MockHttpServletResponse resp = postJson(url, editorA, foreignInput); + assertStatus(HttpServletResponse.SC_BAD_REQUEST, resp); + assertTrue("Expected a material-input scope rejection, was: " + resp.getContentAsString(), + resp.getContentAsString().contains("Material input couldn't be resolved")); + + // Negative (target): deriving INTO a sample type the caller cannot read must be rejected as "not found", so + // the caller can't probe which foreign sample types exist by their LSID. + JSONObject foreignTarget = new JSONObject() + .put("targetSampleType", stB.getLSID()) + .put("materialOutputCount", 1); + resp = postJson(url, editorA, foreignTarget); + assertStatus(HttpServletResponse.SC_BAD_REQUEST, resp); + assertTrue("Expected a target sample type scope rejection, was: " + resp.getContentAsString(), + resp.getContentAsString().contains("Sample type not found")); + + // Positive control: the same request shape with the input and target both in folder A -- which the editor + // can read and insert into -- succeeds and actually derives a new sample, proving the checks reject only the + // cross-container case rather than every request. + JSONObject ok = new JSONObject() + .put("materialInputs", new JSONArray().put(new JSONObject().put("rowId", sampleA.getRowId()))) + .put("targetSampleType", stA.getLSID()) + .put("materialOutputs", new JSONArray().put(new JSONObject().put("values", new JSONObject().put("name", "derivedA")))); + resp = postJson(url, editorA, ok); + assertStatus(HttpServletResponse.SC_OK, resp); + assertTrue("Derivation should report success, was: " + resp.getContentAsString(), + new JSONObject(resp.getContentAsString()).getBoolean("success")); + assertNotNull("A new sample should have been derived in folder A", stA.getSample(folderA, "derivedA")); + } + + @Test + public void testDeriveActionDataContainerScoping() throws Exception + { + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + + // Editor in folder A only: holds the InsertPermission the action requires in A, but has no rights in folder B. + User editorA = createUserInRole(folderA, EditorRole.class); + + // A data class with one data object in each folder. The editor can read its own folder A but not folder B. + ExpDataClass dcA = createDataClass(folderA, "DeriveScopeDcA"); + ExpDataClass dcB = createDataClass(folderB, "DeriveScopeDcB"); + ExpData dataA = createData(folderA, dcA, "srcDataA"); + ExpData dataB = createData(folderB, dcB, "srcDataB"); + + ActionURL url = new ActionURL(DeriveAction.class, folderA); + + // Negative (input): a data input resolved by global rowId that lives in folder B must not be usable as a + // derivation parent by a caller who cannot read B. The Read check fires before the data-class membership + // check, so a foreign data object is rejected as unresolvable rather than silently consumed (an IDOR). + JSONObject foreignInput = new JSONObject() + .put("dataInputs", new JSONArray().put(new JSONObject().put("rowId", dataB.getRowId()))) + .put("targetDataClass", dcA.getLSID()) + .put("dataOutputCount", 1); + MockHttpServletResponse resp = postJson(url, editorA, foreignInput); + assertStatus(HttpServletResponse.SC_BAD_REQUEST, resp); + assertTrue("Expected a data-input scope rejection, was: " + resp.getContentAsString(), + resp.getContentAsString().contains("Data input couldn't be resolved")); + + // Negative (target): deriving INTO a data class the caller cannot read must be rejected as "not found", so + // the caller can't probe which foreign data classes exist by their LSID. + JSONObject foreignTarget = new JSONObject() + .put("targetDataClass", dcB.getLSID()) + .put("dataOutputCount", 1); + resp = postJson(url, editorA, foreignTarget); + assertStatus(HttpServletResponse.SC_BAD_REQUEST, resp); + assertTrue("Expected a target data class scope rejection, was: " + resp.getContentAsString(), + resp.getContentAsString().contains("DataClass not found")); + + // Positive control: the same request shape with the input and target both in folder A -- which the editor + // can read and insert into -- succeeds and actually derives a new data object, proving the checks reject + // only the cross-container case rather than every request. + JSONObject ok = new JSONObject() + .put("dataInputs", new JSONArray().put(new JSONObject().put("rowId", dataA.getRowId()))) + .put("targetDataClass", dcA.getLSID()) + .put("dataOutputs", new JSONArray().put(new JSONObject().put("values", new JSONObject().put("name", "derivedDataA")))); + resp = postJson(url, editorA, ok); + assertStatus(HttpServletResponse.SC_OK, resp); + assertTrue("Derivation should report success, was: " + resp.getContentAsString(), + new JSONObject(resp.getContentAsString()).getBoolean("success")); + assertNotNull("A new data object should have been derived in folder A", ExperimentService.get().getExpData(dcA, "derivedDataA")); + } + + // Create a sample type with a single string "name" property, mirroring the idiom in testSetEntitySequenceContainerScoping. + private ExpSampleType createSampleType(Container c, String name) throws Exception + { + List props = List.of(new GWTPropertyDescriptor("name", "string")); + return SampleTypeService.get().createSampleType(c, getAdmin(), name, null, props, Collections.emptyList(), -1, -1, -1, -1, null, null); + } + + // Create a saved sample in the given sample type, mirroring the sample-creation idiom in LineageTest. + private ExpMaterial createSample(Container c, ExpSampleType st, String name) throws Exception + { + ExpMaterial m = ExperimentService.get().createExpMaterial(c, st.generateSampleLSID().setObjectId(name).toString(), name); + m.setCpasType(st.getLSID()); + m.save(getAdmin()); + return m; + } + + // Create a data class with a single string "name" property, mirroring the idiom in LineageTest. + private ExpDataClass createDataClass(Container c, String name) throws Exception + { + List props = List.of(new GWTPropertyDescriptor("name", "string")); + return ExperimentServiceImpl.get().createDataClass(c, getAdmin(), name, null, props, Collections.emptyList(), null, null); + } + + // Insert a single named row into the data class and return the resulting ExpData. + private ExpData createData(Container c, ExpDataClass dc, String name) throws Exception + { + UserSchema dataSchema = new ExpSchema(getAdmin(), c).getUserSchema(ExpSchema.NestedSchemas.data.name()); + BatchValidationException errors = new BatchValidationException(); + dataSchema.getTable(dc.getName()).getUpdateService() + .insertRows(getAdmin(), c, List.of(CaseInsensitiveHashMap.of("name", name)), errors, null, null); + if (errors.hasErrors()) + throw errors; + return ExperimentService.get().getExpData(dc, name); + } + + // Dispatch a JSON POST to a @Marshal(Jackson) API action, with the form supplied as a JSON request body. + private MockHttpServletResponse postJson(ActionURL url, User user, JSONObject body) throws Exception + { + return ViewServlet.POST(url, user, Map.of("Content-Type", "application/json"), body.toString()); + } + + // Create a minimal saved experiment run in the given container, mirroring the run-creation idiom in LineageTest. + private ExpRun createRun(Container c, String name) throws Exception + { + ExpRun run = ExperimentService.get().createExperimentRun(c, name); + run.setFilePathRoot(PipelineService.get().findPipelineRoot(c).getRootPath()); + run.setProtocol(ExperimentService.get().ensureSampleDerivationProtocol(getAdmin())); + return ExperimentService.get().saveSimpleExperimentRun(run, Map.of(), Map.of(), Map.of(), Map.of(), Map.of(), + new ViewBackgroundInfo(c, getAdmin(), null), null, false); + } } } diff --git a/mothership/src/org/labkey/mothership/MothershipController.java b/mothership/src/org/labkey/mothership/MothershipController.java index 398dec07b60..9fa03a8975d 100644 --- a/mothership/src/org/labkey/mothership/MothershipController.java +++ b/mothership/src/org/labkey/mothership/MothershipController.java @@ -566,22 +566,6 @@ 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; } @@ -2032,6 +2016,43 @@ public void testUpdateInstallationContainerScoping() throws Exception 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()); } + + @Test + public void testUpdateStackTraceContainerScoping() throws Exception + { + User admin = getAdmin(); + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + + // An exception stack trace that lives in folder B (StackTraceHash is derived from the stack trace text) + ExceptionStackTrace st = new ExceptionStackTrace(); + st.setContainer(folderB.getId()); + st.setStackTrace("java.lang.NullPointerException\n\tat org.labkey.scoping.Test.run(Test.java:1)\n"); + st.setComments("original"); + st = Table.insert(admin, MothershipManager.get().getTableInfoExceptionStackTrace(), st); + int id = st.getExceptionStackTraceId(); + + // Updating it through folder A must 404 rather than overwrite/re-home it. doUpdate() keys Table.update on + // the id alone and rewrites the container, so without the handlePost guard a site admin (who CAN update + // folder B) would edit B's row through folder A and re-home it. + ActionURL url = new ActionURL(UpdateStackTraceAction.class, folderA) + .addParameter("ExceptionStackTraceId", String.valueOf(id)) + .addParameter("Comments", "hacked"); + assertStatus(HttpServletResponse.SC_NOT_FOUND, post(url, admin)); + + // The row in folder B must be untouched + ExceptionStackTrace reloaded = MothershipManager.get().getExceptionStackTrace(id, folderB); + assertNotNull("Stack trace should still exist in its own container", reloaded); + assertEquals("Comments must not have been overwritten", "original", reloaded.getComments()); + + // 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(UpdateStackTraceAction.class, folderB) + .addParameter("ExceptionStackTraceId", String.valueOf(id)) + .addParameter("Comments", "updated"); + assertStatus(HttpServletResponse.SC_FOUND, post(ownUrl, admin)); + assertEquals("Comments should have been updated through the row's own container", "updated", MothershipManager.get().getExceptionStackTrace(id, folderB).getComments()); + } } } diff --git a/pipeline/src/org/labkey/pipeline/PipelineController.java b/pipeline/src/org/labkey/pipeline/PipelineController.java index bb3f466f0c5..6a276bb9126 100644 --- a/pipeline/src/org/labkey/pipeline/PipelineController.java +++ b/pipeline/src/org/labkey/pipeline/PipelineController.java @@ -15,9 +15,11 @@ */ package org.labkey.pipeline; +import jakarta.servlet.http.HttpServletResponse; import org.apache.commons.lang3.StringUtils; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import org.junit.Test; import org.json.JSONArray; import org.json.JSONObject; import org.labkey.api.action.ApiJsonForm; @@ -46,6 +48,7 @@ import org.labkey.api.data.ContainerFilter; import org.labkey.api.data.ContainerManager; import org.labkey.api.data.SimpleFilter; +import org.labkey.api.data.Table; import org.labkey.api.data.TableSelector; import org.labkey.api.exp.property.DomainUtil; import org.labkey.api.files.FileContentService; @@ -64,6 +67,7 @@ import org.labkey.api.pipeline.PipelineStatusUrls; import org.labkey.api.pipeline.PipelineUrls; import org.labkey.api.pipeline.browse.PipelinePathForm; +import org.labkey.api.pipeline.file.FileAnalysisTaskPipeline; import org.labkey.api.pipeline.view.SetupForm; import org.labkey.api.query.FieldKey; import org.labkey.api.query.QueryUrls; @@ -77,11 +81,13 @@ import org.labkey.api.security.UserManager; import org.labkey.api.security.ValidEmail; 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.DeletePermission; import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.security.permissions.UserManagementPermission; +import org.labkey.api.security.roles.FolderAdminRole; import org.labkey.api.security.roles.Role; import org.labkey.api.security.roles.RoleManager; import org.labkey.api.settings.AdminConsole; @@ -89,6 +95,7 @@ import org.labkey.api.trigger.TriggerConfiguration; import org.labkey.api.util.DateUtil; import org.labkey.api.util.FileUtil; +import org.labkey.api.util.GUID; import org.labkey.api.util.HtmlStringBuilder; import org.labkey.api.util.LinkBuilder; import org.labkey.api.util.NetworkDrive; @@ -104,6 +111,7 @@ import org.labkey.api.view.JspView; import org.labkey.api.view.NavTree; import org.labkey.api.view.NotFoundException; +import org.labkey.api.view.ViewServlet; import org.labkey.api.view.UnauthorizedException; import org.labkey.api.view.VBox; import org.labkey.api.view.ViewContext; @@ -117,6 +125,7 @@ import org.labkey.pipeline.api.PipelineStatusManager; import org.labkey.pipeline.status.StatusController; import org.springframework.beans.MutablePropertyValues; +import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.validation.BindException; import org.springframework.validation.Errors; import org.springframework.web.servlet.ModelAndView; @@ -1551,6 +1560,15 @@ public static class SavePipelineTriggerAction extends MutatingApiAction getIntegrationTests() PipelineServiceImpl.TestCase.class, StatusController.TestCase.class, StatusController.ContainerScopingTestCase.class, + PipelineController.ContainerScopingTestCase.class, ClusterStartup.TestCase.class ); } diff --git a/pipeline/src/org/labkey/pipeline/api/PipelineStatusManager.java b/pipeline/src/org/labkey/pipeline/api/PipelineStatusManager.java index e0b34abe367..78dc459a7c0 100644 --- a/pipeline/src/org/labkey/pipeline/api/PipelineStatusManager.java +++ b/pipeline/src/org/labkey/pipeline/api/PipelineStatusManager.java @@ -690,10 +690,13 @@ public static void completeStatus(User user, Collection rowIds) if (!statusSet) { - // Fall back to updating the simple bean in the case where can can't deserialize the job itself + // Fall back to updating the simple bean in the case where we can't deserialize the job itself PipelineStatusFileImpl sf = PipelineStatusManager.getStatusFile(rowId); if (sf != null) { + Container c = sf.lookupContainer(); + if (c == null || !c.hasPermission(user, UpdatePermission.class)) + throw new UnauthorizedException(); LOG.info("Job " + sf.getFilePath() + " was marked as complete by " + user); sf.setStatus(PipelineJob.TaskStatus.complete.toString()); sf.setInfo(null); diff --git a/pipeline/src/org/labkey/pipeline/query/TriggerConfigurationsTable.java b/pipeline/src/org/labkey/pipeline/query/TriggerConfigurationsTable.java index 009a03b5061..d5b33d4ac72 100644 --- a/pipeline/src/org/labkey/pipeline/query/TriggerConfigurationsTable.java +++ b/pipeline/src/org/labkey/pipeline/query/TriggerConfigurationsTable.java @@ -320,7 +320,10 @@ protected Map deleteRow(User user, Container container, Map deleteRow = super.deleteRow(user, container, oldRowMap); // call the stop() method for this config if it was successfully deleted diff --git a/pipeline/src/org/labkey/pipeline/status/StatusController.java b/pipeline/src/org/labkey/pipeline/status/StatusController.java index 94965407a05..1e5050799a8 100644 --- a/pipeline/src/org/labkey/pipeline/status/StatusController.java +++ b/pipeline/src/org/labkey/pipeline/status/StatusController.java @@ -60,9 +60,11 @@ 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.EditorRole; import org.labkey.api.security.roles.ReaderRole; import org.labkey.api.settings.AdminConsole; import org.labkey.api.util.FileUtil; +import org.labkey.api.util.GUID; import org.labkey.api.util.HtmlString; import org.labkey.api.util.NetworkDrive; import org.labkey.api.util.PageFlowUtil; @@ -97,6 +99,7 @@ import java.nio.file.InvalidPathException; import java.nio.file.Path; import java.util.Date; +import java.util.List; import java.util.Set; import java.util.TreeSet; @@ -397,8 +400,12 @@ public ModelAndView getView(RowIdForm form, BindException errors) if (!getContainer().equals(_statusFile.lookupContainer())) { + Container target = _statusFile.lookupContainer(); + // Only redirect if the user can read the job's container; otherwise don't reveal that it exists + if (target == null || !target.hasPermission(getUser(), ReadPermission.class)) + throw new NotFoundException("Could not find status file for rowId " + form.getRowId()); ActionURL url = getViewContext().cloneActionURL(); - url.setContainer(_statusFile.lookupContainer()); + url.setContainer(target); throw new RedirectException(url); } @@ -494,15 +501,20 @@ public ActionURL getRedirectURL(RowIdForm form) Container c = getContainerCheckAdmin(); PipelineStatusFile sf = getStatusFile(form.getRowId()); + if (sf == null) throw new NotFoundException("Could not find status file for rowId " + form.getRowId()); + Container sfContainer = sf.lookupContainer(); + // Only navigate to the job's container if the user can read it + if (sfContainer == null || !sfContainer.hasPermission(getUser(), ReadPermission.class)) + throw new NotFoundException("Could not find status file for rowId " + form.getRowId()); if (sf.getDataUrl() != null) { throw new RedirectException(sf.getDataUrl()); } - return urlDetails(c, form.getRowId()); + return urlDetails(sfContainer, form.getRowId()); } } @@ -517,8 +529,12 @@ public ActionURL getRedirectURL(RowIdForm form) if (c == null || c.isRoot()) { PipelineStatusFileImpl sf = getStatusFile(form.getRowId()); - if (sf.getContainerId() != null) - c = ContainerManager.getForId(sf.getContainerId()); + if (sf == null) + throw new NotFoundException("Could not find status file for rowId " + form.getRowId()); + Container sfContainer = sf.lookupContainer(); + // Only navigate to the job's container if the user can read it; otherwise don't reveal it exists + if (sfContainer != null && sfContainer.hasPermission(getUser(), ReadPermission.class)) + c = sfContainer; } if (c != null) @@ -550,7 +566,8 @@ public void render(ShowFileForm form, BindException errors, PrintWriter out) thr String fileName; PipelineStatusFile sf = getStatusFile(form.getRowId()); - if (sf != null) + // Resolved by global rowId; only serve files for a job that belongs to the current container + if (sf != null && getContainer().equals(sf.lookupContainer())) { fileName = form.getFilename(); @@ -924,8 +941,12 @@ public boolean handlePost(RowIdForm form, BindException errors) for (Integer rowId : rowIds) { var sf = getStatusFile(rowId); + // Resolved by global rowId; reject a job that belongs to another container if (sf == null) throw new NotFoundException("Could not find status file for rowId " + form.getRowId()); + Container sfContainer = sf.lookupContainer(); + if (sfContainer == null || !sfContainer.hasPermission(getUser(), UpdatePermission.class)) + throw new NotFoundException("Could not find status file for rowId " + form.getRowId()); if (firstDetailsURL == null) firstDetailsURL = urlDetails(sf); @@ -1094,13 +1115,7 @@ public void testStatusDetailsContainerScoping() throws Exception 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); + PipelineStatusFileImpl sf = insertStatusFile(folderB, PipelineJob.TaskStatus.complete.toString()); long rowId = sf.getRowId(); ActionURL foreignUrl = new ActionURL(StatusDetailsAction.class, folderA).addParameter("rowId", String.valueOf(rowId)); @@ -1117,5 +1132,115 @@ public void testStatusDetailsContainerScoping() throws Exception ActionURL ownUrl = new ActionURL(StatusDetailsAction.class, folderB).addParameter("rowId", String.valueOf(rowId)); assertStatus(HttpServletResponse.SC_OK, get(ownUrl, admin)); } + + @Test + public void testRetryStatusContainerScoping() throws Exception + { + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + + // A job that lives in folder B + long rowId = insertStatusFile(folderB, PipelineJob.TaskStatus.error.toString()).getRowId(); + + // A caller who can Update folder A (Editor) but has no rights in folder B + User editorA = createUserInRole(folderA, EditorRole.class); + + // Retrying B's job through folder A must 404 rather than re-queue a job in a container the caller can't touch. + ActionURL foreignUrl = new ActionURL(RetryStatusAction.class, folderA).addParameter("rowId", String.valueOf(rowId)); + assertStatus(HttpServletResponse.SC_NOT_FOUND, post(foreignUrl, editorA)); + + // The job must be untouched in its own container + assertEquals("Job status must be unchanged after a cross-container retry", + PipelineJob.TaskStatus.error.toString(), getStatusFile((int) rowId).getStatus()); + + // Positive control (a real successful retry needs a serialized job) is covered by existing pipeline retry tests. + } + + @Test + public void testShowDataContainerScoping() throws Exception + { + User admin = getAdmin(); + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + User readerA = createUserInRole(folderA, ReaderRole.class); + + long rowId = insertStatusFile(folderB, PipelineJob.TaskStatus.complete.toString()).getRowId(); + + // Addressing B's job through folder A must 404 rather than redirect to / expose it + ActionURL foreignUrl = new ActionURL(ShowDataAction.class, folderA).addParameter("rowId", String.valueOf(rowId)); + assertStatus(HttpServletResponse.SC_NOT_FOUND, get(foreignUrl, readerA)); + // A site admin who CAN read folder B gets a redirect + assertStatus(HttpServletResponse.SC_FOUND, get(foreignUrl, admin)); + + // Positive control: through its own container the action redirects (302) rather than 404 + ActionURL ownUrl = new ActionURL(ShowDataAction.class, folderB).addParameter("rowId", String.valueOf(rowId)); + assertStatus(HttpServletResponse.SC_FOUND, get(ownUrl, admin)); + } + + @Test + public void testCompleteStatusContainerScoping() throws Exception + { + User admin = getAdmin(); + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + User readerA = createUserInRole(folderA, ReaderRole.class); + + // A job in folder B with no serialized job store, so completeStatus() takes its fallback (bean update) path + int rowId = insertStatusFile(folderB, PipelineJob.TaskStatus.error.toString()).getRowId(); + + // A caller with no rights in folder B must not be able to mark B's job complete via the fallback path + try + { + completeStatus(readerA, List.of(rowId)); + fail("Expected UnauthorizedException marking a job complete in a container the caller can't update"); + } + catch (UnauthorizedException ignored) {} + assertEquals("Status must be unchanged after an unauthorized complete", + PipelineJob.TaskStatus.error.toString(), getStatusFile(rowId).getStatus()); + + // Positive control: a site admin (Update in B) can complete it through the same fallback path + completeStatus(admin, List.of(rowId)); + assertEquals("Admin should be able to mark the job complete", + PipelineJob.TaskStatus.complete.toString(), getStatusFile(rowId).getStatus()); + } + + @Test + public void testDetailsContainerScoping() throws Exception + { + // DetailsAction is the HTML job-details page (distinct from the StatusDetailsAction API). It resolves the + // job by global rowId; without the guard a caller could view another folder's job details through their own + // folder. The guard 404s when the caller cannot read the job's container, and otherwise redirects to it. + User admin = getAdmin(); + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + User readerA = createUserInRole(folderA, ReaderRole.class); + + long rowId = insertStatusFile(folderB, PipelineJob.TaskStatus.complete.toString()).getRowId(); + + ActionURL foreignUrl = new ActionURL(DetailsAction.class, folderA).addParameter("rowId", String.valueOf(rowId)); + // A caller who can read folder A but NOT folder B must get 404 -- the page must not reveal B's job exists. + assertStatus(HttpServletResponse.SC_NOT_FOUND, get(foreignUrl, readerA)); + // A site admin who CAN read folder B is redirected (302) to B rather than rendering B's job through A. + assertStatus(HttpServletResponse.SC_FOUND, get(foreignUrl, admin)); + + // Positive control: through its own container the details page renders (200), proving the guard rejects + // only the cross-container case rather than every request. + ActionURL ownUrl = new ActionURL(DetailsAction.class, folderB).addParameter("rowId", String.valueOf(rowId)); + assertStatus(HttpServletResponse.SC_OK, get(ownUrl, admin)); + } + + // Insert a bare status file in the given container. FilePath is a required column; point it at a non-existent + // log so nothing tries to read it, and so getJobStore().getJob() returns null (exercising the bean-only paths). + private PipelineStatusFileImpl insertStatusFile(Container c, String status) + { + User admin = getAdmin(); + PipelineStatusFileImpl sf = new PipelineStatusFileImpl(); + sf.beforeInsert(admin, c.getId()); + sf.setStatus(status); + // uq_statusfiles_filepath is a global unique constraint, so the path must be unique per run -- a GUID keeps + // a leftover row from an interrupted prior run from colliding with this insert. + sf.setFilePath(FileUtil.appendName(FileUtil.getTempDirectory(), "pipeline-scoping-test-" + c.getRowId() + "-" + status + "-" + GUID.makeGUID() + ".log").getAbsolutePath()); + return Table.insert(admin, PipelineSchema.getInstance().getTableInfoStatusFiles(), sf); + } } } diff --git a/query/src/org/labkey/query/QueryModule.java b/query/src/org/labkey/query/QueryModule.java index ada49cf15ba..f0e028f724f 100644 --- a/query/src/org/labkey/query/QueryModule.java +++ b/query/src/org/labkey/query/QueryModule.java @@ -353,6 +353,7 @@ public Set getIntegrationTests() return Set.of( ModuleReportCache.TestCase.class, OlapController.TestCase.class, + OlapController.ContainerScopingTestCase.class, QueryController.TestCase.class, QueryController.SaveRowsTestCase.class, QueryServiceImpl.TestCase.class, diff --git a/query/src/org/labkey/query/controllers/OlapController.java b/query/src/org/labkey/query/controllers/OlapController.java index eac4835c06f..382c3557716 100644 --- a/query/src/org/labkey/query/controllers/OlapController.java +++ b/query/src/org/labkey/query/controllers/OlapController.java @@ -23,6 +23,7 @@ import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.junit.Test; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.json.JSONObject; @@ -56,8 +57,11 @@ import org.labkey.api.data.RuntimeSQLException; import org.labkey.api.data.SQLFragment; import org.labkey.api.data.SqlSelector; +import org.labkey.api.data.Table; import org.labkey.api.data.queryprofiler.QueryProfiler; import org.labkey.api.query.DefaultSchema; +import org.labkey.api.data.SimpleFilter; +import org.labkey.api.data.TableSelector; import org.labkey.api.query.FieldKey; import org.labkey.api.query.QueryParseException; import org.labkey.api.query.QueryParseExceptionUnresolvedField; @@ -67,8 +71,10 @@ 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.AdminPermission; import org.labkey.api.security.permissions.ReadPermission; +import org.labkey.api.security.roles.FolderAdminRole; import org.labkey.api.study.DataspaceContainerFilter; import org.labkey.api.util.Compress; import org.labkey.api.util.ConfigurationException; @@ -463,6 +469,21 @@ protected DataView createView(CustomOlapDescriptorForm form, Errors errors) return new UpdateView(form, (BindException)errors); } + @Override + public void validateCommand(CustomOlapDescriptorForm form, Errors errors) + { + // Confirm definition lives in this container before touching it + Object pk = form.getPkVal(); + if (pk == null) + throw new NotFoundException("Custom olap definition not found"); + SimpleFilter filter = SimpleFilter.createContainerFilter(getContainer()); + filter.addCondition(FieldKey.fromParts("RowId"), pk); + if (!new TableSelector(QueryManager.get().getTableInfoOlapDef(), filter, null).exists()) + throw new NotFoundException("Custom olap definition not found in this folder"); + + super.validateCommand(form, errors); + } + @Override protected boolean doAction(CustomOlapDescriptorForm form, Errors errors) throws SQLException { @@ -1595,4 +1616,50 @@ controller.new ListAppsAction() ); } } + + public static class ContainerScopingTestCase extends AbstractContainerScopingTest + { + @Test + public void testEditDefinitionContainerScoping() throws Exception + { + User admin = getAdmin(); + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + + // A custom OLAP definition that lives in folder B. Its stored content need not be valid Rolap: the update + // action validates only the *submitted* form, and the container guard runs before that validation. + OlapDef def = new OlapDef(); + def.beforeInsert(admin, folderB.getId()); + def.setName("scoping-test-cube"); + def.setModule("query"); + def.setDefinition(""); + def = Table.insert(admin, QueryManager.get().getTableInfoOlapDef(), def); + int rowId = def.getRowId(); + + // A caller who is folder admin in A (so the @RequiresPermission(AdminPermission.class) check passes) but + // has no rights in folder B. + User adminA = createUserInRole(folderA, FolderAdminRole.class); + + // Editing B's definition through folder A must 404 rather than overwrite/re-home it. Without the + // validateCommand guard the unscoped doUpdate() would edit B's row and rewrite its container to A. + ActionURL foreignUrl = new ActionURL(EditDefinitionAction.class, folderA).addParameter("RowId", String.valueOf(rowId)); + assertStatus(HttpServletResponse.SC_NOT_FOUND, post(foreignUrl, adminA)); + // A site admin, who CAN administer folder B, still gets 404 through folder A (no cross-container write). + assertStatus(HttpServletResponse.SC_NOT_FOUND, post(foreignUrl, admin)); + + // The definition must be untouched in its own container. + OlapDef after = new TableSelector(QueryManager.get().getTableInfoOlapDef()).getObject(rowId, OlapDef.class); + assertNotNull("Definition must still exist", after); + assertEquals("Container must be unchanged after a cross-container edit", folderB.getId(), after.getContainerId()); + + // Positive control: a same-container request passes the container guard and proceeds to form validation. The + // submitted definition is intentionally invalid Rolap, so the action redisplays the form (200) instead of + // 404 -- proving the guard rejects only the cross-container case, not legitimate same-container edits. + ActionURL ownUrl = new ActionURL(EditDefinitionAction.class, folderB) + .addParameter("RowId", String.valueOf(rowId)) + .addParameter("Module", "query") + .addParameter("Definition", "not-valid-rolap-xml"); + assertStatus(HttpServletResponse.SC_OK, post(ownUrl, admin)); + } + } } diff --git a/query/src/org/labkey/query/controllers/QueryController.java b/query/src/org/labkey/query/controllers/QueryController.java index fb14b8f5b05..7ad549da27f 100644 --- a/query/src/org/labkey/query/controllers/QueryController.java +++ b/query/src/org/labkey/query/controllers/QueryController.java @@ -7739,7 +7739,10 @@ public void setSchemas(Map>> schemas) } - @RequiresPermission(ReadPermission.class) + // Named sets are stored in a process-global cache keyed only by name (no container component) and are consumed + // server-wide by LabKey SQL IN-clauses, filters, and OLAP member resolution. Mutating one therefore affects every + // folder, so require admin rather than letting any container Reader create/overwrite a global named set. + @RequiresPermission(AdminPermission.class) public static class SaveNamedSetAction extends MutatingApiAction { @Override @@ -7784,7 +7787,9 @@ public List parseSetList() } - @RequiresPermission(ReadPermission.class) + // Same global-cache surface as SaveNamedSetAction: deleting a named set affects every folder's queries, so require + // admin rather than ReadPermission. + @RequiresPermission(AdminPermission.class) public static class DeleteNamedSetAction extends MutatingApiAction { @@ -8525,8 +8530,6 @@ controller.new ImportAction(), new AuditHistoryAction(), new AuditDetailsAction(), new ExportTablesAction(), - new SaveNamedSetAction(), - new DeleteNamedSetAction(), new ApiTestAction(), new GetDefaultVisibleColumnsAction() ); @@ -8556,7 +8559,9 @@ controller.new ManageViewsAction(), controller.new InternalDeleteView(), controller.new InternalSourceViewAction(), controller.new InternalNewViewAction(), - new QueryExportAuditRedirectAction() + new QueryExportAuditRedirectAction(), + new SaveNamedSetAction(), + new DeleteNamedSetAction() ); // @RequiresPermission(AdminOperationsPermission.class) diff --git a/specimen/src/org/labkey/specimen/actions/SpecimenApiController.java b/specimen/src/org/labkey/specimen/actions/SpecimenApiController.java index e9a7dd6dd86..2621a19e267 100644 --- a/specimen/src/org/labkey/specimen/actions/SpecimenApiController.java +++ b/specimen/src/org/labkey/specimen/actions/SpecimenApiController.java @@ -286,6 +286,7 @@ public class GetRequestAction extends ReadOnlyApiAction @Override public ApiResponse execute(RequestIdForm requestIdForm, BindException errors) { + // OK for anyone with read access to see any request in this container, even if they didn't create it SpecimenRequest request = getRequest(getUser(), getContainer(), requestIdForm.getRequestId(), false, false); final Map response = new HashMap<>(); response.put("request", request != null ? getRequestResponse(getViewContext(), request) : null); diff --git a/specimen/src/org/labkey/specimen/actions/SpecimenController.java b/specimen/src/org/labkey/specimen/actions/SpecimenController.java index f3d79169d7e..65d888d8165 100644 --- a/specimen/src/org/labkey/specimen/actions/SpecimenController.java +++ b/specimen/src/org/labkey/specimen/actions/SpecimenController.java @@ -63,6 +63,7 @@ import org.labkey.api.gwt.client.AuditBehaviorType; import org.labkey.api.module.FolderType; import org.labkey.api.module.Module; +import org.labkey.api.module.ModuleLoader; import org.labkey.api.pipeline.PipeRoot; import org.labkey.api.pipeline.PipelineService; import org.labkey.api.pipeline.PipelineStatusUrls; @@ -87,6 +88,7 @@ import org.labkey.api.security.permissions.AdminPermission; import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.security.permissions.UpdatePermission; +import org.labkey.api.security.roles.ReaderRole; import org.labkey.api.specimen.SpecimenQuerySchema; import org.labkey.api.specimen.SpecimenSchema; import org.labkey.api.specimen.Vial; @@ -151,8 +153,10 @@ import org.labkey.specimen.RequestedSpecimens; import org.labkey.specimen.SpecimenManager; import org.labkey.specimen.SpecimenRequestException; +import org.labkey.specimen.SpecimenModule; import org.labkey.specimen.SpecimenRequestManager; import org.labkey.specimen.SpecimenRequestStatus; +import org.labkey.specimen.security.roles.SpecimenRequesterRole; import org.labkey.specimen.importer.RequestabilityManager; import org.labkey.specimen.importer.SimpleSpecimenImporter; import org.labkey.specimen.model.ExtendedSpecimenRequestView; @@ -3801,6 +3805,8 @@ public boolean handlePost(final ManageRequestStatusForm form, BindException erro if (_specimenRequest == null) throw new NotFoundException(); + requiresEditRequestPermissions(_specimenRequest); + boolean statusChanged = form.getStatus() != _specimenRequest.getStatusId(); boolean detailsChanged = !nullSafeEqual(form.getRequestDescription(), _specimenRequest.getComments()); @@ -5847,5 +5853,77 @@ public void testDeleteRequirementActionRejectsForeignRequirement() throws Except post(ownUrl, getAdmin()); assertNull("Same-container delete should remove the requirement", provider.getRequirement(folderA, requirementId)); } + + @Test + public void testDeleteActorActionRejectsForeignActor() throws Exception + { + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + SpecimenRequestRequirementProvider provider = SpecimenRequestRequirementProvider.get(); + + // An actor that lives in folder A + SpecimenRequestActor actor = createActor(folderA, "Delete actor scoping actor"); + int actorId = actor.getRowId(); + + // Deleting through folder B, where the actor does not live, must be a graceful no-op rather than a 500: + // the fix makes getActor container-scoped, so handlePost sees null and skips actor.delete() instead of + // NPEing. The action still redirects (302) and the actor must survive in its own folder. + ActionURL foreignUrl = new ActionURL(DeleteActorAction.class, folderB) + .addParameter("id", String.valueOf(actorId)); + assertStatus(HttpServletResponse.SC_FOUND, post(foreignUrl, getAdmin())); + assertNotNull("Cross-container delete must not remove the actor", provider.getActor(folderA, actorId)); + + // Positive control: deleting through the actor's own folder removes it, proving the guard rejects only the + // cross-container case rather than every delete + ActionURL ownUrl = new ActionURL(DeleteActorAction.class, folderA) + .addParameter("id", String.valueOf(actorId)); + assertStatus(HttpServletResponse.SC_FOUND, post(ownUrl, getAdmin())); + assertNull("Same-container delete should remove the actor", provider.getActor(folderA, actorId)); + } + + @Test + public void testManageRequestStatusRequiresEditPermission() throws Exception + { + // ManageRequestStatusAction is annotated @RequiresPermission(RequestSpecimensPermission.class), which only + // proves the caller may make requests. However, a requester shouldn't be able to change ANOTHER user's request + // + // SpecimenRequesterRole (RequestSpecimensPermission only, no ManageRequestsPermission) is applicable only + // in a study folder that has the Specimen module, so stand up a minimal study. + Container folder = createContainer("ManageStatus"); + Set modules = new HashSet<>(folder.getActiveModules()); + modules.add(ModuleLoader.getInstance().getModule("study")); + modules.add(ModuleLoader.getInstance().getModule(SpecimenModule.NAME)); + folder.setActiveModules(modules, getAdmin()); + StudyService.get().createStudy(folder, getAdmin(), "Specimen scoping study", TimepointType.VISIT, true); + + // A request OWNED BY THE ADMIN (not the attacker), in a freshly created status + SpecimenRequestStatus status = new SpecimenRequestStatus(); + status.setContainer(folder); + status.setLabel("Manage status 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(folder); + request.setStatusId(status.getRowId()); + request = SpecimenRequestManager.get().createRequest(getAdmin(), request, false); + int requestId = request.getRowId(); + + // A plain requester: holds RequestSpecimensPermission (so the action's annotation passes) but is neither a + // coordinator nor the request's owner, so hasEditRequestPermissions() is false. + User requester = createUserInRole(folder, SpecimenRequesterRole.class); + + // Mutating another user's request must be rejected at the per-request guard (403) rather than applied. The + // request id and a no-op status keep the form valid, so without the fix the action proceeds and does NOT 403. + ActionURL url = new ActionURL(ManageRequestStatusAction.class, folder) + .addParameter("id", String.valueOf(requestId)) + .addParameter("status", String.valueOf(status.getRowId())); + assertStatus(HttpServletResponse.SC_FORBIDDEN, post(url, requester)); + + // Positive control: an admin holds ManageRequestsPermission, so the same request passes the guard rather + // than being blocked at 403 -- proving the guard rejects only the unprivileged requester, not every caller. + assertNotEquals("An admin must pass the edit-request guard, not be blocked at 403", + HttpServletResponse.SC_FORBIDDEN, post(url, getAdmin()).getStatus()); + } } } diff --git a/study/src/org/labkey/study/StudyModule.java b/study/src/org/labkey/study/StudyModule.java index 60bb3b9d968..178b3751383 100644 --- a/study/src/org/labkey/study/StudyModule.java +++ b/study/src/org/labkey/study/StudyModule.java @@ -125,6 +125,7 @@ import org.labkey.study.audit.ParticipantGroupAuditProvider; import org.labkey.study.audit.StudyAuditProvider; import org.labkey.study.controllers.CohortController; +import org.labkey.study.controllers.CreateChildStudyAction; import org.labkey.study.controllers.DatasetController; import org.labkey.study.controllers.ParticipantGroupController; import org.labkey.study.controllers.SharedStudyController; @@ -711,6 +712,7 @@ public Set getIntegrationTests() return Set.of( DatasetDefinition.TestCleanupOrphanedDatasetDomains.class, ParticipantGroupManager.ParticipantGroupTestCase.class, + ParticipantGroupManager.ContainerScopingTestCase.class, StudyImpl.ProtocolDocumentTestCase.class, StudyManager.StudySnapshotTestCase.class, StudyManager.VisitCreationTestCase.class, @@ -718,7 +720,9 @@ public Set getIntegrationTests() VisitImpl.TestCase.class, DatasetUpdateService.TestCase.class, DatasetLsidImportHelper.TestCase.class, - org.labkey.study.controllers.CreateChildStudyAction.ContainerScopingTestCase.class); + CreateChildStudyAction.ContainerScopingTestCase.class, + StudyController.ContainerScopingTestCase.class, + ReportsController.ContainerScopingTestCase.class); } @Override diff --git a/study/src/org/labkey/study/controllers/StudyController.java b/study/src/org/labkey/study/controllers/StudyController.java index 6ca457fdf43..fe3cc9c94ae 100644 --- a/study/src/org/labkey/study/controllers/StudyController.java +++ b/study/src/org/labkey/study/controllers/StudyController.java @@ -163,7 +163,9 @@ import org.labkey.api.security.RequiresNoPermission; import org.labkey.api.security.RequiresPermission; import org.labkey.api.security.SecurityManager; +import org.junit.Test; 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.BrowserDeveloperPermission; import org.labkey.api.security.permissions.DeletePermission; @@ -1588,7 +1590,8 @@ private void deleteFromParticipantGroupMapTable(TableInfo ti, String participant { try { - SQLFragment sql = new SQLFragment("DELETE FROM study.participantgroupmap WHERE participantid = ?", participantId); + // Scope the raw DELETE to the request container (DeletePermission is only proven there) + SQLFragment sql = new SQLFragment("DELETE FROM study.participantgroupmap WHERE participantid = ? AND container = ?", participantId, getContainer().getId()); new SqlExecutor(ti.getSchema()).execute(sql); } catch (Exception e) @@ -7899,4 +7902,54 @@ public void addNavTrail(NavTree root) root.addChild(_study != null ? "Overview: " + _study.getLabel() : "No Study"); } } + + public static class ContainerScopingTestCase extends AbstractContainerScopingTest + { + @Test + public void testDeleteParticipantContainerScoping() throws Exception + { + // DeleteParticipantAction removes a participant's dataset rows AND its participant-group memberships. The + // group-map DELETE was keyed only by participantId, so deleting a participant through folder A also wiped + // that participant's group memberships in OTHER folders. The fix scopes the DELETE to the request container. + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + StudyService.get().createStudy(folderA, getAdmin(), "Study A", TimepointType.VISIT, true); + StudyService.get().createStudy(folderB, getAdmin(), "Study B", TimepointType.VISIT, true); + + // P1 must be a known participant in folder B before it can be added to a group there + insertParticipant(folderB, "P1"); + + // Put P1 into a participant group in folder B (creates a study.participantgroupmap row scoped to B) + ParticipantCategoryImpl cat = new ParticipantCategoryImpl(); + cat.setContainer(folderB.getId()); + cat.setLabel("scoping-category"); + cat.setType("list"); + ParticipantGroupManager.getInstance().setParticipantCategory(folderB, getAdmin(), cat, new String[]{"P1"}, null, "scoping"); + assertEquals("Setup: P1 must be in a group in folder B", 1, groupMapCount(folderB, "P1")); + + // Delete P1 through folder A. Folder A has its own study, so the action runs; without the fix its group-map + // DELETE (keyed only by participantId) would also remove P1's membership in folder B. + ActionURL url = new ActionURL(DeleteParticipantAction.class, folderA).addParameter("participantId", "P1"); + post(url, getAdmin()); + + // P1's group membership in folder B must survive a delete issued through folder A. + assertEquals("P1's group membership in folder B must survive a cross-container participant delete", + 1, groupMapCount(folderB, "P1")); + } + + private void insertParticipant(Container c, String ptid) + { + Map row = new HashMap<>(); + row.put("Container", c.getId()); + row.put("ParticipantId", ptid); + Table.insert(getAdmin(), StudySchema.getInstance().getTableInfoParticipant(), row); + } + + private long groupMapCount(Container c, String ptid) + { + SimpleFilter f = new SimpleFilter(FieldKey.fromParts("ParticipantId"), ptid); + f.addCondition(FieldKey.fromParts("Container"), c.getId()); + return new TableSelector(StudySchema.getInstance().getTableInfoParticipantGroupMap(), f, null).getRowCount(); + } + } } diff --git a/study/src/org/labkey/study/controllers/reports/ReportsController.java b/study/src/org/labkey/study/controllers/reports/ReportsController.java index e09c6265b59..6635281bbcc 100644 --- a/study/src/org/labkey/study/controllers/reports/ReportsController.java +++ b/study/src/org/labkey/study/controllers/reports/ReportsController.java @@ -42,6 +42,12 @@ import org.labkey.api.query.ValidationError; import org.labkey.api.reports.Report; import org.labkey.api.reports.ReportService; +import org.labkey.api.reports.report.QueryReport; +import org.labkey.api.security.permissions.AbstractContainerScopingTest; +import org.labkey.api.security.roles.ReaderRole; +import org.labkey.api.writer.DefaultContainerUser; +import jakarta.servlet.http.HttpServletResponse; +import org.junit.Test; import org.labkey.api.reports.report.ReportDescriptor; import org.labkey.api.reports.report.ReportIdentifier; import org.labkey.api.reports.report.ReportUrls; @@ -71,6 +77,7 @@ import org.labkey.api.view.JspView; import org.labkey.api.view.NavTree; import org.labkey.api.view.NotFoundException; +import org.labkey.api.view.UnauthorizedException; import org.labkey.api.view.VBox; import org.labkey.api.view.ViewContext; import org.labkey.api.view.ViewForm; @@ -315,6 +322,11 @@ public ModelAndView getView(ShowReportForm form, BindException errors) throws Ex throw new NotFoundException(message); } + // getReport(container, rowId) is container-scoped but does no owner/SecurityPolicy filtering. Enforce the + // per-report read check + if (!ReportManager.get().canReadReport(getUser(), getContainer(), report)) + throw new UnauthorizedException("You do not have permission to view this report."); + return report.renderReport(getViewContext()); } @@ -1217,4 +1229,33 @@ public void bindJson(JSONObject json) } } } + + public static class ContainerScopingTestCase extends AbstractContainerScopingTest + { + @Test + public void testShowReportRequiresReadPermission() throws Exception + { + // ShowReportAction resolves a report by global rowId with getReport(container, rowId) -- container-scoped, + // but with no owner/policy check. Without the fix a plain container Reader could render another user's + // PRIVATE report by guessing its rowId. The fix adds the per-report canReadReport() check. + Container folder = createContainer("A"); + + // A PRIVATE report: owned by the admin (descriptor owner set), so only the owner / a site admin may read it. + Report report = ReportService.get().createReportInstance(QueryReport.TYPE); + report.getDescriptor().setReportName("scoping-private-report"); + report.getDescriptor().setOwner(getAdmin().getUserId()); + int reportId = ReportService.get().saveReportEx(new DefaultContainerUser(folder, getAdmin()), "scoping-report-key", report).getRowId(); + + ActionURL url = new ActionURL(ShowReportAction.class, folder).addParameter("reportId", String.valueOf(reportId)); + + // A Reader who does not own the report must be rejected (403) rather than shown another user's private report. + User reader = createUserInRole(folder, ReaderRole.class); + assertStatus(HttpServletResponse.SC_FORBIDDEN, get(url, reader)); + + // Positive control: the report's owner passes the per-report read check rather than being blocked at 403, + // proving the guard rejects only the unauthorized reader. + assertNotEquals("The report owner must pass the read check, not be blocked at 403", + HttpServletResponse.SC_FORBIDDEN, get(url, getAdmin()).getStatus()); + } + } } diff --git a/study/src/org/labkey/study/model/ParticipantGroupManager.java b/study/src/org/labkey/study/model/ParticipantGroupManager.java index 945b07a8029..7a148b7210a 100644 --- a/study/src/org/labkey/study/model/ParticipantGroupManager.java +++ b/study/src/org/labkey/study/model/ParticipantGroupManager.java @@ -41,6 +41,8 @@ import org.labkey.api.query.ValidationError; import org.labkey.api.query.ValidationException; import org.labkey.api.security.User; +import org.labkey.api.security.permissions.AbstractContainerScopingTest; +import org.labkey.api.security.roles.ReaderRole; import org.labkey.api.security.permissions.AdminPermission; import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.settings.ResourceURL; @@ -48,6 +50,7 @@ import org.labkey.api.study.ParticipantCategory; import org.labkey.api.study.Study; import org.labkey.api.study.StudyService; +import org.labkey.api.study.TimepointType; import org.labkey.api.study.model.ParticipantGroup; import org.labkey.api.study.permissions.SharedParticipantGroupPermission; import org.labkey.api.util.MemTracker; @@ -578,11 +581,10 @@ private ParticipantGroup _setParticipantGroup(Container c, User user, Participan if (cat == null) throw new ValidationException("The specified category was not found."); - if (cat.isShared()) - { - if (!c.hasPermission(user, SharedParticipantGroupPermission.class) && !c.hasPermission(user, AdminPermission.class)) - throw new ValidationException("You must be in the Editor role or an Admin to assign a group to a shared participant category"); - } + // canEdit enforces the SharedParticipantGroupPermission/Admin check for shared categories AND the + // owner check for private categories + if (!cat.canEdit(c, user)) + throw new ValidationException("You do not have permission to modify groups in this participant category"); } ParticipantGroup ret; @@ -1176,4 +1178,56 @@ public void test() p.getParticipantGroups(null, null, def); } } + + public static class ContainerScopingTestCase extends AbstractContainerScopingTest + { + @Test + public void testSetParticipantGroupRequiresOwnership() throws Exception + { + // setParticipantGroup() saves a group keyed by a global rowId. For a PRIVATE category, canEdit() allows only + // the category's creator -- but the manager previously enforced only the shared-category case, so a Read + // user could overwrite another user's private group via a guessable rowId. The fix gates _setParticipantGroup + // on canEdit() for the private case too. + Container folder = createContainer("A"); + StudyService.get().createStudy(folder, getAdmin(), "Study", TimepointType.VISIT, true); + insertParticipant(folder, "P1"); + + ParticipantGroupManager mgr = ParticipantGroupManager.getInstance(); + + // A PRIVATE participant category + group owned by the admin (ownerId != OWNER_SHARED makes it private; the + // admin is its creator, so only the admin may edit it). + ParticipantCategoryImpl cat = new ParticipantCategoryImpl(); + cat.setContainer(folder.getId()); + cat.setLabel("private-category"); + cat.setType("list"); + cat.setOwnerId(getAdmin().getUserId()); + cat = mgr.setParticipantCategory(folder, getAdmin(), cat, new String[]{"P1"}, null, "private"); + ParticipantGroup group = mgr.getParticipantGroups(folder, getAdmin(), cat).get(0); + + // A different user with only Read access (not the owner, not an admin) + User attacker = createUserInRole(folder, ReaderRole.class); + + // Saving (overwriting) the admin's private group as the attacker must be rejected. + try + { + mgr.setParticipantGroup(folder, attacker, group); + fail("A non-owner must not be able to modify another user's private participant group"); + } + catch (ValidationException expected) + { + } + + // Positive control: the owner (admin) can still save their own private group -- the guard rejects only the + // non-owner, not every caller. + mgr.setParticipantGroup(folder, getAdmin(), group); + } + + private void insertParticipant(Container c, String ptid) + { + Map row = new HashMap<>(); + row.put("Container", c.getId()); + row.put("ParticipantId", ptid); + Table.insert(getAdmin(), StudySchema.getInstance().getTableInfoParticipant(), row); + } + } } diff --git a/studydesign/src/org/labkey/studydesign/StudyDesignController.java b/studydesign/src/org/labkey/studydesign/StudyDesignController.java index 12b5224afff..18122fcbe5e 100644 --- a/studydesign/src/org/labkey/studydesign/StudyDesignController.java +++ b/studydesign/src/org/labkey/studydesign/StudyDesignController.java @@ -17,6 +17,7 @@ import org.apache.commons.lang3.StringUtils; import org.jetbrains.annotations.Nullable; +import org.junit.Test; import org.json.JSONArray; import org.json.JSONObject; import org.labkey.api.action.ApiJsonForm; @@ -31,11 +32,13 @@ import org.labkey.api.data.DbScope; import org.labkey.api.data.SimpleFilter; import org.labkey.api.data.Table; +import org.labkey.api.data.TableSelector; import org.labkey.api.query.FieldKey; import org.labkey.api.query.ValidationException; import org.labkey.api.security.ActionNames; 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.ReadPermission; import org.labkey.api.security.permissions.UpdatePermission; import org.labkey.api.study.Cohort; @@ -51,6 +54,7 @@ import org.labkey.api.util.PageFlowUtil; import org.labkey.api.view.ActionURL; import org.labkey.api.view.HttpView; +import org.labkey.api.view.NotFoundException; import org.labkey.api.view.JspView; import org.labkey.api.view.NavTree; import org.labkey.studydesign.model.AssaySpecimenConfigImpl; @@ -946,4 +950,52 @@ private void updateAssayPlan(User user, Study study, String assayPlan) StudyService.get().updateAssayPlan(user, study, assayPlan); } } + + /** + * Verifies the cross-container guard on the DoseAndRoute save path reached by UpdateStudyProductsAction. That + * action's DoseAndRoute branch alone writes through the raw storage table keyed by a client-supplied RowId, so the + * guard lives in TreatmentManager.saveStudyProductDoseAndRoute and is exercised directly here (building a full study + * + product + JSON product payload to drive the action end-to-end would add a large fixture for the same assertion). + */ + public static class ContainerScopingTestCase extends AbstractContainerScopingTest + { + @Test + public void testSaveDoseAndRouteContainerScoping() + { + User admin = getAdmin(); + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + + // A DoseAndRoute row that lives in folder B + DoseAndRoute existing = Table.insert(admin, StudyDesignSchema.getInstance().getTableInfoDoseAndRoute(), + new DoseAndRoute("1 mg", "IV", 1, folderB)); + int rowId = existing.getRowId(); + + // Deny: an update keyed by B's RowId while operating in folder A must be rejected, not silently + // overwrite/re-home the foreign row. This is the path an Editor in folder A reaches via + // UpdateStudyProductsAction by submitting a products[].DoseAndRoute[].RowId owned by folder B. + DoseAndRoute attack = new DoseAndRoute("HACKED", "HACKED", 1, folderA); + attack.setRowId(rowId); + try + { + TreatmentManager.getInstance().saveStudyProductDoseAndRoute(folderA, admin, attack); + fail("Expected NotFoundException updating a DoseAndRoute owned by another container"); + } + catch (NotFoundException ignored) {} + + // The row in folder B must be untouched + DoseAndRoute reloaded = new TableSelector(StudyDesignSchema.getInstance().getTableInfoDoseAndRoute()) + .getObject(rowId, DoseAndRoute.class); + assertNotNull("DoseAndRoute must still exist", reloaded); + assertEquals("Dose must be unchanged after a cross-container update", "1 mg", reloaded.getDose()); + + // Positive control: updating through the row's own container succeeds and persists the change + DoseAndRoute ok = new DoseAndRoute("2 mg", "IM", 1, folderB); + ok.setRowId(rowId); + TreatmentManager.getInstance().saveStudyProductDoseAndRoute(folderB, admin, ok); + DoseAndRoute afterOk = new TableSelector(StudyDesignSchema.getInstance().getTableInfoDoseAndRoute()) + .getObject(rowId, DoseAndRoute.class); + assertEquals("Dose should be updated by a same-container request", "2 mg", afterOk.getDose()); + } + } } \ No newline at end of file diff --git a/studydesign/src/org/labkey/studydesign/StudyDesignModule.java b/studydesign/src/org/labkey/studydesign/StudyDesignModule.java index eff8232833b..1de6480178a 100644 --- a/studydesign/src/org/labkey/studydesign/StudyDesignModule.java +++ b/studydesign/src/org/labkey/studydesign/StudyDesignModule.java @@ -77,7 +77,8 @@ public Set getIntegrationTests() { return Set.of( TreatmentManager.TreatmentDataTestCase.class, - TreatmentManager.AssayScheduleTestCase.class + TreatmentManager.AssayScheduleTestCase.class, + StudyDesignController.ContainerScopingTestCase.class ); } } \ No newline at end of file diff --git a/studydesign/src/org/labkey/studydesign/model/TreatmentManager.java b/studydesign/src/org/labkey/studydesign/model/TreatmentManager.java index 8bfae0273ed..1a5beb968a1 100644 --- a/studydesign/src/org/labkey/studydesign/model/TreatmentManager.java +++ b/studydesign/src/org/labkey/studydesign/model/TreatmentManager.java @@ -33,6 +33,7 @@ import org.labkey.api.module.ModuleLoader; import org.labkey.api.query.BatchValidationException; import org.labkey.api.query.FieldKey; +import org.labkey.api.view.NotFoundException; import org.labkey.api.query.FilteredTable; import org.labkey.api.query.QueryService; import org.labkey.api.query.QueryUpdateService; @@ -362,8 +363,14 @@ public DoseAndRoute saveStudyProductDoseAndRoute(Container container, User user, { if (doseAndRoute.isNew()) return Table.insert(user, StudyDesignSchema.getInstance().getTableInfoDoseAndRoute(), doseAndRoute); - else - return Table.update(user, StudyDesignSchema.getInstance().getTableInfoDoseAndRoute(), doseAndRoute, doseAndRoute.getRowId()); + + // Confirm the existing row belongs to this container before updating. + SimpleFilter filter = SimpleFilter.createContainerFilter(container); + filter.addCondition(FieldKey.fromParts("RowId"), doseAndRoute.getRowId()); + if (!new TableSelector(StudyDesignSchema.getInstance().getTableInfoDoseAndRoute(), filter, null).exists()) + throw new NotFoundException("Dose and route not found in this folder"); + + return Table.update(user, StudyDesignSchema.getInstance().getTableInfoDoseAndRoute(), doseAndRoute, doseAndRoute.getRowId()); } public Collection getStudyProductsDoseAndRoute(Container container, User user, int productId) diff --git a/wiki/src/org/labkey/wiki/WikiController.java b/wiki/src/org/labkey/wiki/WikiController.java index 0525ecec9d7..3fed40cf90f 100644 --- a/wiki/src/org/labkey/wiki/WikiController.java +++ b/wiki/src/org/labkey/wiki/WikiController.java @@ -62,7 +62,9 @@ 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.EditorRole; import org.labkey.api.security.roles.FolderAdminRole; +import org.labkey.api.security.roles.ReaderRole; import org.labkey.api.settings.AdminConsole; import org.labkey.api.settings.AppProps; import org.labkey.api.util.GUID; @@ -2502,6 +2504,10 @@ public ApiResponse execute(AttachFilesForm form, BindException errors) if (null == wiki) throw new IllegalArgumentException("Could not find the wiki with entity id '" + form.getEntityId() + "'!"); + // Mutating a page's attachments requires UpdatePermission on the page + if (!getPermissions().allowUpdate(wiki)) + throw new UnauthorizedException("You do not have permission to modify this wiki page's attachments."); + if (!(getViewContext().getRequest() instanceof MultipartHttpServletRequest)) throw new IllegalArgumentException("You must use the 'multipart/form-data' mimetype when posting to attachFiles.api"); @@ -2932,29 +2938,66 @@ private WikiManager getWikiManager() public static class CopyWikiContainerScopingTestCase extends AbstractContainerScopingTest { + private Container _source; + private Container _dest; + + @Before + public void createWikiToCopy() + { + // Two folders and a wiki page that lives in the source. Each test makes a limited user an admin on exactly + // one of the folders to exercise a different side of CopyWikiAction's cross-container authorization. + _source = createContainer("Source"); + _dest = createContainer("Dest"); + WikiManager.get().insertWiki(getAdmin(), _source, "page", "body", WikiRendererType.HTML, "Page"); + } + @Test public void testCopyWikiRequiresSourceAdmin() throws Exception { - Container dest = createContainer("Dest"); - Container source = createContainer("Source"); + // Caller administers the destination only + assertCrossContainerCopyRejected(_dest); + } - // A user who is a folder admin in the destination only (no rights in the source) - User destAdminOnly = createUserInRole(dest, FolderAdminRole.class); + @Test + public void testCopyWikiRequiresDestAdmin() throws Exception + { + // Caller administers the source only + assertCrossContainerCopyRejected(_source); + } - // A wiki page that lives in the source folder - WikiManager.get().insertWiki(getAdmin(), source, "secretPage", "secret body", WikiRendererType.HTML, "Secret Page"); + // Drives a source->dest copy posted through callerAdminContainer an admin on BOTH folders must be + // accepted (redirect) and actually copy the page + private void assertCrossContainerCopyRejected(Container callerAdminContainer) throws Exception + { + User limitedAdmin = createUserInRole(callerAdminContainer, FolderAdminRole.class); + ActionURL url = new ActionURL(CopyWikiAction.class, callerAdminContainer) + .addParameter("sourceContainer", _source.getPath()) + .addParameter("destContainer", _dest.getPath()); - 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()); + assertStatus(HttpServletResponse.SC_NOT_FOUND, post(url, limitedAdmin)); + 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()); + assertFalse("An admin copy across both folders should have copied the wiki page", WikiSelectManager.getPageNames(_dest).isEmpty()); + } + + @Test + public void testAttachFilesRequiresUpdate() throws Exception + { + Container folder = createContainer("AttachFolder"); + WikiManager.get().insertWiki(getAdmin(), folder, "attachPage", "body", WikiRendererType.HTML, "Attach Page"); + String entityId = WikiSelectManager.getWiki(folder, "attachPage").getEntityId(); + + ActionURL url = new ActionURL(AttachFilesAction.class, folder).addParameter("entityId", entityId); + + // A Reader must not be able to delete/replace the page's attachments + User reader = createUserInRole(folder, ReaderRole.class); + assertStatus(HttpServletResponse.SC_FORBIDDEN, post(url, reader)); + + // Positive control: an Editor passes the UpdatePermission guard. + User editor = createUserInRole(folder, EditorRole.class); + assertNotEquals("An editor must pass the attachment UpdatePermission guard, not be blocked at 403", + HttpServletResponse.SC_FORBIDDEN, post(url, editor).getStatus()); } } }