Deprecate LabKeyTransformSessionId cookie auth

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

@@ -124,6 +124,7 @@
 import org.labkey.api.reader.MapLoader;
 import org.labkey.api.reader.StrictBoundedReader;
 import org.labkey.api.reader.TabLoader;
+import org.labkey.api.reports.ExternalScriptEngine;
 import org.labkey.api.reports.model.ViewCategoryManager;
 import org.labkey.api.reports.report.ReportType;
 import org.labkey.api.reports.report.r.RReport;
@@ -440,6 +441,7 @@ public void registerServlets(ServletContext servletCtx)
             ExcelWriter.TestCase.class,
             ExistingRecordDataIterator.TestCase.class,
             ExperimentJSONConverter.TestCase.class,
+            ExternalScriptEngine.TestCase.class,
             ExtUtil.TestCase.class,
             FieldKey.TestCase.class,
             FileType.TestCase.class,

api/src/org/labkey/api/assay/transform/DataTransformService.java

@@ -48,6 +48,8 @@
 import java.util.Map;
 import java.util.Set;
 
+import static org.labkey.api.security.SecurityManager.FEATUREFLAG_ALLOW_TRANSFORM_SESSION_ID;
+
 public class DataTransformService
 {
     private static final DataTransformService _instance = new DataTransformService();
@@ -257,12 +259,14 @@ public void addStandardParameters(@Nullable HttpServletRequest request, @Nullabl
             if (srcDir != null && srcDir.exists())
                 paramMap.put(SRC_DIR_REPLACEMENT, srcDir.toNioPathForRead().toFile().getAbsolutePath().replaceAll("\\\\", "/"));
         }
-        paramMap.put(R_SESSIONID_REPLACEMENT, getSessionInfo(request, apiKey));
-        paramMap.put(LEGACY_SESSION_COOKIE_NAME_REPLACEMENT, getSessionCookieName(request));
-        paramMap.put(LEGACY_SESSION_ID_REPLACEMENT, getSessionId(request, apiKey));
+        if (AppProps.getInstance().isOptionalFeatureEnabled(FEATUREFLAG_ALLOW_TRANSFORM_SESSION_ID))
+        {
+            paramMap.put(R_SESSIONID_REPLACEMENT, getSessionInfo(request, apiKey));
+            paramMap.put(LEGACY_SESSION_COOKIE_NAME_REPLACEMENT, getSessionCookieName(request));
+            paramMap.put(LEGACY_SESSION_ID_REPLACEMENT, getSessionId(request, apiKey));
+        }
         paramMap.put(SecurityManager.API_KEY, apiKey);
-        paramMap.put(BASE_SERVER_URL_REPLACEMENT, AppProps.getInstance().getBaseServerUrl()
-                + AppProps.getInstance().getContextPath());
+        paramMap.put(BASE_SERVER_URL_REPLACEMENT, AppProps.getInstance().getBaseServerUrl() + AppProps.getInstance().getContextPath());
         paramMap.put(CONTAINER_PATH, container == null ? null : container.getPath());
     }
 
@@ -312,6 +316,12 @@ private boolean isDefault(ExpProtocol protocol)
      */
     private String getSessionInfo(@Nullable HttpServletRequest request, String apiKey)
     {
+        if (request == null)
+        {
+            // GH Issue 1489: background/pipeline jobs have no live HTTP session, so use apikey authentication
+            // directly instead of the deprecated LabKeyTransformSessionId cookie.
+            return "labkey.setDefaults(apiKey = \"" + apiKey + "\")\n";
+        }
         return "labkey.sessionCookieName = \"" + getSessionCookieName(request) + "\"\n" +
                 "labkey.sessionCookieContents = \"" + getSessionId(request, apiKey) + "\"\n";
     }

api/src/org/labkey/api/pipeline/TaskFactory.java

@@ -17,6 +17,7 @@
 
 import org.apache.logging.log4j.Logger;
 import org.labkey.api.module.Module;
+import org.labkey.api.pipeline.PipelineJob.Task;
 import org.labkey.api.pipeline.file.FileAnalysisJobSupport;
 import org.labkey.api.util.FileType;
 
@@ -27,26 +28,24 @@
  * <code>TaskFactory</code> is responsible for creating a task to run on a
  * PipelineJob.  Create an implementation of this interface to support custom
  * Task configuration inside the Mule configuration Spring context.
