Merge from release25.7-SNAPSHOT

Author: labkey-adam

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,10 +32,17 @@
 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.springframework.mock.web.MockHttpServletResponse;
 import org.springframework.validation.BindException;
 import org.springframework.validation.Errors;
 import org.springframework.web.servlet.ModelAndView;
@@ -123,6 +132,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 +218,30 @@ 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);
+
+            // A Reader must not be able to create/modify tours
+            User reader = createUserInRole(folder, ReaderRole.class);
+            JSONObject body = new JSONObject().put("rowId", -1);
+            assertStatus(HttpServletResponse.SC_FORBIDDEN, postJson(url, reader, body));
+
+            // Positive control: a folder admin passes the permission gate and the tour is created (success, 200).
+            User folderAdmin = createUserInRole(folder, FolderAdminRole.class);
+            JSONObject adminBody = new JSONObject()
+                    .put("rowId", -1)
+                    .put("title", "scoping-test-tour")
+                    .put("description", "d")
+                    .put("mode", "0")
+                    .put("tour", new JSONObject());
+            MockHttpServletResponse resp = postJson(url, folderAdmin, adminBody);
+            assertStatus(HttpServletResponse.SC_OK, resp);
+        }
+    }
 }

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,15 +227,11 @@ public boolean canEdit(Container container, User user, List<ValidationError> err
         {
             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 unshare this participant category"));
-            }
+            if (!isOwner(user))
+                errors.add(new SimpleValidationError("You must be the owner to unshare this participant category"));
         }
+
         return errors.isEmpty();
     }
 
@@ -254,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);
     }
 }

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/exp/OntologyManager.java

@@ -2841,13 +2841,14 @@ public static Object getRemappedValueForLookup(User user, Container container, R
         return cache.remap(SchemaKey.fromParts(lookup.getSchemaKey()), lookup.getQueryName(), user, lkContainer, ContainerFilter.Type.CurrentPlusProjectAndShared, String.valueOf(value));
     }
 
-    public static List<PropertyUsages> findPropertyUsages(User user, List<Integer> propertyIds, int maxUsageCount)
+    public static List<PropertyUsages> findPropertyUsagesByIds(User user, Container container, List<Integer> propertyIds, int maxUsageCount)
     {
         List<PropertyUsages> ret = new ArrayList<>(propertyIds.size());
         for (int propertyId : propertyIds)
         {
             var pd = getPropertyDescriptor(propertyId);
-            if (pd == null)
+            // Kanban #1924: Get property descriptors for the current container only
+            if (pd == null || !pd.getContainer().equals(container))
                 throw new IllegalArgumentException("property not found: " + propertyId);
 
             ret.add(findPropertyUsages(user, pd, maxUsageCount));

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

@@ -16,6 +16,7 @@
 package org.labkey.api.security.permissions;
 
 import jakarta.servlet.http.HttpServletResponse;
+import org.json.JSONObject;
 import org.junit.After;
 import org.junit.Assert;
 import org.labkey.api.data.Container;
@@ -57,6 +58,7 @@
 public abstract class AbstractContainerScopingTest extends Assert
 {
     private static final Map<String, Object> FORM_HEADERS = Map.of("Content-Type", "application/x-www-form-urlencoded");
+    private static final Map<String, Object> JSON_HEADERS = Map.of("Content-Type", "application/json");
 
     private final List<Container> _containers = new ArrayList<>();
     private final List<User> _users = new ArrayList<>();
@@ -75,7 +77,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;
     }
@@ -124,6 +136,12 @@ protected MockHttpServletResponse post(ActionURL url, User user) throws Exceptio
         return ViewServlet.POST(url, user, FORM_HEADERS, null);
     }
 
+    /** Dispatch a JSON POST to a {@code @Marshal(Jackson)} API action, with {@code body} supplied as the request body. */
+    protected MockHttpServletResponse postJson(ActionURL url, User user, JSONObject body) throws Exception
+    {
+        return ViewServlet.POST(url, user, JSON_HEADERS, body.toString());
+    }
+
     /** Assert that a dispatched response has the expected HTTP status code. */
     protected void assertStatus(int expected, HttpServletResponse response)
     {

api/src/org/labkey/api/study/publish/AbstractPublishConfirmAction.java

@@ -42,6 +42,7 @@
 import org.labkey.api.view.ActionURL;
 import org.labkey.api.view.JspView;
 import org.labkey.api.view.RedirectException;
+import org.labkey.api.view.UnauthorizedException;
 import org.labkey.api.view.VBox;
 import org.labkey.api.view.template.ClientDependency;
 import org.springframework.validation.BindException;
@@ -107,6 +108,10 @@ public void validateCommand(FORM form, Errors errors)
             {
                 errors.reject(SpringActionController.ERROR_MSG, "Could not find target study");
             }
+            else if (!_targetStudy.hasPermission(getUser(), InsertPermission.class))
+            {
+                throw new UnauthorizedException("You do not have permission to insert into the target study");
+            }
         }
 
         if (_targetStudy != null)