Merge remote-tracking branch 'origin/develop' into fb_parentAliasDefa…

…ults

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

@@ -11,11 +11,8 @@
  * A simple scanner for SQL text that understands block & single-line comments, double-quoted identifiers, single-quoted strings,
  * and quote escaping. It is not a SQL tokenizer or parser... it merely enables simple text operations that are aware of comments
  * and strings. It currently supports finding characters and substrings outside of comments and quoted strings, as well as
- * stripping comments. It could be extended to support search & replace and other useful text operations.
- *
+ * stripping comments and quoted strings. It could be extended to support search & replace and other useful text operations.
  * Consider: Use in SQLFragment.getFilterText() replacements
- *
- * Created by adam on 6/29/2017.
  */
 public class SqlScanner extends BaseScanner
 {
@@ -75,6 +72,7 @@ else if ("--".equals(twoChars))
             else if ('\'' == c || '"' == c)
             {
                 String escape = "" + c + c;
+                int startIndex = i;
 
                 while (++i < _text.length())
                 {
@@ -85,9 +83,15 @@ else if ('\'' == c || '"' == c)
                         twoChars = _text.substring(i, i + 2);
 
                     if (escape.equals(twoChars))
+                    {
                         i++;
+                    }
                     else if (c == c2)
+                    {
+                        if (!handler.string(startIndex, i + 1))
+                            return;
                         break next;
+                    }
                 }
 
                 throw new NotFoundException("Expected ending quote (" + c + ") was not found");

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

@@ -161,36 +161,45 @@ public static void setParameters(PreparedStatement stmt, Collection<?> parameter
     }
 
     /**
-     *  @return if this is a SELECT statement that contains FROM, possibly proceeded by a CTE (WITH clause). Comments
-     *  are ignored.
+     *  @return if this is a SELECT statement that contains FROM, possibly proceeded by one or more CTEs (WITH clause).
+     *  Comments and quoted strings are ignored.
      */
     public static boolean isSelect(String sql)
     {
-        // Remove comments and trim
-        String strippedSql = new SqlScanner(sql).stripComments().toString().trim();
+        // Remove comments and quoted strings, then trim
+        String noComments = new SqlScanner(sql).stripComments().toString();
+        String strippedSql = new SqlScanner(noComments).stripQuotedStrings().toString().trim();
 
         // If present, remove WITH statement, which should leave behind a SELECT
         if (strippedSql.matches("^(?ims)WITH\\s.*$")) // flags: case-insensitive, multiline, dot-all
         {
-            SqlScanner scanner = new SqlScanner(strippedSql);
-            SkipWithStatementHandler handler = new SkipWithStatementHandler();
-            scanner.scan("WITH ".length(), handler);
-            String error = handler.getError();
+            int fromIndex = "WITH ".length();
 
-            if (null != error)
+            // WITH clause contains one or more CTEs; skip them all
+            do
             {
-                _log.warn(error + "!\n" + strippedSql);
-                return false;
-            }
+                SqlScanner scanner = new SqlScanner(strippedSql);
+                SkipCteHandler handler = new SkipCteHandler();
+                scanner.scan(fromIndex, handler);
+                String error = handler.getError();
+
+                if (null != error)
+                {
+                    _log.warn(error + "!\n" + strippedSql);
+                    return false;
+                }
 
-            strippedSql = strippedSql.substring(handler.getSelectIdx()).trim();
+                strippedSql = strippedSql.substring(handler.getSelectIdx()).trim();
+                fromIndex = 1;
+            }
+            while (strippedSql.startsWith(","));
         }
 
         // We must see a FROM clause, so we don't flag stored procedure invocations, #22648
         return strippedSql.matches("^(?ims)SELECT\\s.*\\sFROM\\s.*$"); // flags: case-insensitive, multiline, dot-all
     }
 
-    private static class SkipWithStatementHandler implements BaseScanner.Handler
+    private static class SkipCteHandler implements BaseScanner.Handler
     {
         private boolean _parenSeen = false;
         private int _parens = -1;
@@ -251,13 +260,36 @@ public void test()
             assertTrue(isSelect("WITH cte AS (SELECT CAST('1' AS INTEGER) AS i) SELECT * FROM cte"));
             assertTrue(isSelect("with lowercase as (select cast('1' as integer) as i) select * from lowercase"));
             assertTrue(isSelect("WITH\nMultiLine AS (SELECT * FROM foo.bar)\nSELECT *\nFROM Blue"));
+            assertTrue(isSelect("""
+                WITH RECURSIVE
+                /*CTE*/
+                    org_lk_exp_CHILDREN_INNER AS (SELECT
+                      0 AS depth)
+                ,/*CTE*/
+                    org_lk_exp_CHILDREN AS (SELECT
+                        I.depth,
+                        I.self),
+                /*CTE*/
+                    org_lk_exp_CHILDREN_OUTER AS (SELECT 2 AS depth)
+                SELECT *
+                FROM (
+                SELECT
+                SourceSamples.SampleState AS SampleState
+                FROM org_lk_exp_CHILDREN
+                WHERE depth != 0
+                AND objectid <> self
+                AND depth <= 10
+                GROUP BY self, objectid) AS X))
+                ORDER BY rowid ASC
+                LIMIT 21"""));
 
             assertFalse(isSelect("WITH Nothing SELECT * FROM foo.bar"));
             assertFalse(isSelect("WITH Mismatched AS (SELECT * FROM (SELECT * FROM foo.bar) SELECT * FROM Mismatched"));
             assertFalse(isSelect("WITH Incomplete AS (SELECT * FROM foo.bar)"));
             assertFalse(isSelect("WITH NonSelect AS (SELECT * FROM foo.bar) UPDATE foo.bar SET bam = 1"));
             assertFalse(isSelect("WITH NonSelect AS (SELECT * FROM foo.bar) DELETE FROM foo.bar WHERE 1=1"));
             assertFalse(isSelect("WITHOUT NonCTE AS (SELECT * FROM foo.bar) SELECT NonCTE WHERE 1=1"));
+            assertFalse(isSelect("SELECT \" FROM \" /* FROM */ ' FROM ' foo.bar WHERE 1=1"));
         }
     }
 

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