- *
- * @author brendanx
  */
 public interface TaskFactory<SettingsType extends TaskFactorySettings>
 {
     TaskId getId();
 
     TaskId getActiveId(PipelineJob job);
 
-    PipelineJob.Task createTask(PipelineJob job);
+    Task<?> createTask(PipelineJob job);
 
-    TaskFactory cloneAndConfigure(SettingsType settings) throws CloneNotSupportedException;
+    TaskFactory<?> cloneAndConfigure(SettingsType settings) throws CloneNotSupportedException;
 
     /** @return the types of files that are consumable by this task as input */
     List<FileType> getInputTypes();
 
     /**
-     * All of the ProtocolAction names that this task may include when it runs. It need not execute all of them for
-     * each invocation.
-     * These names are used to build up a full Experiment Protocol for each pipeline to which this tasks belongs.
+     * All the ProtocolAction names that this task may include when it runs. It need not execute all of them for
+     * each invocation. These names are used to build up a full Experiment Protocol for each pipeline to which this
+     * task belongs.
      */
     List<String> getProtocolActionNames();
 
@@ -57,7 +56,7 @@ public interface TaskFactory<SettingsType extends TaskFactorySettings>
     String getGroupParameterName();
 
     /**
-     * @return true if this task operates on all of the split items (say, multiple input files) as a whole, or false
+     * @return true if this task operates on all the split items (say, multiple input files) as a whole, or false
      * if each split item should be operated on independently (and potentially in parallel)
      */
     boolean isJoin();

api/src/org/labkey/api/reports/ExternalScriptEngine.java

@@ -19,6 +19,10 @@
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.jetbrains.annotations.Nullable;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
 import org.labkey.api.miniprofiler.CustomTiming;
 import org.labkey.api.miniprofiler.MiniProfiler;
 import org.labkey.api.pipeline.PipelineJobService;
@@ -39,6 +43,7 @@
 import javax.script.ScriptEngineFactory;
 import javax.script.ScriptException;
 import javax.script.SimpleBindings;
+import javax.script.SimpleScriptContext;
 import java.io.BufferedReader;
 import java.io.BufferedWriter;
 import java.io.File;
@@ -49,8 +54,10 @@
 import java.io.Writer;
 import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
@@ -59,6 +66,8 @@
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
+import static org.labkey.api.reports.report.r.ParamReplacementSvc.SubstitutionSyntax.INLINE;
+
 /*
 * User: Karl Lum
 * Date: Dec 2, 2008
@@ -146,7 +155,7 @@ protected Object evalScript(String script, ScriptContext context) throws ScriptE
      * Prepare the on-disk script file that will be executed. The default writes the script as-is; subclasses (e.g. the
      * R engine's knitr handling) may wrap it in a different driver script.
      */
-    protected FileLike prepareScriptFile(String script, ScriptContext context, List<String> extensions)
+    protected FileLike prepareScriptFile(String script, ScriptContext context, List<String> extensions) throws ScriptException
     {
         return writeScriptFile(script, context, extensions);
     }
@@ -552,7 +561,7 @@ protected int runProcess(ScriptContext context, LabKeyProcessBuilder pb, StringB
         }
     }
 
-    protected FileLike writeScriptFile(String script, ScriptContext context, List<String> extensions)
+    protected FileLike writeScriptFile(String script, ScriptContext context, List<String> extensions) throws ScriptException
     {
         // write out the script file to disk using the first extension as the default
         FileLike scriptFile;
@@ -590,12 +599,25 @@ protected FileLike writeScriptFile(String script, ScriptContext context, List<St
                     }
                 }
 
+                // Fail fast if there are unreplaced substitutions, to provide a much better user-facing error message.
+                Matcher matcher = INLINE.getMatchPattern().matcher(script);
+                Set<String> unreplaced = new LinkedHashSet<>();
+                while (matcher.find())
+                    unreplaced.add(matcher.group(1));
+
+                if (!unreplaced.isEmpty())
+                    throw new ScriptException("Unreplaced substitution parameter(s) found in script: " + String.join(", ", unreplaced));
+
                 try (PrintWriter pw = new PrintWriter(new BufferedWriter(new OutputStreamWriter(scriptFile.openOutputStream(), StandardCharsets.UTF_8))))
                 {
                     pw.write(script);
                 }
             }
         }
