[bot] Merge 26.7 to develop

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

api/src/org/labkey/api/ApiModule.java

@@ -58,6 +58,7 @@
 import org.labkey.api.data.DbSchema;
 import org.labkey.api.data.DbScope;
 import org.labkey.api.data.DbSequenceManager;
+import org.labkey.api.data.DisplayColumn;
 import org.labkey.api.data.ExcelColumn;
 import org.labkey.api.data.ExcelWriter;
 import org.labkey.api.data.InlineInClauseGenerator;
@@ -526,6 +527,7 @@ public void registerServlets(ServletContext servletCtx)
             DbScope.SchemaNameTestCase.class,
             DbScope.TransactionTestCase.class,
             DbSequenceManager.TestCase.class,
+                DisplayColumn.TestCase.class,
             DomTestCase.class,
             DomainTemplateGroup.TestCase.class,
             Encryption.TestCase.class,

api/src/org/labkey/api/cache/Throttle.java

@@ -15,6 +15,7 @@
  */
 package org.labkey.api.cache;
 
+import java.util.concurrent.atomic.AtomicLong;
 import java.util.function.Consumer;
 
 /**
@@ -36,22 +37,41 @@
  *        THROTTLE.execute(user);
  * }
  * </pre>
+ *
+ * <p>Tracks how many times the consumer actually ran ({@link #getExecutionCount()}) versus was throttled
+ * ({@link #getThrottledCount()}), useful for metrics and for tests asserting the throttling behavior.</p>
  */
 public class Throttle<K>
 {
     private final BlockingCache<K, K> _cache;
+    private final AtomicLong _executionCount = new AtomicLong();
+    private final AtomicLong _requestCount = new AtomicLong();
 
     public Throttle(String name, int limit, long timeToLive, Consumer<K> consumer)
     {
         _cache = CacheManager.getBlockingCache(limit, timeToLive, "Throttle for " + name, (key, argument) ->
         {
+            _executionCount.incrementAndGet();
             consumer.accept(key);
             return key;
         });
     }
 
     public void execute(K key)
     {
+        _requestCount.incrementAndGet();
         _cache.get(key);
     }
+
+    /** @return the number of times the consumer actually ran (i.e., calls that were not throttled) */
+    public long getExecutionCount()
+    {
+        return _executionCount.get();
+    }
+
+    /** @return the number of {@link #execute} calls that were throttled (the consumer did not run) */
+    public long getThrottledCount()
+    {
+        return _requestCount.get() - _executionCount.get();
+    }
 }

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

@@ -24,7 +24,11 @@
 import org.apache.poi.ss.usermodel.Workbook;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
+import org.junit.Assert;
+import org.junit.Test;
 import org.labkey.api.action.HasViewContext;
+import org.labkey.api.cache.CacheManager;
+import org.labkey.api.cache.Throttle;
 import org.labkey.api.collections.NullPreventingSet;
 import org.labkey.api.compliance.PhiTransformedColumnInfo;
 import org.labkey.api.ontology.Concept;
@@ -64,6 +68,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.atomic.AtomicInteger;
 
 import static org.apache.commons.lang3.StringUtils.isBlank;
 import static org.apache.commons.lang3.StringUtils.trimToEmpty;
@@ -112,6 +117,10 @@ public abstract class DisplayColumn extends RenderColumn
     private String _description = null;
     private String _displayClass;
 
+    // GH Issue 1332: Throttle to one warning per column + value type per hour, across all renders.
+    private static final Throttle<String> FORMAT_MISMATCH_THROTTLE = new Throttle<>("DisplayColumn format mismatch", 1000, CacheManager.HOUR, key ->
+            LOG.warn("Unable to apply format to {}, likely a SQL type mismatch between XML metadata and actual ResultSet", key));
+
     private final List<ColumnAnalyticsProvider> _analyticsProviders = new ArrayList<>();
 
     /** Handles spanning multiple rows in a grid. A separate interface to allow for easier mixing and matching with DisplayColumn implementations. */
