Improve scoping checks for objects, round 2

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

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

announcements/src/org/labkey/announcements/ToursController.java

@@ -15,7 +15,9 @@
  */
 package org.labkey.announcements;
 
+import jakarta.servlet.http.HttpServletResponse;
 import org.json.JSONObject;
+import org.junit.Test;
 import org.labkey.announcements.model.TourManager;
 import org.labkey.announcements.model.TourModel;
 import org.labkey.announcements.query.AnnouncementSchema;
@@ -30,14 +32,24 @@
 import org.labkey.api.query.QueryView;
 import org.labkey.api.security.ActionNames;
 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.security.roles.FolderAdminRole;
+import org.labkey.api.security.roles.ReaderRole;
 import org.labkey.api.view.ActionURL;
 import org.labkey.api.view.JspView;
 import org.labkey.api.view.NavTree;
+import org.labkey.api.view.UnauthorizedException;
+import org.labkey.api.view.ViewServlet;
+import org.springframework.mock.web.MockHttpServletResponse;
 import org.springframework.validation.BindException;
 import org.springframework.validation.Errors;
 import org.springframework.web.servlet.ModelAndView;
 
+import java.util.Map;
+
 /**
  * Created by Marty on 1/19/2015.
  */
@@ -123,6 +135,12 @@ public static class SaveTourAction extends MutatingApiAction<SimpleApiJsonForm>
         @Override
         public void validateForm(SimpleApiJsonForm form, Errors errors)
         {
+            // The "//will check below" gate on the annotation was never implemented: this action inserts/updates tour
+            // content (a folder-level configuration asset) but performed no insert/update/admin check, so a Read user
+            // could create or overwrite tours. Require folder admin to manage tours.
+            if (!getContainer().hasPermission(getUser(), AdminPermission.class))
+                throw new UnauthorizedException("You do not have permission to modify tours in this folder.");
+
             TourModel model;
             JSONObject json = form.getJsonObject();
             int rowId = json.getInt("rowId");
@@ -203,4 +221,32 @@ public void setRowid(String rowid)
             _rowid = rowid;
         }
     }
+
+    public static class ContainerScopingTestCase extends AbstractContainerScopingTest
+    {
+        @Test
+        public void testSaveTourRequiresAdmin() throws Exception
+        {
+            Container folder = createContainer("A");
+            ActionURL url = new ActionURL(SaveTourAction.class, folder);
+            Map<String, Object> jsonHeaders = Map.of("Content-Type", "application/json");
+
+            // A Reader must not be able to create/modify tours
+            User reader = createUserInRole(folder, ReaderRole.class);
+            String body = new JSONObject().put("rowId", -1).toString();
+            assertStatus(HttpServletResponse.SC_FORBIDDEN, ViewServlet.POST(url, reader, jsonHeaders, body));
+
+            // Positive control: a folder admin passes the permission gate and the tour is created (success, 200).
+            User folderAdmin = createUserInRole(folder, FolderAdminRole.class);
+            String adminBody = new JSONObject()
+                    .put("rowId", -1)
+                    .put("title", "scoping-test-tour")
+                    .put("description", "d")
+                    .put("mode", "0")
+                    .put("tour", new JSONObject())
+                    .toString();
+            MockHttpServletResponse resp = ViewServlet.POST(url, folderAdmin, jsonHeaders, adminBody);
+            assertStatus(HttpServletResponse.SC_OK, resp);
+        }
+    }
 }

api/src/org/labkey/api/data/AbstractParticipantCategory.java

@@ -229,7 +229,10 @@ public boolean canEdit(Container container, User user, List<ValidationError> err
             else
             {
                 User owner = UserManager.getUser(getCreatedBy());
-                boolean allowed = (owner != null && !owner.isGuest()) ? owner.equals(user) : false;
+                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"));

api/src/org/labkey/api/data/TableViewForm.java

@@ -34,6 +34,7 @@
 import org.labkey.api.action.NullSafeBindException;
 import org.labkey.api.action.SpringActionController;
 import org.labkey.api.collections.CaseInsensitiveHashMap;
+import org.labkey.api.query.FieldKey;
 import org.labkey.api.query.SchemaKey;
 import org.labkey.api.security.permissions.DeletePermission;
 import org.labkey.api.security.permissions.InsertPermission;
@@ -183,8 +184,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");
+
             set("container", _c.getId());
+        }
 
         Object[] pkVal = getPkVals();
         Map<String, Object> newMap = Table.update(_user, _tinfo, getTypedValues(), pkVal);
@@ -207,21 +221,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 " + _tinfo.getName() + " on request " + _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 " + _tinfo.getName() + " on request " + _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.
      */

api/src/org/labkey/api/security/permissions/AbstractContainerScopingTest.java

@@ -74,7 +74,17 @@ protected User getAdmin()
     protected Container createContainer(String name)
     {
         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());
         _containers.add(c);
         return c;
     }