@@ -66,17 +66,13 @@
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 
-/**
- * User: arauch
- * Date: Dec 28, 2004
- * Time: 8:58:25 AM
- */
-
-// Base dialect for PostgreSQL. PostgreSQL 9.1 is no longer supported, however, we keep this class versioned as "91" to
+// Base dialect for PostgreSQL. PostgreSQL 9.1 is no longer supported, however, we keep the dialects versioned to
 // track changes we've implemented for each version over time.
 public abstract class PostgreSql91Dialect extends SqlDialect
 {
     public static final int TEMPTABLE_GENERATOR_MINSIZE = 1000;
+    public static final String PRODUCT_NAME = "PostgreSQL";
+    public static final String RECOMMENDED = PRODUCT_NAME + " 16.x is the recommended version.";
 
     private final Map<String, Integer> _domainScaleMap = new ConcurrentHashMap<>();
     private final AtomicBoolean _arraySortFunctionExists = new AtomicBoolean(false);
@@ -1889,8 +1885,7 @@ public void addAdminWarningMessages(Warnings warnings, boolean showAllWarnings)
         if (null != _adminWarning)
             warnings.add(_adminWarning);
         else if (showAllWarnings) // PostgreSqlDialectFactory.getStandardWarningMessage() is not accessible from here, so hard-code a sample warning
-            warnings.add(HtmlString.of("LabKey Server has not been tested against this version. PostgreSQL 15.x is the recommended version."));
-
+            warnings.add(HtmlString.of("LabKey Server has not been tested against this version. " + RECOMMENDED));
     }
 
     @Override

api/src/org/labkey/api/security/AuthenticationManager.java

@@ -1448,28 +1448,31 @@ public URLHelper getRedirectURL()
 
         List<AuthenticationValidator> validators = new LinkedList<>();
 