@@ -488,7 +497,7 @@ public String getFormattedText(RenderContext ctx)
             }
             catch (IllegalArgumentException e)
             {
-                LOG.warn("Unable to apply format to {} value \"{}\" for column \"{}\", likely a SQL type mismatch between XML metadata and actual ResultSet", value.getClass().getName(), value, getName());
+                warnFormatMismatch(value);
                 return value.toString();
             }
         }
@@ -497,6 +506,13 @@ public String getFormattedText(RenderContext ctx)
     }
 
 
+    private void warnFormatMismatch(Object value)
+    {
+        ColumnInfo col = getColumnInfo();
+        String key = "column \"" + (col != null ? col.getFieldKey() : getName()) + "\" (value type " + value.getClass().getName() + ")";
+        FORMAT_MISMATCH_THROTTLE.execute(key);
+    }
+
     /**
      * Render the value as text using the <code>expr</code> or <code>format</code> if provided without
      * any html encoding.
@@ -540,7 +556,7 @@ else if (null != format)
             }
             catch (IllegalArgumentException e)
             {
-                LOG.warn("Unable to apply format to {} value \"{}\" for column \"{}\", likely a SQL type mismatch between XML metadata and actual ResultSet", value.getClass().getName(), value, getName());
+                warnFormatMismatch(value);
                 formattedString = ConvertUtils.convert(value);
             }
         }
@@ -1344,4 +1360,61 @@ else if (null == formatString && col.isNumericType())
             }
         }
     }
+
+    // GH Issue 1332: a type-mismatched column logs in format() on every cell; verify we warn once per column, not once per row
+    public static class TestCase extends Assert
+    {
+        // Unique per column so each test gets its own throttle key
+        private static final AtomicInteger UNIQUE = new AtomicInteger();
+        public static final String NOT_A_NUMBER_VALUE = "not-a-number";
+
+        // A column whose configured format rejects the value type, mimicking XML metadata that disagrees with the ResultSet.
+        private DisplayColumn columnThatFailsFormatting()
+        {
+            String name = "formatMismatchTestColumn-" + UNIQUE.incrementAndGet();
+            Format numberFormat = new DecimalFormat("0.00"); // format() throws IllegalArgumentException on a non-Number
+            return new SimpleDisplayColumn()
+            {
+                @Override
+                public Object getValue(RenderContext ctx)
+                {
+                    return NOT_A_NUMBER_VALUE;
+                }
+
+                @Override
+                public Format getFormat()
+                {
+                    return numberFormat;
+                }
+
+                @Override
+                public String getName()
+                {
+                    return name;
+                }
+            };
+        }
+
+        @Test
+        public void getFormattedTextWarnsOncePerColumn()
+        {
+            RenderContext ctx = new RenderContext(new ViewContext());
+            DisplayColumn dc = columnThatFailsFormatting();
+            long before = FORMAT_MISMATCH_THROTTLE.getExecutionCount();
+            for (int row = 0; row < 1000; row++)
+                assertEquals("Fallback must be the unformatted value", NOT_A_NUMBER_VALUE, dc.getFormattedText(ctx));
+            assertEquals("A format mismatch must warn once per column, not once per row", 1, FORMAT_MISMATCH_THROTTLE.getExecutionCount() - before);
+        }
+
+        @Test
+        public void formatValueWarnsOncePerColumn()
+        {
+            RenderContext ctx = new RenderContext(new ViewContext());
+            DisplayColumn dc = columnThatFailsFormatting();
+            long before = FORMAT_MISMATCH_THROTTLE.getExecutionCount();
+            for (int row = 0; row < 1000; row++)
+                assertEquals("Fallback must be the converted value", NOT_A_NUMBER_VALUE, dc.formatValue(ctx, NOT_A_NUMBER_VALUE, null, dc.getFormat(), null));
+            assertEquals("A format mismatch must warn once per column, not once per row", 1, FORMAT_MISMATCH_THROTTLE.getExecutionCount() - before);
+        }
+    }
 }

api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java

@@ -924,6 +924,12 @@ public boolean supportsNativeGreatestAndLeast()
         return true;
     }
 
+    @Override
+    public boolean supportsNativeIsDistinctFrom()
+    {
+        return true;
+    }
+
     @Override
     public boolean supportsIsNumeric()
     {

api/src/org/labkey/api/data/dialect/SqlDialect.java

@@ -885,6 +885,17 @@ public SQLFragment isNumericExpr(SQLFragment expression)
         throw new UnsupportedOperationException(getClass().getSimpleName() + " does not implement");
     }
 
+    /**
+     * Does the dialect natively support the standard "IS [NOT] DISTINCT FROM" predicate? PostgreSQL and Snowflake
+     * do; SQL Server, MySQL, and Oracle do not (SQL Server added GREATEST/LEAST in recent versions but has never
+     * added this predicate). Dialects that return false here get a portable CASE-based rewrite instead; see
+     * Method.IsDistinctFromMethodInfo.
+     */
+    public boolean supportsNativeIsDistinctFrom()
+    {
+        return false;
+    }
+
     public void handleCreateDatabaseException(SQLException e) throws ServletException
     {
         throw(new ServletException("Can't create database", e));

api/src/org/labkey/api/dataiterator/SimpleTranslator.java

@@ -692,6 +692,10 @@ public class ContainerColumn implements Supplier
 
         final Set<Object> allowableContainers = new HashSet<>();
 
+        // GH Issue 1332: a bad container value recurs on every row; warn once per distinct value, not per row
+        final Set<Object> loggedUnresolvedContainers = new HashSet<>();
+        final Set<Object> loggedRejectedContainers = new HashSet<>();
+
         public ContainerColumn(UserSchema us, TableInfo tableInfo, String containerId, int idx)
         {
             this.us = us;
@@ -723,7 +727,8 @@ public Object get()
                     if (!this.us.getContainer().allowRowMutationForContainer(rowContainer))
                     {
                         getRowError().addError(new SimpleValidationError("Row supplied container value: " + rowContainerVal + " cannot be used for actions against the container: " + us.getContainer().getPath()));
-                        LOG.warn("Resolved container to {} but rejected as valid location for import into {} in {}.{}", rowContainer.getPath(), us.getContainer().getPath(), us.getSchemaName(), tableInfo.getPublicSchemaName());
+                        if (loggedRejectedContainers.add(rowContainerVal))
+                            LOG.warn("Resolved container to {} but rejected as valid location for import into {} in {}.{}", rowContainer.getPath(), us.getContainer().getPath(), us.getSchemaName(), tableInfo.getPublicSchemaName());
                     }
                     else
                     {
@@ -734,8 +739,8 @@ public Object get()
                 }
                 else
                 {
-                    // only log if the incoming value is GUID-like
-                    if (rowContainerVal instanceof String && GUID.isGUID((String)rowContainerVal))
+                    // only log if the incoming value is GUID-like, and only once per distinct value
+                    if (rowContainerVal instanceof String s && GUID.isGUID(s) && loggedUnresolvedContainers.add(rowContainerVal))
                     {
                         LOG.warn("Failed to resolve container value '{}' to container for import into {}.{}, defaulting to original target container of {}", rowContainerVal, us.getSchemaName(), tableInfo.getPublicSchemaName(), us.getContainer().getPath());
                     }

api/src/org/labkey/api/settings/AppProps.java

@@ -185,6 +185,11 @@ static WriteableAppProps getWriteableInstance()
     /** Timeout in seconds for read-only HTTP requests, after which resources like DB connections and spawned processes will be killed. Set to 0 to disable. */
     int getReadOnlyHttpRequestTimeout();
 
+    int DEFAULT_SCRIPT_EXECUTION_TIMEOUT = 60;
+
+    /** Timeout in seconds for server-side JavaScript (e.g. trigger scripts), measured in wall-clock time including database and other Java operations invoked by the script. Set to 0 to disable. */
+    int getScriptExecutionTimeout();
+
     int getMaxBLOBSize();
 
     ExceptionReportingLevel getExceptionReportingLevel();