+        catch (ScriptException e)
+        {
+            throw e;
+        }
         catch (Exception e)
         {
             ExceptionUtil.logExceptionToMothership(null, e);
@@ -705,4 +727,57 @@ public boolean supportsContext(LabKeyScriptEngineManager.EngineContext context)
     {
         return true;
     }
+
+    public static class TestCase extends Assert
+    {
+        private static final String KNOWN_PARAM = "knownParam";
+        private static final String KNOWN_VALUE = "replacement value";
+
+        private ExternalScriptEngine _engine;
+        private ScriptContext _context;
+        private FileLike _scriptFile;
+
+        @Before
+        public void setUp()
+        {
+            _engine = new ExternalScriptEngine(null);
+            _context = new SimpleScriptContext();
+            Bindings bindings = _engine.createBindings();
+            bindings.put(PARAM_REPLACEMENT_MAP, Map.of(KNOWN_PARAM, KNOWN_VALUE));
+            _context.setBindings(bindings, ScriptContext.ENGINE_SCOPE);
+        }
+
+        @After
+        public void tearDown() throws IOException
+        {
+            if (null != _scriptFile && _scriptFile.exists())
+                _scriptFile.delete();
+        }
+
+        @Test
+        public void testFullyReplacedScriptSucceeds() throws ScriptException
+        {
+            String script = "print(\"${" + KNOWN_PARAM + "}\")";
+            _scriptFile = _engine.writeScriptFile(script, _context, List.of("R"));
+            assertTrue("Script file should have been written", _scriptFile.exists());
+        }
+
+        @Test
+        public void testUnreplacedSubstitutionThrows()
+        {
+            String script = "print(\"${unknownParam}\")";
+            ScriptException e = assertThrows(ScriptException.class, () -> _engine.writeScriptFile(script, _context, List.of("R")));
+            assertTrue("Exception message should name the unreplaced parameter", e.getMessage().contains("unknownParam"));
+        }
+
+        @Test
+        public void testMultipleUnreplacedSubstitutionsAreAllNamed()
+        {
+            String script = "${firstUnknown} and ${" + KNOWN_PARAM + "} and ${secondUnknown}";
+            ScriptException e = assertThrows(ScriptException.class, () -> _engine.writeScriptFile(script, _context, List.of("R")));
+            assertTrue("Exception message should name the first unreplaced parameter", e.getMessage().contains("firstUnknown"));
+            assertTrue("Exception message should name the second unreplaced parameter", e.getMessage().contains("secondUnknown"));
+            assertFalse("Exception message should not include the known, replaced parameter", e.getMessage().contains(KNOWN_PARAM));
+        }
+    }
 }

api/src/org/labkey/api/reports/report/r/RScriptEngine.java

@@ -63,7 +63,7 @@ public ScriptEngineFactory getFactory()
     }
 
     @Override
-    protected FileLike prepareScriptFile(String script, ScriptContext context, List<String> extensions)
+    protected FileLike prepareScriptFile(String script, ScriptContext context, List<String> extensions) throws ScriptException
     {
         FileLike scriptFile;
         if (getKnitrFormat(context) != RReportDescriptor.KnitrFormat.None)

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

@@ -30,6 +30,7 @@
 import org.labkey.api.module.ModuleLoader;
 import org.labkey.api.module.SafeFlushResponseWrapper;
 import org.labkey.api.query.QueryService;
+import org.labkey.api.security.SecurityManager.AuthenticationAttempt;
 import org.labkey.api.security.impersonation.ImpersonationContextFactory;
 import org.labkey.api.security.impersonation.UnauthorizedImpersonationException;
 import org.labkey.api.settings.AppProps;
@@ -39,7 +40,6 @@
 import org.labkey.api.util.GUID;
 import org.labkey.api.util.HttpUtil;
 import org.labkey.api.util.HttpsUtil;
-import org.labkey.api.util.Pair;
 import org.labkey.api.view.UnauthorizedException;
 import org.labkey.api.view.ViewServlet;
 
@@ -168,12 +168,12 @@ else if (!AppProps.getInstance().isDevMode())
 
         try
         {
-            Pair<User, HttpServletRequest> pair = SecurityManager.attemptAuthentication(req, resp);
+            AuthenticationAttempt attempt = SecurityManager.attemptAuthentication(req, resp);
 
-            if (null != pair)
+            if (null != attempt)
             {
-                user = pair.getKey();
-                req = pair.getValue();
+                user = attempt.user();
+                req = attempt.request();
             }
         }
         catch (UnauthorizedImpersonationException uie)