Study container scope

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,18 +227,11 @@ public boolean canEdit(Container container, User user, List<ValidationError> err
         {
             if (isNew())
                 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));
-
-                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();
     }
 
@@ -257,44 +251,28 @@ public boolean canDelete(Container container, User user, List<ValidationError> 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<ValidationError> 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);
     }
 }

study/src/org/labkey/study/StudyModule.java

@@ -713,13 +713,13 @@ public Set<Class> getIntegrationTests()
         return Set.of(
                 DatasetDefinition.TestCleanupOrphanedDatasetDomains.class,
                 DataStatesTest.class,
-                ParticipantGroupManager.ParticipantGroupTestCase.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,
                 CreateChildStudyAction.ContainerScopingTestCase.class,

study/src/org/labkey/study/controllers/DatasetController.java

@@ -17,20 +17,27 @@
 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;
 import org.labkey.api.data.Container;
 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);
+        }
+    }
 }