-        for (SecondaryAuthenticationConfiguration<?> configuration : getActiveConfigurations(SecondaryAuthenticationConfiguration.class))
+        if (primaryAuthResult.getResponse().requireSecondary())
         {
-            User secondaryAuthUser = getSecondaryAuthenticationUser(session, configuration.getRowId());
-
-            if (null == secondaryAuthUser)
+            for (SecondaryAuthenticationConfiguration<?> configuration : getActiveConfigurations(SecondaryAuthenticationConfiguration.class))
             {
-                SecondaryAuthenticationProvider<?> provider = configuration.getAuthenticationProvider();
-                if (provider.bypass())
+                User secondaryAuthUser = getSecondaryAuthenticationUser(session, configuration.getRowId());
+
+                if (null == secondaryAuthUser)
                 {
-                    _log.info("Per configuration, bypassing secondary authentication for provider: " + provider.getClass());
-                    setSecondaryAuthenticationUser(session, configuration.getRowId(), primaryAuthUser);
-                    continue;
-                }
+                    SecondaryAuthenticationProvider<?> provider = configuration.getAuthenticationProvider();
+                    if (provider.bypass())
+                    {
+                        _log.info("Per configuration, bypassing secondary authentication for provider: " + provider.getClass());
+                        setSecondaryAuthenticationUser(session, configuration.getRowId(), primaryAuthUser);
+                        continue;
+                    }
 
-                return new AuthenticationResult(configuration.getRedirectURL(primaryAuthUser, c));
-            }
+                    return new AuthenticationResult(configuration.getRedirectURL(primaryAuthUser, c));
+                }
 
-            // Validate that secondary auth user matches primary auth user
-            if (!secondaryAuthUser.equals(primaryAuthUser))
-                throw new IllegalStateException("Wrong user");
+                // Validate that secondary auth user matches primary auth user
+                if (!secondaryAuthUser.equals(primaryAuthUser))
+                    throw new IllegalStateException("Wrong user");
 
-            // validators.add();  TODO: provide mechanism for secondary auth providers to convey a validator
+                // validators.add();  TODO: provide mechanism for secondary auth providers to convey a validator
+            }
         }
 
         // Get the redirect URL from the current session

api/src/org/labkey/api/security/AuthenticationProvider.java

@@ -279,8 +279,9 @@ class AuthenticationResponse
         private final @Nullable ActionURL _redirectURL;
         private final @NotNull Map<String, String> _attributeMap;
         private final @Nullable String _successDetails;
+        private final boolean _requireSecondary;
 
