diff --git a/announcements/src/org/labkey/announcements/AnnouncementModule.java b/announcements/src/org/labkey/announcements/AnnouncementModule.java index 7daa6d907a8..ba9c68000d2 100644 --- a/announcements/src/org/labkey/announcements/AnnouncementModule.java +++ b/announcements/src/org/labkey/announcements/AnnouncementModule.java @@ -242,7 +242,8 @@ public void startBackgroundThreads() public @NotNull Set> getIntegrationTests() { return Set.of( - AnnouncementManager.TestCase.class + AnnouncementManager.TestCase.class, + AnnouncementsController.ContainerScopingTestCase.class ); } diff --git a/announcements/src/org/labkey/announcements/AnnouncementsController.java b/announcements/src/org/labkey/announcements/AnnouncementsController.java index 236fb8df9c3..fe47452e09c 100644 --- a/announcements/src/org/labkey/announcements/AnnouncementsController.java +++ b/announcements/src/org/labkey/announcements/AnnouncementsController.java @@ -25,6 +25,8 @@ import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.json.JSONObject; +import org.junit.Before; +import org.junit.Test; import org.labkey.announcements.model.AnnouncementDigestProvider; import org.labkey.announcements.model.AnnouncementFullModel; import org.labkey.announcements.model.AnnouncementManager; @@ -94,12 +96,14 @@ import org.labkey.api.security.SecurityManager; import org.labkey.api.security.User; import org.labkey.api.security.UserManager; +import org.labkey.api.security.permissions.AbstractContainerScopingTest; import org.labkey.api.security.permissions.AdminPermission; import org.labkey.api.security.permissions.DeletePermission; import org.labkey.api.security.permissions.InsertPermission; import org.labkey.api.security.permissions.Permission; import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.security.roles.EditorRole; +import org.labkey.api.security.roles.ReaderRole; import org.labkey.api.security.roles.Role; import org.labkey.api.security.roles.RoleManager; import org.labkey.api.util.DateUtil; @@ -2447,9 +2451,15 @@ public boolean handlePost(SubscriptionBean bean, BindException errors) { throw new NotFoundException("No such message thread: " + id); } - // Make sure they have permission to see the container for the specific message they're - // requesting - if (!ann.lookupContainer().hasPermission(getUser(), ReadPermission.class)) + // Resolve permissions against the thread's own container since this action resolves threads cross-container. + // Use allowRead() not a plain container ReadPermission check: secure boards additionally enforce member-list membership. + Container threadContainer = ann.lookupContainer(); + if (threadContainer == null) + { + throw new UnauthorizedException(); + } + Permissions perm = getPermissions(threadContainer, getUser(), getSettings(threadContainer)); + if (!perm.allowRead(ann)) { throw new UnauthorizedException(); } @@ -2883,4 +2893,79 @@ public Object execute(ThreadForm form, BindException errors) return success(updatedThread); } } + + public static class ContainerScopingTestCase extends AbstractContainerScopingTest + { + private Container _folderA; + private Container _folderB; + private AnnouncementModel _thread; + + @Before + public void createThread() throws Exception + { + // A message thread that lives in folder B. SubscribeThreadAction resolves threads by global entityId + // (cross-container by design), so each test addresses B's thread through a folder-A request. + _folderA = createContainer("A"); + _folderB = createContainer("B"); + AnnouncementModel insert = new AnnouncementModel(); + insert.setTitle("Container scoping test thread"); + insert.setBody("body"); + _thread = AnnouncementManager.insertAnnouncement(_folderB, getAdmin(), insert, null, false); + } + + @Test + public void testSubscribeThreadRequiresReadOnThreadContainer() throws Exception + { + ActionURL url = new ActionURL(SubscribeThreadAction.class, _folderA) + .addParameter("threadId", _thread.getEntityId()); + + // Negative: the @RequiresPermission(ReadPermission) gate only proves read on folder A; a caller who + // cannot read the thread's own folder must not be able to subscribe to it. + User readerA = createUserInRole(_folderA, ReaderRole.class); + assertStatus(HttpServletResponse.SC_FORBIDDEN, post(url, readerA)); + + // Positive control: the same subscription succeeds (302 to the success URL) once the caller can also + // read the thread's folder. + User readerAB = createUserInRole(_folderA, ReaderRole.class); + grantRole(readerAB, _folderB, ReaderRole.class); + assertStatus(HttpServletResponse.SC_FOUND, post(url, readerAB)); + } + + @Test + public void testSubscribeThreadEnforcesSecureBoardMemberList() throws Exception + { + // A secure board scopes read to its member list, so allowRead() rejects a board reader who is not on the + // thread's member list. The old plain container-ReadPermission check missed exactly this: it let any + // reader subscribe to a secure thread they cannot read. Address the request to the thread's own folder so + // only member-list membership varies. + Container secure = createContainer("Secure"); + Settings settings = AnnouncementManager.getMessageBoardSettings(secure); + settings.setSecure(Settings.SECURE_WITHOUT_EMAIL); + settings.setMemberList(true); + AnnouncementManager.saveMessageBoardSettings(secure, settings); + + // Both are plain readers (ReaderRole lacks SecureMessageBoardReadPermission, so neither is an editor who + // could read every thread). Create them before insert: the member-list validation checks that each listed + // member can read the thread. + User member = createUserInRole(secure, ReaderRole.class); + User nonMember = createUserInRole(secure, ReaderRole.class); + + AnnouncementModel insert = new AnnouncementModel(); + insert.setTitle("Secure member-list test thread"); + insert.setBody("body"); + // insertAnnouncement rebuilds memberListIds from memberListInput, so set the input, not the ids directly. + insert.setMemberListInput(String.valueOf(member.getUserId())); + AnnouncementModel secureThread = AnnouncementManager.insertAnnouncement(secure, getAdmin(), insert, null, false); + + ActionURL url = new ActionURL(SubscribeThreadAction.class, secure) + .addParameter("threadId", secureThread.getEntityId()); + + // Negative: a reader of the secure board who is not on the member list cannot read the thread, so cannot + // subscribe. Under the old container-ReadPermission check this incorrectly succeeded. + assertStatus(HttpServletResponse.SC_FORBIDDEN, post(url, nonMember)); + + // Positive control: a reader who is on the member list can read the thread, so the subscription succeeds. + assertStatus(HttpServletResponse.SC_FOUND, post(url, member)); + } + } }