GitHub Issue 1306: Refine announcement permission check

announcements/src/org/labkey/announcements/AnnouncementModule.java

@@ -242,7 +242,8 @@ public void startBackgroundThreads()
     public @NotNull Set<Class<?>> getIntegrationTests()
     {
         return Set.of(
-            AnnouncementManager.TestCase.class
+            AnnouncementManager.TestCase.class,
+            AnnouncementsController.ContainerScopingTestCase.class
         );
     }
 

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));
+        }
+    }
 }