-        private AuthenticationResponse(@NotNull PrimaryAuthenticationConfiguration<?> configuration, @NotNull ValidEmail email, @Nullable AuthenticationValidator validator, @NotNull Map<String, String> attributeMap, @Nullable String successDetails)
+        private AuthenticationResponse(@NotNull PrimaryAuthenticationConfiguration<?> configuration, @NotNull ValidEmail email, @Nullable AuthenticationValidator validator, @NotNull Map<String, String> attributeMap, @Nullable String successDetails, boolean requireSecondary)
         {
             _configuration = configuration;
             _email = email;
@@ -289,6 +290,7 @@ private AuthenticationResponse(@NotNull PrimaryAuthenticationConfiguration<?> co
             _failureReason = null;
             _redirectURL = null;
             _successDetails = null != successDetails ? successDetails : "the \"" + configuration.getDescription() + "\" configuration";
+            _requireSecondary = requireSecondary;
         }
 
         private AuthenticationResponse(@NotNull PrimaryAuthenticationConfiguration<?> configuration, @NotNull FailureReason failureReason, @Nullable ActionURL redirectURL)
@@ -300,6 +302,7 @@ private AuthenticationResponse(@NotNull PrimaryAuthenticationConfiguration<?> co
             _redirectURL = redirectURL;
             _attributeMap = Collections.emptyMap();
             _successDetails = null;
+            _requireSecondary = true;
         }
 
         /**
@@ -309,23 +312,25 @@ private AuthenticationResponse(@NotNull PrimaryAuthenticationConfiguration<?> co
          */
         public static AuthenticationResponse createSuccessResponse(PrimaryAuthenticationConfiguration<?> configuration, ValidEmail email)
         {
-            return createSuccessResponse(configuration, email, null, Collections.emptyMap(), null);
+            return createSuccessResponse(configuration, email, null, Collections.emptyMap(), null, true);
         }
 
         /**
          * Creates an authentication provider response that can include a validator to be called on every request and a
          * map of user attributes
-         * @param configuration The PrimaryAuthenticationConfiguration that was used in this authentication attempt
-         * @param email Valid email address of the authenticated user
-         * @param validator An authentication validator
-         * @param attributeMap A <b>case-insensitive</b> map of attribute names and values associated with this authentication
-         * @param successDetails An optional string describing how successful authentication took place, which will appear in
-         *                       the audit log. If null, the configuration's description will be used.
+         *
+         * @param configuration     The PrimaryAuthenticationConfiguration that was used in this authentication attempt
+         * @param email             Valid email address of the authenticated user
+         * @param validator         An authentication validator
+         * @param attributeMap      A <b>case-insensitive</b> map of attribute names and values associated with this authentication
+         * @param successDetails    An optional string describing how successful authentication took place, which will appear in
+         *                          the audit log. If null, the configuration's description will be used.
+         * @param requireSecondary  Require secondary authentication
          * @return A new successful authentication response containing the email address of the authenticated user and a validator
          */
-        public static AuthenticationResponse createSuccessResponse(@NotNull PrimaryAuthenticationConfiguration<?> configuration, ValidEmail email, @Nullable AuthenticationValidator validator, @NotNull Map<String, String> attributeMap, @Nullable String successDetails)
+        public static AuthenticationResponse createSuccessResponse(@NotNull PrimaryAuthenticationConfiguration<?> configuration, ValidEmail email, @Nullable AuthenticationValidator validator, @NotNull Map<String, String> attributeMap, @Nullable String successDetails, boolean requireSecondary)
         {
-            return new AuthenticationResponse(configuration, email, validator, attributeMap, successDetails);
+            return new AuthenticationResponse(configuration, email, validator, attributeMap, successDetails, requireSecondary);
         }
 
         public static AuthenticationResponse createFailureResponse(@NotNull PrimaryAuthenticationConfiguration<?> configuration, FailureReason failureReason)
@@ -384,6 +389,11 @@ public String getSuccessDetails()
         {
             return _successDetails;
         }
+
+        public boolean requireSecondary()
+        {
+            return _requireSecondary;
+        }
     }
 
     // FailureReasons are only reported to administrators (in the audit log and/or server log), NOT to users (and potential

api/src/org/labkey/api/security/SecurityManager.java

@@ -17,7 +17,6 @@
 package org.labkey.api.security;
 
 import org.apache.commons.codec.binary.Base64;
-import org.apache.commons.collections4.bag.HashBag;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.logging.log4j.Level;
 import org.apache.logging.log4j.Logger;

api/src/org/labkey/api/util/BaseScanner.java

@@ -150,7 +150,7 @@ public Collection<String> split(char ch)
 
     /**
      * Returns the text stripped of all comments (while correctly handling comment characters in quoted strings).
-     * @return StringBuilder containing the stripped text
+     * @return StringBuilder containing the text stripped of comments
      */
     public StringBuilder stripComments()
     {
@@ -174,6 +174,32 @@ public boolean comment(int startIndex, int endIndex)
         return ret;
     }
 
+    /**
+     * Returns the text stripped of all quoted strings (while correctly handling quote characters in comments).
+     * @return StringBuilder containing the text stripped of quoted strings
+     */
+    public StringBuilder stripQuotedStrings()
+    {
+        StringBuilder ret = new StringBuilder();
+        MutableInt previous = new MutableInt(0);
+
+        scan(0, new Handler()
+        {
+            @Override
+            public boolean string(int startIndex, int endIndex)
+            {
+                ret.append(_text, previous.getValue(), startIndex);
+                previous.setValue(endIndex);
+
+                return true;
+            }
+        });
+
+        ret.append(_text.substring(previous.getValue()));
+
+        return ret;
+    }
+
     /**
      * Scans the text, handling all comments and quoted strings (while correctly handling escaping rules within those
      * strings) and calling handler methods along the way. See subclasses for examples.

core/src/org/labkey/core/dialect/PostgreSql92Dialect.java

@@ -63,7 +63,7 @@ protected Set<String> getReservedWords()
     @Override
     public String getProductName()
     {
-        return PostgreSqlDialectFactory.PRODUCT_NAME;
+        return PostgreSql91Dialect.PRODUCT_NAME;
     }
 
     @Override