From f4fc9c7ce20bb656a8baa89d9cb232877feeb354 Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Tue, 16 Jun 2026 12:12:29 -0700 Subject: [PATCH 1/6] Check ParticipantCategory canRead --- .../api/data/AbstractParticipantCategory.java | 49 ++-- .../ParticipantGroupController.java | 90 ++++--- .../study/controllers/StudyController.java | 2 +- .../study/model/ParticipantGroupManager.java | 227 ++++++++++++------ 4 files changed, 215 insertions(+), 153 deletions(-) diff --git a/api/src/org/labkey/api/data/AbstractParticipantCategory.java b/api/src/org/labkey/api/data/AbstractParticipantCategory.java index 00ab7ad438b..d3045815c8e 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; @@ -228,16 +229,16 @@ public boolean canEdit(Container container, User user, List err return true; else { - User owner = UserManager.getUser(getCreatedBy()); boolean allowed = container.hasPermission(user, SharedParticipantGroupPermission.class) || container.hasPermission(user, AdminPermission.class) || - (owner != null && !owner.isGuest() && owner.equals(user)); + isOwner(user); if (!allowed) errors.add(new SimpleValidationError("You must be the owner to unshare this participant category")); } } + return errors.isEmpty(); } @@ -257,44 +258,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/study/src/org/labkey/study/controllers/ParticipantGroupController.java b/study/src/org/labkey/study/controllers/ParticipantGroupController.java index b37e76997f2..ed5f2a7c16d 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 fe3cc9c94ae..051b6e21f83 100644 --- a/study/src/org/labkey/study/controllers/StudyController.java +++ b/study/src/org/labkey/study/controllers/StudyController.java @@ -6741,7 +6741,7 @@ public ModelAndView getView(SendParticipantGroupForm form, boolean reshow, BindE if (group != null) { ParticipantCategoryImpl category = ParticipantGroupManager.getInstance().getParticipantCategory(getContainer(), getUser(), group.getCategoryId()); - if (category != null && category.canRead(getContainer(), getUser())) + if (category != null) { form.setLabel(group.getLabel()); return new JspView<>("/org/labkey/study/view/sendParticipantGroup.jsp", form, errors); diff --git a/study/src/org/labkey/study/model/ParticipantGroupManager.java b/study/src/org/labkey/study/model/ParticipantGroupManager.java index 7a148b7210a..0eb325ef199 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; @@ -42,9 +41,9 @@ import org.labkey.api.query.ValidationException; import org.labkey.api.security.User; import org.labkey.api.security.permissions.AbstractContainerScopingTest; -import org.labkey.api.security.roles.ReaderRole; import org.labkey.api.security.permissions.AdminPermission; import org.labkey.api.security.permissions.ReadPermission; +import org.labkey.api.security.roles.ReaderRole; import org.labkey.api.settings.ResourceURL; import org.labkey.api.study.CohortFilter; import org.labkey.api.study.ParticipantCategory; @@ -78,17 +77,11 @@ 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; import java.util.concurrent.CopyOnWriteArrayList; -/** - * User: klum - * Date: Jun 1, 2011 - * Time: 2:26:02 PM - */ public class ParticipantGroupManager { private static final ParticipantGroupManager _instance = new ParticipantGroupManager(); @@ -119,17 +112,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) @@ -137,14 +126,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) @@ -152,12 +141,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) @@ -165,12 +153,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, @@ -406,31 +393,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) @@ -440,14 +422,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(category -> { - filtered.add(category); - }); - return filtered; + return ParticipantGroupCache.getParticipantCategories(c) + .stream() + .filter(category -> category.canRead(user)) + .toList(); } @Deprecated // create participant categories and groups separately @@ -559,7 +537,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); } @@ -848,7 +826,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 @@ -861,7 +838,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 @@ -873,12 +849,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); @@ -893,21 +877,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(); } @@ -1028,8 +1002,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); @@ -1168,8 +1144,21 @@ public void clearCache(Container c) ParticipantGroupCache.uncache(c); } - public static class ParticipantGroupTestCase extends Assert + public static class ParticipantGroupTestCase 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 test() { @@ -1177,6 +1166,102 @@ public void test() ParticipantCategoryImpl def = new ParticipantCategoryImpl(); p.getParticipantGroups(null, null, def); } + + /** The by-label getter must apply canRead(), so by-label does not disclose a private category to a non-owner. */ + @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()); + } + + /** 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 + { + ParticipantCategoryImpl def = new ParticipantCategoryImpl(); + 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); + } } public static class ContainerScopingTestCase extends AbstractContainerScopingTest From 8e35d35c0e99f22adf6d30384e63125911e86d49 Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Tue, 16 Jun 2026 12:27:23 -0700 Subject: [PATCH 2/6] By rowId test cases --- .../study/model/ParticipantGroupManager.java | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/study/src/org/labkey/study/model/ParticipantGroupManager.java b/study/src/org/labkey/study/model/ParticipantGroupManager.java index 0eb325ef199..f7f91ab7262 100644 --- a/study/src/org/labkey/study/model/ParticipantGroupManager.java +++ b/study/src/org/labkey/study/model/ParticipantGroupManager.java @@ -1195,6 +1195,34 @@ public void byLabelGetterReturnsSharedCategoryForAnyUser() throws Exception 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 From c44457be1fd1283ff2ebcaf2e74f486c44b2c72a Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Tue, 16 Jun 2026 14:00:25 -0700 Subject: [PATCH 3/6] Check permissions before redirecting --- .../labkey/study/controllers/StudyController.java | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/study/src/org/labkey/study/controllers/StudyController.java b/study/src/org/labkey/study/controllers/StudyController.java index 051b6e21f83..fa8b9d980f1 100644 --- a/study/src/org/labkey/study/controllers/StudyController.java +++ b/study/src/org/labkey/study/controllers/StudyController.java @@ -2953,12 +2953,16 @@ public static class DatasetItemDetailsAction extends SimpleViewAction Date: Tue, 16 Jun 2026 14:56:39 -0700 Subject: [PATCH 4/6] Add test --- study/src/org/labkey/study/StudyModule.java | 1 + .../study/controllers/DatasetController.java | 64 +++++++++++++++++++ 2 files changed, 65 insertions(+) diff --git a/study/src/org/labkey/study/StudyModule.java b/study/src/org/labkey/study/StudyModule.java index 44a8ddb7a11..5c1ba374589 100644 --- a/study/src/org/labkey/study/StudyModule.java +++ b/study/src/org/labkey/study/StudyModule.java @@ -720,6 +720,7 @@ public Set getIntegrationTests() StudyManager.VisitCreationTestCase.class, StudyModule.TestCase.class, VisitImpl.TestCase.class, + DatasetController.DatasetAuditHistoryScopingTestCase.class, DatasetUpdateService.TestCase.class, DatasetLsidImportHelper.TestCase.class, CreateChildStudyAction.ContainerScopingTestCase.class, diff --git a/study/src/org/labkey/study/controllers/DatasetController.java b/study/src/org/labkey/study/controllers/DatasetController.java index 394cdf23fc8..bddda0a3331 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.StringUtils; +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); + } + } } From 353a83a54ff1e4c5c2b39664be11cf148481211e Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Wed, 17 Jun 2026 08:40:40 -0700 Subject: [PATCH 5/6] Consolidate tests --- study/src/org/labkey/study/StudyModule.java | 1 - .../study/model/ParticipantGroupManager.java | 85 ++++++++----------- 2 files changed, 36 insertions(+), 50 deletions(-) diff --git a/study/src/org/labkey/study/StudyModule.java b/study/src/org/labkey/study/StudyModule.java index 5c1ba374589..98f7579ea8a 100644 --- a/study/src/org/labkey/study/StudyModule.java +++ b/study/src/org/labkey/study/StudyModule.java @@ -713,7 +713,6 @@ public Set getIntegrationTests() return Set.of( DatasetDefinition.TestCleanupOrphanedDatasetDomains.class, DataStatesTest.class, - ParticipantGroupManager.ParticipantGroupTestCase.class, ParticipantGroupManager.ContainerScopingTestCase.class, StudyImpl.ProtocolDocumentTestCase.class, StudyManager.StudySnapshotTestCase.class, diff --git a/study/src/org/labkey/study/model/ParticipantGroupManager.java b/study/src/org/labkey/study/model/ParticipantGroupManager.java index f7f91ab7262..6a8bfde21c0 100644 --- a/study/src/org/labkey/study/model/ParticipantGroupManager.java +++ b/study/src/org/labkey/study/model/ParticipantGroupManager.java @@ -1144,7 +1144,7 @@ public void clearCache(Container c) ParticipantGroupCache.uncache(c); } - public static class ParticipantGroupTestCase extends AbstractContainerScopingTest + public static class ContainerScopingTestCase extends AbstractContainerScopingTest { private static final ParticipantGroupManager MANAGER = ParticipantGroupManager.getInstance(); private Container _container; @@ -1160,11 +1160,42 @@ public void setupParticipantGroupFixtures() throws Exception } @Test - public void test() + public void testSetParticipantGroupRequiresOwnership() throws Exception { - ParticipantGroupManager p = new ParticipantGroupManager(); - ParticipantCategoryImpl def = new ParticipantCategoryImpl(); - p.getParticipantGroups(null, null, def); + // setParticipantGroup() saves a group keyed by a global rowId. For a PRIVATE category, canEdit() allows only + // the category's creator -- but the manager previously enforced only the shared-category case, so a Read + // user could overwrite another user's private group via a guessable rowId. The fix gates _setParticipantGroup + // on canEdit() for the private case too. + Container folder = createContainer("A"); + StudyService.get().createStudy(folder, getAdmin(), "Study", TimepointType.VISIT, true); + insertParticipant(folder, "P1"); + + // A PRIVATE participant category + group owned by the admin (ownerId != OWNER_SHARED makes it private; the + // admin is its creator, so only the admin may edit it). + ParticipantCategoryImpl cat = new ParticipantCategoryImpl(); + cat.setContainer(folder.getId()); + cat.setLabel("private-category"); + cat.setType("list"); + cat.setOwnerId(getAdmin().getUserId()); + cat = MANAGER.setParticipantCategory(folder, getAdmin(), cat, new String[]{"P1"}, null, "private"); + ParticipantGroup group = MANAGER.getParticipantGroups(folder, getAdmin(), cat).get(0); + + // A different user with only Read access (not the owner, not an admin) + User attacker = createUserInRole(folder, ReaderRole.class); + + // Saving (overwriting) the admin's private group as the attacker must be rejected. + try + { + MANAGER.setParticipantGroup(folder, attacker, 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(folder, getAdmin(), group); } /** The by-label getter must apply canRead(), so by-label does not disclose a private category to a non-owner. */ @@ -1290,50 +1321,6 @@ private ParticipantCategoryImpl createSharedCategory(String label) throws Valida // OwnerId defaults to OWNER_SHARED return MANAGER.setParticipantCategory(_container, getAdmin(), def); } - } - - public static class ContainerScopingTestCase extends AbstractContainerScopingTest - { - @Test - public void testSetParticipantGroupRequiresOwnership() throws Exception - { - // setParticipantGroup() saves a group keyed by a global rowId. For a PRIVATE category, canEdit() allows only - // the category's creator -- but the manager previously enforced only the shared-category case, so a Read - // user could overwrite another user's private group via a guessable rowId. The fix gates _setParticipantGroup - // on canEdit() for the private case too. - Container folder = createContainer("A"); - StudyService.get().createStudy(folder, getAdmin(), "Study", TimepointType.VISIT, true); - insertParticipant(folder, "P1"); - - ParticipantGroupManager mgr = ParticipantGroupManager.getInstance(); - - // A PRIVATE participant category + group owned by the admin (ownerId != OWNER_SHARED makes it private; the - // admin is its creator, so only the admin may edit it). - ParticipantCategoryImpl cat = new ParticipantCategoryImpl(); - cat.setContainer(folder.getId()); - cat.setLabel("private-category"); - cat.setType("list"); - cat.setOwnerId(getAdmin().getUserId()); - cat = mgr.setParticipantCategory(folder, getAdmin(), cat, new String[]{"P1"}, null, "private"); - ParticipantGroup group = mgr.getParticipantGroups(folder, getAdmin(), cat).get(0); - - // A different user with only Read access (not the owner, not an admin) - User attacker = createUserInRole(folder, ReaderRole.class); - - // Saving (overwriting) the admin's private group as the attacker must be rejected. - try - { - mgr.setParticipantGroup(folder, attacker, group); - fail("A non-owner must not be able to modify another user's private participant group"); - } - catch (ValidationException expected) - { - } - - // Positive control: the owner (admin) can still save their own private group -- the guard rejects only the - // non-owner, not every caller. - mgr.setParticipantGroup(folder, getAdmin(), group); - } private void insertParticipant(Container c, String ptid) { From 22743baa9fe5d1dce8f8cf18be95250f5d06641e Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Wed, 17 Jun 2026 09:28:50 -0700 Subject: [PATCH 6/6] Revise permission check --- .../api/data/AbstractParticipantCategory.java | 11 ++------ .../study/model/ParticipantGroupManager.java | 26 +++++++------------ 2 files changed, 12 insertions(+), 25 deletions(-) diff --git a/api/src/org/labkey/api/data/AbstractParticipantCategory.java b/api/src/org/labkey/api/data/AbstractParticipantCategory.java index d3045815c8e..35e40383e13 100644 --- a/api/src/org/labkey/api/data/AbstractParticipantCategory.java +++ b/api/src/org/labkey/api/data/AbstractParticipantCategory.java @@ -227,16 +227,9 @@ public boolean canEdit(Container container, User user, List err { if (isNew()) return true; - else - { - boolean allowed = - container.hasPermission(user, SharedParticipantGroupPermission.class) || - container.hasPermission(user, AdminPermission.class) || - isOwner(user); - 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(); diff --git a/study/src/org/labkey/study/model/ParticipantGroupManager.java b/study/src/org/labkey/study/model/ParticipantGroupManager.java index 6a8bfde21c0..e62017e5bd6 100644 --- a/study/src/org/labkey/study/model/ParticipantGroupManager.java +++ b/study/src/org/labkey/study/model/ParticipantGroupManager.java @@ -555,12 +555,9 @@ 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."); - // canEdit enforces the SharedParticipantGroupPermission/Admin check for shared categories AND the - // owner check for private categories if (!cat.canEdit(c, user)) throw new ValidationException("You do not have permission to modify groups in this participant category"); } @@ -1166,27 +1163,24 @@ public void testSetParticipantGroupRequiresOwnership() throws Exception // the category's creator -- but the manager previously enforced only the shared-category case, so a Read // user could overwrite another user's private group via a guessable rowId. The fix gates _setParticipantGroup // on canEdit() for the private case too. - Container folder = createContainer("A"); - StudyService.get().createStudy(folder, getAdmin(), "Study", TimepointType.VISIT, true); - insertParticipant(folder, "P1"); + 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(folder.getId()); + cat.setContainer(_container.getId()); cat.setLabel("private-category"); - cat.setType("list"); + cat.setType(ParticipantCategory.Type.list.name()); cat.setOwnerId(getAdmin().getUserId()); - cat = MANAGER.setParticipantCategory(folder, getAdmin(), cat, new String[]{"P1"}, null, "private"); - ParticipantGroup group = MANAGER.getParticipantGroups(folder, getAdmin(), cat).get(0); - - // A different user with only Read access (not the owner, not an admin) - User attacker = createUserInRole(folder, ReaderRole.class); + 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 the attacker must be rejected. + // Saving (overwriting) the admin's private group as a non-owner must be rejected. try { - MANAGER.setParticipantGroup(folder, attacker, group); + // 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) @@ -1195,7 +1189,7 @@ public void testSetParticipantGroupRequiresOwnership() throws Exception // Positive control: the owner (admin) can still save their own private group -- the guard rejects only the // non-owner, not every caller. - MANAGER.setParticipantGroup(folder, getAdmin(), group); + 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. */