diff --git a/announcements/src/org/labkey/announcements/AnnouncementsController.java b/announcements/src/org/labkey/announcements/AnnouncementsController.java index 40dada4a7b5..0c8ecb6a1e6 100644 --- a/announcements/src/org/labkey/announcements/AnnouncementsController.java +++ b/announcements/src/org/labkey/announcements/AnnouncementsController.java @@ -26,6 +26,7 @@ import org.jetbrains.annotations.Nullable; import org.json.JSONObject; import org.labkey.announcements.model.AnnouncementDigestProvider; +import org.labkey.announcements.model.AnnouncementFullModel; import org.labkey.announcements.model.AnnouncementManager; import org.labkey.announcements.model.AnnouncementModel; import org.labkey.announcements.model.DailyDigestEmailPrefsSelector; @@ -214,7 +215,6 @@ public static ActionURL getBeginURL(Container c) public static AnnouncementModel copyEditableProps(AnnouncementModel target, AnnouncementModel source, boolean isInsert) { - if (source.getApproved() != null) target.setApproved(source.getApproved()); if (source.getAssignedTo() != null) target.setAssignedTo(source.getAssignedTo()); if (source.getBody() != null) target.setBody(source.getBody()); if (source.getExpires() != null) target.setExpires(source.getExpires()); @@ -915,7 +915,7 @@ public BindException bindParameters(PropertyValues m) throws Exception public ModelAndView getInsertUpdateView(AnnouncementForm form, boolean reshow, BindException errors) { Permissions perm = getPermissions(); - AnnouncementModel parent = null; + AnnouncementFullModel parent = null; Container c = getContainer(); if (null != form.getParentId()) @@ -2240,7 +2240,7 @@ protected DataRegion getDataRegion(Permissions perm, Settings settings) public static class ThreadViewBean { - public AnnouncementModel announcementModel; + public AnnouncementFullModel announcementModel; public String message = ""; public Permissions perm = null; public boolean isResponse = false; @@ -2261,7 +2261,7 @@ private ThreadView() super("/org/labkey/announcements/announcementThread.jsp", new ThreadViewBean()); } - public ThreadView(Container c, ActionURL url, AnnouncementModel ann, Permissions perm) + public ThreadView(Container c, ActionURL url, AnnouncementFullModel ann, Permissions perm) { this(); init(c, ann, url, perm, true, false); @@ -2270,11 +2270,11 @@ public ThreadView(Container c, ActionURL url, AnnouncementModel ann, Permissions public ThreadView(AnnouncementForm form, Container c, ActionURL url, Permissions perm, boolean print) { this(); - AnnouncementModel ann = findThread(c, form.getAsString("rowId"), form.getAsString("entityId")); + AnnouncementFullModel ann = findThread(c, form.getAsString("rowId"), form.getAsString("entityId")); init(c, ann, url, perm, false, print); } - protected void init(Container c, AnnouncementModel ann, URLHelper currentURL, Permissions perm, boolean isResponse, boolean print) + protected void init(Container c, AnnouncementFullModel ann, URLHelper currentURL, Permissions perm, boolean isResponse, boolean print) { if (null == c || !perm.allowRead(ann)) { @@ -2378,7 +2378,7 @@ public AnnouncementModel getAnnouncement() } - private static @Nullable AnnouncementModel findThread(Container c, String rowIdVal, String entityId) + private static @Nullable AnnouncementFullModel findThread(Container c, String rowIdVal, String entityId) { int rowId = 0; if (rowIdVal != null) diff --git a/announcements/src/org/labkey/announcements/announcementThread.jsp b/announcements/src/org/labkey/announcements/announcementThread.jsp index 1b8711bc415..e89269c1b9e 100644 --- a/announcements/src/org/labkey/announcements/announcementThread.jsp +++ b/announcements/src/org/labkey/announcements/announcementThread.jsp @@ -20,6 +20,7 @@ <%@ page import="org.labkey.announcements.AnnouncementsController.RespondAction" %> <%@ page import="org.labkey.announcements.AnnouncementsController.ThreadView" %> <%@ page import="org.labkey.announcements.AnnouncementsController.ThreadViewBean" %> +<%@ page import="org.labkey.announcements.model.AnnouncementFullModel" %> <%@ page import="org.labkey.announcements.model.AnnouncementManager" %> <%@ page import="org.labkey.announcements.model.AnnouncementModel" %> <%@ page import="org.labkey.announcements.model.Settings" %> @@ -38,7 +39,7 @@ Container c = getContainer(); User user = getUser(); ThreadViewBean bean = me.getModelBean(); - AnnouncementModel announcementModel = bean.announcementModel; + AnnouncementFullModel announcementModel = bean.announcementModel; Settings settings = bean.settings; if (null == announcementModel) diff --git a/announcements/src/org/labkey/announcements/model/AnnouncementFullModel.java b/announcements/src/org/labkey/announcements/model/AnnouncementFullModel.java new file mode 100644 index 00000000000..fb6c5e07ccd --- /dev/null +++ b/announcements/src/org/labkey/announcements/model/AnnouncementFullModel.java @@ -0,0 +1,23 @@ +package org.labkey.announcements.model; + +import java.util.Date; + +public class AnnouncementFullModel extends AnnouncementModel +{ + private Date _approved = null; + + public Date getApproved() + { + return _approved; + } + + public void setApproved(Date approved) + { + _approved = approved; + } + + public boolean isSpam() + { + return AnnouncementManager.SPAM_MAGIC_DATE.equals(getApproved()); + } +} diff --git a/announcements/src/org/labkey/announcements/model/AnnouncementManager.java b/announcements/src/org/labkey/announcements/model/AnnouncementManager.java index 06136316a62..a73cf01c500 100644 --- a/announcements/src/org/labkey/announcements/model/AnnouncementManager.java +++ b/announcements/src/org/labkey/announcements/model/AnnouncementManager.java @@ -121,20 +121,20 @@ private AnnouncementManager() { } - private static @Nullable AnnouncementModel getAnnouncement(@Nullable Container c, @NotNull SimpleFilter filter) + private static @Nullable AnnouncementFullModel getAnnouncement(@Nullable Container c, @NotNull SimpleFilter filter) { if (c != null) filter.addCondition(FieldKey.fromParts("Container"), c); - return new TableSelector(_comm.getTableInfoAnnouncements(), filter, null).getObject(AnnouncementModel.class); + return new TableSelector(_comm.getTableInfoAnnouncements(), filter, null).getObject(AnnouncementFullModel.class); } - public static @Nullable AnnouncementModel getAnnouncement(@Nullable Container c, long rowId) + public static @Nullable AnnouncementFullModel getAnnouncement(@Nullable Container c, long rowId) { return getAnnouncement(c, new SimpleFilter(FieldKey.fromParts("RowId"), rowId)); } - public static @Nullable AnnouncementModel getAnnouncement(@Nullable Container c, String entityId) + public static @Nullable AnnouncementFullModel getAnnouncement(@Nullable Container c, String entityId) { try { @@ -522,7 +522,7 @@ private static AnnouncementModel validateModelWithSideEffects(AnnouncementModel } // Magic date value used to mark an announcement that a moderator has reviewed and marked as spam - private static final Date SPAM_MAGIC_DATE = new Date(0); + static final Date SPAM_MAGIC_DATE = new Date(0); // Standard filters for retrieving specific classes of messages (approved, spam, needs review) public static final SimpleFilter IS_APPROVED_FILTER = new SimpleFilter(FieldKey.fromParts("Approved"), AnnouncementManager.SPAM_MAGIC_DATE, CompareType.GT); @@ -534,11 +534,6 @@ public static void markAsSpam(Container c, AnnouncementModel ann) updateApproved(c, ann, SPAM_MAGIC_DATE); } - public static boolean isSpam(AnnouncementModel ann) - { - return SPAM_MAGIC_DATE.equals(ann.getApproved()); - } - // Execute direct SQL (not Table.update())... I don't think we want to change Modified or ModifiedBy. Could consider adding column for Moderator, though. // Returns true if an update was made, false if not (e.g., message was already reviewed). private static boolean updateApproved(Container c, AnnouncementModel ann, Date date) diff --git a/announcements/src/org/labkey/announcements/model/AnnouncementModel.java b/announcements/src/org/labkey/announcements/model/AnnouncementModel.java index 571069630b8..37ff890ac66 100644 --- a/announcements/src/org/labkey/announcements/model/AnnouncementModel.java +++ b/announcements/src/org/labkey/announcements/model/AnnouncementModel.java @@ -80,7 +80,6 @@ public class AnnouncementModel extends Entity implements Serializable private Collection _responses = null; private Set _authors; - private Date _approved = null; /** * Standard constructor. @@ -418,21 +417,5 @@ public AttachmentParent getAttachmentParent() { return new AnnouncementAttachmentParent(this); } - - public Date getApproved() - { - return _approved; - } - - public void setApproved(Date approved) - { - _approved = approved; - } - - @JsonIgnore - public boolean isSpam() - { - return AnnouncementManager.isSpam(this); - } } diff --git a/api/src/org/labkey/api/action/ApiJsonWriter.java b/api/src/org/labkey/api/action/ApiJsonWriter.java index e54328c3540..48f9eaecfde 100644 --- a/api/src/org/labkey/api/action/ApiJsonWriter.java +++ b/api/src/org/labkey/api/action/ApiJsonWriter.java @@ -485,7 +485,9 @@ public void testExceptionNotCommitted() throws IOException var responseText = ((MockHttpServletResponse)writer.getResponse()).getContentAsString(); var json = new JSONObject(responseText); assertEquals("throwing up", json.getString("exception")); - assertTrue(json.has("stackTrace")); + assertFalse(json.getBoolean("success")); + assertEquals("java.lang.IllegalStateException", json.get("exceptionClass")); + assertFalse(json.has("stackTrace")); assertFalse(json.has("schemaName")); } diff --git a/api/src/org/labkey/api/action/ApiResponseWriter.java b/api/src/org/labkey/api/action/ApiResponseWriter.java index fcb9d70691d..54da88f921b 100644 --- a/api/src/org/labkey/api/action/ApiResponseWriter.java +++ b/api/src/org/labkey/api/action/ApiResponseWriter.java @@ -453,7 +453,6 @@ public JSONObject toJSON(Throwable e) JSONObject json = new JSONObject(); json.put("exception", e.getMessage() != null ? e.getMessage() : e.getClass().getName()); json.put("exceptionClass", e.getClass().getName()); - json.put("stackTrace", e.getStackTrace()); return json; } diff --git a/api/src/org/labkey/api/assay/AssayUrls.java b/api/src/org/labkey/api/assay/AssayUrls.java index ab7e159b08f..e3d3696ba67 100644 --- a/api/src/org/labkey/api/assay/AssayUrls.java +++ b/api/src/org/labkey/api/assay/AssayUrls.java @@ -61,8 +61,6 @@ public interface AssayUrls extends UrlProvider ActionURL getChooseCopyDestinationURL(ExpProtocol protocol, Container container); - ActionURL getDeleteDesignURL(ExpProtocol protocol); - /** * Returns the URL for the assay import data wizard for an existing assay definition. * path and files may be null, in which case it is assumed that the POST will include data object RowIds diff --git a/api/src/org/labkey/api/assay/actions/AssayHeaderView.java b/api/src/org/labkey/api/assay/actions/AssayHeaderView.java index 1f2ffc8cfdc..8e2573c6e90 100644 --- a/api/src/org/labkey/api/assay/actions/AssayHeaderView.java +++ b/api/src/org/labkey/api/assay/actions/AssayHeaderView.java @@ -94,17 +94,6 @@ public List getLinks() return links; } - public static String getDeleteOnClick(ExpProtocol protocol, Container currentContainer) - { - ActionURL deleteURL = PageFlowUtil.urlProvider(AssayUrls.class).getDeleteDesignURL(protocol); - String extraWarning = ""; - if (!protocol.getContainer().equals(currentContainer)) - { - extraWarning = " It is defined in " + protocol.getContainer().getPath() + " and deleting it will remove it from all subfolders."; - } - return "if (window.confirm('Are you sure you want to delete this assay design and all of its runs?" + extraWarning + "')) { window.location = '" + deleteURL + "' }"; - } - public ExpProtocol getProtocol() { return _protocol; diff --git a/api/src/org/labkey/api/data/AbstractParticipantCategory.java b/api/src/org/labkey/api/data/AbstractParticipantCategory.java index 914e1a939ee..35e40383e13 100644 --- a/api/src/org/labkey/api/data/AbstractParticipantCategory.java +++ b/api/src/org/labkey/api/data/AbstractParticipantCategory.java @@ -15,6 +15,7 @@ */ package org.labkey.api.data; +import org.jetbrains.annotations.NotNull; import org.json.JSONArray; import org.json.JSONObject; import org.labkey.api.query.SimpleValidationError; @@ -226,15 +227,11 @@ public boolean canEdit(Container container, User user, List err { if (isNew()) return true; - else - { - User owner = UserManager.getUser(getCreatedBy()); - boolean allowed = (owner != null && !owner.isGuest()) ? owner.equals(user) : false; - if (!allowed) - errors.add(new SimpleValidationError("You must be the owner to unshare this participant category")); - } + if (!isOwner(user)) + errors.add(new SimpleValidationError("You must be the owner to unshare this participant category")); } + return errors.isEmpty(); } @@ -254,44 +251,28 @@ public boolean canDelete(Container container, User user, List e { if (isNew()) return true; - else - { - User owner = UserManager.getUser(getCreatedBy()); - boolean allowed = (owner != null && !owner.isGuest()) ? owner.equals(user) : false; - if (!allowed) - errors.add(new SimpleValidationError("You must be the owner to delete this participant category")); - } + if (!isOwner(user)) + errors.add(new SimpleValidationError("You must be the owner to delete this participant category")); } + return errors.isEmpty(); } - public boolean canRead(Container c, User user) + public boolean canRead(@NotNull User user) { - return canRead(c, user, new ArrayList<>()); + if (isShared() || isNew()) + return true; + + // Issue 16645: Do not show participant groups that may have been created by guests, which was possible + // before this bug was fixed. When admins can update and delete private groups, we can make + // guest-created groups visible again. + return isOwner(user); } - public boolean canRead(Container c, User user, List errors) + private boolean isOwner(@NotNull User user) { - if (!isShared()) - { - if (isNew()) - return true; - else - { - // issue 16645 : don't show participant groups that may have been created by guests, which was possible - // before this bug was fixed. When admins have the ability to update and delete private groups we can - // make guest created groups visible again. - User owner = UserManager.getUser(getCreatedBy()); - boolean allowed = (owner != null && !owner.isGuest()) ? owner.equals(user) : false; - - if (!allowed) - { - errors.add(new SimpleValidationError("You don't have permission to read this private participant category")); - return false; - } - } - } - return true; + User owner = UserManager.getUser(getCreatedBy()); + return owner != null && !owner.isGuest() && owner.equals(user); } } diff --git a/api/src/org/labkey/api/data/TableViewForm.java b/api/src/org/labkey/api/data/TableViewForm.java index 8233761b434..aafd09a3cee 100644 --- a/api/src/org/labkey/api/data/TableViewForm.java +++ b/api/src/org/labkey/api/data/TableViewForm.java @@ -30,6 +30,8 @@ import org.labkey.api.action.NullSafeBindException; import org.labkey.api.action.SpringActionController; import org.labkey.api.collections.CaseInsensitiveHashMap; +import org.labkey.api.ontology.Quantity; +import org.labkey.api.query.FieldKey; import org.labkey.api.security.permissions.DeletePermission; import org.labkey.api.security.permissions.InsertPermission; import org.labkey.api.security.permissions.Permission; @@ -165,8 +167,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"); + setValueToBind("container", _c.getId()); + } Object[] pkVal = getPkVals(); Map newMap = Table.update(_user, _tinfo, _getTypedValues(), pkVal); @@ -189,21 +204,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 {} on request {}", _tinfo.getName(), _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 {} on request {}", _tinfo.getName(), _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/defaults/DefaultValuesAction.java b/api/src/org/labkey/api/defaults/DefaultValuesAction.java index 0d1c34c01ad..46f13b18b85 100644 --- a/api/src/org/labkey/api/defaults/DefaultValuesAction.java +++ b/api/src/org/labkey/api/defaults/DefaultValuesAction.java @@ -18,8 +18,10 @@ import org.labkey.api.action.FormViewAction; import org.labkey.api.exp.property.Domain; import org.labkey.api.exp.property.PropertyService; +import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.view.ActionURL; import org.labkey.api.view.NotFoundException; +import org.labkey.api.view.UnauthorizedException; import org.springframework.validation.Errors; public abstract class DefaultValuesAction extends FormViewAction @@ -41,6 +43,16 @@ protected Domain getDomain(FormType domainIdForm) { throw new NotFoundException(); } + // Multi-container domains allow overriding default values outside the domain's container. The subclasses of + // this action ensure that the user has admin (or list/assay designer) permission in the current container + // (where writes will occur). We also want to ensure the user has read in the domain's container to prevent + // arbitrary reads of domain information. We're not checking that current container is in scope for this domain + // because there's no definitive way to do that. An admin creating default values outside a domain's scope is + // fairly harmless. + if (!domain.getContainer().hasPermission(getUser(), ReadPermission.class)) + { + throw new UnauthorizedException(); + } return domain; } diff --git a/api/src/org/labkey/api/defaults/SetDefaultValuesAction.java b/api/src/org/labkey/api/defaults/SetDefaultValuesAction.java index 9c1c64f304d..6427a1f965c 100644 --- a/api/src/org/labkey/api/defaults/SetDefaultValuesAction.java +++ b/api/src/org/labkey/api/defaults/SetDefaultValuesAction.java @@ -179,52 +179,6 @@ public void render(RenderContext ctx, HtmlWriter out) return app; } ).appendTo(out); -// oldWriter.write(""); - - -// renderer.renderDetailsCaptionCell(ctx, out, "control-label lk-form-row-label"); -// -// if (isFile) -// TD().appendTo(out); // No input for file -// else -// renderer.renderInputCell(ctx, out); -// -// TD( -// (Renderable) ret -> { -// if (isFile) -// { -// out.write("Defaults cannot be set for file fields."); -// } -// else -// { -// DefaultValueType defaultType = ((DefaultableDisplayColumn) renderer).getDefaultValueType(); -// if (defaultType == null) -// defaultType = DefaultValueType.FIXED_EDITABLE; -// out.write(defaultType.getLabel()); -// out.write(PageFlowUtil.popupHelp(HtmlString.of(defaultType.getHelpText()), "Default Value Type: " + defaultType.getLabel())); -// } -// return ret; -// } -// ).appendTo(out); -// oldWriter.write(""); - -// oldWriter.write(""); -// } -// oldWriter.write("
"); -// if (isFile) -// { -// out.write("Defaults cannot be set for file fields."); -// } -// else -// { -// DefaultValueType defaultType = ((DefaultableDisplayColumn) renderer).getDefaultValueType(); -// if (defaultType == null) -// defaultType = DefaultValueType.FIXED_EDITABLE; -// out.write(defaultType.getLabel()); -// out.write(PageFlowUtil.popupHelp(HtmlString.of(defaultType.getHelpText()), "Default Value Type: " + defaultType.getLabel())); -// } -// oldWriter.write("
"); - ButtonBar bbar = getButtonBar(MODE_INSERT); bbar.setStyle(ButtonBar.Style.separateButtons); diff --git a/api/src/org/labkey/api/exp/OntologyManager.java b/api/src/org/labkey/api/exp/OntologyManager.java index b284960bccf..87d7655c707 100644 --- a/api/src/org/labkey/api/exp/OntologyManager.java +++ b/api/src/org/labkey/api/exp/OntologyManager.java @@ -2784,13 +2784,14 @@ public static Object getRemappedValueForLookup(User user, Container container, R return cache.remap(SchemaKey.fromParts(lookup.getSchemaKey()), lookup.getQueryName(), user, lkContainer, ContainerFilter.Type.CurrentPlusProjectAndShared, String.valueOf(value)); } - public static List findPropertyUsages(User user, List propertyIds, int maxUsageCount) + public static List findPropertyUsagesByIds(User user, Container container, List propertyIds, int maxUsageCount) { List ret = new ArrayList<>(propertyIds.size()); for (int propertyId : propertyIds) { var pd = getPropertyDescriptor(propertyId); - if (pd == null) + // Kanban #1924: Get property descriptors for the current container only + if (pd == null || !pd.getContainer().equals(container)) throw new IllegalArgumentException("property not found: " + propertyId); ret.add(findPropertyUsages(user, pd, maxUsageCount)); diff --git a/api/src/org/labkey/api/exp/api/ExperimentService.java b/api/src/org/labkey/api/exp/api/ExperimentService.java index 100df3d2af9..91a59d968c3 100644 --- a/api/src/org/labkey/api/exp/api/ExperimentService.java +++ b/api/src/org/labkey/api/exp/api/ExperimentService.java @@ -178,6 +178,9 @@ enum DataTypeForExclusion @Nullable ExpRun getExpRun(long rowId); + @Nullable + ExpRun getExpRun(long rowId, @Nullable Container container); + List getExpRuns(Collection rowIds); @Nullable diff --git a/api/src/org/labkey/api/qc/AbstractManageQCStatesAction.java b/api/src/org/labkey/api/qc/AbstractManageQCStatesAction.java index 93a782b1f6d..6d7cd95b0c5 100644 --- a/api/src/org/labkey/api/qc/AbstractManageQCStatesAction.java +++ b/api/src/org/labkey/api/qc/AbstractManageQCStatesAction.java @@ -192,5 +192,4 @@ protected HtmlString getQcStateHtml(Container container, DataStateHandler
return qcStateHtml.getHtmlString(); } - } diff --git a/api/src/org/labkey/api/qc/DataStateManager.java b/api/src/org/labkey/api/qc/DataStateManager.java index 9e9a62b8bfc..80d6330051f 100644 --- a/api/src/org/labkey/api/qc/DataStateManager.java +++ b/api/src/org/labkey/api/qc/DataStateManager.java @@ -15,6 +15,8 @@ */ package org.labkey.api.qc; +import org.apache.logging.log4j.Level; +import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; import org.labkey.api.cache.Cache; import org.labkey.api.cache.CacheManager; @@ -27,6 +29,7 @@ import org.labkey.api.data.Table; import org.labkey.api.data.TableSelector; import org.labkey.api.security.User; +import org.labkey.api.util.logging.LogHelper; import java.util.ArrayList; import java.util.Collections; @@ -36,6 +39,7 @@ public class DataStateManager { + private static final Logger LOG = LogHelper.getLogger(DataStateManager.class, "Data state persistence issues"); private static final DataStateManager _instance = new DataStateManager(); private static final Map> _DataStateHandlers = new HashMap<>(); private static final Cache DATA_STATE_DB_CACHE = CacheManager.getBlockingCache(CacheManager.UNLIMITED, CacheManager.DAY, "Data states", @@ -126,7 +130,8 @@ public DataState insertState(User user, DataState state) public DataState updateState(User user, DataState state) { DATA_STATE_DB_CACHE.remove(state.getContainer()); - return Table.update(user, CoreSchema.getInstance().getTableInfoDataStates(), state, state.getRowId()); + SimpleFilter filter = SimpleFilter.createContainerFilter(state.getContainer()); + return Table.update(user, CoreSchema.getInstance().getTableInfoDataStates(), state, state.getRowId(), filter, Level.WARN); } public boolean deleteState(DataState state) diff --git a/api/src/org/labkey/api/security/permissions/AbstractContainerScopingTest.java b/api/src/org/labkey/api/security/permissions/AbstractContainerScopingTest.java index bb5db4d34a6..543cccbb6e2 100644 --- a/api/src/org/labkey/api/security/permissions/AbstractContainerScopingTest.java +++ b/api/src/org/labkey/api/security/permissions/AbstractContainerScopingTest.java @@ -15,10 +15,13 @@ */ package org.labkey.api.security.permissions; +import jakarta.servlet.http.HttpServletResponse; +import org.json.JSONObject; import org.junit.After; import org.junit.Assert; import org.labkey.api.data.Container; import org.labkey.api.data.ContainerManager; +import org.labkey.api.module.Module; import org.labkey.api.security.MutableSecurityPolicy; import org.labkey.api.security.SecurityManager; import org.labkey.api.security.SecurityPolicyManager; @@ -33,8 +36,11 @@ import org.springframework.mock.web.MockHttpServletResponse; import java.util.ArrayList; +import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; /** * Base class for "container scoping" (a.k.a. broken-object-level-authorization / BOLA / IDOR) integration tests. These @@ -56,6 +62,7 @@ public abstract class AbstractContainerScopingTest extends Assert { private static final Map FORM_HEADERS = Map.of("Content-Type", "application/x-www-form-urlencoded"); + private static final Map JSON_HEADERS = Map.of("Content-Type", "application/json"); private final List _containers = new ArrayList<>(); private final List _users = new ArrayList<>(); @@ -71,14 +78,36 @@ protected User getAdmin() * automatic cleanup. Callers pass a short local name (e.g. "A"/"B"/"Source"); the class name is prepended so two * test classes can both ask for "A" without colliding. */ - protected Container createContainer(String name) + protected Container createContainer(String name, org.labkey.api.module.Module... enabledModules) { 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()); + activateModules(c, enabledModules); _containers.add(c); return c; } + protected Container activateModules(Container c, Module... enabledModules) + { + if (enabledModules.length > 0) + { + Set m = new HashSet<>(c.getActiveModules()); + Collections.addAll(m, enabledModules); + c.setActiveModules(m, getAdmin()); + } + return c; + } + /** * Create a user that has {@code role} assigned in {@code scope} only (it has no rights in any other * container), registered for automatic cleanup. This is the canonical way to build a caller who is privileged in @@ -123,8 +152,14 @@ protected MockHttpServletResponse post(ActionURL url, User user) throws Exceptio return ViewServlet.POST(url, user, FORM_HEADERS, null); } + /** Dispatch a JSON POST to a {@code @Marshal(Jackson)} API action, with {@code body} supplied as the request body. */ + protected MockHttpServletResponse postJson(ActionURL url, User user, JSONObject body) throws Exception + { + return ViewServlet.POST(url, user, JSON_HEADERS, body.toString()); + } + /** Assert that a dispatched response has the expected HTTP status code. */ - protected void assertStatus(int expected, MockHttpServletResponse response) + protected void assertStatus(int expected, HttpServletResponse response) { assertEquals("Unexpected HTTP status", expected, response.getStatus()); } diff --git a/api/src/org/labkey/api/study/publish/AbstractPublishConfirmAction.java b/api/src/org/labkey/api/study/publish/AbstractPublishConfirmAction.java index b6c3f5a5355..428923d332a 100644 --- a/api/src/org/labkey/api/study/publish/AbstractPublishConfirmAction.java +++ b/api/src/org/labkey/api/study/publish/AbstractPublishConfirmAction.java @@ -45,6 +45,7 @@ import org.labkey.api.view.ActionURL; import org.labkey.api.view.JspView; import org.labkey.api.view.RedirectException; +import org.labkey.api.view.UnauthorizedException; import org.labkey.api.view.VBox; import org.labkey.api.view.template.ClientDependency; import org.springframework.validation.BindException; @@ -110,10 +111,17 @@ public void validateCommand(FORM form, Errors errors) { errors.reject(SpringActionController.ERROR_MSG, "Could not find target study"); } + else if (!_targetStudy.hasPermission(getUser(), InsertPermission.class)) + { + throw new UnauthorizedException("You do not have permission to insert into the target study"); + } } if (_targetStudy != null) { + if (!_targetStudy.hasPermission(getUser(), InsertPermission.class)) + errors.reject(SpringActionController.ERROR_MSG, "You do not have permission to link data to the study in " + _targetStudy.getPath() + "."); + Study study = StudyService.get().getStudy(_targetStudy); if (study == null) { @@ -334,6 +342,7 @@ private void attemptLinkage(FORM form, BindException errors, boolean missingStudy = false; boolean badVisitIds = false; boolean badDates = false; + boolean noPermissionStudy = false; int index = 0; for (long objectId : allObjects) { @@ -378,6 +387,9 @@ private void attemptLinkage(FORM form, BindException errors, } else { + if (selected && !rowLevelTargetStudy.hasPermission(getUser(), InsertPermission.class)) + noPermissionStudy = true; + postedTargetStudies.put(objectId, rowLevelTargetStudy.getId()); if (StudyPublishService.get().getTimepointType(rowLevelTargetStudy) == TimepointType.VISIT) @@ -440,6 +452,8 @@ private void attemptLinkage(FORM form, BindException errors, if (missingStudy) errors.reject(null, "You must specify a Target Study for all selected rows."); + if (noPermissionStudy) + errors.reject(null, "You do not have permission to link data to one or more of the selected target studies."); if (missingPtid) errors.reject(null, "You must specify a Participant ID for all selected rows."); if (missingVisitId) diff --git a/api/src/org/labkey/api/study/publish/PublishConfirmForm.java b/api/src/org/labkey/api/study/publish/PublishConfirmForm.java index ffff5318fe8..d4c3fda02d7 100644 --- a/api/src/org/labkey/api/study/publish/PublishConfirmForm.java +++ b/api/src/org/labkey/api/study/publish/PublishConfirmForm.java @@ -49,7 +49,7 @@ private void convertStringArrayParam(PropertyValue pv) return BaseViewAction.springBindParameters(this, "form", pvs); } - private Integer _rowId; + private Long _rowId; private String[] _targetStudy; private String[] _participantId; private String[] _visitId; @@ -64,12 +64,12 @@ private void convertStringArrayParam(PropertyValue pv) private String _defaultValueSource = DefaultValueSource.PublishSource.name(); private String _autoLinkCategory; - public Integer getRowId() + public Long getRowId() { return _rowId; } - public void setRowId(Integer rowId) + public void setRowId(Long rowId) { _rowId = rowId; } diff --git a/api/src/org/labkey/api/view/ViewServlet.java b/api/src/org/labkey/api/view/ViewServlet.java index 1eb99b5afa9..91412cefdd5 100644 --- a/api/src/org/labkey/api/view/ViewServlet.java +++ b/api/src/org/labkey/api/view/ViewServlet.java @@ -507,6 +507,12 @@ public void setActionURL(ActionURL actionURL) return _actionURL.getParameterMap(); } + @Override + public String getParameter(@NotNull String name) + { + return _actionURL.getParameter(name); + } + @Override public @NotNull String @NotNull [] getParameterValues(@NotNull String name) { diff --git a/api/src/org/labkey/api/webdav/AbstractWebdavResource.java b/api/src/org/labkey/api/webdav/AbstractWebdavResource.java index 74462e6899d..5dbdef5d9f1 100644 --- a/api/src/org/labkey/api/webdav/AbstractWebdavResource.java +++ b/api/src/org/labkey/api/webdav/AbstractWebdavResource.java @@ -403,6 +403,15 @@ public boolean canDelete(User user, boolean forDelete, /* OUT */ @Nullable List< return perms.contains(DeletePermission.class); } + @Override + public boolean canMove(User user) + { + // A MOVE removes the resource from its source location, so by default it requires the same rights + // as deleting it from there. Resource types where moving and deleting differ (see FileSystemResource) + // override this. + return canDelete(user, true, null); + } + @Override public boolean canRename(User user, boolean forRename) { diff --git a/api/src/org/labkey/api/webdav/FileSystemResource.java b/api/src/org/labkey/api/webdav/FileSystemResource.java index 618e64e4d19..91bd64a1158 100644 --- a/api/src/org/labkey/api/webdav/FileSystemResource.java +++ b/api/src/org/labkey/api/webdav/FileSystemResource.java @@ -507,27 +507,38 @@ public boolean canCreate(User user, boolean forCreate) } + // Shared access checks for canDelete() and canMove(): Delete permission plus a writable file on disk. + // Deliberately does NOT apply the "imported by an assay" restriction, which is specific to outright + // deletion - a file that has been imported may still be relocated within the file root. + private boolean hasDeletePermissionAndWritableFile(User user, boolean forDelete, @Nullable List message) + { + if (!super.canDelete(user, forDelete, message) || !hasFileSystem()) + return false; + File f = getFile(); + if (null == f) + return false; + if (!f.canWrite()) + { + SecurityLogger.log("File.canWrite()==false",user,null,false); + if (forDelete) + { + if (null != message) + message.add("File is not writable on server"); + _log.warn("{} attempted to delete file that is not writable by LabKey Server. This may be a configuration problem. file: {}", user.getEmail(), f.getPath()); + } + return false; + } + return true; + } + + @Override public boolean canDelete(User user, boolean forDelete, @Nullable List message) { try { - if (!super.canDelete(user, forDelete, message) || !hasFileSystem()) - return false; - File f = getFile(); - if (null == f) + if (!hasDeletePermissionAndWritableFile(user, forDelete, message)) return false; - if (!f.canWrite()) - { - SecurityLogger.log("File.canWrite()==false",user,null,false); - if (forDelete) - { - if (null != message) - message.add("File is not writable on server"); - _log.warn("{} attempted to delete file that is not writable by LabKey Server. This may be a configuration problem. file: {}", user.getEmail(), f.getPath()); - } - return false; - } // can't delete if already processed if (!getActions(user).isEmpty()) { @@ -544,6 +555,15 @@ public boolean canDelete(User user, boolean forDelete, @Nullable List me } + @Override + public boolean canMove(User user) + { + // A MOVE removes the file from its source location, so it requires Delete permission and a writable + // file. Unlike canDelete(), it does NOT require the file to be eligible for outright deletion: a file + // that has been imported by an assay may still be relocated within the file root. + return hasDeletePermissionAndWritableFile(user, false, null); + } + @Override public boolean canRename(User user, boolean forRename) { diff --git a/api/src/org/labkey/api/webdav/WebdavResource.java b/api/src/org/labkey/api/webdav/WebdavResource.java index 46121632467..29480322a7c 100644 --- a/api/src/org/labkey/api/webdav/WebdavResource.java +++ b/api/src/org/labkey/api/webdav/WebdavResource.java @@ -194,6 +194,14 @@ public interface WebdavResource extends Resource */ boolean canDelete(User user, boolean forDelete); boolean canDelete(User user, boolean forDelete, /* OUT */ List message); + /** + * A MOVE removes the resource from its source location, so it requires Delete permission there. + * Unlike {@link #canDelete}, it does not require the resource to be eligible for outright deletion + * (for example, a file that has been imported by an assay may still be relocated within the file root). + * @param user authenticated user + * @return true if the user has permission and server has capability + */ + boolean canMove(User user); /** * @param user authenticated user * @param forRename true if user wants to rename, false if checking capabilities (affects logging) diff --git a/api/src/org/labkey/api/webdav/WebdavResourceReadOnly.java b/api/src/org/labkey/api/webdav/WebdavResourceReadOnly.java index 36065f5e890..e9f5f59cd43 100644 --- a/api/src/org/labkey/api/webdav/WebdavResourceReadOnly.java +++ b/api/src/org/labkey/api/webdav/WebdavResourceReadOnly.java @@ -291,6 +291,12 @@ public boolean canDelete(User user, boolean forDelete, List message) return false; } + @Override + public boolean canMove(User user) + { + return false; + } + @Override public boolean canRename(User user, boolean forRename) { diff --git a/assay/api-src/org/labkey/api/assay/nab/view/DilutionGraphAction.java b/assay/api-src/org/labkey/api/assay/nab/view/DilutionGraphAction.java index 43787be934c..194313e095a 100644 --- a/assay/api-src/org/labkey/api/assay/nab/view/DilutionGraphAction.java +++ b/assay/api-src/org/labkey/api/assay/nab/view/DilutionGraphAction.java @@ -44,7 +44,8 @@ public ModelAndView getView(GraphForm form, BindException errors) throws Excepti { if (form.getRowId() == -1) throw new NotFoundException("Run ID not specified."); - ExpRun run = ExperimentService.get().getExpRun(form.getRowId()); + // GitHub Issue #1892: Resolve the run scoped to the current container + ExpRun run = ExperimentService.get().getExpRun(form.getRowId(), getContainer()); if (run == null) throw new NotFoundException("Run " + form.getRowId() + " does not exist."); diff --git a/assay/api-src/org/labkey/api/assay/nab/view/GraphSelectedAction.java b/assay/api-src/org/labkey/api/assay/nab/view/GraphSelectedAction.java index 7d14c91f984..605e66c9b97 100644 --- a/assay/api-src/org/labkey/api/assay/nab/view/GraphSelectedAction.java +++ b/assay/api-src/org/labkey/api/assay/nab/view/GraphSelectedAction.java @@ -71,6 +71,8 @@ public ModelAndView getView(FormType form, BindException errors) throws Exceptio objectIds[idx++] = Integer.parseInt(objectIdString); } + // GitHub Issue #1892: (NAB-9) The object ids come straight from the request and getDilutionSummaries() resolves them to runs + verifyObjectIdsReadable(objectIds); Set cutoffSet = new HashSet<>(); DilutionAssayProvider provider = (DilutionAssayProvider) AssayService.get().getProvider(_protocol); Map summaries = provider.getDataHandler().getDilutionSummaries(getUser(), form.getFitTypeEnum(), objectIds); @@ -92,6 +94,13 @@ public ModelAndView getView(FormType form, BindException errors) throws Exceptio return new VBox(new AssayHeaderView(_protocol, provider, false, true, null), multiGraphView); } + /** + * Verify that the current user may view each of the requested object ids before any run data is loaded. + */ + protected void verifyObjectIdsReadable(long[] ids) throws Exception + { + } + protected abstract GraphSelectedBean createSelectionBean(ViewContext context, ExpProtocol protocol, int[] cutoffs, long[] dataObjectIds, String caption, String title); diff --git a/assay/api-src/org/labkey/api/assay/nab/view/MultiGraphAction.java b/assay/api-src/org/labkey/api/assay/nab/view/MultiGraphAction.java index 19fee6491b9..97221395c05 100644 --- a/assay/api-src/org/labkey/api/assay/nab/view/MultiGraphAction.java +++ b/assay/api-src/org/labkey/api/assay/nab/view/MultiGraphAction.java @@ -16,13 +16,13 @@ package org.labkey.api.assay.nab.view; import org.labkey.api.action.SimpleViewAction; +import org.labkey.api.assay.AssayService; import org.labkey.api.assay.dilution.DilutionAssayProvider; import org.labkey.api.assay.dilution.DilutionAssayRun; import org.labkey.api.assay.dilution.DilutionSummary; import org.labkey.api.assay.nab.NabGraph; import org.labkey.api.exp.api.ExpProtocol; import org.labkey.api.exp.api.ExperimentService; -import org.labkey.api.assay.AssayService; import org.labkey.api.view.NavTree; import org.springframework.validation.BindException; import org.springframework.web.servlet.ModelAndView; @@ -35,12 +35,13 @@ * User: klum * Date: 6/11/13 */ -public class MultiGraphAction extends SimpleViewAction +public abstract class MultiGraphAction extends SimpleViewAction { @Override public ModelAndView getView(FormType form, BindException errors) throws Exception { long[] ids = form.getId(); + verifyObjectIdsReadable(ids); ExpProtocol protocol = ExperimentService.get().getExpProtocol(form.getProtocolId()); DilutionAssayProvider provider = (DilutionAssayProvider)AssayService.get().getProvider(protocol); Map summaries = provider.getDataHandler().getDilutionSummaries(getUser(), form.getFitTypeEnum(), ids); @@ -63,6 +64,13 @@ public ModelAndView getView(FormType form, BindException errors) throws Exceptio return null; } + /** + * Verify that the current user may view each of the requested object ids before any run data is loaded. + */ + protected void verifyObjectIdsReadable(long[] ids) throws Exception + { + } + protected NabGraph.Config getGraphConfig(FormType form) { NabGraph.Config config = new NabGraph.Config(); diff --git a/assay/src/org/labkey/assay/AssayController.java b/assay/src/org/labkey/assay/AssayController.java index 4a894e88402..cd47a334e78 100644 --- a/assay/src/org/labkey/assay/AssayController.java +++ b/assay/src/org/labkey/assay/AssayController.java @@ -137,7 +137,6 @@ import org.labkey.assay.actions.AssayBatchDetailsAction; import org.labkey.assay.actions.AssayBatchesAction; import org.labkey.assay.actions.AssayResultsAction; -import org.labkey.assay.actions.DeleteAction; import org.labkey.assay.actions.DeleteProtocolAction; import org.labkey.assay.actions.GetAssayBatchAction; import org.labkey.assay.actions.GetAssayBatchesAction; @@ -192,7 +191,6 @@ public class AssayController extends SpringActionController AssayResultsAction.class, AssayRunDetailsAction.class, AssayRunsAction.class, - DeleteAction.class, DeleteProtocolAction.class, DesignerAction.class, GetAssayBatchAction.class, @@ -425,7 +423,6 @@ private static Map serializeAssayLinks(ExpProtocol protocol, Ass links.put("batches", urlProvider.getAssayBatchesURL(c, protocol, null)); links.put("begin", urlProvider.getProtocolURL(c, protocol, AssayBeginAction.class)); links.put("designCopy", urlProvider.getDesignerURL(c, protocol, true, null)); - links.put("designDelete", urlProvider.getDeleteDesignURL(protocol)); links.put("designEdit", urlProvider.getDesignerURL(c, protocol, false, null)); links.put("import", provider.getImportURL(c, protocol)); links.put("results", urlProvider.getAssayResultsURL(c, protocol)); @@ -1145,12 +1142,6 @@ public ActionURL getChooseCopyDestinationURL(ExpProtocol protocol, Container con return getProtocolURL(container, protocol, ChooseCopyDestinationAction.class); } - @Override - public ActionURL getDeleteDesignURL(ExpProtocol protocol) - { - return getProtocolURL(protocol.getContainer(), protocol, DeleteAction.class); - } - @Override public ActionURL getImportURL(Container container, ExpProtocol protocol, String path, File[] files) { @@ -1376,6 +1367,9 @@ public Object execute(Object form, BindException errors) throws Exception ExpRun expRun = ExperimentService.get().getExpRun(NumberUtils.toInt(run)); if (expRun != null) { + // Kanban #1924 assure permissions to the run's container, which might be different from the current container + if (!expRun.getContainer().hasPermission(getUser(), AssayReadPermission.class)) + throw new UnauthorizedException("User does not have " + AssayReadPermission.class.getSimpleName() + " for run " + run); response.put("success", true); DataState state = AssayQCService.getProvider().getQCState(expRun.getProtocol(), expRun.getRowId()); if (state != null) @@ -1524,6 +1518,14 @@ public ApiSimpleResponse execute(UpdateQCStateForm form, BindException errors) t ApiSimpleResponse response = new ApiSimpleResponse(); if (form.getRuns() != null && _firstRun != null) { + for (Long id : form.getRuns()) + { + // Support cross-container operations but confirm permission + 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()); @@ -1681,6 +1683,11 @@ public Object execute(AssayOperationConfirmationForm form, BindException errors) ExperimentService service = ExperimentService.get(); ExpProtocol protocol = service.getExpProtocol(form.getProtocolId()); + if (protocol == null) + throw new NotFoundException("Protocol with id " + form.getProtocolId() + " not found."); + // Kanban #1924: Assure permission in the protocol's container, which may be different than the current container + if (!protocol.getContainer().hasPermission(getUser(), ReadPermission.class)) + throw new UnauthorizedException("User does not have permission to read protocol " + protocol.getName()); AssayProvider provider = AssayService.get().getProvider(protocol); if (provider == null) throw new NotFoundException("No provider found for protocol " + form.getProtocolId()); diff --git a/assay/src/org/labkey/assay/actions/DeleteAction.java b/assay/src/org/labkey/assay/actions/DeleteAction.java deleted file mode 100644 index adae1bb54cc..00000000000 --- a/assay/src/org/labkey/assay/actions/DeleteAction.java +++ /dev/null @@ -1,74 +0,0 @@ -/* - * Copyright (c) 2019 LabKey Corporation - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.labkey.assay.actions; - -import org.labkey.api.assay.AssayUrls; -import org.labkey.api.assay.actions.BaseAssayAction; -import org.labkey.api.assay.actions.ProtocolIdForm; -import org.labkey.api.assay.security.DesignAssayPermission; -import org.labkey.api.exp.api.ExpProtocol; -import org.labkey.api.security.RequiresPermission; -import org.labkey.api.security.User; -import org.labkey.api.security.permissions.DeletePermission; -import org.labkey.api.security.permissions.ReadPermission; -import org.labkey.api.util.PageFlowUtil; -import org.labkey.api.view.RedirectException; -import org.labkey.api.view.UnauthorizedException; -import org.labkey.api.view.ViewContext; -import org.springframework.validation.BindException; -import org.springframework.web.servlet.ModelAndView; - -import java.util.Set; - -/** - * User: brittp -* Date: Jul 26, 2007 -* Time: 7:23:24 PM -*/ -@RequiresPermission(ReadPermission.class) //will check explicitly in code below -public class DeleteAction extends BaseAssayAction -{ - @Override - public ModelAndView getView(ProtocolIdForm protocolIdForm, BindException errors) - { - ExpProtocol protocol; - try - { - protocol = protocolIdForm.getProtocol(); - } - catch (ProtocolIdForm.ProviderNotFoundException e) - { - // We're OK attempting to delete even though the AssayProvider can't be found - protocol = e.getProtocol(); - } - - if(!allowDelete(protocol)) - throw new UnauthorizedException("You do not have sufficient permissions to delete this assay design."); - - protocol.delete(getUser()); - throw new RedirectException(PageFlowUtil.urlProvider(AssayUrls.class).getAssayListURL(getContainer())); - } - - private boolean allowDelete(ExpProtocol protocol) - { - ViewContext ctx = getViewContext(); - User user = ctx.getUser(); - - //user must have both design assay AND delete permission, as this will delete both the design and uploaded data - return protocol.getContainer().hasPermissions(user, Set.of(DesignAssayPermission.class, DeletePermission.class)); - } -} diff --git a/assay/src/org/labkey/assay/plate/PlateManager.java b/assay/src/org/labkey/assay/plate/PlateManager.java index 4c1da61c95a..c2990b83444 100644 --- a/assay/src/org/labkey/assay/plate/PlateManager.java +++ b/assay/src/org/labkey/assay/plate/PlateManager.java @@ -3322,6 +3322,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/CoreController.java b/core/src/org/labkey/core/CoreController.java index 902be7af686..307e36f34cb 100644 --- a/core/src/org/labkey/core/CoreController.java +++ b/core/src/org/labkey/core/CoreController.java @@ -28,6 +28,7 @@ import org.jetbrains.annotations.Nullable; import org.json.JSONArray; import org.json.JSONObject; +import org.junit.Assert; import org.junit.Test; import org.labkey.api.action.ApiResponse; import org.labkey.api.action.ApiSimpleResponse; @@ -108,10 +109,16 @@ import org.labkey.api.reports.LabKeyScriptEngineManager; import org.labkey.api.security.AdminConsoleAction; import org.labkey.api.security.IgnoresTermsOfUse; +import org.labkey.api.security.MutableSecurityPolicy; import org.labkey.api.security.RequiresLogin; import org.labkey.api.security.RequiresNoPermission; import org.labkey.api.security.RequiresPermission; +import org.labkey.api.security.SecurityManager; +import org.labkey.api.security.SecurityManager.NewUserStatus; +import org.labkey.api.security.SecurityPolicyManager; import org.labkey.api.security.User; +import org.labkey.api.security.UserManager; +import org.labkey.api.security.ValidEmail; import org.labkey.api.security.permissions.AbstractActionPermissionTest; import org.labkey.api.security.permissions.AdminOperationsPermission; import org.labkey.api.security.permissions.AdminPermission; @@ -120,6 +127,9 @@ import org.labkey.api.security.permissions.Permission; import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.security.permissions.UpdatePermission; +import org.labkey.api.security.roles.FolderAdminRole; +import org.labkey.api.security.roles.ProjectAdminRole; +import org.labkey.api.security.roles.ReaderRole; import org.labkey.api.security.roles.RoleManager; import org.labkey.api.services.ServiceRegistry; import org.labkey.api.settings.AdminConsole; @@ -136,6 +146,7 @@ import org.labkey.api.util.HtmlString; import org.labkey.api.util.HtmlStringBuilder; import org.labkey.api.util.JsonUtil; +import org.labkey.api.util.JunitUtil; import org.labkey.api.util.MimeMap; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.util.PageFlowUtil.Content; @@ -157,6 +168,7 @@ import org.labkey.api.view.RedirectException; import org.labkey.api.view.UnauthorizedException; import org.labkey.api.view.ViewContext; +import org.labkey.api.view.ViewServlet; import org.labkey.api.view.template.ClientDependency; import org.labkey.api.view.template.WarningService; import org.labkey.api.view.template.Warnings; @@ -177,8 +189,10 @@ import org.labkey.core.workbook.MoveWorkbooksBean; import org.labkey.core.workbook.WorkbookFolderType; import org.labkey.folder.xml.FolderDocument; +import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.validation.BindException; import org.springframework.validation.Errors; +import org.springframework.web.bind.annotation.RequestMethod; import org.springframework.web.servlet.ModelAndView; import org.springframework.web.servlet.mvc.Controller; @@ -400,6 +414,9 @@ else if (form.getObjectURI() != null) if (!obj.getContainer().equals(getContainer())) { + // Kanban #1924: Assure permission in the object's container + if (!obj.getContainer().hasPermission(getUser(), ReadPermission.class)) + throw new UnauthorizedException(); ActionURL correctedURL = getViewContext().getActionURL().clone(); Container objectContainer = obj.getContainer(); if (objectContainer == null) @@ -861,36 +878,11 @@ public static class MoveContainerAction extends MutatingApiAction children = ContainerManager.getAllChildren(target, getUser()); // assumes read permission if (children.contains(parent)) { - errors.reject(ERROR_MSG, "The container '" + parentIdentifier + "' is not a valid parent folder."); - return; + errors.reject(ERROR_MSG, "The container '" + json.get("parent") + "' is not a valid parent folder."); + } + } + + private @Nullable Container validateContainer(JSONObject json, String key, String description, Errors errors) + { + String identifier = json.optString(key, null); + + if (null == identifier) + { + errors.reject(ERROR_MSG, description + " container must be specified for move operation."); + return null; + } + + Path path = Path.parse(identifier); + Container c = ContainerManager.getForPath(path); + + if (null == c) + { + c = ContainerManager.getForId(identifier); + } + + // Treat non-existent and bad permissions equivalently to not leak any info + if (null == c || !c.hasPermission(getUser(), ReadPermission.class)) + { + throw new NotFoundException(description + " not found"); } - if (!target.hasPermission(getUser(), AdminPermission.class)) + if (!c.hasPermission(getUser(), AdminPermission.class)) { throw new UnauthorizedException("You must be an administrator for the target container"); } + + return c; } @Override @@ -952,7 +966,7 @@ public ApiResponse execute(SimpleApiJsonForm form, BindException errors) throws // Prepare aliases JSONObject object = form.getJsonObject(); - Boolean addAlias = (Boolean) object.get("addAlias"); + boolean addAlias = object.optBoolean("addAlias"); // optional, false by default List aliasList = new ArrayList<>(ContainerManager.getAliasesForContainer(target)); aliasList.add(target.getPath()); @@ -1771,6 +1785,11 @@ public ApiResponse execute(ContainerInfoForm form, BindException errors) { // Provide information about container, specifically an array of child tab folders that were deleted Container container = form.getContainerPath() != null ? ContainerManager.getForPath(form.getContainerPath()) : getContainer(); + if (container == null) + throw new NotFoundException("No container found for path: " + form.getContainerPath()); + // Kanban #1924: Assure permission to the container + if (!container.hasPermission(getUser(), ReadPermission.class)) + throw new UnauthorizedException("You do not have permission to view the container information."); JSONArray deletedFolders = new JSONArray(); for (FolderTab folderTab : container.getDeletedTabFolders(form.getNewFolderType())) { @@ -2922,4 +2941,107 @@ public ActionURL getRedirectURL(ReturnUrlForm form) throws Exception return form.getReturnActionURL(AppProps.getInstance().getHomePageActionURL()); } } + + public static class MoveContainerTestCase extends Assert + { + private static final String FOLDER_NAME = "MoveContainerFolder"; + private static final String NEW_PARENT = "NewParent"; + private static final ValidEmail TEST_EMAIL; + + static + { + try + { + TEST_EMAIL = new ValidEmail("testMove@myDomain.com"); + } + catch (ValidEmail.InvalidEmailException e) + { + throw new RuntimeException(e); + } + } + + @Test + public void testMoveContainer() throws Exception + { + doCleanup(); + + User adminUser = TestContext.get().getUser(); + Container junit = JunitUtil.getTestContainer(); + Container folder = ContainerManager.createContainer(junit, FOLDER_NAME, adminUser); + Container newParent = ContainerManager.createContainer(junit, NEW_PARENT, adminUser); + + NewUserStatus newUserStatus = SecurityManager.addUser(TEST_EMAIL, null); + User nonAdminUser = newUserStatus.getUser(); + // Create and save a new, non-empty policy for the folder so it doesn't inherit permissions + MutableSecurityPolicy policy = new MutableSecurityPolicy(folder.getPolicy()); + policy.addRoleAssignment(nonAdminUser, ReaderRole.class); + SecurityPolicyManager.savePolicyForTests(policy, adminUser); + + // Admin permissions nowhere... should fail + moveFolder(nonAdminUser, folder, newParent, HttpServletResponse.SC_FORBIDDEN); + + // Give nonAdminUser admin permission in just the folder being moved and try again (should fail) + policy = new MutableSecurityPolicy(folder.getPolicy()); + policy.addRoleAssignment(nonAdminUser, FolderAdminRole.class); + SecurityPolicyManager.savePolicyForTests(policy, adminUser); + moveFolder(nonAdminUser, folder, newParent, HttpServletResponse.SC_FORBIDDEN); + + // Give nonAdminUser admin permission in the new parent as well and try again (should still fail) + MutableSecurityPolicy newParentPolicy = new MutableSecurityPolicy(newParent); + newParentPolicy.addRoleAssignment(nonAdminUser, FolderAdminRole.class); + SecurityPolicyManager.savePolicyForTests(newParentPolicy, adminUser); + moveFolder(nonAdminUser, folder, newParent, HttpServletResponse.SC_FORBIDDEN); + + // Give nonAdminUser admin permission in the folder's current parent and try again (should now succeed) + policy = new MutableSecurityPolicy(folder.getParent().getPolicy()); + policy.addRoleAssignment(nonAdminUser, FolderAdminRole.class); + SecurityPolicyManager.savePolicyForTests(policy, adminUser); + moveFolder(nonAdminUser, folder, newParent, HttpServletResponse.SC_OK); + folder = ContainerManager.getForId(folder.getId()); // Refresh its path + assertNotNull(folder); + assertEquals(junit.getPath() + "/" + NEW_PARENT + "/" + FOLDER_NAME, folder.getPath()); + // Should be able to move it back + moveFolder(nonAdminUser, folder, junit, HttpServletResponse.SC_OK); + folder = ContainerManager.getForId(folder.getId()); // Refresh its path + assertNotNull(folder); + assertEquals(junit.getPath() + "/" + FOLDER_NAME, folder.getPath()); + + // Happy path -- admin user should be able to move folder to new parent and back + moveFolder(adminUser, folder, newParent, HttpServletResponse.SC_OK); + folder = ContainerManager.getForId(folder.getId()); // Refresh its path + assertNotNull(folder); + assertEquals(junit.getPath() + "/" + NEW_PARENT + "/" + FOLDER_NAME, folder.getPath()); + moveFolder(adminUser, folder, junit, HttpServletResponse.SC_OK); + folder = ContainerManager.getForId(folder.getId()); // Refresh its path + assertNotNull(folder); + assertEquals(junit.getPath() + "/" + FOLDER_NAME, folder.getPath()); + } + + private void moveFolder(User user, Container folder, Container newParent, int expectedResponseCode) throws Exception + { + JSONObject json = new JSONObject().put("container", folder.getId()).put("parent", newParent.getId()); + HttpServletRequest request = ViewServlet.mockRequest(RequestMethod.POST.name(), new ActionURL(MoveContainerAction.class, folder), user, Map.of("Content-Type", "application/json"), json.toString()); + MockHttpServletResponse response = ViewServlet.mockDispatch(request, null); + assertEquals("Unexpected response code", expectedResponseCode, response.getStatus()); + } + + private static void doCleanup() throws Exception + { + Container folder = ContainerManager.getForPath(JunitUtil.getTestContainer().getPath() + "/" + FOLDER_NAME); + if (folder != null) + { + ContainerManager.deleteAll(folder, TestContext.get().getUser()); + } + + Container newParent = ContainerManager.getForPath(JunitUtil.getTestContainer().getPath() + "/" + NEW_PARENT); + if (newParent != null) + { + ContainerManager.deleteAll(newParent, TestContext.get().getUser()); + } + + User u = UserManager.getUser(TEST_EMAIL); + if (u != null) + UserManager.deleteUser(u.getUserId()); + } + } } diff --git a/core/src/org/labkey/core/CoreModule.java b/core/src/org/labkey/core/CoreModule.java index f18b077e5df..5f5686d8232 100644 --- a/core/src/org/labkey/core/CoreModule.java +++ b/core/src/org/labkey/core/CoreModule.java @@ -1450,9 +1450,11 @@ public TabDisplayMode getTabDisplayMode() AdminController.TestCase.class, AdminController.WorkbookDeleteTestCase.class, AdminController.ImportFolderSourceScopingTestCase.class, + AdminController.RevertFolderScopingTestCase.class, AllowListType.TestCase.class, AttachmentServiceImpl.TestCase.class, CoreController.TestCase.class, + CoreController.MoveContainerTestCase.class, DataRegion.TestCase.class, DavController.TestCase.class, DavController.MoveActionContainerScopingTestCase.class, diff --git a/core/src/org/labkey/core/admin/AdminController.java b/core/src/org/labkey/core/admin/AdminController.java index 2a11953396c..da7c31c0dbb 100644 --- a/core/src/org/labkey/core/admin/AdminController.java +++ b/core/src/org/labkey/core/admin/AdminController.java @@ -8093,6 +8093,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 @@ -8515,6 +8517,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()); @@ -10442,10 +10447,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()); @@ -12676,4 +12681,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/core/src/org/labkey/core/user/UserController.java b/core/src/org/labkey/core/user/UserController.java index 881ce5309ed..218c1c7c005 100644 --- a/core/src/org/labkey/core/user/UserController.java +++ b/core/src/org/labkey/core/user/UserController.java @@ -2639,6 +2639,10 @@ protected Collection getProjectGroupUsers(GetUsersForm form, ApiSimpleResp if (null == group) throw new NotFoundException("Cannot find group with id " + groupId); + // Kanban #1924: Assure permission in the group's container + Container groupContainer = ContainerManager.getForId(group.getContainer()); + if (null != groupContainer && !groupContainer.hasPermission(getUser(), ReadPermission.class)) + throw new UnauthorizedException("You do not have permission to see information about the group '" + group.getName() + "'"); response.put("groupId", group.getUserId()); response.put("groupName", group.getName()); response.put("groupCaption", SecurityManager.getDisambiguatedGroupName(group)); diff --git a/core/src/org/labkey/core/webdav/DavController.java b/core/src/org/labkey/core/webdav/DavController.java index c48a22ddfd6..7a42127a2a6 100644 --- a/core/src/org/labkey/core/webdav/DavController.java +++ b/core/src/org/labkey/core/webdav/DavController.java @@ -57,12 +57,17 @@ import org.labkey.api.data.ContainerManager; import org.labkey.api.data.ConvertHelper; import org.labkey.api.data.Sort; +import org.labkey.api.exp.api.DataType; import org.labkey.api.exp.api.ExpData; +import org.labkey.api.exp.api.ExpProtocol; +import org.labkey.api.exp.api.ExpRun; import org.labkey.api.exp.api.ExperimentService; import org.labkey.api.files.FileContentService; import org.labkey.api.miniprofiler.MiniProfiler; import org.labkey.api.miniprofiler.RequestInfo; import org.labkey.api.module.ModuleLoader; +import org.labkey.api.pipeline.PipeRoot; +import org.labkey.api.pipeline.PipelineService; import org.labkey.api.premium.AntiVirusService; import org.labkey.api.premium.PremiumService; import org.labkey.api.query.ValidationException; @@ -3555,17 +3560,24 @@ WebdavStatus doMethod() throws DavException, IOException throw new DavException(WebdavStatus.SC_METHOD_NOT_ALLOWED); } + WebdavResource resource = resolvePath(); + if (null == resource) + { + throw new DavException(WebdavStatus.SC_NOT_FOUND); + } + + if (!resource.canWrite(getUser(), true)) + { + throw new DavException(WebdavStatus.SC_FORBIDDEN); + } + FileTime lastModified = getLastModified(getRequest().getInputStream()); if (lastModified != null) { try { - WebdavResource resource = resolvePath(); - if (resource != null) - { - resource.setLastModified(lastModified.toMillis()); - } + resource.setLastModified(lastModified.toMillis()); } catch (ConversionException ignored) {} } @@ -3575,12 +3587,6 @@ WebdavStatus doMethod() throws DavException, IOException assert track(writer); try { - WebdavResource resource = resolvePath(); - if (resource == null) - { - throw new DavException(WebdavStatus.SC_NOT_FOUND); - } - XMLResourceWriter resourceWriter = new XMLResourceWriter(writer); resourceWriter.beginResponse(getResponse()); @@ -3743,8 +3749,8 @@ WebdavStatus doMethod() throws DavException, IOException if (!src.canRead(getUser(), true)) return unauthorized(src); - // MOVE is effectively a delete operation from the source's perspective so confirm access - if (!src.canDelete(getUser(), true)) + // MOVE removes the resource from the source location, so confirm the caller could delete it there. + if (!src.canMove(getUser())) return unauthorized(src); if (exists && !dest.canWrite(getUser(),true) || !exists && !dest.canCreate(getUser(),true)) return unauthorized(dest); @@ -6738,11 +6744,78 @@ public void testMoveActionRequiresTargetCreate() throws Exception assertTrue("Destination file should exist after a successful move", FileUtil.appendName(targetDir, "moved.txt").exists()); } + // A file that backs experiment data (used by a run) can be moved but not deleted. Test via Java API + @Test + public void testImportedFileCanMoveButNotDelete() throws Exception + { + File dir = ensureFilesDir(_folder); + File srcFile = writeFile(dir, "imported.txt"); + importFileIntoRun(_folder, srcFile); + + User admin = getAdmin(); + WebdavResource resource = WebdavService.get().lookup(filesPath(_folder).append("imported.txt")); + assertNotNull("Imported file should resolve through the resolver", resource); + assertTrue("Imported file should exist", resource.exists()); + assertFalse("Precondition: a file used by a run must report a non-empty action list", resource.getActions(admin).isEmpty()); + + List messages = new ArrayList<>(); + assertFalse("Admin must NOT be able to delete a file that has been imported by an assay", resource.canDelete(admin, true, messages)); + assertTrue("canDelete() should explain that the file was imported by an assay", messages.stream().anyMatch(m -> m.contains("imported by an assay"))); + assertTrue("Admin MUST still be able to move a file that has been imported by an assay", resource.canMove(admin)); + } + + // A file that backs experiment data (used by a run) can be moved but not deleted. Test via WebDAV HTTP request + @Test + public void testMoveActionMovesImportedFileButDeleteForbidden() throws Exception + { + File dir = ensureFilesDir(_folder); + + // DELETE of a file imported by a run is forbidden, and the file survives. + File imported = writeFile(dir, "imported.txt"); + importFileIntoRun(_folder, imported); + MockHttpServletResponse deleteResp = doDelete(_folder, filesPath(_folder).append(imported.getName()), getAdmin()); + assertEquals("DELETE of a file imported by an assay must be forbidden", HttpServletResponse.SC_FORBIDDEN, deleteResp.getStatus()); + assertTrue("File must still exist after a forbidden delete", imported.exists()); + + // MOVE of an imported file succeeds, relocating it within the file root. + Path src = filesPath(_folder).append(imported.getName()); + Path dest = filesPath(_folder).append("moved.txt"); + MockHttpServletResponse moveResp = doMove(_folder, src, dest, getAdmin()); + assertEquals("MOVE of a file imported by an assay must succeed", HttpServletResponse.SC_CREATED, moveResp.getStatus()); + assertFalse("Source file should no longer exist after a successful move", imported.exists()); + assertTrue("Destination file should exist after a successful move", FileUtil.appendName(dir, dest.getName()).exists()); + } + + // Ensure special nodes like @pipeline can't be deleted or moved + @Test + public void testNonDeletableNodeIsNotMovable() throws Exception + { + // Configure an explicit pipeline root so the @pipeline webdav node resolves to a PipelineFolderResource. + File fileRoot = ensureFilesDir(_folder).getParentFile(); + File pipelineDir = FileUtil.appendName(fileRoot, "pipelineOverrideRoot"); + if (!pipelineDir.exists()) + assertTrue("Test requires a writable pipeline root directory", pipelineDir.mkdirs()); + PipelineService.get().setPipelineRoot(getAdmin(), _folder, PipelineService.PRIMARY_ROOT, false, pipelineDir.toURI()); + + WebdavResource pipelineNode = WebdavService.get().lookup(pipelinePath(_folder)); + assertNotNull("Test requires the @pipeline webdav node to resolve to a PipelineFolderResource", pipelineNode); + assertNotNull("The pipeline node must be backed by a writable file root", pipelineNode.getFile()); + + User admin = getAdmin(); + assertFalse("The pipeline node must never be deletable", pipelineNode.canDelete(admin, true, null)); + assertFalse("A node that is not deletable must also not be movable", pipelineNode.canMove(admin)); + } + private static Path filesPath(Container c) { return WebdavService.getPath().append(c.getParsedPath()).append(FileContentService.FILES_LINK); } + private static Path pipelinePath(Container c) + { + return WebdavService.getPath().append(c.getParsedPath()).append(FileContentService.PIPELINE_LINK); + } + private static File ensureFilesDir(Container c) { WebdavResource filesNode = WebdavService.get().lookup(filesPath(c)); @@ -6756,7 +6829,12 @@ private static File ensureFilesDir(Container c) private static File writeFile(File dir) throws IOException { - File f = FileUtil.appendName(dir, "secret.txt"); + return writeFile(dir, "secret.txt"); + } + + private static File writeFile(File dir, String name) throws IOException + { + File f = FileUtil.appendName(dir, name); try (FileOutputStream os = new FileOutputStream(f)) { os.write("secret".getBytes(StandardCharsets.UTF_8)); @@ -6764,6 +6842,47 @@ private static File writeFile(File dir) throws IOException return f; } + // Registers the file as experiment data used by a run, so the resolver's WebdavResource reports a + // non-empty action list - the same state that, in production, blocks deletion of an imported file + // while still permitting a move. + private void importFileIntoRun(Container c, File file) throws Exception + { + ExperimentService expSvc = ExperimentService.get(); + User admin = getAdmin(); + + ExpData data = expSvc.createData(c, new DataType("Data"), file.getName()); + data.setDataFileURI(file.toURI()); + data.save(admin); + + ExpRun run = expSvc.createExperimentRun(c, "import of " + file.getName()); + PipeRoot pipeRoot = PipelineService.get().findPipelineRoot(c); + assertNotNull("Test requires a pipeline root for " + c.getName(), pipeRoot); + run.setFilePathRoot(pipeRoot.getRootPath()); + ExpProtocol protocol = expSvc.ensureSampleDerivationProtocol(admin); + run.setProtocol(protocol); + + ViewBackgroundInfo info = new ViewBackgroundInfo(c, admin, null); + // Wiring the data as a run input is enough for getRunsUsingDatas() to associate the run with the file. + expSvc.saveSimpleExperimentRun(run, Collections.emptyMap(), Map.of(data, "Data"), Collections.emptyMap(), + Collections.emptyMap(), Collections.emptyMap(), info, null, false); + } + + private static MockHttpServletResponse doDelete(Container sourceContainer, Path srcResource, User user) throws Exception + { + String srcWebdav = srcResource.toString(); + String servletPath = "/" + WebdavService.getServletPath(); + + HttpServletRequest base = ViewServlet.mockRequest("DELETE", new ActionURL(name, "delete", sourceContainer), user, Map.of(), null); + MockHttpServletRequest req = (MockHttpServletRequest) base; + req.setServletPath(servletPath); + req.setPathInfo(srcWebdav.substring(servletPath.length())); + req.setRequestURI(srcWebdav); + + MockHttpServletResponse resp = new MockHttpServletResponse(); + new WebdavServlet(false).service(req, resp); + return resp; + } + private static MockHttpServletResponse doMove(Container sourceContainer, Path srcResource, Path destResource, User user) throws Exception { String srcWebdav = srcResource.toString(); diff --git a/experiment/src/org/labkey/experiment/ScriptsResourceProvider.java b/experiment/src/org/labkey/experiment/ScriptsResourceProvider.java index 8e42e00d262..b37d26ac508 100644 --- a/experiment/src/org/labkey/experiment/ScriptsResourceProvider.java +++ b/experiment/src/org/labkey/experiment/ScriptsResourceProvider.java @@ -142,6 +142,12 @@ public boolean canList(User user, boolean forRead) return hasAccess(user); } + @Override + public boolean canMove(User user) + { + return hasAccess(user); + } + @Override public boolean shouldIndex() { diff --git a/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java b/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java index 528fd27df02..88d440d4e97 100644 --- a/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java @@ -387,10 +387,23 @@ public void clearDataClassCache(@Nullable Container c) @Override public @Nullable ExpRunImpl getExpRun(long rowId) + { + return getExpRun(rowId, null); + } + + @Override + public @Nullable ExpRunImpl getExpRun(long rowId, @Nullable Container container) { SimpleFilter filter = new SimpleFilter(FieldKey.fromParts(ExpRunTable.Column.RowId.name()), rowId); ExperimentRun run = new TableSelector(getTinfoExperimentRun(), filter, null).getObject(ExperimentRun.class); - return run == null ? null : new ExpRunImpl(run); + if (run == null) + return null; + + // GitHub Issue #1892: if container provided, ensure the run belongs to the container + if (container != null && !run.getContainer().equals(container)) + return null; + + return new ExpRunImpl(run); } private List getExpRuns(SimpleFilter filter) diff --git a/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java b/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java index 7b387cc5663..547732f041d 100644 --- a/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java +++ b/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java @@ -154,6 +154,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; @@ -206,7 +207,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; @@ -742,7 +746,8 @@ public ApiResponse execute(SimpleApiJsonForm form, BindException errors) throws JSONArray runIds = json.getJSONArray("runIds"); for (int i = 0; i < runIds.length(); i++) { - ExpRunImpl run = ExperimentServiceImpl.get().getExpRun(runIds.getInt(i)); + // Kanban #1924: Make sure the run belongs to the current container. + ExpRunImpl run = ExperimentServiceImpl.get().getExpRun(runIds.getInt(i), getContainer()); if (run != null) { runs.add(run); @@ -2373,7 +2378,8 @@ public static class CheckDataFileAction extends MutatingApiAction public void validateForm(DataFileForm form, Errors errors) { _data = form.lookupData(); - if (_data == null) + // Not using ensureCorrectContainer() because we don't redirect API actions + if (_data == null || !getContainer().equals(_data.getContainer())) { errors.reject(ERROR_MSG, "No ExpData found for id: " + form.getRowId()); } @@ -3695,6 +3701,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()); } } @@ -4670,6 +4680,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(); @@ -5236,7 +5252,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 (long 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; } @@ -5977,9 +6005,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; } @@ -6019,9 +6047,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; } @@ -6044,9 +6072,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 @@ -6057,9 +6084,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 @@ -6540,10 +6566,11 @@ public boolean handlePost(MoveRunsForm form, BindException errors) for (Long 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(); @@ -7928,7 +7955,13 @@ public Object execute(EntitySequenceForm form, BindException errors) throws Exce { ExpSampleType sampleType = SampleTypeService.get().getSampleType(form.getRowId()); if (sampleType != null) + { + // Kanban #1924: Assure permission in the sample type's container + if (!sampleType.getContainer().hasPermission(getUser(), ReadPermission.class)) + throw new UnauthorizedException("You do not have permission to read this sample type."); value = sampleType.getCurrentGenId(); + } + } else { @@ -7940,7 +7973,12 @@ else if (DataClassDomainKind.NAME.equalsIgnoreCase(form.getKindName())) { ExpDataClass dataClass = ExperimentService.get().getDataClass(form.getRowId()); if (dataClass != null) + { + // Kanban #1924: assure permission in the data class's container + if (!dataClass.getContainer().hasPermission(getUser(), ReadPermission.class)) + throw new UnauthorizedException("You do not have permission to read this data class."); value = dataClass.getCurrentGenId(); + } } ApiSimpleResponse resp = new ApiSimpleResponse(); @@ -7981,6 +8019,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(); } @@ -8364,7 +8405,340 @@ public void testDataClassAttachmentContainerScoping() throws Exception .addParameter("lsid", lsid) .addParameter("name", attachmentName); assertStatus(HttpServletResponse.SC_OK, get(ownUrl, admin)); + + ActionURL checkDataFileUrl = new ActionURL(CheckDataFileAction.class, folderB) + .addParameter("rowId", data.getRowId()); + assertStatus(HttpServletResponse.SC_OK, post(checkDataFileUrl, admin)); + assertStatus(HttpServletResponse.SC_FORBIDDEN, post(checkDataFileUrl, readerA)); // No perms + checkDataFileUrl.setContainer(folderA); + assertStatus(HttpServletResponse.SC_FORBIDDEN, post(checkDataFileUrl, readerA)); // Has read in folder A, but not admin + resp = post(checkDataFileUrl, admin); // Wrong container. Not found. + assertStatus(HttpServletResponse.SC_BAD_REQUEST, resp); + JSONObject json = new JSONObject(resp.getContentAsString()); + assertEquals("No ExpData found for id: " + data.getRowId(), json.get("exception")); + } + + @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); + long 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); + long 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"); + long 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()); + long 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); + long 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); + } + + // 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/experiment/src/org/labkey/experiment/controllers/property/PropertyController.java b/experiment/src/org/labkey/experiment/controllers/property/PropertyController.java index 5ba3dfc65d8..f54e39047ca 100644 --- a/experiment/src/org/labkey/experiment/controllers/property/PropertyController.java +++ b/experiment/src/org/labkey/experiment/controllers/property/PropertyController.java @@ -2195,7 +2195,7 @@ public Object execute(PropertyUsagesForm form, BindException errors) throws Exce List usages = null; if (form.getPropertyIds() != null) { - usages = OntologyManager.findPropertyUsages(getUser(), form.getPropertyIds(), form.maxUsageCount); + usages = OntologyManager.findPropertyUsagesByIds(getUser(), getContainer(), form.getPropertyIds(), form.maxUsageCount); } else if (form.getPropertyURIs() != null) { diff --git a/filecontent/src/org/labkey/filecontent/FileContentController.java b/filecontent/src/org/labkey/filecontent/FileContentController.java index 77bf9bbc72f..2810aaaf0e2 100644 --- a/filecontent/src/org/labkey/filecontent/FileContentController.java +++ b/filecontent/src/org/labkey/filecontent/FileContentController.java @@ -16,10 +16,13 @@ package org.labkey.filecontent; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; import org.apache.commons.io.IOUtils; import org.apache.commons.lang3.StringUtils; import org.json.JSONArray; import org.json.JSONObject; +import org.junit.Test; import org.labkey.api.action.ApiJsonWriter; import org.labkey.api.action.ApiResponse; import org.labkey.api.action.ApiResponseWriter; @@ -78,19 +81,27 @@ import org.labkey.api.query.UserSchema; import org.labkey.api.query.ValidationError; import org.labkey.api.reader.Readers; +import org.labkey.api.security.MutableSecurityPolicy; import org.labkey.api.security.RequiresLogin; import org.labkey.api.security.RequiresNoPermission; import org.labkey.api.security.RequiresPermission; import org.labkey.api.security.RequiresSiteAdmin; +import org.labkey.api.security.SecurityManager; +import org.labkey.api.security.SecurityManager.NewUserStatus; +import org.labkey.api.security.SecurityPolicyManager; import org.labkey.api.security.User; +import org.labkey.api.security.UserManager; +import org.labkey.api.security.ValidEmail; import org.labkey.api.security.permissions.AbstractActionPermissionTest; import org.labkey.api.security.permissions.AdminOperationsPermission; import org.labkey.api.security.permissions.AdminPermission; import org.labkey.api.security.permissions.InsertPermission; import org.labkey.api.security.permissions.ReadPermission; +import org.labkey.api.security.roles.ReaderRole; import org.labkey.api.util.DOM; import org.labkey.api.util.FileUtil; import org.labkey.api.util.HtmlString; +import org.labkey.api.util.JunitUtil; import org.labkey.api.util.LinkBuilder; import org.labkey.api.util.MimeMap; import org.labkey.api.util.NetworkDrive; @@ -107,6 +118,7 @@ import org.labkey.api.view.Portal; import org.labkey.api.view.UnauthorizedException; import org.labkey.api.view.ViewContext; +import org.labkey.api.view.ViewServlet; import org.labkey.api.view.WebPartView; import org.labkey.api.view.template.PageConfig; import org.labkey.api.webdav.WebdavResource; @@ -114,10 +126,13 @@ import org.labkey.api.writer.HtmlWriter; import org.labkey.filecontent.message.FileEmailConfig; import org.labkey.filecontent.message.ShortMessageDigest; +import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.validation.BindException; import org.springframework.validation.Errors; import org.springframework.validation.ObjectError; +import org.springframework.web.bind.annotation.RequestMethod; import org.springframework.web.servlet.ModelAndView; +import org.springframework.web.servlet.mvc.Controller; import java.io.BufferedReader; import java.io.File; @@ -719,6 +734,9 @@ public Set> getChildren(NodeForm form, BindException errors) if (c == null) c = ContainerManager.getRoot(); + if (!c.hasPermission(getUser(), ReadPermission.class)) + throw new UnauthorizedException("You do not have permission to read this summary."); + ActionURL browse = new ActionURL(BeginAction.class, c); Set> children = FileContentServiceImpl.getInstance().getNodes(form.isShowOverridesOnly(), browse.getEncodedLocalURIString(), c); @@ -755,6 +773,9 @@ protected Set> getChildren(NodeForm form, BindException erro if (c == null) c = ContainerManager.getRoot(); + if (!c.hasPermission(getUser(), ReadPermission.class)) + throw new UnauthorizedException("You do not have permission to read this project summary."); + Set> children = new LinkedHashSet<>(); FileContentService svc = FileContentService.get(); @@ -923,7 +944,15 @@ public void validateForm(SimpleApiJsonForm form, Errors errors) { FileContentServiceImpl fileContentService = FileContentServiceImpl.getInstance(); WebdavResource resource = fileContentService.getResource(String.valueOf(fileProps.get("id"))); - if (resource != null && !resource.getActions(getUser()).isEmpty()) + + // GitHub Issue 1243: check resource container + if (resource == null || !getContainer().getEntityId().equals(resource.getContainerId())) + { + errors.reject(ERROR_MSG, String.format(FILE_PROP_ERROR, "Invalid file", fileProps.get("id"))); + return; + } + + if (!resource.getActions(getUser()).isEmpty()) { errors.reject(ERROR_MSG, String.format(FILE_PROP_ERROR, resource.getName(), "has been previously processed, properties cannot be edited")); return; @@ -1604,5 +1633,35 @@ public void testActionPermissions() new SendShortDigestAction() ); } + + @Test + public void testSummaryActions() throws Exception + { + Container folder = JunitUtil.getTestContainer(); + User adminUser = TestContext.get().getUser(); + + // Happy path -- admin should be able to invoke summary actions in root + testAction(FileContentSummaryAction.class, folder, adminUser, HttpServletResponse.SC_OK); + testAction(FileContentProjectSummaryAction.class, folder, adminUser, HttpServletResponse.SC_OK); + + NewUserStatus newUserStatus = SecurityManager.addUser(new ValidEmail("testSummaryActions@myDomain.com"), null); + User nonAdminUser = newUserStatus.getUser(); + MutableSecurityPolicy policy = new MutableSecurityPolicy(folder.getPolicy()); + policy.addRoleAssignment(nonAdminUser, ReaderRole.class); + SecurityPolicyManager.savePolicyForTests(policy, adminUser); + + // Non-admin user should be forbidden + testAction(FileContentSummaryAction.class, folder, nonAdminUser, HttpServletResponse.SC_FORBIDDEN); + testAction(FileContentProjectSummaryAction.class, folder, nonAdminUser, HttpServletResponse.SC_FORBIDDEN); + + UserManager.deleteUser(nonAdminUser.getUserId()); + } + + private void testAction(Class actionClass, Container folder, User user, int expectedResponseCode) throws Exception + { + HttpServletRequest request = ViewServlet.mockRequest(RequestMethod.POST.name(), new ActionURL(actionClass, folder), user, Map.of("Content-Type", "application/json"), null); + MockHttpServletResponse response = ViewServlet.mockDispatch(request, null); + assertEquals("Unexpected response code", expectedResponseCode, response.getStatus()); + } } } diff --git a/issues/src/org/labkey/issue/IssuesController.java b/issues/src/org/labkey/issue/IssuesController.java index 903fd42a4b3..38e68a5cbdd 100644 --- a/issues/src/org/labkey/issue/IssuesController.java +++ b/issues/src/org/labkey/issue/IssuesController.java @@ -28,6 +28,8 @@ import org.jetbrains.annotations.Nullable; import org.json.JSONArray; import org.json.JSONObject; +import org.junit.After; +import org.junit.Before; import org.junit.Test; import org.labkey.api.action.ApiResponse; import org.labkey.api.action.ApiSimpleResponse; @@ -106,6 +108,7 @@ import org.labkey.api.security.permissions.InsertPermission; import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.security.permissions.UpdatePermission; +import org.labkey.api.security.roles.EditorRole; import org.labkey.api.security.roles.FolderAdminRole; import org.labkey.api.security.roles.OwnerRole; import org.labkey.api.security.roles.ReaderRole; @@ -151,6 +154,7 @@ import org.labkey.issue.view.IssuesListView; import org.springframework.beans.MutablePropertyValues; import org.springframework.beans.PropertyValues; +import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.util.LinkedCaseInsensitiveMap; import org.springframework.validation.BindException; import org.springframework.validation.Errors; @@ -1748,6 +1752,13 @@ public URLHelper getSuccessURL(IssuesDomainKindProperties form) } } + private static List getSelectableAssignmentGroups(Container c, User user) + { + return SecurityManager.getGroups(c.getProject(), true).stream() + .filter(group -> !group.isGuests() && (!group.isUsers() || user.hasRootAdminPermission())) + .toList(); + } + @Marshal(Marshaller.Jackson) @RequiresPermission(AdminPermission.class) public static class GetProjectGroupsAction extends ReadOnlyApiAction @@ -1757,7 +1768,7 @@ public Object execute(Object form, BindException errors) { List groups = new ArrayList<>(); - SecurityManager.getGroups(getContainer().getProject(), true).stream().filter(group -> !group.isGuests() && (!group.isUsers() || getUser().hasRootAdminPermission())).forEach(group -> { + getSelectableAssignmentGroups(getContainer(), getUser()).forEach(group -> { String displayText = (group.isProjectGroup() ? "" : "Site: ") + group.getName(); UserGroupForm userGroups = new UserGroupForm(); @@ -1781,8 +1792,11 @@ public Object execute(UserGroupForm form, BindException errors) if (null != form.getGroupId()) { + // ensure group is in scope for the container Group group = SecurityManager.getGroup(form.getGroupId()); - if (group != null) + boolean inScope = group != null && getSelectableAssignmentGroups(getContainer(), getUser()) + .stream().anyMatch(g -> g.getUserId() == group.getUserId()); + if (inScope) { for (User user : SecurityManager.getAllGroupMembers(group, MemberType.ACTIVE_USERS, group.isUsers())) { @@ -2475,4 +2489,79 @@ private static void ensureIssuesEnabled(Container c) } } } + + /** + * GitHub Issue #1243 regression test. + */ + public static class GetUsersForGroupScopingTestCase extends AbstractContainerScopingTest + { + private static final String PROJECT_A = "IssuesGroupScopingA"; + private static final String PROJECT_B = "IssuesGroupScopingB"; + + private Container _projectA; + private Group _groupA; + private Group _groupB; + private User _member; + + @Before + public void setup() throws Exception + { + deleteProjects(); + User admin = getAdmin(); + _projectA = ContainerManager.createContainer(ContainerManager.getRoot(), PROJECT_A, admin); + Container projectB = ContainerManager.createContainer(ContainerManager.getRoot(), PROJECT_B, admin); + + // Security groups must be associated with a project, so each group's container is its owning project. + _groupA = SecurityManager.createGroup(_projectA, "ScopingGroupA", admin); + _groupB = SecurityManager.createGroup(projectB, "ScopingGroupB", admin); + + // A user who can be assigned issues in project A (UpdatePermission via Editor) and is a member of both groups. + _member = createUserInRole(_projectA, EditorRole.class); + SecurityManager.addMember(_groupA, _member); + SecurityManager.addMember(_groupB, _member); + } + + @After + public void tearDown() + { + deleteProjects(); + } + + @Test + public void doesNotEnumerateAnotherProjectsGroupMembers() throws Exception + { + // group B is in a different project from the source request. + String content = requestUsersForGroup(_projectA, _groupB).getContentAsString(); + assertFalse("A group id from another project must not enumerate that group's members through this folder", + content.contains("\"userId\" : " + _member.getUserId())); + } + + @Test + public void enumeratesOwnProjectGroupMembers() throws Exception + { + // Control: the request project's own group resolves in scope and returns its members. + String content = requestUsersForGroup(_projectA, _groupA).getContentAsString(); + assertTrue("An in-project group id must enumerate its members", + content.contains("\"userId\" : " + _member.getUserId())); + } + + private MockHttpServletResponse requestUsersForGroup(Container project, Group group) throws Exception + { + ActionURL url = new ActionURL(GetUsersForGroupAction.class, project) + .addParameter("groupId", group.getUserId()); + return get(url, getAdmin()); + } + + private void deleteProjects() + { + User admin = getAdmin(); + for (String name : new String[]{PROJECT_A, PROJECT_B}) + { + Container c = ContainerManager.getForPath("/" + name); + if (c != null) + ContainerManager.deleteAll(c, admin); + } + _projectA = null; + } + } } diff --git a/issues/src/org/labkey/issue/IssuesModule.java b/issues/src/org/labkey/issue/IssuesModule.java index 44ea17cf54e..1b06988a612 100644 --- a/issues/src/org/labkey/issue/IssuesModule.java +++ b/issues/src/org/labkey/issue/IssuesModule.java @@ -192,7 +192,8 @@ public ActionURL getTabURL(Container c, User user) { return Set.of( org.labkey.issue.model.IssueManager.TestCase.class, - org.labkey.issue.IssuesController.MoveActionContainerScopingTestCase.class + org.labkey.issue.IssuesController.MoveActionContainerScopingTestCase.class, + org.labkey.issue.IssuesController.GetUsersForGroupScopingTestCase.class ); } diff --git a/list/src/org/labkey/list/ListModule.java b/list/src/org/labkey/list/ListModule.java index 5ee59fbf316..0bb4d32456c 100644 --- a/list/src/org/labkey/list/ListModule.java +++ b/list/src/org/labkey/list/ListModule.java @@ -232,7 +232,8 @@ public Collection getProvisionedSchemaNames() { return Set.of( ListManager.TestCase.class, - ListWriter.TestCase.class + ListWriter.TestCase.class, + ListAuditProvider.TestCase.class ); } } diff --git a/list/src/org/labkey/list/controllers/ListController.java b/list/src/org/labkey/list/controllers/ListController.java index 2a5b90e4e8f..906cd9cbb61 100644 --- a/list/src/org/labkey/list/controllers/ListController.java +++ b/list/src/org/labkey/list/controllers/ListController.java @@ -974,7 +974,14 @@ public ModelAndView getView(ListItemDetailsForm form, BindException errors) if (eventRowId == null || eventRowId <= 0) return HtmlView.of("Unable to resolve event details. An event \"rowId\" must be specified."); - ListAuditProvider.ListAuditEvent event = AuditLogService.get().getAuditEvent(getUser(), ListManager.LIST_AUDIT_EVENT, eventRowId); + ListAuditProvider.ListAuditEvent event = AuditLogService.get().getAuditEvent( + getUser(), ListManager.LIST_AUDIT_EVENT, eventRowId, ContainerFilter.current(_list.getContainer(), getUser())); + + // Tie the loaded event to the URL-requested listId — rowId is user-controlled (CWE-639). + if (!ListAuditProvider.auditEventMatchesList(event, _list.getListId(), _list.getContainer())) + { + event = null; + } if (event != null) { diff --git a/list/src/org/labkey/list/model/ListAuditProvider.java b/list/src/org/labkey/list/model/ListAuditProvider.java index b8583f2bc82..1fdef977390 100644 --- a/list/src/org/labkey/list/model/ListAuditProvider.java +++ b/list/src/org/labkey/list/model/ListAuditProvider.java @@ -35,13 +35,19 @@ import org.labkey.api.query.FieldKey; import org.labkey.api.query.UserSchema; import org.labkey.api.util.StringExpressionFactory; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; +import org.junit.Assert; +import org.junit.Test; import java.util.ArrayList; import java.util.Collections; +import java.util.Date; import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; public class ListAuditProvider extends AbstractAuditTypeProvider implements AuditTypeProvider @@ -128,6 +134,22 @@ public int moveEvents(Container targetContainer, List listRowEntityIds) return moveEvents(targetContainer, COLUMN_NAME_LIST_ITEM_ENTITY_ID, listRowEntityIds); } + /** + * Verifies that a loaded {@link ListAuditEvent} actually pertains to the requested list and container before its + * old/new record maps are surfaced to the caller. + *

+ * Called by {@code ListController.ListItemDetailsAction} to close CWE-639 (IDOR via user-controlled {@code rowId}): + * without this predicate, a user with audit-read access in container A could pass {@code listId=X&rowId=N-for-Y} + * and have List Y's audit payload render inside List X's details page. The container check is defense-in-depth + * in case the audit schema's default ContainerFilter is ever changed away from {@code Current}. + */ + public static boolean auditEventMatchesList(@Nullable ListAuditEvent event, int expectedListId, @NotNull Container expectedContainer) + { + return event != null + && event.getListId() == expectedListId + && Objects.equals(event.getContainer(), expectedContainer); + } + public static class ListAuditEvent extends DetailedAuditTypeEvent { private int _listId; @@ -204,6 +226,67 @@ public Map getAuditLogMessageElements() } } + public static class TestCase extends Assert + { + private static final String CONTAINER_A = "11111111-1111-1111-1111-111111111111"; + private static final String CONTAINER_B = "22222222-2222-2222-2222-222222222222"; + + @Test + public void matches_whenListIdAndContainerAgree() + { + Container c = testContainer(CONTAINER_A); + assertTrue(auditEventMatchesList(eventFor(42, c), 42, c)); + } + + @Test + public void rejects_nullEvent() + { + assertFalse(auditEventMatchesList(null, 42, testContainer(CONTAINER_A))); + } + + @Test + public void rejects_wrongListId() + { + // Event's listId is 99, but URL asked for list 42: the cross-list-in-same-container + // attack. Without this check, List 42's details page renders List 99's payload. + Container c = testContainer(CONTAINER_A); + assertFalse(auditEventMatchesList(eventFor(99, c), 42, c)); + } + + @Test + public void rejects_wrongContainer() + { + // Defense-in-depth: even if a future audit-schema CF change ever let an event from + // another container leak through getAuditEvent(), this check would still block it. + assertFalse(auditEventMatchesList( + eventFor(42, testContainer(CONTAINER_B)), 42, testContainer(CONTAINER_A))); + } + + @Test + public void rejects_nullEventContainer() + { + // Event present but its container is null (could happen if the audit event was + // hand-constructed or persisted without a container): must not match. + assertFalse(auditEventMatchesList(eventFor(42, null), 42, testContainer(CONTAINER_A))); + } + + private static Container testContainer(String guid) + { + // Container.equals compares the GUID id, so any non-null parent / name / rowId + // values are fine for this test; only the id field is read by the assertion. + return new Container(null, "junit-" + guid.substring(0, 8), guid, 0, 0, new Date(0), 0, false); + } + + private static ListAuditEvent eventFor(int listId, @Nullable Container container) + { + ListAuditEvent event = new ListAuditEvent(); + event.setListId(listId); + if (container != null) + event.setContainer(container); + return event; + } + } + public static class ListAuditDomainKind extends AbstractAuditDomainKind { public static final String NAME = "ListAuditDomain"; diff --git a/mothership/src/org/labkey/mothership/MothershipController.java b/mothership/src/org/labkey/mothership/MothershipController.java index 88b31be6bc7..3257c7079d3 100644 --- a/mothership/src/org/labkey/mothership/MothershipController.java +++ b/mothership/src/org/labkey/mothership/MothershipController.java @@ -54,6 +54,7 @@ import org.labkey.api.data.TableInfo; import org.labkey.api.data.TableSelector; import org.labkey.api.module.AllowedDuringUpgrade; +import org.labkey.api.module.ModuleLoader; import org.labkey.api.query.DetailsURL; import org.labkey.api.query.FieldKey; import org.labkey.api.query.QueryService; @@ -334,12 +335,21 @@ public static class ShowServerSessionDetailAction extends SimpleViewAction + public static class ServerSessionForm { - public ServerSessionForm() + private Integer _serverSessionId = null; + + public Integer getServerSessionId() + { + return _serverSessionId; + } + + public void setServerSessionId(Integer serverSessionId) { - super(ServerSession.class, MothershipManager.get().getTableInfoServerSession()); + _serverSessionId = serverSessionId; } } @@ -1887,8 +1888,8 @@ public static class ContainerScopingTestCase extends AbstractContainerScopingTes public void testUpdateInstallationContainerScoping() throws Exception { User admin = getAdmin(); - Container folderA = createContainer("A"); - Container folderB = createContainer("B"); + Container folderA = createContainer("A", ModuleLoader.getInstance().getModule(MothershipModule.class)); + Container folderB = createContainer("B", ModuleLoader.getInstance().getModule(MothershipModule.class)); // An installation row that lives in folder B ServerInstallation si = new ServerInstallation(); @@ -1917,6 +1918,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/mothership/src/org/labkey/mothership/MothershipManager.java b/mothership/src/org/labkey/mothership/MothershipManager.java index 8b5bcf91999..26a7654b779 100644 --- a/mothership/src/org/labkey/mothership/MothershipManager.java +++ b/mothership/src/org/labkey/mothership/MothershipManager.java @@ -43,6 +43,7 @@ import org.labkey.api.util.ReentrantLockWithName; import org.labkey.api.util.StringUtilsLabKey; import org.labkey.api.util.logging.LogHelper; +import org.labkey.api.view.NotFoundException; import java.io.IOException; import java.util.ArrayList; @@ -212,6 +213,13 @@ public SoftwareRelease ensureSoftwareRelease(Container container, String revisio } } + public SoftwareRelease getSoftwareRelease(int softwareReleaseId, Container container) + { + SimpleFilter filter = SimpleFilter.createContainerFilter(container); + filter.addCondition(FieldKey.fromString("SoftwareReleaseId"), softwareReleaseId); + return new TableSelector(getTableInfoSoftwareRelease(), filter, null).getObject(SoftwareRelease.class); + } + public ServerInstallation getServerInstallation(@NotNull String serverGUID, @NotNull String serverHostName, @NotNull Container c) { SimpleFilter filter = SimpleFilter.createContainerFilter(c); @@ -227,6 +235,13 @@ public ServerSession getServerSession(String serverSessionGUID, Container c) return new TableSelector(getTableInfoServerSession(), filter, null).getObject(ServerSession.class); } + public ServerSession getServerSession(int serverSessionId, Container c) + { + SimpleFilter filter = SimpleFilter.createContainerFilter(c); + filter.addCondition(FieldKey.fromString("ServerSessionId"), serverSessionId); + return new TableSelector(getTableInfoServerSession(), filter, null).getObject(ServerSession.class); + } + public ExceptionStackTrace getExceptionStackTrace(String stackTraceHash, String containerId) { SimpleFilter filter = new SimpleFilter(FieldKey.fromParts("Container"), containerId); @@ -598,6 +613,12 @@ public void setStatusCakeApiKey(String statusCakeApiKey) public void updateSoftwareRelease(Container container, User user, SoftwareRelease bean) { + // Verify the target row actually belongs to this container before updating. The raw Table.update below is + // keyed only on the primary key, so without this check a user with UpdatePermission in one folder could edit + // (and, via setContainer, re-home) a SoftwareRelease owned by another folder. + if (getSoftwareRelease(bean.getSoftwareReleaseId(), container) == null) + throw new NotFoundException("SoftwareRelease not found in this folder: " + bean.getSoftwareReleaseId()); + bean.setContainer(container.getId()); Table.update(user, getTableInfoSoftwareRelease(), bean, bean.getSoftwareReleaseId()); } diff --git a/mothership/src/org/labkey/mothership/query/MothershipSchema.java b/mothership/src/org/labkey/mothership/query/MothershipSchema.java index b4fc75f7476..4305a81f66f 100644 --- a/mothership/src/org/labkey/mothership/query/MothershipSchema.java +++ b/mothership/src/org/labkey/mothership/query/MothershipSchema.java @@ -27,6 +27,7 @@ import org.labkey.api.data.DataColumn; import org.labkey.api.data.ForeignKey; import org.labkey.api.data.JdbcType; +import org.labkey.api.data.RenderContext; import org.labkey.api.data.SQLFragment; import org.labkey.api.data.TableInfo; import org.labkey.api.data.dialect.SqlDialect; @@ -411,18 +412,25 @@ public FilteredTable createExceptionReportTable(ContainerFilte FilteredTable result = new FilteredTable<>(MothershipManager.get().getTableInfoExceptionReport(), this, cf); result.setDetailsURL(AbstractTableInfo.LINK_DISABLER); result.wrapAllColumns(true); + // Reports are submitted by anonymous users, so untrusted. Don't render these two URLs as links. result.getMutableColumnOrThrow("URL").setDisplayColumnFactory(colInfo -> - { - DataColumn result1 = new DataColumn(colInfo); - result1.setURLExpression(StringExpressionFactory.create("${URL}", false)); - return result1; - }); + new DataColumn(colInfo) + { + @Override + protected String renderURLorValueURL(RenderContext ctx) + { + return null; + } + }); result.getMutableColumnOrThrow("ReferrerURL").setDisplayColumnFactory(colInfo -> - { - DataColumn result12 = new DataColumn(colInfo); - result12.setURLExpression(StringExpressionFactory.create("${ReferrerURL}", false)); - return result12; - }); + new DataColumn(colInfo) + { + @Override + protected String renderURLorValueURL(RenderContext ctx) + { + return null; + } + }); // Container column is on another table so join to it to filter appropriately SQLFragment containerSQL = new SQLFragment("ExceptionStackTraceId IN (SELECT ExceptionStackTraceId FROM "); diff --git a/pipeline/src/org/labkey/pipeline/PipelineController.java b/pipeline/src/org/labkey/pipeline/PipelineController.java index 733bcbd29ef..2bb1cd6c404 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; @@ -45,6 +47,7 @@ import org.labkey.api.data.Container; 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; @@ -62,6 +65,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; @@ -75,17 +79,20 @@ 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; 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; @@ -115,6 +122,7 @@ import org.labkey.pipeline.status.StatusController; import org.labkey.vfs.FileLike; 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; @@ -1456,6 +1464,15 @@ public static class SavePipelineTriggerAction extends MutatingApiAction msg) return false; } + @Override + public boolean canMove(User user) + { + // The pipeline folder node is never deletable (see canDelete()), so it must not be movable either. + // FileSystemResource.canMove() relaxes only the assay-import restriction, not this categorical block. + return false; + } + @Override protected boolean hasAccess(User user) { diff --git a/pipeline/src/org/labkey/pipeline/api/PipelineStatusManager.java b/pipeline/src/org/labkey/pipeline/api/PipelineStatusManager.java index 48c421f4513..0d72f730285 100644 --- a/pipeline/src/org/labkey/pipeline/api/PipelineStatusManager.java +++ b/pipeline/src/org/labkey/pipeline/api/PipelineStatusManager.java @@ -690,7 +690,7 @@ 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) { 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 094e1d9b459..03a66e344cc 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.DeletePermission; import org.labkey.api.security.permissions.ReadPermission; 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; @@ -98,6 +100,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; @@ -398,8 +401,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); } @@ -478,7 +485,7 @@ public Object execute(StatusDetailsForm form, BindException errors) throws Excep Container c = getContainerCheckAdmin(); PipelineStatusFile psf = getStatusFile(form.getRowId()); - if (psf == null || !getContainer().equals(psf.lookupContainer())) + if (psf == null || psf.lookupContainer() == null || !psf.lookupContainer().hasPermission(getUser(), ReadPermission.class)) throw new NotFoundException("Could not find status file for rowId " + form.getRowId()); var status = StatusDetailsBean.create(c, psf, form.getOffset(), form.getCount()); @@ -495,8 +502,13 @@ 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) { @@ -511,7 +523,7 @@ public ActionURL getRedirectURL(RowIdForm form) } } - return urlDetails(c, form.getRowId()); + return urlDetails(sfContainer, form.getRowId()); } } @@ -526,8 +538,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) @@ -559,7 +575,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(); @@ -933,8 +950,12 @@ public boolean handlePost(RowIdForm form, BindException errors) for (Long 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); @@ -1103,28 +1124,133 @@ 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)); - // The API is scoped to its own container: addressing B's job through folder A is 404, regardless of the - // caller's rights in B. This is the case that fails without the fix (the unscoped action would serve B's - // job through folder A). - // A caller who can read folder A but NOT folder B: + // This API authorizes against the job's OWN container (folder B), not the container in the URL, so it + // intentionally supports referencing a job from another container -- but only for a caller who can read the + // container the job actually lives in. + // A caller who can read folder A but NOT folder B must still get 404: the job must not be revealed to + // someone without rights to its container. This is the case that fails without the fix. assertStatus(HttpServletResponse.SC_NOT_FOUND, get(foreignUrl, readerA)); - // ...and a site admin, who CAN read folder B, still gets 404 through folder A (no cross-container redirect). - assertStatus(HttpServletResponse.SC_NOT_FOUND, get(foreignUrl, admin)); + // A site admin, who CAN read folder B, is served B's job through folder A -- the supported cross-container reference. + assertStatus(HttpServletResponse.SC_OK, get(foreignUrl, admin)); - // Positive control: addressing the job through its own container still succeeds. + // Positive control: addressing the job through its own container also succeeds for a caller who can read it. 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 + long 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 289aee0ebb1..7dae62ad851 100644 --- a/query/src/org/labkey/query/QueryModule.java +++ b/query/src/org/labkey/query/QueryModule.java @@ -396,6 +396,7 @@ public Set getSchemaNames() return Set.of( ModuleReportCache.TestCase.class, OlapController.TestCase.class, + OlapController.ContainerScopingTestCase.class, QueryController.SaveRowsTestCase.class, QueryController.TestCase.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 47737a06241..79d6ad28784 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/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 df88d82234c..41a5cc701d9 100644 --- a/specimen/src/org/labkey/specimen/actions/SpecimenController.java +++ b/specimen/src/org/labkey/specimen/actions/SpecimenController.java @@ -64,6 +64,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.PipelineJob; import org.labkey.api.pipeline.PipelineService; @@ -90,6 +91,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; @@ -154,8 +156,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.QueryBasedSpecimenTransform; import org.labkey.specimen.importer.RequestabilityManager; import org.labkey.specimen.importer.SimpleSpecimenImporter; @@ -1182,12 +1186,16 @@ public static class SpecimenEventsRedirectAction extends SimpleViewAction { @@ -5565,26 +5577,26 @@ public SelectedSpecimensAction() protected ModelAndView getHtmlView(SpecimenViewTypeForm form, BindException errors) throws Exception { Study study = getStudyRedirectIfNull(); - Set> ptidVisits = new HashSet<>(); + Set ptidVisits = new HashSet<>(); for (ParticipantDataset pd : getFilterPds()) { if (pd.getSequenceNum() == null) { - ptidVisits.add(new Pair<>(pd.getParticipantId(), null)); + ptidVisits.add(new PtidVisit(pd.getParticipantId(), null)); } else if (study.getTimepointType() != TimepointType.VISIT && pd.getVisitDate() != null) { - ptidVisits.add(new Pair<>(pd.getParticipantId(), DateUtil.formatDate(pd.getContainer(), pd.getVisitDate()))); + ptidVisits.add(new PtidVisit(pd.getParticipantId(), DateUtil.formatDate(pd.getContainer(), pd.getVisitDate()))); } else { Visit visit = pd.getSequenceNum() != null ? StudyInternalService.get().getVisitForSequence(study, pd.getSequenceNum()) : null; - ptidVisits.add(new Pair<>(pd.getParticipantId(), visit != null ? visit.getLabel() : "" + StudyInternalService.get().formatSequenceNum(pd.getSequenceNum()))); + ptidVisits.add(new PtidVisit(pd.getParticipantId(), visit != null ? visit.getLabel() : StudyInternalService.get().formatSequenceNum(pd.getSequenceNum()))); } } SpecimenQueryView view = createInitializedQueryView(form, errors, form.getExportType() != null, null); JspView header = new JspView<>("/org/labkey/specimen/view/specimenHeader.jsp", - new SpecimenHeaderBean(getViewContext(), view, ptidVisits)); + new SpecimenHeaderBean(getViewContext(), view, ptidVisits)); return new VBox(header, view); } @@ -5990,5 +6002,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/specimen/src/org/labkey/specimen/actions/SpecimenHeaderBean.java b/specimen/src/org/labkey/specimen/actions/SpecimenHeaderBean.java index fe32d12dc24..fd0cbd575df 100644 --- a/specimen/src/org/labkey/specimen/actions/SpecimenHeaderBean.java +++ b/specimen/src/org/labkey/specimen/actions/SpecimenHeaderBean.java @@ -5,13 +5,13 @@ import org.labkey.api.query.FieldKey; import org.labkey.api.query.QueryService; import org.labkey.api.specimen.SpecimenQuerySchema; -import org.labkey.specimen.query.SpecimenQueryView; import org.labkey.api.study.Study; import org.labkey.api.study.StudyService; -import org.labkey.api.util.Pair; import org.labkey.api.view.ActionURL; import org.labkey.api.view.NotFoundException; import org.labkey.api.view.ViewContext; +import org.labkey.specimen.actions.SpecimenController.PtidVisit; +import org.labkey.specimen.query.SpecimenQueryView; import java.util.Collections; import java.util.Iterator; @@ -24,7 +24,7 @@ public final class SpecimenHeaderBean private final ActionURL _otherViewURL; private final ViewContext _viewContext; private final boolean _showingVials; - private final Set> _filteredPtidVisits; + private final Set _ptidVisits; private Integer _selectedRequest; @@ -33,7 +33,7 @@ public SpecimenHeaderBean(ViewContext context, SpecimenQueryView view) this(context, view, Collections.emptySet()); } - public SpecimenHeaderBean(ViewContext context, SpecimenQueryView view, Set> filteredPtidVisits) throws RuntimeException + public SpecimenHeaderBean(ViewContext context, SpecimenQueryView view, Set ptidVisits) throws RuntimeException { Map params = context.getRequest().getParameterMap(); @@ -90,11 +90,11 @@ public SpecimenHeaderBean(ViewContext context, SpecimenQueryView view, Set> getFilteredPtidVisits() + public Set getPtidVisits() { - return _filteredPtidVisits; + return _ptidVisits; } public boolean isSingleVisitFilter() { - if (getFilteredPtidVisits().isEmpty()) + if (getPtidVisits().isEmpty()) return false; - Iterator> visitIt = getFilteredPtidVisits().iterator(); - String firstVisit = visitIt.next().getValue(); - while (visitIt.hasNext()) + Iterator ptidVisit = getPtidVisits().iterator(); + String firstVisit = ptidVisit.next().visit(); + while (ptidVisit.hasNext()) { - if (!Objects.equals(firstVisit, visitIt.next().getValue())) + if (!Objects.equals(firstVisit, ptidVisit.next().visit())) return false; } return true; diff --git a/specimen/src/org/labkey/specimen/report/SpecimenVisitReportParameters.java b/specimen/src/org/labkey/specimen/report/SpecimenVisitReportParameters.java index 882a5e078b7..7106faff06b 100644 --- a/specimen/src/org/labkey/specimen/report/SpecimenVisitReportParameters.java +++ b/specimen/src/org/labkey/specimen/report/SpecimenVisitReportParameters.java @@ -289,9 +289,13 @@ protected void addParticipantGroupFilter(SimpleFilter filter, int ptidListId) ParticipantGroup group = ParticipantGroupService.get().getParticipantGroup(getContainer(), getUser(), ptidListId); if (group != null) { - SQLFragment sql = new SQLFragment(); - sql.append("(").append(StudyService.get().getSubjectColumnName(getContainer())).append(" IN (SELECT "); - sql.append("ParticipantId FROM ").append(SpecimenSchema.get().getTableInfoParticipantGroupMap()).append(" WHERE GroupId = ?))").add(ptidListId); + SQLFragment sql = new SQLFragment("(") + .appendIdentifier(StudyService.get().getSubjectColumnName(getContainer())) + .append(" IN (SELECT ") + .append("ParticipantId FROM ") + .append(SpecimenSchema.get().getTableInfoParticipantGroupMap()) + .append(" WHERE GroupId = ?))") + .add(ptidListId); filter.addWhereClause(sql); } } @@ -303,11 +307,15 @@ protected void addCohortFilter(SimpleFilter filter, CohortFilter cohortFilter) if (cohortFilter == CohortService.get().getUnassignedCohortFilter()) { - filter.addWhereClause("(" + StudyService.get().getSubjectColumnName(getContainer()) + " IN\n" + - "(SELECT ParticipantId FROM study.participant WHERE CurrentCohortId IS NULL AND Container = ?)" + - " OR (" + StudyService.get().getSubjectColumnName(getContainer()) + - " NOT IN (SELECT ParticipantId FROM study.participant WHERE Container = ?)))", - new Object[] { getContainer().getId(), getContainer().getId()}); + SQLFragment whereClause = new SQLFragment("(") + .appendIdentifier(StudyService.get().getSubjectColumnName(getContainer())) + .append(" IN\n" + "(SELECT ParticipantId FROM study.participant WHERE CurrentCohortId IS NULL AND Container = ?)" + " OR (") + .appendIdentifier(StudyService.get().getSubjectColumnName(getContainer())) + .append(" NOT IN (SELECT ParticipantId FROM study.participant WHERE Container = ?)))") + .add(getContainer()) + .add(getContainer()); + + filter.addWhereClause(whereClause); } else if (cohortFilter != null) { @@ -317,18 +325,25 @@ else if (cohortFilter != null) switch (cohortFilter.getType()) { case DATA_COLLECTION: - filter.addWhereClause("CollectionCohort = ? AND Container = ?", - new Object[] { cohortId, getContainer().getId()} ); + filter.addWhereClause(new SQLFragment("CollectionCohort = ? AND Container = ?") + .add(cohortId) + .add(getContainer()) + ); break; case PTID_CURRENT: - filter.addWhereClause(StudyService.get().getSubjectColumnName(getContainer()) + " IN\n" + - "(SELECT ParticipantId FROM study.participant WHERE CurrentCohortId = ? AND Container = ?)", - new Object[] { cohortId, getContainer().getId()}); + filter.addWhereClause(new SQLFragment() + .appendIdentifier(StudyService.get().getSubjectColumnName(getContainer())) + .append(" IN\n(SELECT ParticipantId FROM study.participant WHERE CurrentCohortId = ? AND Container = ?)") + .add(cohortId) + .add(getContainer()) + ); break; case PTID_INITIAL: - filter.addWhereClause(StudyService.get().getSubjectColumnName(getContainer()) + " IN\n" + - "(SELECT ParticipantId FROM study.participant WHERE InitialCohortId = ? AND Container = ?)", - new Object[] { cohortId, getContainer().getId()}); + filter.addWhereClause(new SQLFragment() + .appendIdentifier(StudyService.get().getSubjectColumnName(getContainer())) + .append(" IN\n(SELECT ParticipantId FROM study.participant WHERE InitialCohortId = ? AND Container = ?)") + .add(cohortId).add(getContainer()) + ); break; } } diff --git a/specimen/src/org/labkey/specimen/view/manageRequirement.jsp b/specimen/src/org/labkey/specimen/view/manageRequirement.jsp index 68109262af7..6721bbc61de 100644 --- a/specimen/src/org/labkey/specimen/view/manageRequirement.jsp +++ b/specimen/src/org/labkey/specimen/view/manageRequirement.jsp @@ -84,7 +84,7 @@ Description - <%= unsafe(requirement.getDescription()) %> + <%= h(requirement.getDescription()) %> <% if (!bean.isRequestManager()) diff --git a/specimen/src/org/labkey/specimen/view/specimenHeader.jsp b/specimen/src/org/labkey/specimen/view/specimenHeader.jsp index b0599197eab..b1e2c7b5089 100644 --- a/specimen/src/org/labkey/specimen/view/specimenHeader.jsp +++ b/specimen/src/org/labkey/specimen/view/specimenHeader.jsp @@ -15,16 +15,18 @@ * limitations under the License. */ %> -<%@ page import="org.labkey.api.security.permissions.AdminPermission"%> -<%@ page import="org.labkey.api.study.StudyService"%> -<%@ page import="org.labkey.api.study.StudyUrls"%> -<%@ page import="org.labkey.api.util.Pair"%> +<%@ page import="org.apache.commons.lang3.StringUtils" %> +<%@ page import="org.labkey.api.security.permissions.AdminPermission" %> +<%@ page import="org.labkey.api.study.StudyService" %> +<%@ page import="org.labkey.api.study.StudyUrls" %> +<%@ page import="org.labkey.api.util.HtmlStringBuilder" %> <%@ page import="org.labkey.api.view.ActionURL" %> <%@ page import="org.labkey.api.view.HttpView" %> <%@ page import="org.labkey.api.view.JspView" %> <%@ page import="org.labkey.api.view.template.ClientDependencies" %> <%@ page import="org.labkey.specimen.actions.ShowSearchAction" %> <%@ page import="org.labkey.specimen.actions.SpecimenController" %> +<%@ page import="org.labkey.specimen.actions.SpecimenController.PtidVisit" %> <%@ page import="org.labkey.specimen.actions.SpecimenController.SpecimensAction" %> <%@ page import="org.labkey.specimen.actions.SpecimenHeaderBean" %> <%@ page import="java.util.Iterator" %> @@ -74,47 +76,57 @@ <%=link("Search", ShowSearchAction.getShowSearchURL(getContainer(), bean.isShowingVials()))%>  <%=link("Reports", urlFor(SpecimenController.AutoReportListAction.class)) %> <% - if (!bean.getFilteredPtidVisits().isEmpty()) + if (!bean.getPtidVisits().isEmpty()) { // get the first visit label: - StringBuilder filterString = new StringBuilder(); - filterString.append("This view is displaying specimens only from "); - boolean usePlural = bean.getFilteredPtidVisits().size() != 1; + HtmlStringBuilder builder = HtmlStringBuilder.of() + .unsafeAppend("") + .append("This view is displaying specimens only from "); + boolean usePlural = bean.getPtidVisits().size() != 1; if (bean.isSingleVisitFilter()) { - filterString.append(h((usePlural?subjectNounPlural:subjectNounSingle).toLowerCase())).append(" "); - for (Iterator> it = bean.getFilteredPtidVisits().iterator(); it.hasNext();) + builder.append((usePlural ? subjectNounPlural : subjectNounSingle).toLowerCase()) + .append(" "); + for (Iterator it = bean.getPtidVisits().iterator(); it.hasNext();) { - String ptid = it.next().getKey(); - filterString.append(ptid); + String ptid = it.next().ptid(); + builder.append(ptid); if (it.hasNext()) - filterString.append(", "); + builder.append(", "); } - String visit = bean.getFilteredPtidVisits().iterator().next().getValue(); + String visit = bean.getPtidVisits().iterator().next().visit(); if (visit != null) - filterString.append(" at visit ").append(visit); - filterString.append(".
"); + builder.append(" at visit ").append(visit); + + builder.append(".") + .unsafeAppend("
"); } else { - filterString.append(" the following ").append(h(subjectNounSingle.toLowerCase())).append("/visit ").append(usePlural?"pairs":"pair").append(":
"); - for (Iterator> it = bean.getFilteredPtidVisits().iterator(); it.hasNext();) + builder.append(" the following ") + .append(subjectNounSingle.toLowerCase()) + .append("/visit ").append(usePlural ? "pairs" : "pair") + .append(":") + .unsafeAppend("
"); + for (Iterator it = bean.getPtidVisits().iterator(); it.hasNext();) { - Pair ptidVisit = it.next(); - filterString.append(ptidVisit.getKey()).append("/").append(ptidVisit.getValue()); + PtidVisit ptidVisit = it.next(); + builder.append(ptidVisit.ptid()) + .append("/") + .append(StringUtils.trimToEmpty(ptidVisit.visit())); if (it.hasNext()) - filterString.append(", "); + builder.append(", "); } - filterString.append("."); + builder.append("."); } - ActionURL noFitlerUrl = getViewContext().cloneActionURL().setAction(SpecimensAction.class); + ActionURL noFilterUrl = getViewContext().cloneActionURL().setAction(SpecimensAction.class); %>

- +
<%= unsafe(filterString.toString()) %>
<%=builder%>

-<%= link("Remove " + subjectNounSingle + "/Visit Filter", noFitlerUrl)%><% +<%= link("Remove " + subjectNounSingle + "/Visit Filter", noFilterUrl)%><% } %>
diff --git a/study/src/org/labkey/study/StudyModule.java b/study/src/org/labkey/study/StudyModule.java index 1ed015a1e01..ce6b5da3f61 100644 --- a/study/src/org/labkey/study/StudyModule.java +++ b/study/src/org/labkey/study/StudyModule.java @@ -132,15 +132,18 @@ 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; import org.labkey.study.controllers.StudyController; import org.labkey.study.controllers.StudyDefinitionController; import org.labkey.study.controllers.StudyPropertiesController; +import org.labkey.study.controllers.publish.PublishConfirmContainerScopingTest; import org.labkey.study.controllers.publish.PublishController; import org.labkey.study.controllers.reports.ReportsController; import org.labkey.study.controllers.security.SecurityController; +import org.labkey.study.dataset.DataStatesTest; import org.labkey.study.dataset.DatasetAuditProvider; import org.labkey.study.dataset.DatasetNotificationInfoProvider; import org.labkey.study.dataset.DatasetSnapshotProvider; @@ -762,16 +765,20 @@ public WebPartView getWebPartView(@NotNull ViewContext portalCtx, @NotNull Po { return Set.of( DatasetDefinition.TestCleanupOrphanedDatasetDomains.class, - ParticipantGroupManager.ParticipantGroupTestCase.class, + DataStatesTest.class, + ParticipantGroupManager.ContainerScopingTestCase.class, StudyImpl.ProtocolDocumentTestCase.class, StudyManager.StudySnapshotTestCase.class, StudyManager.VisitCreationTestCase.class, StudyModule.TestCase.class, VisitImpl.TestCase.class, + DatasetController.DatasetAuditHistoryScopingTestCase.class, DatasetUpdateService.TestCase.class, DatasetLsidImportHelper.TestCase.class, - org.labkey.study.controllers.CreateChildStudyAction.ContainerScopingTestCase.class - ); + PublishConfirmContainerScopingTest.class, + CreateChildStudyAction.ContainerScopingTestCase.class, + StudyController.ContainerScopingTestCase.class, + ReportsController.ContainerScopingTestCase.class); } @Override @@ -789,6 +796,7 @@ public WebPartView getWebPartView(@NotNull ViewContext portalCtx, @NotNull Po DatasetDataWriter.TestCase.class, DefaultStudyDesignWriter.TestCase.class, ParticipantIdImportHelper.ParticipantIdTest.class, + ReportsController.TestCase.class, SequenceNumImportHelper.SequenceNumTest.class, StudyImpl.DateMathTestCase.class ); diff --git a/study/src/org/labkey/study/assay/AssayPublishConfirmAction.java b/study/src/org/labkey/study/assay/AssayPublishConfirmAction.java index b5c41686bbe..2cba5ac3422 100644 --- a/study/src/org/labkey/study/assay/AssayPublishConfirmAction.java +++ b/study/src/org/labkey/study/assay/AssayPublishConfirmAction.java @@ -61,7 +61,7 @@ public static class AssayPublishConfirmForm extends PublishConfirmForm if (_protocol == null) { if (getRowId() != null) - _protocol = ExperimentService.get().getExpProtocol(getRowId().intValue()); + _protocol = ExperimentService.get().getExpProtocol(getRowId()); else throw new NotFoundException("Protocol ID not specified."); diff --git a/study/src/org/labkey/study/controllers/DatasetController.java b/study/src/org/labkey/study/controllers/DatasetController.java index eaeacef9ca8..a9bc0a38bd0 100644 --- a/study/src/org/labkey/study/controllers/DatasetController.java +++ b/study/src/org/labkey/study/controllers/DatasetController.java @@ -17,8 +17,11 @@ package org.labkey.study.controllers; import org.apache.commons.lang3.Strings; +import org.junit.Before; +import org.junit.Test; import org.labkey.api.action.FormViewAction; import org.labkey.api.action.SimpleViewAction; +import org.labkey.api.audit.AbstractAuditTypeProvider; import org.labkey.api.audit.AuditLogService; import org.labkey.api.audit.permissions.CanSeeAuditLogPermission; import org.labkey.api.audit.view.AuditChangesView; @@ -26,11 +29,15 @@ import org.labkey.api.data.DataRegion; import org.labkey.api.data.DbScope; 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.study.EditDatasetRowForm; import org.labkey.api.study.InsertUpdateAction; import org.labkey.api.study.Study; +import org.labkey.api.study.TimepointType; +import org.labkey.api.util.GUID; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.view.ActionURL; import org.labkey.api.view.HtmlView; @@ -42,6 +49,7 @@ import org.labkey.study.model.DatasetDefinition; import org.labkey.study.model.StudyImpl; import org.labkey.study.model.StudyManager; +import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.validation.BindException; import org.springframework.validation.Errors; import org.springframework.web.servlet.ModelAndView; @@ -118,6 +126,8 @@ public ModelAndView getView(DatasetAuditHistoryForm form, BindException errors) VBox view = new VBox(); + // getAuditEvent() resolves against the current container's audit schema (default ContainerFilter.Current + + // CanSeeAuditLog clause), so a foreign auditRowId resolves to null and cannot disclose a record in another folder. DatasetAuditProvider.DatasetAuditEvent event = AuditLogService.get().getAuditEvent(getUser(), DatasetAuditProvider.DATASET_AUDIT_EVENT, auditRowId); if (event != null) { @@ -283,4 +293,58 @@ public static class DatasetAuditHistoryForm public void setAuditRowId(int auditRowId) {this.auditRowId = auditRowId;} } + + public static class DatasetAuditHistoryScopingTestCase extends AbstractContainerScopingTest + { + private static final String FIELD_VALUE = GUID.makeGUID(); + private Container _requestContainer; + private long _foreignAuditRowId; + + @Before + public void setup() + { + _requestContainer = createContainer("Request"); + StudyImpl study = new StudyImpl(_requestContainer, "Request Study"); + study.setTimepointType(TimepointType.VISIT); + StudyManager.getInstance().createTestStudy(getAdmin(), study); + + _foreignAuditRowId = addDatasetAuditEvent(createContainer("Event")); + } + + @Test + public void doesNotDiscloseForeignFolderDatasetAuditEvent() throws Exception + { + // Even the site auditor, who may see audit logs everywhere, must not be served another folder's dataset + // audit record when requesting it through this folder's URL: the lookup is scoped to the request container. + String content = requestAuditHistory(_foreignAuditRowId, getAdmin()).getContentAsString(); + + assertFalse("A foreign auditRowId must not disclose dataset audit record in another folder", content.contains(FIELD_VALUE)); + assertTrue("Should fall through to the 'no additional details' view", content.contains("No additional details recorded")); + } + + @Test + public void disclosesOwnFolderDatasetAuditEvent() throws Exception + { + // Control: an event in the request folder must be shown, proving the negative case isn't passing simply + // because the view never renders record data. + long localRowId = addDatasetAuditEvent(_requestContainer); + String content = requestAuditHistory(localRowId, getAdmin()).getContentAsString(); + assertTrue("An audit event in the request folder must be shown", content.contains(FIELD_VALUE)); + } + + private long addDatasetAuditEvent(Container c) + { + DatasetAuditProvider.DatasetAuditEvent event = new DatasetAuditProvider.DatasetAuditEvent(c, "test dataset audit", 1); + event.setNewRecordMap(AbstractAuditTypeProvider.encodeForDataMap(Map.of("SecretField", FIELD_VALUE))); + event = AuditLogService.get().addEvent(getAdmin(), event); + return event.getRowId(); + } + + private MockHttpServletResponse requestAuditHistory(long auditRowId, User user) throws Exception + { + ActionURL url = new ActionURL(DatasetAuditHistoryAction.class, _requestContainer) + .addParameter("auditRowId", auditRowId); + return get(url, user); + } + } } diff --git a/study/src/org/labkey/study/controllers/ParticipantGroupController.java b/study/src/org/labkey/study/controllers/ParticipantGroupController.java index 5ab663d44e3..a73a66f3cc9 100644 --- a/study/src/org/labkey/study/controllers/ParticipantGroupController.java +++ b/study/src/org/labkey/study/controllers/ParticipantGroupController.java @@ -160,26 +160,21 @@ public static class UpdateParticipantCategory extends MutatingApiAction categories; if (form.getCategoryType() != null && form.getCategoryType().equals("manual")) { @@ -274,11 +269,10 @@ public ApiResponse execute(GetParticipantCategoriesForm form, BindException erro } JSONArray defs = new JSONArray(); - for (ParticipantCategoryImpl pc : categories) - { defs.put(pc.toJSON()); - } + + ApiSimpleResponse resp = new ApiSimpleResponse(); resp.put("success", true); resp.put("categories", defs); @@ -295,7 +289,6 @@ public ApiResponse execute(Object form, BindException errors) Container c = getContainer(); User u = getUser(); JSONArray jsonGroups = new JSONArray(); - ApiSimpleResponse resp = new ApiSimpleResponse(); for (ParticipantCategoryImpl category : ParticipantGroupManager.getInstance().getParticipantCategories(c, u)) { @@ -303,11 +296,12 @@ public ApiResponse execute(Object form, BindException errors) { if (group.hasLiveFilter()) { - jsonGroups.put(group.toJSON(false /*includeParticipants*/)); + jsonGroups.put(group.toJSON(false)); } } } + ApiSimpleResponse resp = new ApiSimpleResponse(); resp.put("success", true); resp.put("participantGroups", jsonGroups); return resp; @@ -537,7 +531,7 @@ public ApiResponse execute(BrowseGroupsForm form, BindException errors) { GroupType groupType = GroupType.valueOf(type); Set selectedParticipants = new HashSet<>(); - switch(groupType) + switch (groupType) { case participantGroup: // the api will support either requesting a specific participant category/group or all of @@ -545,7 +539,8 @@ public ApiResponse execute(BrowseGroupsForm form, BindException errors) if (form.getCategoryId() != -1) { ParticipantCategoryImpl category = ParticipantGroupManager.getInstance().getParticipantCategory(getContainer(), getUser(), form.getCategoryId()); - addCategory(form, category, groups); + if (category != null) + addCategory(form, category, groups); } else if (form.getGroupId() != -1) { @@ -558,6 +553,7 @@ else if (form.getGroupId() != -1) // NOTE: This is intentional as 'personal' groups are not secured and can be shared. group = ParticipantGroupManager.getInstance().getParticipantGroupFromGroupRowId(getContainer(), getUser(), form.getGroupId()); } + if (group != null) { ParticipantCategoryImpl category = ParticipantGroupManager.getInstance().getParticipantCategory(getContainer(), getUser(), group.getCategoryId()); @@ -620,11 +616,8 @@ private void addCategory(BrowseGroupsForm form, ParticipantCategoryImpl category { if (form.isIncludePrivateGroups() || category.isShared()) { - Set selectedParticipants = new HashSet<>(); - for (ParticipantGroup group : category.getGroups()) { - selectedParticipants.addAll(group.getParticipantSet()); JSONGroup jsonGroup = new JSONGroup(group, category); if (form.includeParticipantIds()) jsonGroup.setParticipantIds(group.getParticipantSet()); @@ -1114,16 +1107,15 @@ public ApiResponse execute(ParticipantGroupSpecification form, BindException err } /** - * Code to check whether an implicitly created category needs to be deleted so we don't accumulate orpaned list type + * Code to check whether an implicitly created category needs to be deleted so we don't accumulate orphaned list type * categories. - * */ private void deleteImplicitCategory(Integer prevCategoryId, ParticipantCategoryImpl current) throws ValidationException { if (prevCategoryId != null) { ParticipantCategoryImpl oldCategory = ParticipantGroupManager.getInstance().getParticipantCategory(getContainer(), getUser(), prevCategoryId); - if (oldCategory.getType().equals("list") && !current.getType().equals("list")) + if (oldCategory != null && "list".equals(oldCategory.getType()) && !"list".equals(current.getType())) { ParticipantGroupManager.getInstance().deleteParticipantCategory(getContainer(), getUser(), oldCategory); } diff --git a/study/src/org/labkey/study/controllers/StudyController.java b/study/src/org/labkey/study/controllers/StudyController.java index 0b1dab537da..5fa4ba134e2 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; @@ -1577,7 +1579,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) @@ -2903,12 +2906,16 @@ public static class DatasetItemDetailsAction extends SimpleViewAction("/org/labkey/study/view/sendParticipantGroup.jsp", form, errors); @@ -7820,4 +7827,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/publish/PublishConfirmContainerScopingTest.java b/study/src/org/labkey/study/controllers/publish/PublishConfirmContainerScopingTest.java new file mode 100644 index 00000000000..5685ce2b157 --- /dev/null +++ b/study/src/org/labkey/study/controllers/publish/PublishConfirmContainerScopingTest.java @@ -0,0 +1,117 @@ +/* + * Copyright (c) 2026 LabKey Corporation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.labkey.study.controllers.publish; + +import org.junit.Before; +import org.junit.Test; +import org.labkey.api.data.Container; +import org.labkey.api.exp.PropertyType; +import org.labkey.api.exp.api.ExpSampleType; +import org.labkey.api.exp.api.SampleTypeService; +import org.labkey.api.gwt.client.model.GWTPropertyDescriptor; +import org.labkey.api.security.User; +import org.labkey.api.security.permissions.AbstractContainerScopingTest; +import org.labkey.api.security.roles.EditorRole; +import org.labkey.api.study.StudyService; +import org.labkey.api.study.TimepointType; +import org.labkey.api.view.ActionURL; +import org.labkey.api.view.UnauthorizedException; +import org.labkey.api.view.ViewContext; +import org.springframework.validation.BindException; + +import java.util.List; + +public class PublishConfirmContainerScopingTest extends AbstractContainerScopingTest +{ + private Container _source; + private Container _target; + private ExpSampleType _sampleType; + + @Before + public void setup() throws Exception + { + _source = createContainer("Source"); + _target = createContainer("Target"); + + StudyService.get().createStudy(_target, getAdmin(), "STUDY-4 scoping target", TimepointType.DATE, false); + _sampleType = createSampleType(_source); + + } + + @Test + public void testConfirmRejectsTargetStudyWithoutInsertPermission() throws Exception + { + // Editor in the source folder only: holds InsertPermission where the action runs, but none in the target study + User editor = createUserInRole(_source, EditorRole.class); + + assertFalse("Linking into a target study the caller cannot insert into must be rejected", + validate(_source, _sampleType, _target, editor)); + } + + @Test + public void testConfirmAllowsTargetStudyWithInsertPermission() throws Exception + { + // Positive control: the same caller is also granted insert (Editor) in the target study, so the guard must not + // fire — proving it rejects only the cross-container case rather than every link + User editor = createUserInRole(_source, EditorRole.class); + grantRole(editor, _target, EditorRole.class); + + assertTrue("Linking into a target study the caller can insert into must be allowed", + validate(_source, _sampleType, _target, editor)); + } + + /** + * Run the publish-confirm form's validation as {@code user}, linking {@code sampleType} (in {@code source}) to the + * study in {@code target}, returns true if validation succeeds, false otherwise. + */ + private boolean validate(Container source, ExpSampleType sampleType, Container target, User user) + { + ActionURL url = new ActionURL("study", "sampleTypePublishConfirm", source); + ViewContext context = ViewContext.getMockViewContext(user, source, url, false); + + SampleTypePublishConfirmAction action = new SampleTypePublishConfirmAction(); + action.setViewContext(context); + + SampleTypePublishConfirmAction.SampleTypePublishConfirmForm form = new SampleTypePublishConfirmAction.SampleTypePublishConfirmForm(); + form.setViewContext(context); + form.setRowId(sampleType.getRowId()); + form.setTargetStudy(new String[]{target.getId()}); + form.setReturnUrl(url.toString()); + + try + { + BindException errors = new BindException(form, "form"); + action.validateCommand(form, errors); + } + catch (UnauthorizedException e) + { + return false; + } + return true; + } + + private ExpSampleType createSampleType(Container c) throws Exception + { + List props = List.of( + new GWTPropertyDescriptor("Name", "string"), + new GWTPropertyDescriptor("Date", PropertyType.DATE.getTypeUri()), + new GWTPropertyDescriptor("PTID", "string") + ); + + return SampleTypeService.get().createSampleType(c, getAdmin(), "STUDY4ScopingSamples", null, + props, List.of(), -1,-1,-1,-1,null); + } +} diff --git a/study/src/org/labkey/study/controllers/reports/ReportsController.java b/study/src/org/labkey/study/controllers/reports/ReportsController.java index 67cce89bcb1..2ffcab9dabc 100644 --- a/study/src/org/labkey/study/controllers/reports/ReportsController.java +++ b/study/src/org/labkey/study/controllers/reports/ReportsController.java @@ -22,6 +22,7 @@ import org.json.JSONArray; import org.json.JSONException; import org.json.JSONObject; +import org.junit.Test; import org.labkey.api.action.Action; import org.labkey.api.action.ActionType; import org.labkey.api.action.ApiResponse; @@ -42,6 +43,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; @@ -49,9 +56,9 @@ import org.labkey.api.reports.report.view.ReportUtil; import org.labkey.api.reports.report.view.ScriptReportBean; import org.labkey.api.security.RequiresLogin; -import org.labkey.api.security.RequiresNoPermission; 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.AdminPermission; import org.labkey.api.security.permissions.InsertPermission; import org.labkey.api.security.permissions.ReadPermission; @@ -63,6 +70,8 @@ import org.labkey.api.study.reports.CrosstabReportDescriptor; import org.labkey.api.util.ImageUtil; import org.labkey.api.util.PageFlowUtil; +import org.labkey.api.util.ReturnURLString; +import org.labkey.api.util.TestContext; import org.labkey.api.util.URLHelper; import org.labkey.api.util.UniqueID; import org.labkey.api.view.ActionURL; @@ -71,6 +80,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; @@ -281,8 +291,14 @@ public boolean handlePost(SaveReportViewForm form, BindException errors) @Override public ActionURL getSuccessURL(SaveReportViewForm form) { - if (!StringUtils.isBlank(form.getRedirectUrl())) - return new ActionURL(form.getRedirectUrl()); + // Issue: redirectUrl is client-controlled. ReturnURLString validates it at bind time. + ReturnURLString redirectUrl = form.getRedirectUrl(); + if (redirectUrl != null && !redirectUrl.isEmpty()) + { + ActionURL url = redirectUrl.getActionURL(); + if (url != null) + return url; + } // after the save just redirect to the newly created view, ask the report for its run URL Report r = ReportService.get().getReport(getContainer(), _savedReportId); @@ -315,6 +331,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()); } @@ -447,7 +468,8 @@ public ModelAndView getView(CrosstabDesignBean form, boolean reshow, BindExcepti bean.setQueryName(form.getQueryName()); bean.setDataRegionName(form.getDataRegionName()); bean.setViewName(form.getViewName()); - bean.setRedirectUrl(form.getRedirectUrl()); + if (form.getRedirectUrl() != null) + bean.setRedirectUrl(new ReturnURLString(form.getRedirectUrl())); bean.setErrors(errors); if (!getUser().isGuest()) @@ -541,7 +563,7 @@ public void addNavTrail(NavTree root) } } - @RequiresNoPermission + @RequiresPermission(ReadPermission.class) public class CreateCrosstabReportAction extends SimpleViewAction { @Override @@ -669,13 +691,22 @@ public int getReportId() return reportId; } + @SuppressWarnings("unused") public void setReportId(int reportId) { this.reportId = reportId; } - public void setReportView(String label){_reportView = label;} - public String getReportView(){return _reportView;} + public String getReportView() + { + return _reportView; + } + + @SuppressWarnings("unused") + public void setReportView(String label) + { + _reportView = label; + } } public static class SaveReportForm extends ViewForm @@ -773,13 +804,35 @@ public void setShowWithDataset(int showWithDataset) this.showWithDataset = showWithDataset; } - public void setRedirectToDataset(Integer dataset){redirectToDataset = dataset;} - public Integer getRedirectToDataset(){return redirectToDataset;} + public Integer getRedirectToDataset() + { + return redirectToDataset; + } + + public void setRedirectToDataset(Integer dataset) + { + redirectToDataset = dataset; + } + + public String getDescription() + { + return this.description; + } + + public void setDescription(String description) + { + this.description = description; + } + + public BindException getErrors() + { + return _errors; + } - public void setDescription(String description){this.description = description;} - public String getDescription(){return this.description;} - public void setErrors(BindException errors){_errors = errors;} - public BindException getErrors(){return _errors;} + public void setErrors(BindException errors) + { + _errors = errors; + } public String getDataRegionName() { @@ -799,7 +852,7 @@ public static class SaveReportViewForm extends SaveReportForm private String _schemaName; private String _viewName; private String _dataRegionName; - private String _redirectUrl; + private ReturnURLString _redirectUrl; public SaveReportViewForm() { @@ -837,26 +890,64 @@ public Report getReport(ContainerUser cu) return null; } - public void setShareReport(boolean shareReport){_shareReport = shareReport;} - public boolean getShareReport(){return _shareReport;} + public void setShareReport(boolean shareReport) + { + _shareReport = shareReport; + } + + public boolean getShareReport() + { + return _shareReport; + } + + public void setSchemaName(String schemaName) + { + _schemaName = schemaName; + } + + public String getSchemaName() + { + return _schemaName; + } + + public void setQueryName(String queryName) + { + _queryName = queryName; + } + + public String getQueryName() + { + return _queryName; + } + + public void setViewName(String viewName) + { + _viewName = viewName; + } + + public String getViewName() + { + return _viewName; + } - public void setSchemaName(String schemaName){_schemaName = schemaName;} - public String getSchemaName(){return _schemaName;} - public void setQueryName(String queryName){_queryName = queryName;} - public String getQueryName(){return _queryName;} - public void setViewName(String viewName){_viewName = viewName;} - public String getViewName(){return _viewName;} @Override - public void setDataRegionName(String dataRegionName){_dataRegionName = dataRegionName;} + public void setDataRegionName(String dataRegionName) + { + _dataRegionName = dataRegionName; + } + @Override - public String getDataRegionName(){return _dataRegionName;} + public String getDataRegionName() + { + return _dataRegionName; + } - public String getRedirectUrl() + public ReturnURLString getRedirectUrl() { return _redirectUrl; } - public void setRedirectUrl(String redirectUrl) + public void setRedirectUrl(ReturnURLString redirectUrl) { _redirectUrl = redirectUrl; } @@ -977,7 +1068,9 @@ private void _addNavTrail(NavTree root, String name) if (getContainer().hasPermission(getUser(), AdminPermission.class)) root.addChild("Manage Views", urlProvider(ReportUrls.class).urlManageViews(getContainer())); } - catch (Exception ignored) {} + catch (Exception ignored) + { + } root.addChild(name); } @@ -1225,4 +1318,87 @@ 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()); + } + } + + public static class TestCase extends AbstractActionPermissionTest + { + @Override + @Test + public void testActionPermissions() + { + User user = TestContext.get().getUser(); + assertTrue(user.hasSiteAdminPermission()); + + ReportsController controller = new ReportsController(); + + // @RequiresPermission(ReadPermission.class) + assertForReadPermission(user, false, + controller.new BeginAction(), + new StreamFileAction(), + controller.new SaveReportAction(), + controller.new SaveReportViewAction(), + new ShowReportAction(), + new ParticipantCrosstabAction(), + controller.new CreateCrosstabReportAction(), + controller.new RunRReportAction(), + controller.new ParticipantReportAction(), + new SaveParticipantReportAction() + ); + + // @RequiresPermission(AdminPermission.class) + assertForAdminPermission(user, + controller.new CreateQueryReportAction() + ); + } + + // GitHub Issue #1243 regression test. + @Test + public void redirectUrlConstrainedToAllowableHost() + { + SaveReportViewForm offHost = new SaveReportViewForm(); + offHost.setRedirectUrl(new ReturnURLString("https://evil.example.com/phish")); + assertNull("Off-host redirectUrl must not resolve to an off-site ActionURL", + offHost.getRedirectUrl().getActionURL()); + + SaveReportViewForm local = new SaveReportViewForm(); + local.setRedirectUrl(new ReturnURLString("/study-reports/begin.view?x=1")); + ActionURL localUrl = local.getRedirectUrl().getActionURL(); + assertNotNull("A local redirectUrl should resolve to an ActionURL", localUrl); + assertTrue("A local redirectUrl should be preserved same-origin", + localUrl.getLocalURIString().contains("begin.view")); + + SaveReportViewForm blank = new SaveReportViewForm(); + blank.setRedirectUrl(new ReturnURLString("")); + assertTrue("A blank redirectUrl should be empty so the action falls through to the run URL", + blank.getRedirectUrl().isEmpty()); + } + } } diff --git a/study/src/org/labkey/study/dataset/DataStatesTest.java b/study/src/org/labkey/study/dataset/DataStatesTest.java new file mode 100644 index 00000000000..7d231cf4bf0 --- /dev/null +++ b/study/src/org/labkey/study/dataset/DataStatesTest.java @@ -0,0 +1,56 @@ +package org.labkey.study.dataset; + +import jakarta.servlet.http.HttpServletResponse; +import org.junit.Test; +import org.labkey.api.data.Container; +import org.labkey.api.qc.DataState; +import org.labkey.api.qc.DataStateManager; +import org.labkey.api.security.permissions.AbstractContainerScopingTest; +import org.labkey.api.view.ActionURL; +import org.labkey.study.controllers.StudyController; +import org.springframework.mock.web.MockHttpServletResponse; + +import java.util.List; + +public class DataStatesTest extends AbstractContainerScopingTest +{ + @Test + public void testManageDataStates() throws Exception + { + // Note: This test will log OptimisticConflictException warnings + // We're exercising DataStateManger via study ManageQCStatesAction, but we don't need to create actual studies + Container folderA = createContainer("Folder A"); + Container folderB = createContainer("Folder B"); + + // Create a basic data state in Folder A + DataState state1 = new DataState(); + state1.setContainer(folderA); + state1.setLabel("State 1"); + state1.setPublicData(true); + state1 = DataStateManager.getInstance().insertState(getAdmin(), state1); + List statesInA = DataStateManager.getInstance().getStates(folderA); + assertEquals(1, statesInA.size()); + assertTrue(statesInA.contains(state1)); + + // Attempt to update that data state from the wrong folder, Folder B. Admin should *not* be able to update it. + ActionURL url = new ActionURL(StudyController.ManageQCStatesAction.class, folderB) + .addParameter("ids", state1.getRowId()) + .addParameter("labels", "Here's my new label"); + HttpServletResponse response = post(url, getAdmin()); + List statesInB = DataStateManager.getInstance().getStates(folderB); + assertTrue(statesInB.isEmpty()); + statesInA = DataStateManager.getInstance().getStates(folderA); + assertEquals(1, statesInA.size()); + assertTrue(statesInA.contains(state1)); + assertStatus(MockHttpServletResponse.SC_INTERNAL_SERVER_ERROR, response); // Error response + + // Admin should be able to update the data state in Folder A + url.setContainer(folderA); + post(url, getAdmin()); + statesInA = DataStateManager.getInstance().getStates(folderA); + assertEquals(1, statesInA.size()); + DataState updatedState = statesInA.get(0); + assertEquals("Here's my new label", updatedState.getLabel()); + assertFalse(updatedState.isPublicData()); + } +} diff --git a/study/src/org/labkey/study/model/ParticipantGroupManager.java b/study/src/org/labkey/study/model/ParticipantGroupManager.java index 1b34d68907a..ab276da0591 100644 --- a/study/src/org/labkey/study/model/ParticipantGroupManager.java +++ b/study/src/org/labkey/study/model/ParticipantGroupManager.java @@ -16,7 +16,7 @@ package org.labkey.study.model; import org.jetbrains.annotations.Nullable; -import org.junit.Assert; +import org.junit.Before; import org.junit.Test; import org.labkey.api.audit.AuditLogService; import org.labkey.api.collections.CaseInsensitiveHashMap; @@ -31,7 +31,6 @@ import org.labkey.api.data.SQLFragment; import org.labkey.api.data.SimpleFilter; import org.labkey.api.data.SqlExecutor; -import org.labkey.api.data.SqlSelector; import org.labkey.api.data.Table; import org.labkey.api.data.TableInfo; import org.labkey.api.data.TableSelector; @@ -41,13 +40,16 @@ 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.permissions.AdminPermission; import org.labkey.api.security.permissions.ReadPermission; +import org.labkey.api.security.roles.ReaderRole; import org.labkey.api.settings.ResourceURL; import org.labkey.api.study.CohortFilter; 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; @@ -75,7 +77,6 @@ import java.util.Collections; import java.util.HashMap; import java.util.HashSet; -import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Set; @@ -83,11 +84,6 @@ import static org.labkey.api.util.IntegerUtils.asIntegerElseNull; -/** - * User: klum - * Date: Jun 1, 2011 - * Time: 2:26:02 PM - */ public class ParticipantGroupManager { private static final ParticipantGroupManager _instance = new ParticipantGroupManager(); @@ -118,17 +114,13 @@ public static TableInfo getTableInfoParticipantCategory() return StudySchema.getInstance().getTableInfoParticipantCategory(); } - public ParticipantCategoryImpl getParticipantCategory(Container c, User user, String label) + public @Nullable ParticipantCategoryImpl getParticipantCategory(Container c, User user, String label) { ParticipantCategoryImpl category = ParticipantGroupCache.getParticipantCategoryForLabel(c, label); - if (category != null) + if (category != null && category.canRead(user)) return category; - ParticipantCategoryImpl def = new ParticipantCategoryImpl(); - def.setContainer(c.getId()); - def.setLabel(label); - - return def; + return null; } public boolean categoryExists(Container c, User user, String label, boolean shared) @@ -136,14 +128,14 @@ public boolean categoryExists(Container c, User user, String label, boolean shar assert label != null : "Label cannot be null"; SimpleFilter filter = SimpleFilter.createContainerFilter(c); - Set exsitingCategories = new HashSet<>(); + Set existingCategories = new HashSet<>(); filter.addCondition(FieldKey.fromString("OwnerId"), shared ? ParticipantCategory.OWNER_SHARED : user.getUserId()); TableSelector selector = new TableSelector(getTableInfoParticipantCategory(), Collections.singleton("Label"), filter, null); for (String name : selector.getArrayList(String.class)) - exsitingCategories.add(name.toLowerCase()); + existingCategories.add(name.toLowerCase()); - return exsitingCategories.contains(label.toLowerCase()); + return existingCategories.contains(label.toLowerCase()); } public List getParticipantCategoriesByType(final Container c, final User user, @Nullable String type) @@ -151,12 +143,11 @@ public List getParticipantCategoriesByType(final Contai if (type == null) return _getParticipantCategories(c, user); - List categories = new ArrayList<>(); Collection types = ParticipantGroupCache.getParticipantCategoryForType(c, type); - if (types != null) - categories.addAll(types); + if (types == null) + return Collections.emptyList(); - return categories; + return types.stream().filter(category -> category != null && category.canRead(user)).toList(); } public List getParticipantCategoriesByLabel(final Container c, final User user, @Nullable String label) @@ -164,12 +155,11 @@ public List getParticipantCategoriesByLabel(final Conta if (label == null) return _getParticipantCategories(c, user); - List categories = new ArrayList<>(); - ParticipantCategoryImpl category = ParticipantGroupCache.getParticipantCategoryForLabel(c, label); + ParticipantCategoryImpl category = getParticipantCategory(c, user, label); if (category != null) - categories.add(category); + return List.of(category); - return categories; + return Collections.emptyList(); } public ActionButton createParticipantGroupButton(ViewContext context, String dataRegionName, CohortFilter cohortFilter, @@ -405,31 +395,26 @@ private String createNewParticipantGroupScript(ViewContext context, String dataR } /** - * Returns the list participant categories that the specified user is allowed to see + * Returns the list of participant categories that the specified user is allowed to see. * - * @param distinctCategories if true returns the unique (by label) set of categories. A private category will + * @param distinctCategories if true, returns the unique (by label) set of categories. A private category will * supersede a public category. */ public List getParticipantCategories(Container c, User user, boolean distinctCategories) { - if (distinctCategories) - { - Map categoryMap = new HashMap<>(); - for (ParticipantCategoryImpl category : _getParticipantCategories(c, user)) - { - if (categoryMap.containsKey(category.getLabel())) - { - if (!category.isShared()) - categoryMap.put(category.getLabel(), category); - } - else - categoryMap.put(category.getLabel(), category); - } - return new LinkedList<>(categoryMap.values()); + List categories = _getParticipantCategories(c, user); + + if (!distinctCategories) + return categories; + Map categoryMap = new HashMap<>(); + for (ParticipantCategoryImpl category : categories) + { + if (!categoryMap.containsKey(category.getLabel()) || !category.isShared()) + categoryMap.put(category.getLabel(), category); } - else - return _getParticipantCategories(c, user); + + return new ArrayList<>(categoryMap.values()); } public List getParticipantCategories(Container c, User user) @@ -439,12 +424,10 @@ public List getParticipantCategories(Container c, User private List _getParticipantCategories(Container c, User user) { - Collection categories = ParticipantGroupCache.getParticipantCategories(c); - List filtered = new ArrayList<>(); - - // TODO: Switch ParticipantCategoryImpl internals from arrays to lists... but not right now - categories.stream().filter(category -> category.canRead(c, user)).forEach(filtered::add); - return filtered; + return ParticipantGroupCache.getParticipantCategories(c) + .stream() + .filter(category -> category.canRead(user)) + .toList(); } @Deprecated // create participant categories and groups separately @@ -537,7 +520,7 @@ private ParticipantCategoryImpl _saveParticipantCategory(Container c, User user, } else { - ParticipantCategoryImpl prev = getParticipantCategory(c, user, def.getRowId()); + ParticipantCategoryImpl prev = getParticipantCategory(c, def.getRowId()); ret = Table.update(user, StudySchema.getInstance().getTableInfoParticipantCategory(), def, def.getRowId()); event = ParticipantGroupAuditProvider.EventFactory.categoryChange(c, user, prev, ret); } @@ -555,15 +538,11 @@ private ParticipantGroup _setParticipantGroup(Container c, User user, Participan if (verifyCategory) { ParticipantCategoryImpl cat = getParticipantCategory(c, user, group.getCategoryId()); - 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"); - } + if (!cat.canEdit(c, user)) + throw new ValidationException("You do not have permission to modify groups in this participant category"); } ParticipantGroup ret; @@ -813,7 +792,6 @@ private void addGroupParticipants(Container c, User user, ParticipantGroup group } } - private void removeGroupParticipants(Container c, User user, ParticipantGroup group, String[] participantsToRemove) { // remove the mapping from group to participants @@ -826,7 +804,6 @@ private void removeGroupParticipants(Container c, User user, ParticipantGroup gr ParticipantGroupCache.uncache(c); } - private void deleteGroupParticipants(Container c, User user, ParticipantGroup group) { // remove the mapping from group to participants @@ -838,12 +815,20 @@ private void deleteGroupParticipants(Container c, User user, ParticipantGroup gr ParticipantGroupCache.uncache(c); } - @Nullable - public ParticipantCategoryImpl getParticipantCategory(Container c, User user, int rowId) + public @Nullable ParticipantCategoryImpl getParticipantCategory(Container c, int rowId) { return ParticipantGroupCache.getParticipantCategory(c, rowId); } + public @Nullable ParticipantCategoryImpl getParticipantCategory(Container c, User user, int rowId) + { + ParticipantCategoryImpl category = getParticipantCategory(c, rowId); + if (category != null && category.canRead(user)) + return category; + + return null; + } + public ParticipantGroup getParticipantGroup(Container container, User user, int rowId) { return ParticipantGroupCache.getParticipantGroup(container, rowId); @@ -858,21 +843,11 @@ public ParticipantGroup getParticipantGroupFromGroupRowId(Container container, U return selector.getObject(ParticipantGroup.class); } - public List getAllGroupedParticipants(Container container) - { - SQLFragment sql = new SQLFragment("SELECT DISTINCT ParticipantId FROM "); - sql.append(getTableInfoParticipantGroupMap(), "GroupMap"); - sql.append(" WHERE Container = ? ORDER BY ParticipantId"); - sql.add(container); - - return new SqlSelector(StudySchema.getInstance().getSchema(), sql).getArrayList(String.class); - } - public List getParticipantGroups(final Container c, User user, ParticipantCategoryImpl def) { if (!def.isNew()) { - ParticipantCategoryImpl category = ParticipantGroupCache.getParticipantCategory(c, def.getRowId()); + ParticipantCategoryImpl category = getParticipantCategory(c, def.getRowId()); if (category != null) return Arrays.stream(category.getGroups()).toList(); } @@ -985,8 +960,10 @@ public void deleteParticipantCategory(Container c, User user, ParticipantCategor public void deleteParticipantGroup(Container c, User user, ParticipantGroup group) throws ValidationException { ParticipantCategoryImpl cat = getParticipantCategory(c, user, group.getCategoryId()); - List errors = new ArrayList<>(); + if (cat == null) + return; + List errors = new ArrayList<>(); if (!cat.canDelete(c, user, errors)) throw new ValidationException(errors); @@ -1095,14 +1072,187 @@ public void clearCache(Container c) ParticipantGroupCache.uncache(c); } - public static class ParticipantGroupTestCase extends Assert + public static class ContainerScopingTestCase extends AbstractContainerScopingTest { + private static final ParticipantGroupManager MANAGER = ParticipantGroupManager.getInstance(); + private Container _container; + private User _owner; + private User _otherUser; + + @Before + public void setupParticipantGroupFixtures() throws Exception + { + _container = createContainer("study"); + _owner = createUserInRole(_container, ReaderRole.class); + _otherUser = createUserInRole(_container, ReaderRole.class); + } + + @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. + StudyService.get().createStudy(_container, getAdmin(), "Study", TimepointType.VISIT, true); + insertParticipant(_container, "P1"); + + // 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(_container.getId()); + cat.setLabel("private-category"); + cat.setType(ParticipantCategory.Type.list.name()); + cat.setOwnerId(getAdmin().getUserId()); + cat = MANAGER.setParticipantCategory(_container, getAdmin(), cat, new String[]{"P1"}, null, "private"); + ParticipantGroup group = MANAGER.getParticipantGroups(_container, getAdmin(), cat).get(0); + + // Saving (overwriting) the admin's private group as a non-owner must be rejected. + try + { + // A different user with only Read access (not the owner, not an admin) + MANAGER.setParticipantGroup(_container, _otherUser, group); + fail("A non-owner must not be able to modify another user's private participant group"); + } + catch (ValidationException ignored) + { + } + + // Positive control: the owner (admin) can still save their own private group -- the guard rejects only the + // non-owner, not every caller. + MANAGER.setParticipantGroup(_container, getAdmin(), group); + } + + /** The by-label getter must apply canRead(), so by-label does not disclose a private category to a non-owner. */ @Test - public void test() + public void byLabelGetterHidesAnotherUsersPrivateCategory() throws Exception + { + ParticipantCategoryImpl owned = createPrivateCategory(_owner, "Private-by-label"); + + ParticipantCategoryImpl asOwner = MANAGER.getParticipantCategory(_container, _owner, "Private-by-label"); + assertNotNull("Owner should resolve their own private category", asOwner); + assertEquals("Owner should resolve the real (persisted) category", owned.getRowId(), asOwner.getRowId()); + assertFalse("Resolved category should be private", asOwner.isShared()); + assertEquals(_owner.getUserId(), asOwner.getOwnerId()); + + ParticipantCategoryImpl asOther = MANAGER.getParticipantCategory(_container, _otherUser, "Private-by-label"); + assertNull("Another user must not resolve the owner's private category by label", asOther); + } + + /** A shared category is readable by any user, so the by-label getter must still return it for a non-owner. */ + @Test + public void byLabelGetterReturnsSharedCategoryForAnyUser() throws Exception + { + ParticipantCategoryImpl shared = createSharedCategory("Shared-by-label"); + + ParticipantCategoryImpl asOther = MANAGER.getParticipantCategory(_container, _otherUser, "Shared-by-label"); + assertNotNull("A shared category must be readable by any user", asOther); + assertEquals(shared.getRowId(), asOther.getRowId()); + assertTrue(asOther.isShared()); + } + + /** The by-rowId getter must apply canRead(), so listing a categoryId does not disclose a private category to a non-owner. */ + @Test + public void byRowIdGetterHidesAnotherUsersPrivateCategory() throws Exception + { + ParticipantCategoryImpl owned = createPrivateCategory(_owner, "Private-by-rowId"); + + ParticipantCategoryImpl asOwner = MANAGER.getParticipantCategory(_container, _owner, owned.getRowId()); + assertNotNull("Owner should resolve their own private category by rowId", asOwner); + assertEquals(owned.getRowId(), asOwner.getRowId()); + assertFalse("Resolved category should be private", asOwner.isShared()); + assertEquals(_owner.getUserId(), asOwner.getOwnerId()); + + ParticipantCategoryImpl asOther = MANAGER.getParticipantCategory(_container, _otherUser, owned.getRowId()); + assertNull("Another user must not resolve the owner's private category by rowId", asOther); + } + + /** A shared category is readable by any user, so the by-rowId getter must still return it for a non-owner. */ + @Test + public void byRowIdGetterReturnsSharedCategoryForAnyUser() throws Exception + { + ParticipantCategoryImpl shared = createSharedCategory("Shared-by-rowId"); + + ParticipantCategoryImpl asOther = MANAGER.getParticipantCategory(_container, _otherUser, shared.getRowId()); + assertNotNull("A shared category must be readable by any user", asOther); + assertEquals(shared.getRowId(), asOther.getRowId()); + assertTrue(asOther.isShared()); + } + + /** getParticipantCategoriesByLabel delegates to the by-label getter and must inherit the same canRead() check. */ + @Test + public void byLabelListGetterHidesAnotherUsersPrivateCategory() throws Exception + { + createPrivateCategory(_owner, "Private-by-label-list"); + + assertEquals("Owner should see their private category by label", + 1, MANAGER.getParticipantCategoriesByLabel(_container, _owner, "Private-by-label-list").size()); + assertTrue("Another user must not see the owner's private category by label", + MANAGER.getParticipantCategoriesByLabel(_container, _otherUser, "Private-by-label-list").isEmpty()); + } + + /** getParticipantCategoriesByType must also filter by canRead(), so a private category isn't leaked by type. */ + @Test + public void byTypeGetterHidesAnotherUsersPrivateCategory() throws Exception + { + ParticipantCategoryImpl owned = createPrivateCategory(_owner, "Private-by-type"); + String type = ParticipantCategory.Type.list.name(); + + assertTrue("Owner should see their private category among the typed categories", + containsRowId(MANAGER.getParticipantCategoriesByType(_container, _owner, type), owned.getRowId())); + assertFalse("Another user must not see the owner's private category among the typed categories", + containsRowId(MANAGER.getParticipantCategoriesByType(_container, _otherUser, type), owned.getRowId())); + } + + /** The collection getter must hide another user's private category while still surfacing shared categories. */ + @Test + public void collectionGetterHidesPrivateButReturnsShared() throws Exception + { + ParticipantCategoryImpl owned = createPrivateCategory(_owner, "Private-collection"); + ParticipantCategoryImpl shared = createSharedCategory("Shared-collection"); + + List asOwner = MANAGER.getParticipantCategories(_container, _owner); + assertTrue("Owner should see their own private category", containsRowId(asOwner, owned.getRowId())); + assertTrue("Owner should see the shared category", containsRowId(asOwner, shared.getRowId())); + + List asOther = MANAGER.getParticipantCategories(_container, _otherUser); + assertFalse("Another user must not see the owner's private category", containsRowId(asOther, owned.getRowId())); + assertTrue("Shared categories remain visible to everyone", containsRowId(asOther, shared.getRowId())); + } + + private static boolean containsRowId(Collection categories, int rowId) + { + return categories.stream().anyMatch(category -> category.getRowId() == rowId); + } + + /** Create a private (owner-scoped) participant category, persisted and owned by {@code owner}. */ + private ParticipantCategoryImpl createPrivateCategory(User owner, String label) throws ValidationException { - ParticipantGroupManager p = new ParticipantGroupManager(); ParticipantCategoryImpl def = new ParticipantCategoryImpl(); - p.getParticipantGroups(null, null, def); + def.setContainer(_container.getId()); + def.setLabel(label); + def.setType(ParticipantCategory.Type.list.name()); + def.setOwnerId(owner.getUserId()); + return MANAGER.setParticipantCategory(_container, owner, def); + } + + /** Create a shared participant category. Created as the site admin, who may create shared categories. */ + private ParticipantCategoryImpl createSharedCategory(String label) throws ValidationException + { + ParticipantCategoryImpl def = new ParticipantCategoryImpl(); + def.setContainer(_container.getId()); + def.setLabel(label); + def.setType(ParticipantCategory.Type.list.name()); + // OwnerId defaults to OWNER_SHARED + return MANAGER.setParticipantCategory(_container, getAdmin(), def); + } + + 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/survey/src/org/labkey/survey/SurveyManager.java b/survey/src/org/labkey/survey/SurveyManager.java index 76ab890a908..57b7e14d610 100644 --- a/survey/src/org/labkey/survey/SurveyManager.java +++ b/survey/src/org/labkey/survey/SurveyManager.java @@ -63,6 +63,8 @@ import org.labkey.api.resource.Resource; 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.roles.ReaderRole; import org.labkey.api.survey.model.Survey; import org.labkey.api.survey.model.SurveyDesign; import org.labkey.api.survey.model.SurveyListener; @@ -266,13 +268,16 @@ public Survey saveSurvey(Container container, User user, Survey survey) } } + /** Checks that the user has read permission to the container that owns the design, but we accept cross-container references */ @Nullable public SurveyDesign getSurveyDesign(Container container, User user, int surveyId) { - // Scope by container SimpleFilter filter = new SimpleFilter(FieldKey.fromParts("rowId"), surveyId); - filter.addCondition(FieldKey.fromParts("Container"), container); - return new TableSelector(SurveySchema.getInstance().getSurveyDesignsTable(), filter, null).getObject(SurveyDesign.class); + SurveyDesign result = new TableSelector(SurveySchema.getInstance().getSurveyDesignsTable(), filter, null).getObject(SurveyDesign.class); + if (result == null) + return null; + Container actualContainer = ContainerManager.getForId(result.getContainerId()); + return actualContainer == null || !actualContainer.hasPermission(user, ReadPermission.class) ? null : result; } /** @@ -808,7 +813,7 @@ public void setUp() } @Test - public void testSurveyDesignContainerScoping() + public void testSurveyDesignContainerScoping() throws Exception { SurveyManager sm = SurveyManager.get(); @@ -817,9 +822,21 @@ public void testSurveyDesignContainerScoping() design = sm.saveSurveyDesign(_projectA, _user, design); int designId = design.getRowId(); - // Same-container lookup succeeds; cross-container lookup must return null - assertNotNull("Design should be visible from its own container", sm.getSurveyDesign(_projectA, _user, designId)); - assertNull("Design must NOT be visible from another container", sm.getSurveyDesign(_projectB, _user, designId)); + // 1. Same container: a user with read access in the design's container sees it. + User readerA = createUserInRole(_projectA, ReaderRole.class); + assertNotNull("Design should be visible from its own container to a user with read access", + sm.getSurveyDesign(_projectA, readerA, designId)); + + // 2. Different container, caller can read the design's container: tolerated, design is returned. + User readerAB = createUserInRole(_projectA, ReaderRole.class); + grantRole(readerAB, _projectB, ReaderRole.class); + assertNotNull("Design should be visible from another container when the caller can read the design's container", + sm.getSurveyDesign(_projectB, readerAB, designId)); + + // 3. Different container, caller cannot read the design's container: must return null. + User readerB = createUserInRole(_projectB, ReaderRole.class); + assertNull("Design must NOT be visible to a caller without read access to the design's container", + sm.getSurveyDesign(_projectB, readerB, designId)); // A delete issued from the wrong container must not remove the design sm.deleteSurveyDesign(_projectB, _user, designId, true); diff --git a/wiki/src/org/labkey/wiki/WikiController.java b/wiki/src/org/labkey/wiki/WikiController.java index 2dd39e0034a..a0ec0e99f7f 100644 --- a/wiki/src/org/labkey/wiki/WikiController.java +++ b/wiki/src/org/labkey/wiki/WikiController.java @@ -63,7 +63,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; @@ -2477,6 +2479,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"); @@ -2907,29 +2913,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()); } } }