From a4da44576484410d79691ddc10a4ff13935f23d9 Mon Sep 17 00:00:00 2001 From: labkey-tchad Date: Mon, 2 Jun 2025 17:10:45 -0700 Subject: [PATCH 01/12] Test CSP enforcement --- src/org/labkey/test/TestProperties.java | 2 +- src/org/labkey/test/tests/CrawlerTest.java | 25 +++++++++++++++++-- src/org/labkey/test/util/CspLogUtil.java | 21 ++++++++++++++++ .../test/util/OptionalFeatureHelper.java | 16 +++++++++++- 4 files changed, 60 insertions(+), 4 deletions(-) diff --git a/src/org/labkey/test/TestProperties.java b/src/org/labkey/test/TestProperties.java index 34385b041f..7a7c86800d 100644 --- a/src/org/labkey/test/TestProperties.java +++ b/src/org/labkey/test/TestProperties.java @@ -129,7 +129,7 @@ public static boolean isQueryCheckSkipped() public static boolean isCspCheckSkipped() { - return !getBooleanProperty("webtest.cspCheck", false); + return !getBooleanProperty("webtest.cspCheck", true); } public static boolean isNewWebDriverForEachTest() diff --git a/src/org/labkey/test/tests/CrawlerTest.java b/src/org/labkey/test/tests/CrawlerTest.java index f703ae20fd..d536fa82c4 100644 --- a/src/org/labkey/test/tests/CrawlerTest.java +++ b/src/org/labkey/test/tests/CrawlerTest.java @@ -30,7 +30,7 @@ public class CrawlerTest extends BaseWebDriverTest { private static final String MODULE_NAME = "CrawlerTest"; - private static final String USER = "injectiontester@labkey.injection.test"; + private static final String USER = "injectiontester@labkey.injection.test"; // required by 'injectJsp' page @Override protected void doCleanup(boolean afterTest) @@ -42,13 +42,14 @@ protected void doCleanup(boolean afterTest) @BeforeClass public static void setupProject() { - CrawlerTest init = (CrawlerTest) getCurrentTest(); + CrawlerTest init = getCurrentTest(); init.doSetup(); } private void doSetup() { + CspLogUtil.debugCspWarnings(); _containerHelper.createProject(getProjectName(), null); _userHelper.createUser(USER); new ApiPermissionsHelper(this).addMemberToRole(USER, "Reader", MemberType.user, getProjectName()); @@ -60,6 +61,8 @@ private void doSetup() @Test public void testCrawlerTest() throws Exception { + CspLogUtil.setEnforceCsp(createDefaultConnection(), false); + String safeParam = "OK!"; log("Verify vulnerable page requires specific user"); @@ -100,11 +103,27 @@ public void testCrawlerTest() throws Exception } } + @Test (expected = CspLogUtil.CspWarningDetectedException.class) + public void testEnforceCsp() throws Exception + { + CspLogUtil.setEnforceCsp(createDefaultConnection(), true); + + createDefaultConnection().impersonate(USER); + + log("Verify that page is not vulnerable when CSP is enforced"); + beginAt(getInjectUrl(Crawler.injectScriptBlock), 10_000); + + log("Verify that enforced CSP is also reported"); + CspLogUtil.checkNewCspWarnings(getArtifactCollector()); + } + @Test (expected = CspLogUtil.CspWarningDetectedException.class) public void testCspWarning() { Assume.assumeFalse("Can't test for CSP report", TestProperties.isCspCheckSkipped()); + CspLogUtil.setEnforceCsp(createDefaultConnection(), false); + beginAt(WebTestHelper.buildRelativeUrl(MODULE_NAME, getProjectName(), "cspWarning")); CspLogUtil.checkNewCspWarnings(getArtifactCollector()); } @@ -134,6 +153,8 @@ public void testExternalLink() throws Exception @Test public void testCrawler() throws Exception { + CspLogUtil.setEnforceCsp(createDefaultConnection(), false); + String safeParam = "OK!"; createDefaultConnection().impersonate(USER); diff --git a/src/org/labkey/test/util/CspLogUtil.java b/src/org/labkey/test/util/CspLogUtil.java index 4f7d79ceba..5e75632558 100644 --- a/src/org/labkey/test/util/CspLogUtil.java +++ b/src/org/labkey/test/util/CspLogUtil.java @@ -4,9 +4,12 @@ import org.apache.commons.collections4.multimap.HashSetValuedHashMap; import org.apache.commons.io.IOUtils; import org.junit.Assert; +import org.labkey.remoteapi.Connection; import org.labkey.serverapi.writer.PrintWriters; import org.labkey.test.TestFileUtils; import org.labkey.test.TestProperties; +import org.labkey.test.pages.core.admin.logger.ManagerPage; +import org.labkey.test.pages.core.admin.logger.ManagerPage.LoggingLevel; import java.io.File; import java.io.FileInputStream; @@ -18,9 +21,12 @@ import java.nio.file.attribute.BasicFileAttributes; import java.util.Collection; import java.util.List; +import java.util.Objects; public class CspLogUtil { + private static final String DISABLE_ENFORCE_CSP_OPTIONAL_FEATURE = "disableEnforceCsp"; + private static final List ignoredViolations = List.of( "/_rstudio/", "/_rstudioReport/", @@ -159,6 +165,21 @@ public static void resetCspLogMark() lastModified = logFile.lastModified(); } + public static void setEnforceCsp(Connection connection, boolean enforce) + { + Objects.requireNonNull(OptionalFeatureHelper.setOptionalFeature(connection, DISABLE_ENFORCE_CSP_OPTIONAL_FEATURE, !enforce), () -> "Unable to configure enforce CSP."); + } + + public static void resetEnforceCsp(Connection connection) + { + OptionalFeatureHelper.resetOptionalFeature(connection, DISABLE_ENFORCE_CSP_OPTIONAL_FEATURE); + } + + public static void debugCspWarnings() + { + Log4jUtils.setLogLevel("org.labkey.core.admin.AdminController.ContentSecurityPolicyReportAction", LoggingLevel.DEBUG); + } + public static class CspWarningDetectedException extends AssertionError { public CspWarningDetectedException(Object detailMessage) diff --git a/src/org/labkey/test/util/OptionalFeatureHelper.java b/src/org/labkey/test/util/OptionalFeatureHelper.java index 09d1b54b90..6d89eabaed 100644 --- a/src/org/labkey/test/util/OptionalFeatureHelper.java +++ b/src/org/labkey/test/util/OptionalFeatureHelper.java @@ -28,6 +28,8 @@ public class OptionalFeatureHelper { + private static final Map initialValues = new HashMap<>(); + public static Boolean enableOptionalFeature(Connection cn, String feature) { return setOptionalFeature(cn, feature, true); @@ -56,7 +58,9 @@ public static Boolean setOptionalFeature(Connection cn, String feature, boolean CommandResponse r = command.execute(cn, null); Map response = r.getParsedData(); - return (Boolean)response.get("previouslyEnabled"); + Boolean previouslyEnabled = (Boolean) response.get("previouslyEnabled"); + initialValues.putIfAbsent(feature, previouslyEnabled); + return previouslyEnabled; } catch (IOException e) { @@ -68,6 +72,16 @@ public static Boolean setOptionalFeature(Connection cn, String feature, boolean } } + @LogMethod + public static void resetOptionalFeature(Connection cn, @LoggedParam String feature) + { + if (initialValues.containsKey(feature)) + { + setOptionalFeature(cn, feature, initialValues.get(feature)); + initialValues.remove(feature); + } + } + public static boolean isOptionalFeatureEnabled(Connection cn, String feature) { SimpleGetCommand command = new SimpleGetCommand("admin", "optionalFeature"); From 133f39a835aa466fa8d965663d62bc3bc364329c Mon Sep 17 00:00:00 2001 From: labkey-tchad Date: Wed, 4 Jun 2025 11:41:41 -0700 Subject: [PATCH 02/12] CSP allow list API helper --- src/org/labkey/test/TestProperties.java | 55 +++++++++++++------ .../test/pages/admin/ExternalSourcesPage.java | 22 ++++++++ .../test/tests/AbstractKnitrReportTest.java | 6 +- test.properties.template | 6 +- 4 files changed, 66 insertions(+), 23 deletions(-) diff --git a/src/org/labkey/test/TestProperties.java b/src/org/labkey/test/TestProperties.java index 7a7c86800d..1598346248 100644 --- a/src/org/labkey/test/TestProperties.java +++ b/src/org/labkey/test/TestProperties.java @@ -30,6 +30,7 @@ import java.time.LocalDateTime; import java.time.ZoneId; import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.Iterator; import java.util.List; @@ -264,6 +265,10 @@ public static boolean isAssayProductFeatureAvailable() return isProductFeatureAvailable("assay"); } + /** + * Product features are assumed to be available unless the test environment (usually TeamCity) explicitly specifies + * that it is not. + */ public static boolean isProductFeatureAvailable(String feature) { return "true".equals(System.getProperty("webtest.productFeature." + feature.toLowerCase(), "true")); @@ -276,7 +281,7 @@ public static boolean ignoreDatabaseNotSupportedException() /** * Parses system property 'webtest.server.startup.timeout' to determine maximum allowed server startup time. - * If property is not defined or is not an integer, it defaults to 60 seconds. + * If property is not defined or is not an integer, it defaults to 120 seconds. * @return Maximum number of seconds to wait for server startup */ public static int getServerStartupTimeout() @@ -289,34 +294,48 @@ public static String getAdditionalPipelineTools() return System.getProperty("additional.pipeline.tools"); } + private static Map _optionalFeatures = null; public static Map getOptionalFeatures() { - Map features = new HashMap<>(); - - Properties props = System.getProperties(); - for (Map.Entry entry : props.entrySet()) + if (_optionalFeatures == null) { - String key = String.valueOf(entry.getKey()); - Boolean value = (entry.getValue() instanceof Boolean) - ? (Boolean)entry.getValue() - : Boolean.valueOf(String.valueOf(entry.getValue())); + Map features = new HashMap<>(); - String prefix = "webtest.experimental."; // Can be used with any optional feature flags; "experimental" is used for backward-compatibility purposes. - if (key.startsWith(prefix)) + Properties props = System.getProperties(); + for (Map.Entry entry : props.entrySet()) { - String feature = key.substring(prefix.length()); - features.put(feature, value); + String key = String.valueOf(entry.getKey()); + + String expPrefix = "webtest.experimental."; // "experimental" is accepted for backward-compatibility purposes. + String optPrefix = "webtest.optional."; // Preferred prefix + for (String prefix : List.of(expPrefix, optPrefix)) + { + if (key.startsWith(prefix)) + { + Boolean value = (entry.getValue() instanceof Boolean) + ? (Boolean) entry.getValue() + : Boolean.valueOf(String.valueOf(entry.getValue())); + String feature = key.substring(prefix.length()); + features.put(feature, value); + } + } } - } - return features; + _optionalFeatures = Collections.unmodifiableMap(features); + } + return _optionalFeatures; } + private static List debugLoggingPackages = null; public static List getDebugLoggingPackages() { - String prop = System.getProperty("webtest.debug.server.packages", ""); - String[] packages = prop.split("\\s*,\\s*"); - return Arrays.stream(packages).map(String::trim).filter(s -> !s.isEmpty()).collect(Collectors.toList()); + if (debugLoggingPackages == null) + { + String prop = System.getProperty("webtest.debug.server.packages", ""); + String[] packages = prop.split(","); + debugLoggingPackages = Arrays.stream(packages).map(String::trim).filter(s -> !s.isEmpty()).collect(Collectors.toList()); + } + return debugLoggingPackages; } private static File dumpDir = null; diff --git a/src/org/labkey/test/pages/admin/ExternalSourcesPage.java b/src/org/labkey/test/pages/admin/ExternalSourcesPage.java index 4bcbf201cb..3b75c4e2e6 100644 --- a/src/org/labkey/test/pages/admin/ExternalSourcesPage.java +++ b/src/org/labkey/test/pages/admin/ExternalSourcesPage.java @@ -1,5 +1,9 @@ package org.labkey.test.pages.admin; +import org.json.JSONObject; +import org.labkey.remoteapi.CommandException; +import org.labkey.remoteapi.Connection; +import org.labkey.remoteapi.SimplePostCommand; import org.labkey.test.Locator; import org.labkey.test.WebDriverWrapper; import org.labkey.test.WebTestHelper; @@ -9,9 +13,11 @@ import org.labkey.test.pages.LabKeyPage; import org.labkey.test.util.LogMethod; import org.labkey.test.util.LoggedParam; +import org.labkey.test.util.TestLogger; import org.openqa.selenium.WebDriver; import org.openqa.selenium.WebElement; +import java.io.IOException; import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; @@ -35,6 +41,22 @@ public static ExternalSourcesPage beginAt(WebDriverWrapper webDriverWrapper) return new ExternalSourcesPage(webDriverWrapper.getDriver()); } + public void deleteAllApi() + { + SimplePostCommand postCommand = new SimplePostCommand("admin", "externalSources"); + postCommand.setParameters(Map.of( + "saveAll", true, + "existingValues", "")); + try + { + postCommand.execute(createDefaultConnection(), null); + } + catch (IOException | CommandException e) + { + throw new RuntimeException(e); + } + } + @LogMethod public ExternalSourcesPage ensureHost(Directive directive, String host) { diff --git a/src/org/labkey/test/tests/AbstractKnitrReportTest.java b/src/org/labkey/test/tests/AbstractKnitrReportTest.java index 3f45ddd519..cdd4cd1de4 100644 --- a/src/org/labkey/test/tests/AbstractKnitrReportTest.java +++ b/src/org/labkey/test/tests/AbstractKnitrReportTest.java @@ -24,6 +24,7 @@ import org.labkey.test.pages.reports.ManageViewsPage; import org.labkey.test.util.CodeMirrorHelper; import org.labkey.test.util.LogMethod; +import org.labkey.test.util.LoggedParam; import org.labkey.test.util.PortalHelper; import org.labkey.test.util.RReportHelper; import org.openqa.selenium.WebElement; @@ -62,7 +63,7 @@ private static String readReport(final Path reportFile) @BeforeClass public static void initProject() { - AbstractKnitrReportTest init = (AbstractKnitrReportTest)getCurrentTest(); + AbstractKnitrReportTest init = getCurrentTest(); init.setupProject(); } @@ -88,7 +89,8 @@ protected boolean isDocker() return false; } - protected String createKnitrReport(Path reportSourcePath, RReportHelper.ReportOption knitrOption) + @LogMethod + protected String createKnitrReport(@LoggedParam Path reportSourcePath, @LoggedParam RReportHelper.ReportOption knitrOption) { String reportSource = readReport(reportSourcePath); diff --git a/test.properties.template b/test.properties.template index 90aed9ef8b..6197e500ce 100644 --- a/test.properties.template +++ b/test.properties.template @@ -157,7 +157,7 @@ queryCheck=false ## Verify all custom views after each test class viewCheck=false ## Check for CSP warning after tests -webtest.cspCheck=false +#webtest.cspCheck=true #============================================================================== @@ -173,5 +173,5 @@ use.cloud.pipeline=false cloud.pipeline.bucket= ## List of loggers to enable DEBUG logging for. webtest.debug.server.packages= -## Enable/Disable experimental features. -#webtest.experimental.=[true|false] +## Enable/Disable optional features. +#webtest.optional.=[true|false] From 664c3580cb048f580d8df1793c009f141d26e315 Mon Sep 17 00:00:00 2001 From: labkey-tchad Date: Wed, 4 Jun 2025 15:06:50 -0700 Subject: [PATCH 03/12] Add allowed hosts --- .../test/pages/admin/ExternalSourcesPage.java | 22 ----- .../test/tests/AbstractKnitrReportTest.java | 7 ++ src/org/labkey/test/tests/CrawlerTest.java | 13 +-- src/org/labkey/test/util/CspLogUtil.java | 24 +----- .../test/util/core/admin/CspConfigHelper.java | 84 +++++++++++++++++++ 5 files changed, 100 insertions(+), 50 deletions(-) create mode 100644 src/org/labkey/test/util/core/admin/CspConfigHelper.java diff --git a/src/org/labkey/test/pages/admin/ExternalSourcesPage.java b/src/org/labkey/test/pages/admin/ExternalSourcesPage.java index 3b75c4e2e6..4bcbf201cb 100644 --- a/src/org/labkey/test/pages/admin/ExternalSourcesPage.java +++ b/src/org/labkey/test/pages/admin/ExternalSourcesPage.java @@ -1,9 +1,5 @@ package org.labkey.test.pages.admin; -import org.json.JSONObject; -import org.labkey.remoteapi.CommandException; -import org.labkey.remoteapi.Connection; -import org.labkey.remoteapi.SimplePostCommand; import org.labkey.test.Locator; import org.labkey.test.WebDriverWrapper; import org.labkey.test.WebTestHelper; @@ -13,11 +9,9 @@ import org.labkey.test.pages.LabKeyPage; import org.labkey.test.util.LogMethod; import org.labkey.test.util.LoggedParam; -import org.labkey.test.util.TestLogger; import org.openqa.selenium.WebDriver; import org.openqa.selenium.WebElement; -import java.io.IOException; import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; @@ -41,22 +35,6 @@ public static ExternalSourcesPage beginAt(WebDriverWrapper webDriverWrapper) return new ExternalSourcesPage(webDriverWrapper.getDriver()); } - public void deleteAllApi() - { - SimplePostCommand postCommand = new SimplePostCommand("admin", "externalSources"); - postCommand.setParameters(Map.of( - "saveAll", true, - "existingValues", "")); - try - { - postCommand.execute(createDefaultConnection(), null); - } - catch (IOException | CommandException e) - { - throw new RuntimeException(e); - } - } - @LogMethod public ExternalSourcesPage ensureHost(Directive directive, String host) { diff --git a/src/org/labkey/test/tests/AbstractKnitrReportTest.java b/src/org/labkey/test/tests/AbstractKnitrReportTest.java index cdd4cd1de4..3a59ce3e8d 100644 --- a/src/org/labkey/test/tests/AbstractKnitrReportTest.java +++ b/src/org/labkey/test/tests/AbstractKnitrReportTest.java @@ -21,18 +21,21 @@ import org.labkey.test.Locator; import org.labkey.test.TestFileUtils; import org.labkey.test.TestProperties; +import org.labkey.test.pages.admin.ExternalSourcesPage; import org.labkey.test.pages.reports.ManageViewsPage; import org.labkey.test.util.CodeMirrorHelper; import org.labkey.test.util.LogMethod; import org.labkey.test.util.LoggedParam; import org.labkey.test.util.PortalHelper; import org.labkey.test.util.RReportHelper; +import org.labkey.test.util.core.admin.CspConfigHelper; import org.openqa.selenium.WebElement; import java.io.File; import java.nio.file.Path; import java.util.Arrays; import java.util.List; +import java.util.Map; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -70,6 +73,10 @@ public static void initProject() @LogMethod protected void setupProject() { + new CspConfigHelper(this).setAllowedHosts(Map.of( + ExternalSourcesPage.Directive.Style, List.of("https://cdn.datatables.net"), + ExternalSourcesPage.Directive.Font, List.of("https://mathjax.rstudio.com"))); + _rReportHelper.ensureRConfig(isDocker()); _containerHelper.createProject(getProjectName(), "Collaboration"); diff --git a/src/org/labkey/test/tests/CrawlerTest.java b/src/org/labkey/test/tests/CrawlerTest.java index d536fa82c4..bc10407e81 100644 --- a/src/org/labkey/test/tests/CrawlerTest.java +++ b/src/org/labkey/test/tests/CrawlerTest.java @@ -16,6 +16,7 @@ import org.labkey.test.util.Crawler; import org.labkey.test.util.CspLogUtil; import org.labkey.test.util.PermissionsHelper.MemberType; +import org.labkey.test.util.core.admin.CspConfigHelper; import org.labkey.test.util.selenium.WebDriverUtils; import org.openqa.selenium.UnhandledAlertException; @@ -32,6 +33,8 @@ public class CrawlerTest extends BaseWebDriverTest private static final String MODULE_NAME = "CrawlerTest"; private static final String USER = "injectiontester@labkey.injection.test"; // required by 'injectJsp' page + private final CspConfigHelper _cspConfigHelper = new CspConfigHelper(this); + @Override protected void doCleanup(boolean afterTest) { @@ -49,7 +52,7 @@ public static void setupProject() private void doSetup() { - CspLogUtil.debugCspWarnings(); + CspConfigHelper.debugCspWarnings(); _containerHelper.createProject(getProjectName(), null); _userHelper.createUser(USER); new ApiPermissionsHelper(this).addMemberToRole(USER, "Reader", MemberType.user, getProjectName()); @@ -61,7 +64,7 @@ private void doSetup() @Test public void testCrawlerTest() throws Exception { - CspLogUtil.setEnforceCsp(createDefaultConnection(), false); + _cspConfigHelper.setEnforceCsp(false); String safeParam = "OK!"; @@ -106,7 +109,7 @@ public void testCrawlerTest() throws Exception @Test (expected = CspLogUtil.CspWarningDetectedException.class) public void testEnforceCsp() throws Exception { - CspLogUtil.setEnforceCsp(createDefaultConnection(), true); + _cspConfigHelper.setEnforceCsp(true); createDefaultConnection().impersonate(USER); @@ -122,7 +125,7 @@ public void testCspWarning() { Assume.assumeFalse("Can't test for CSP report", TestProperties.isCspCheckSkipped()); - CspLogUtil.setEnforceCsp(createDefaultConnection(), false); + _cspConfigHelper.setEnforceCsp(false); beginAt(WebTestHelper.buildRelativeUrl(MODULE_NAME, getProjectName(), "cspWarning")); CspLogUtil.checkNewCspWarnings(getArtifactCollector()); @@ -153,7 +156,7 @@ public void testExternalLink() throws Exception @Test public void testCrawler() throws Exception { - CspLogUtil.setEnforceCsp(createDefaultConnection(), false); + _cspConfigHelper.setEnforceCsp(false); String safeParam = "OK!"; diff --git a/src/org/labkey/test/util/CspLogUtil.java b/src/org/labkey/test/util/CspLogUtil.java index 5e75632558..fc506a04f1 100644 --- a/src/org/labkey/test/util/CspLogUtil.java +++ b/src/org/labkey/test/util/CspLogUtil.java @@ -4,12 +4,9 @@ import org.apache.commons.collections4.multimap.HashSetValuedHashMap; import org.apache.commons.io.IOUtils; import org.junit.Assert; -import org.labkey.remoteapi.Connection; import org.labkey.serverapi.writer.PrintWriters; import org.labkey.test.TestFileUtils; import org.labkey.test.TestProperties; -import org.labkey.test.pages.core.admin.logger.ManagerPage; -import org.labkey.test.pages.core.admin.logger.ManagerPage.LoggingLevel; import java.io.File; import java.io.FileInputStream; @@ -21,17 +18,13 @@ import java.nio.file.attribute.BasicFileAttributes; import java.util.Collection; import java.util.List; -import java.util.Objects; public class CspLogUtil { - private static final String DISABLE_ENFORCE_CSP_OPTIONAL_FEATURE = "disableEnforceCsp"; - private static final List ignoredViolations = List.of( "/_rstudio/", "/_rstudioReport/", - "/reports-createScriptReport.view?", - "/reports-runReport.view?" + "/reports-createScriptReport.view?" // Issue 53226: reports-streamFile is blocked by object-src CSP directive ); private static final String logName = "csp-report.log"; private static final File logFile = new File(TestFileUtils.getServerLogDir(), logName); @@ -165,21 +158,6 @@ public static void resetCspLogMark() lastModified = logFile.lastModified(); } - public static void setEnforceCsp(Connection connection, boolean enforce) - { - Objects.requireNonNull(OptionalFeatureHelper.setOptionalFeature(connection, DISABLE_ENFORCE_CSP_OPTIONAL_FEATURE, !enforce), () -> "Unable to configure enforce CSP."); - } - - public static void resetEnforceCsp(Connection connection) - { - OptionalFeatureHelper.resetOptionalFeature(connection, DISABLE_ENFORCE_CSP_OPTIONAL_FEATURE); - } - - public static void debugCspWarnings() - { - Log4jUtils.setLogLevel("org.labkey.core.admin.AdminController.ContentSecurityPolicyReportAction", LoggingLevel.DEBUG); - } - public static class CspWarningDetectedException extends AssertionError { public CspWarningDetectedException(Object detailMessage) diff --git a/src/org/labkey/test/util/core/admin/CspConfigHelper.java b/src/org/labkey/test/util/core/admin/CspConfigHelper.java new file mode 100644 index 0000000000..e458c97e41 --- /dev/null +++ b/src/org/labkey/test/util/core/admin/CspConfigHelper.java @@ -0,0 +1,84 @@ +package org.labkey.test.util.core.admin; + +import org.labkey.remoteapi.CommandException; +import org.labkey.remoteapi.Connection; +import org.labkey.remoteapi.SimplePostCommand; +import org.labkey.test.WebDriverWrapper; +import org.labkey.test.pages.admin.ExternalSourcesPage.Directive; +import org.labkey.test.pages.core.admin.logger.ManagerPage; +import org.labkey.test.util.Log4jUtils; +import org.labkey.test.util.OptionalFeatureHelper; + +import java.io.IOException; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.function.Supplier; + +public class CspConfigHelper +{ + public static final String DISABLE_ENFORCE_CSP_OPTIONAL_FEATURE = "disableEnforceCsp"; + private final Supplier _connectionSupplier; + + public CspConfigHelper(Supplier connectionSupplier) + { + _connectionSupplier = connectionSupplier; + } + + public CspConfigHelper(Connection connection) + { + this(() -> connection); + } + + public CspConfigHelper(WebDriverWrapper driverWrapper) + { + this(driverWrapper::createDefaultConnection); + } + + private Connection getConnection() + { + return _connectionSupplier.get(); + } + + public void setAllowedHosts(Map> allowedHosts) + { + StringBuilder existingValues = new StringBuilder(); + for (Map.Entry> entry : allowedHosts.entrySet()) + { + Directive directive = entry.getKey(); + for (String value : entry.getValue()) + { + existingValues.append(directive.name()).append("|").append(value).append("\n"); + } + } + + SimplePostCommand postCommand = new SimplePostCommand("admin", "externalSources"); + postCommand.setParameters(Map.of( + "saveAll", true, + "existingValues", existingValues)); + try + { + postCommand.execute(getConnection(), null); + } + catch (IOException | CommandException e) + { + throw new RuntimeException(e); + } + } + + public void clearAllowedHosts() + { + setAllowedHosts(Collections.emptyMap()); + } + + public void setEnforceCsp(boolean enforce) + { + Objects.requireNonNull(OptionalFeatureHelper.setOptionalFeature(getConnection(), DISABLE_ENFORCE_CSP_OPTIONAL_FEATURE, !enforce), () -> "Unable to configure enforce CSP."); + } + + public static void debugCspWarnings() + { + Log4jUtils.setLogLevel("org.labkey.core.admin.AdminController.ContentSecurityPolicyReportAction", ManagerPage.LoggingLevel.DEBUG); + } +} From 4f6502320f7bae168f6778db033a82a2615a8b2e Mon Sep 17 00:00:00 2001 From: labkey-tchad Date: Thu, 5 Jun 2025 12:07:06 -0700 Subject: [PATCH 04/12] Allow object-src 'self' --- src/org/labkey/test/util/CspLogUtil.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/org/labkey/test/util/CspLogUtil.java b/src/org/labkey/test/util/CspLogUtil.java index fc506a04f1..db1df5f8c0 100644 --- a/src/org/labkey/test/util/CspLogUtil.java +++ b/src/org/labkey/test/util/CspLogUtil.java @@ -23,9 +23,11 @@ public class CspLogUtil { private static final List ignoredViolations = List.of( "/_rstudio/", - "/_rstudioReport/", - "/reports-createScriptReport.view?" // Issue 53226: reports-streamFile is blocked by object-src CSP directive + "/_rstudioReport/" ); + // Issue 53226: reports-streamFile is blocked by object-src CSP directive + private static final List ignoredDirectives = List.of("object-src"); + private static final String logName = "csp-report.log"; private static final File logFile = new File(TestFileUtils.getServerLogDir(), logName); From 92ff5090bd8052c9200c2dfbbf4f651bd819675c Mon Sep 17 00:00:00 2001 From: labkey-tchad Date: Thu, 5 Jun 2025 16:51:30 -0700 Subject: [PATCH 05/12] Debug knitr csp --- src/org/labkey/test/TestProperties.java | 2 + .../test/tests/AbstractKnitrReportTest.java | 4 + src/org/labkey/test/util/CspLogUtil.java | 86 ++++++++++++++++--- 3 files changed, 80 insertions(+), 12 deletions(-) diff --git a/src/org/labkey/test/TestProperties.java b/src/org/labkey/test/TestProperties.java index 1598346248..83d56286fc 100644 --- a/src/org/labkey/test/TestProperties.java +++ b/src/org/labkey/test/TestProperties.java @@ -18,6 +18,7 @@ import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.SystemUtils; import org.labkey.serverapi.reader.Readers; +import org.labkey.test.util.CspLogUtil; import org.labkey.test.util.TestLogger; import org.openqa.selenium.Dimension; @@ -79,6 +80,7 @@ public abstract class TestProperties public static void load() { /* Force static block to run */ + CspLogUtil.init(); } public static boolean isTestCleanupSkipped() diff --git a/src/org/labkey/test/tests/AbstractKnitrReportTest.java b/src/org/labkey/test/tests/AbstractKnitrReportTest.java index 3a59ce3e8d..1847d2c649 100644 --- a/src/org/labkey/test/tests/AbstractKnitrReportTest.java +++ b/src/org/labkey/test/tests/AbstractKnitrReportTest.java @@ -22,8 +22,10 @@ import org.labkey.test.TestFileUtils; import org.labkey.test.TestProperties; import org.labkey.test.pages.admin.ExternalSourcesPage; +import org.labkey.test.pages.core.admin.logger.ManagerPage; import org.labkey.test.pages.reports.ManageViewsPage; import org.labkey.test.util.CodeMirrorHelper; +import org.labkey.test.util.Log4jUtils; import org.labkey.test.util.LogMethod; import org.labkey.test.util.LoggedParam; import org.labkey.test.util.PortalHelper; @@ -73,6 +75,8 @@ public static void initProject() @LogMethod protected void setupProject() { + CspConfigHelper.debugCspWarnings(); // TODO: temp debug on Teamcity + Log4jUtils.setLogLevel("org.labkey.query.reports.ReportsController", ManagerPage.LoggingLevel.DEBUG); new CspConfigHelper(this).setAllowedHosts(Map.of( ExternalSourcesPage.Directive.Style, List.of("https://cdn.datatables.net"), ExternalSourcesPage.Directive.Font, List.of("https://mathjax.rstudio.com"))); diff --git a/src/org/labkey/test/util/CspLogUtil.java b/src/org/labkey/test/util/CspLogUtil.java index db1df5f8c0..1e320d1764 100644 --- a/src/org/labkey/test/util/CspLogUtil.java +++ b/src/org/labkey/test/util/CspLogUtil.java @@ -3,6 +3,7 @@ import org.apache.commons.collections4.MultiValuedMap; import org.apache.commons.collections4.multimap.HashSetValuedHashMap; import org.apache.commons.io.IOUtils; +import org.json.JSONObject; import org.junit.Assert; import org.labkey.serverapi.writer.PrintWriters; import org.labkey.test.TestFileUtils; @@ -16,6 +17,7 @@ import java.nio.charset.Charset; import java.nio.file.Files; import java.nio.file.attribute.BasicFileAttributes; +import java.util.ArrayList; import java.util.Collection; import java.util.List; @@ -37,6 +39,25 @@ public class CspLogUtil private CspLogUtil() { } + public static void init() + { + if (lastModified == 0) + { + try + { + BasicFileAttributes logFileAttributes; + logFileAttributes = Files.readAttributes(logFile.toPath(), BasicFileAttributes.class); + + lastSize = logFileAttributes.size(); + lastModified = logFileAttributes.lastModifiedTime().toMillis(); + } + catch (IOException e) + { + lastModified = System.currentTimeMillis(); + } + } + } + public static void checkNewCspWarnings(ArtifactCollector artifactCollector) { if (TestProperties.isServerRemote() || missingLog) @@ -88,22 +109,19 @@ public static void checkNewCspWarnings(ArtifactCollector artifactCollector) boolean foundVioloation = false; MultiValuedMap violoations = new HashSetValuedHashMap<>(); + List cspReports = new ArrayList<>(); + StringBuilder sb = new StringBuilder(); for (String line : warningLines) { - String[] split = line.split("ContentSecurityPolicy warning on page: "); - if (split.length > 1) + if (!sb.isEmpty() || line.equals("{")) { + sb.append(line); + } + if (line.equals("}")) + { + cspReports.add(new CspReport(sb.toString())); + sb = new StringBuilder(); foundVioloation = true; - String url = split[1]; - if (ignoredViolations.stream().anyMatch(url::contains)) - { - TestLogger.warn("Ignoring CSP warning on page: " + url); - } - else - { - Crawler.ControllerActionId actionId = new Crawler.ControllerActionId(url); - violoations.put(actionId, url); - } } } @@ -112,6 +130,21 @@ public static void checkNewCspWarnings(ArtifactCollector artifactCollector) throw new AssertionError("Detected CSP violations but unable to parse log file: " + recentWarningsFile.getAbsolutePath()); } + for (CspReport cspReport : cspReports) + { + String url = cspReport.getDocument(); + String violatedDirective = cspReport.getViolatedDirective(); + if (ignoredViolations.stream().anyMatch(url::contains) || ignoredDirectives.stream().anyMatch(violatedDirective::contains)) + { + TestLogger.warn("Ignoring %s CSP warning on page: %s".formatted(violatedDirective, url)); + } + else + { + Crawler.ControllerActionId actionId = new Crawler.ControllerActionId(url); + violoations.put(actionId, cspReport.toString()); + } + } + if (!violoations.isEmpty()) { StringBuilder errorMessage = new StringBuilder() @@ -168,3 +201,32 @@ public CspWarningDetectedException(Object detailMessage) } } } + +class CspReport +{ + private final String _violatedDirective; + private final String _document; + + CspReport(String reportStr) + { + JSONObject report = new JSONObject(reportStr).getJSONObject("csp-report"); + _violatedDirective = report.getString("violated-directive"); + _document = report.getString("document-uri"); + } + + public String getViolatedDirective() + { + return _violatedDirective; + } + + public String getDocument() + { + return _document; + } + + @Override + public String toString() + { + return getViolatedDirective() + ": " + getDocument(); + } +} From 23a4de29de198f6f7da0de28797c245d3e8fb22e Mon Sep 17 00:00:00 2001 From: labkey-tchad Date: Tue, 10 Jun 2025 10:59:54 -0700 Subject: [PATCH 06/12] Minor cleanup --- src/org/labkey/test/util/data/ColumnNameMapper.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/org/labkey/test/util/data/ColumnNameMapper.java b/src/org/labkey/test/util/data/ColumnNameMapper.java index e8160139e3..5cd85a1b5f 100644 --- a/src/org/labkey/test/util/data/ColumnNameMapper.java +++ b/src/org/labkey/test/util/data/ColumnNameMapper.java @@ -29,9 +29,8 @@ public ColumnNameMapper(Map mappings) public static ColumnNameMapper labelToName(Collection fields) { - return new ColumnNameMapper(label -> fields.stream() - .collect(Collectors.toMap(FieldDefinition::getEffectiveLabel, FieldDefinition::getName)) - .getOrDefault(label, label)); + Map labelToNameMap = fields.stream().collect(Collectors.toMap(FieldDefinition::getEffectiveLabel, FieldDefinition::getName)); + return new ColumnNameMapper(label -> labelToNameMap.getOrDefault(label, label)); } public static ColumnNameMapper nameToLabel(Collection fields) From ec52dd6b5a71b5914e3757c7c081a7939b73f564 Mon Sep 17 00:00:00 2001 From: labkey-tchad Date: Tue, 10 Jun 2025 16:59:44 -0700 Subject: [PATCH 07/12] page updates --- .../labkey/test/components/html/Table.java | 5 + .../test/pages/admin/ExternalSourcesPage.java | 103 ++++++++++++++++-- .../core/admin/CspResourceHostsTest.java | 60 ++++++++++ src/org/labkey/test/util/CspLogUtil.java | 17 +-- 4 files changed, 170 insertions(+), 15 deletions(-) create mode 100644 src/org/labkey/test/tests/core/admin/CspResourceHostsTest.java diff --git a/src/org/labkey/test/components/html/Table.java b/src/org/labkey/test/components/html/Table.java index fca62fc405..6ffc86c94d 100644 --- a/src/org/labkey/test/components/html/Table.java +++ b/src/org/labkey/test/components/html/Table.java @@ -88,6 +88,11 @@ public int getRowCount() return elementCache().getRows().size(); } + public List getRows() + { + return elementCache().getRows(); + } + /** * For a well formed html table get the text from the th elements in the thead. * diff --git a/src/org/labkey/test/pages/admin/ExternalSourcesPage.java b/src/org/labkey/test/pages/admin/ExternalSourcesPage.java index 4bcbf201cb..98f37fc727 100644 --- a/src/org/labkey/test/pages/admin/ExternalSourcesPage.java +++ b/src/org/labkey/test/pages/admin/ExternalSourcesPage.java @@ -1,6 +1,7 @@ package org.labkey.test.pages.admin; import org.labkey.test.Locator; +import org.labkey.test.Locators; import org.labkey.test.WebDriverWrapper; import org.labkey.test.WebTestHelper; import org.labkey.test.components.html.Input; @@ -57,9 +58,48 @@ public ExternalSourcesPage addHost(@LoggedParam Directive directive, @LoggedPara clickAndWait(elementCache().addButton); clearCache(); + assertNoLabKeyErrors(); return this; } + @LogMethod (quiet = true) + public List addHostExpectingError(@LoggedParam Directive directive, @LoggedParam String host) + { + elementCache().directiveSelect.selectOption(directive); + elementCache().hostInput.set(host); + + clickAndWait(elementCache().addButton); + clearCache(); + + return getTexts(Locators.labkeyError.findElements(elementCache())); + } + + public ExternalSourcesPage editHost(int rowIndex, String newHost) + { + elementCache().getExistingSourceRows().get(rowIndex).hostInput.set(newHost); + + return this; + } + + @LogMethod (quiet = true) + public ExternalSourcesPage saveChanges() + { + clickAndWait(elementCache().saveButton); + clearCache(); + + assertNoLabKeyErrors(); + return this; + } + + @LogMethod (quiet = true) + public List saveChangesExpectingError() + { + clickAndWait(elementCache().saveButton); + clearCache(); + + return getTexts(Locators.labkeyError.findElements(elementCache())); + } + public Map> getExistingHosts() { Map> existingHosts = new HashMap<>(); @@ -74,16 +114,12 @@ public Map> getExistingHosts() public Map> getExistingHostInputs() { - List directiveColumn = elementCache().existingValuesTable.getColumnAsElement(1); - List hostsColumn = elementCache().existingValuesTable.getColumnAsElement(2); - Map> existingHosts = new HashMap<>(); - for (int i = 0; i < hostsColumn.size(); i++) + for (ExistingSourceRow row : elementCache().getExistingSourceRows()) { - WebElement directiveInput = Locator.tag("input").findElement(directiveColumn.get(i)); - Directive directive = Directive.valueOf(directiveInput.getDomAttribute("data-directive")); - Input hostInput = Input.Input(Locator.tag("input"), getDriver()).find(hostsColumn.get(i)); + Directive directive = row.getDirective(); + Input hostInput = row.getHostInput(); existingHosts.computeIfAbsent(directive, d -> new ArrayList<>()).add(hostInput); } @@ -105,6 +141,59 @@ protected class ElementCache extends LabKeyPage.ElementCache final WebElement existingValuesForm = Locator.name("existingValues").findWhenNeeded(this); final Table existingValuesTable = new Table(getDriver(), Locator.byClass("labkey-data-region-legacy").findWhenNeeded(existingValuesForm), 0); + private List existingSourceRows; + + protected List getExistingSourceRows() + { + if (existingSourceRows == null) + { + existingSourceRows = existingValuesTable.getRows().stream().map(ExistingSourceRow::new).toList(); + } + return existingSourceRows; + } + + final WebElement saveButton = Locator.lkButton("Save").findWhenNeeded(existingValuesForm); + } + + protected class ExistingSourceRow + { + private final WebElement directiveInput; + private final Input hostInput; + private final WebElement deleteButton; + + private Directive directive; + + ExistingSourceRow(WebElement row) + { + directiveInput = Locator.tag("input").withAttributeContaining("name", "directive").findWhenNeeded(row); + hostInput = Input.Input(Locator.tag("input").withAttributeContaining("name", "host"), getDriver()).findWhenNeeded(row); + deleteButton = Locator.lkButton().findWhenNeeded(row); + } + + Directive getDirective() + { + if (directive == null) + { + directive = Directive.valueOf(directiveInput.getDomAttribute("data-directive")); + } + return directive; + } + + public Input getHostInput() + { + return hostInput; + } + + public String getHost() + { + return hostInput.get(); + } + + public void clickDelete() + { + clickAndWait(deleteButton); + clearCache(); + } } public enum Directive implements OptionSelect.SelectOption diff --git a/src/org/labkey/test/tests/core/admin/CspResourceHostsTest.java b/src/org/labkey/test/tests/core/admin/CspResourceHostsTest.java new file mode 100644 index 0000000000..73ccbdcf20 --- /dev/null +++ b/src/org/labkey/test/tests/core/admin/CspResourceHostsTest.java @@ -0,0 +1,60 @@ +package org.labkey.test.tests.core.admin; + +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.labkey.test.BaseWebDriverTest; +import org.labkey.test.categories.Daily; +import org.labkey.test.util.ApiPermissionsHelper; +import org.labkey.test.util.core.admin.CspConfigHelper; + +import java.util.Arrays; +import java.util.List; + +import static org.junit.Assert.assertTrue; + +@Category({Daily.class}) +public class CspResourceHostsTest extends BaseWebDriverTest +{ + private static final String USER = "csp_app_admin@cspresourcehoststest.test"; + + private final CspConfigHelper _cspConfigHelper = new CspConfigHelper(this); + + @Override + protected void doCleanup(boolean afterTest) + { + _userHelper.deleteUsers(afterTest, USER); + } + + @BeforeClass + public static void setupProject() + { + CspResourceHostsTest init = getCurrentTest(); + + init.doSetup(); + } + + private void doSetup() + { + _userHelper.createUser(USER); + new ApiPermissionsHelper(this).addUserAsAppAdmin(USER); + } + + @Test + public void testSomething() + { + assertTrue("Failing stub test", false); + } + + @Override + protected String getProjectName() + { + return null; + } + + @Override + public List getAssociatedModules() + { + return Arrays.asList(); + } +} diff --git a/src/org/labkey/test/util/CspLogUtil.java b/src/org/labkey/test/util/CspLogUtil.java index 1e320d1764..62cf9d042c 100644 --- a/src/org/labkey/test/util/CspLogUtil.java +++ b/src/org/labkey/test/util/CspLogUtil.java @@ -20,6 +20,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.List; +import java.util.Set; public class CspLogUtil { @@ -28,7 +29,7 @@ public class CspLogUtil "/_rstudioReport/" ); // Issue 53226: reports-streamFile is blocked by object-src CSP directive - private static final List ignoredDirectives = List.of("object-src"); + private static final Set ignoredDirectives = Set.of("object-src"); private static final String logName = "csp-report.log"; private static final File logFile = new File(TestFileUtils.getServerLogDir(), logName); @@ -132,9 +133,9 @@ public static void checkNewCspWarnings(ArtifactCollector artifactCollector) for (CspReport cspReport : cspReports) { - String url = cspReport.getDocument(); + String url = cspReport.getDocumentUri(); String violatedDirective = cspReport.getViolatedDirective(); - if (ignoredViolations.stream().anyMatch(url::contains) || ignoredDirectives.stream().anyMatch(violatedDirective::contains)) + if (ignoredViolations.stream().anyMatch(url::contains) || ignoredDirectives.contains(violatedDirective)) { TestLogger.warn("Ignoring %s CSP warning on page: %s".formatted(violatedDirective, url)); } @@ -205,13 +206,13 @@ public CspWarningDetectedException(Object detailMessage) class CspReport { private final String _violatedDirective; - private final String _document; + private final String _documentUri; CspReport(String reportStr) { JSONObject report = new JSONObject(reportStr).getJSONObject("csp-report"); _violatedDirective = report.getString("violated-directive"); - _document = report.getString("document-uri"); + _documentUri = report.getString("document-uri"); } public String getViolatedDirective() @@ -219,14 +220,14 @@ public String getViolatedDirective() return _violatedDirective; } - public String getDocument() + public String getDocumentUri() { - return _document; + return _documentUri; } @Override public String toString() { - return getViolatedDirective() + ": " + getDocument(); + return getViolatedDirective() + ": " + getDocumentUri(); } } From f1edc98e605071c5d6263a1c8bda4a74fef2aa5a Mon Sep 17 00:00:00 2001 From: labkey-tchad Date: Wed, 11 Jun 2025 20:59:03 -0700 Subject: [PATCH 08/12] CspResourceHostsTest --- .../test/pages/admin/ExternalSourcesPage.java | 59 +++-- .../test/pages/core/admin/ShowAdminPage.java | 15 +- .../test/tests/AbstractKnitrReportTest.java | 4 +- .../core/admin/CspResourceHostsTest.java | 212 +++++++++++++++++- .../test/util/core/admin/CspConfigHelper.java | 13 +- 5 files changed, 259 insertions(+), 44 deletions(-) diff --git a/src/org/labkey/test/pages/admin/ExternalSourcesPage.java b/src/org/labkey/test/pages/admin/ExternalSourcesPage.java index 98f37fc727..acc798b94f 100644 --- a/src/org/labkey/test/pages/admin/ExternalSourcesPage.java +++ b/src/org/labkey/test/pages/admin/ExternalSourcesPage.java @@ -10,18 +10,22 @@ import org.labkey.test.pages.LabKeyPage; import org.labkey.test.util.LogMethod; import org.labkey.test.util.LoggedParam; +import org.openqa.selenium.NotFoundException; import org.openqa.selenium.WebDriver; import org.openqa.selenium.WebElement; import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; import java.util.stream.Stream; /** - * Wraps `AdminController.ExternalSourceAction` + * Wraps `AdminController.ExternalSourcesAction` */ public class ExternalSourcesPage extends LabKeyPage { @@ -39,7 +43,7 @@ public static ExternalSourcesPage beginAt(WebDriverWrapper webDriverWrapper) @LogMethod public ExternalSourcesPage ensureHost(Directive directive, String host) { - if (!getExistingHosts().getOrDefault(directive, Collections.emptyList()).contains(host)) + if (!getExistingHosts().getOrDefault(directive, Collections.emptySet()).contains(host)) { return addHost(directive, host); } @@ -74,13 +78,25 @@ public List addHostExpectingError(@LoggedParam Directive directive, @Log return getTexts(Locators.labkeyError.findElements(elementCache())); } - public ExternalSourcesPage editHost(int rowIndex, String newHost) + public ExternalSourcesPage editExistingSource(Directive directive, String host, String newHost) { - elementCache().getExistingSourceRows().get(rowIndex).hostInput.set(newHost); + getExistingSourceRow(directive, host).getHostInput().set(newHost); + return this; + } + public ExternalSourcesPage deleteExistingSource(Directive directive, String host, String newHost) + { + getExistingSourceRow(directive, host).clickDelete(); return this; } + private ExistingSourceRow getExistingSourceRow(Directive directive, String host) + { + return elementCache().getExistingSourceRows().stream() + .filter(row -> row.getDirective().equals(directive) && row.getHost().equalsIgnoreCase(host)) + .findFirst().orElseThrow(() -> new NotFoundException("Host " + host + " does not exist for directive " + directive)); + } + @LogMethod (quiet = true) public ExternalSourcesPage saveChanges() { @@ -100,27 +116,15 @@ public List saveChangesExpectingError() return getTexts(Locators.labkeyError.findElements(elementCache())); } - public Map> getExistingHosts() - { - Map> existingHosts = new HashMap<>(); - - for (Map.Entry> entry : getExistingHostInputs().entrySet()) - { - existingHosts.put(entry.getKey(), entry.getValue().stream().map(Input::getValue).toList()); - } - - return existingHosts; - } - - public Map> getExistingHostInputs() + public Map> getExistingHosts() { - Map> existingHosts = new HashMap<>(); + Map> existingHosts = new HashMap<>(); for (ExistingSourceRow row : elementCache().getExistingSourceRows()) { Directive directive = row.getDirective(); - Input hostInput = row.getHostInput(); - existingHosts.computeIfAbsent(directive, d -> new ArrayList<>()).add(hostInput); + String hostInput = row.getHost(); + existingHosts.computeIfAbsent(directive, d -> new HashSet<>()).add(hostInput); } return existingHosts; @@ -147,7 +151,14 @@ protected List getExistingSourceRows() { if (existingSourceRows == null) { - existingSourceRows = existingValuesTable.getRows().stream().map(ExistingSourceRow::new).toList(); + List rows = existingValuesTable.getRows(); + List temp = new ArrayList<>(); + for (int i = 0; i < rows.size(); i++) + { + WebElement row = rows.get(i); + temp.add(new ExistingSourceRow(row, i)); + } + existingSourceRows = Collections.unmodifiableList(temp); } return existingSourceRows; } @@ -160,14 +171,16 @@ protected class ExistingSourceRow private final WebElement directiveInput; private final Input hostInput; private final WebElement deleteButton; + private final int rowIndex; private Directive directive; - ExistingSourceRow(WebElement row) + ExistingSourceRow(WebElement row, int rowIndex) { directiveInput = Locator.tag("input").withAttributeContaining("name", "directive").findWhenNeeded(row); hostInput = Input.Input(Locator.tag("input").withAttributeContaining("name", "host"), getDriver()).findWhenNeeded(row); - deleteButton = Locator.lkButton().findWhenNeeded(row); + deleteButton = Locator.lkButton("Delete").findWhenNeeded(row); + this.rowIndex = rowIndex; } Directive getDirective() diff --git a/src/org/labkey/test/pages/core/admin/ShowAdminPage.java b/src/org/labkey/test/pages/core/admin/ShowAdminPage.java index 7417290227..16a591b25d 100644 --- a/src/org/labkey/test/pages/core/admin/ShowAdminPage.java +++ b/src/org/labkey/test/pages/core/admin/ShowAdminPage.java @@ -21,6 +21,7 @@ import org.labkey.test.components.DomainDesignerPage; import org.labkey.test.pages.ConfigureReportsAndScriptsPage; import org.labkey.test.pages.LabKeyPage; +import org.labkey.test.pages.admin.ExternalSourcesPage; import org.labkey.test.pages.compliance.ComplianceSettingsAccountsPage; import org.labkey.test.pages.core.login.LoginConfigurePage; import org.labkey.test.util.OptionalFeatureHelper; @@ -99,6 +100,13 @@ public ShowAuditLogPage clickAuditLog() return new ShowAuditLogPage(getDriver()); } + public ExternalSourcesPage clickAllowedExternalResourceHosts() + { + goToSettingsSection(); + clickAndWait(elementCache().externalResourceHostsLink); + return new ExternalSourcesPage(getDriver()); + } + public AllowedFileExtensionAdminPage clickAllowedFileExtensions() { goToSettingsSection(); @@ -278,7 +286,7 @@ protected ElementCache newElementCache() return new ElementCache(); } - protected class ElementCache extends LabKeyPage.ElementCache + protected class ElementCache extends LabKeyPage.ElementCache { protected WebElement sectionServerInfo = Locator.linkWithText("Server Information").findWhenNeeded(this); protected WebElement sectionSettingsLinks = Locator.linkWithText("Settings").findWhenNeeded(this); @@ -286,8 +294,9 @@ protected class ElementCache extends LabKeyPage.ElementCache protected WebElement sectionActiveUsers = Locator.linkWithText("Active Users").findWhenNeeded(this); protected WebElement analyticsSettingsLink = Locator.linkWithText("analytics settings").findWhenNeeded(this); - protected WebElement externalRedirectHostLink = Locator.linkWithText("allowed external redirect hosts").findElement(this); - protected WebElement allowedFileExtensionLink = Locator.linkWithText("allowed file extensions").findElement(this); + protected WebElement externalRedirectHostLink = Locator.linkWithText("allowed external redirect hosts").findWhenNeeded(this); + protected WebElement externalResourceHostsLink = Locator.linkWithText("allowed external resource hosts").findWhenNeeded(this); + protected WebElement allowedFileExtensionLink = Locator.linkWithText("allowed file extensions").findWhenNeeded(this); protected WebElement auditLogLink = Locator.linkWithText("audit log").findWhenNeeded(this); protected WebElement auditLogMaintenanceLink = Locator.linkWithText("Audit Log Maintenance").findWhenNeeded(this); protected WebElement authenticationLink = Locator.linkWithText("authentication").findWhenNeeded(this); diff --git a/src/org/labkey/test/tests/AbstractKnitrReportTest.java b/src/org/labkey/test/tests/AbstractKnitrReportTest.java index 1847d2c649..4a21d8247a 100644 --- a/src/org/labkey/test/tests/AbstractKnitrReportTest.java +++ b/src/org/labkey/test/tests/AbstractKnitrReportTest.java @@ -66,14 +66,14 @@ private static String readReport(final Path reportFile) } @BeforeClass - public static void initProject() + public static void initProject() throws Exception { AbstractKnitrReportTest init = getCurrentTest(); init.setupProject(); } @LogMethod - protected void setupProject() + protected void setupProject() throws Exception { CspConfigHelper.debugCspWarnings(); // TODO: temp debug on Teamcity Log4jUtils.setLogLevel("org.labkey.query.reports.ReportsController", ManagerPage.LoggingLevel.DEBUG); diff --git a/src/org/labkey/test/tests/core/admin/CspResourceHostsTest.java b/src/org/labkey/test/tests/core/admin/CspResourceHostsTest.java index 73ccbdcf20..290ed344be 100644 --- a/src/org/labkey/test/tests/core/admin/CspResourceHostsTest.java +++ b/src/org/labkey/test/tests/core/admin/CspResourceHostsTest.java @@ -1,29 +1,51 @@ package org.labkey.test.tests.core.admin; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.apache.hc.core5.http.HttpStatus; +import org.assertj.core.api.Assertions; +import org.junit.Assert; +import org.junit.Before; import org.junit.BeforeClass; import org.junit.Test; import org.junit.experimental.categories.Category; +import org.labkey.remoteapi.CommandException; import org.labkey.test.BaseWebDriverTest; +import org.labkey.test.Locator; import org.labkey.test.categories.Daily; +import org.labkey.test.components.html.SelectWrapper; +import org.labkey.test.pages.admin.ExternalSourcesPage; +import org.labkey.test.pages.admin.ExternalSourcesPage.Directive; +import org.labkey.test.pages.core.admin.ShowAdminPage; import org.labkey.test.util.ApiPermissionsHelper; import org.labkey.test.util.core.admin.CspConfigHelper; +import java.io.IOException; import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; +import static org.labkey.test.util.PermissionsHelper.MemberType.user; @Category({Daily.class}) public class CspResourceHostsTest extends BaseWebDriverTest { - private static final String USER = "csp_app_admin@cspresourcehoststest.test"; + private static final String APP_ADMIN = "csp_app_admin@cspresourcehoststest.test"; + private static final String TROUBLESHOOTER = "csp_troubleshooter@cspresourcehoststest.test"; + private static final Log log = LogFactory.getLog(CspResourceHostsTest.class); private final CspConfigHelper _cspConfigHelper = new CspConfigHelper(this); @Override protected void doCleanup(boolean afterTest) { - _userHelper.deleteUsers(afterTest, USER); + _userHelper.deleteUsers(afterTest, APP_ADMIN, TROUBLESHOOTER); } @BeforeClass @@ -36,14 +58,192 @@ public static void setupProject() private void doSetup() { - _userHelper.createUser(USER); - new ApiPermissionsHelper(this).addUserAsAppAdmin(USER); + _userHelper.createUser(APP_ADMIN); + _userHelper.createUser(TROUBLESHOOTER); + ApiPermissionsHelper apiPermissionsHelper = new ApiPermissionsHelper(this); + apiPermissionsHelper.addUserAsAppAdmin(APP_ADMIN); + apiPermissionsHelper.addMemberToRole(TROUBLESHOOTER, "Troubleshooter", user, null); + } + + @Before + public void preTest() throws Exception + { + _cspConfigHelper.clearAllowedHosts(); + } + + @Test + public void testTroubleshooterPermissions() throws Exception + { + Directive directive = Directive.Connection; + String host = "https://labkey.org"; + Locator addButton = Locator.lkButton("Add"); + Locator saveButton = Locator.lkButton("Save"); + Locator deleteButton = Locator.lkButton("Delete"); + Locator doneButton = Locator.lkButton("Done"); + + ExternalSourcesPage externalSourcesPage = ExternalSourcesPage.beginAt(this); + externalSourcesPage.addHost(directive, host); + assertElementPresent(addButton); + assertElementPresent(saveButton); + assertElementPresent(deleteButton); + assertElementNotPresent(doneButton); + assertEquals("Defined directives", Map.of(directive, Set.of(host)), externalSourcesPage.getExistingHosts()); + + impersonate(TROUBLESHOOTER); + externalSourcesPage = ShowAdminPage.beginAt(this).clickAllowedExternalResourceHosts(); + assertElementNotPresent(addButton); + assertElementNotPresent(saveButton); + assertElementNotPresent(deleteButton); + assertElementPresent(doneButton); + assertEquals("Defined directives", Map.of(directive, Set.of(host)), externalSourcesPage.getExistingHosts()); + + clickAndWait(doneButton); + assertEquals("Done button destination", "/admin-showAdmin.view", getCurrentRelativeURL()); + + try + { + _cspConfigHelper.clearAllowedHosts(); + Assert.fail("Troubleshooter should not be able to change allowed hosts"); + } + catch (CommandException e) + { + if (e.getStatusCode() != HttpStatus.SC_FORBIDDEN) + throw e; + } + } + + @Test + public void testAddDuplicateHostsErrors() + { + String host1 = "https://labkey.org"; + Map> expectedDirectives = new HashMap<>(); + + impersonate(APP_ADMIN); + ExternalSourcesPage externalSourcesPage = ExternalSourcesPage.beginAt(this); + + log("Verify that multiple directives can have the same host"); + for (Directive directive : Directive.values()) + { + expectedDirectives.put(directive, Set.of(host1)); + externalSourcesPage.addHost(directive, " " + host1 + " "); + } + + log("Verify that each directive can't have duplicate entries that differ only by casing"); + for (Directive directive : Directive.values()) + { + String host1UpperCase = host1.toUpperCase(); + List errors = externalSourcesPage.addHostExpectingError(directive, " " + host1UpperCase + " "); + Assertions.assertThat(errors).as("error count").hasSize(1); + Assertions.assertThat(errors.get(0)).contains("Duplicate values are not allowed.", directive.name(), host1UpperCase); + } + + assertEquals("Defined directives", expectedDirectives, externalSourcesPage.getExistingHosts()); + } + + @Test + public void testEditDuplicateHostsErrors() throws Exception + { + String host1 = "https://labkey.org"; + String host2 = "https://labkey.com"; + Map> expectedDirectives = new HashMap<>(); + + impersonate(APP_ADMIN); + ExternalSourcesPage externalSourcesPage = ExternalSourcesPage.beginAt(this); + + log("Setup directives with multiple hosts"); + for (Directive directive : Directive.values()) + { + expectedDirectives.put(directive, Set.of(host1, host2)); + externalSourcesPage.addHost(directive, " " + host1 + " "); + externalSourcesPage.addHost(directive, " " + host2 + " "); + } + + log("Verify that directive can't be edited to have duplicate entries"); + for (Directive directive : Directive.values()) + { + String host1UpperCase = host1.toUpperCase(); + externalSourcesPage.editExistingSource(directive, host2, " " + host1UpperCase + " "); + List errors = externalSourcesPage.saveChangesExpectingError(); + Assertions.assertThat(errors).as("error count").hasSize(1); + Assertions.assertThat(errors.get(0)).contains("Duplicate values are not allowed.", directive.name(), host1); + } + + assertEquals("Defined directives", expectedDirectives, externalSourcesPage.getExistingHosts()); + } + + @Test + public void testAddInvalidHostsErrors() throws Exception + { + String host1 = "https://labkey.org"; + + impersonate(APP_ADMIN); + ExternalSourcesPage externalSourcesPage = ExternalSourcesPage.beginAt(this); + + log("Verify that each directive can't add invalid hosts"); + for (Directive directive : Directive.values()) + { + String badHost1 = host1 + ";"; + List errors = externalSourcesPage.addHostExpectingError(directive, badHost1); + Assertions.assertThat(errors).as("error").containsExactly("Semicolons are not allowed in host names"); + } + + log("Verify that each directive can't add blank hosts"); + for (Directive directive : Directive.values()) + { + List errors = externalSourcesPage.addHostExpectingError(directive, ""); + Assertions.assertThat(errors).as("error").containsExactly("Host must not be blank"); + } + + assertEquals("Defined directives", Collections.emptyMap(), externalSourcesPage.getExistingHosts()); } @Test - public void testSomething() + public void testEditInvalidHostsErrors() throws Exception { - assertTrue("Failing stub test", false); + String host1 = "https://labkey.org"; + Map> expectedDirectives = new HashMap<>(); + + impersonate(APP_ADMIN); + ExternalSourcesPage externalSourcesPage = ExternalSourcesPage.beginAt(this); + + log("Setup directives with multiple hosts"); + for (Directive directive : Directive.values()) + { + expectedDirectives.put(directive, Set.of(host1)); + externalSourcesPage.addHost(directive, " " + host1 + " "); + } + + log("Verify that directive can't be edited to have semicolon"); + for (Directive directive : Directive.values()) + { + String host1UpperCase = host1.toUpperCase(); + externalSourcesPage.editExistingSource(directive, host1, host1UpperCase + " ;"); + List errors = externalSourcesPage.saveChangesExpectingError(); + Assertions.assertThat(errors).as("error").containsExactly("Semicolons are not allowed in host names"); + } + + log("Verify that directive can't be edited to be blank"); + for (Directive directive : Directive.values()) + { + externalSourcesPage.editExistingSource(directive, host1, ""); + List errors = externalSourcesPage.saveChangesExpectingError(); + Assertions.assertThat(errors).as("error").containsExactly("Host must not be blank"); + } + + assertEquals("Defined directives", expectedDirectives, externalSourcesPage.getExistingHosts()); + } + + @Test + public void testAvailableDirectives() + { + List expectedDirectives = Arrays.stream(Directive.values()).map(Directive::name).toList(); + + ExternalSourcesPage.beginAt(this); + List availableDirectives = SelectWrapper.Select(Locator.id("newDirective")).find(getDriver()) + .getOptions().stream().map(el -> el.getDomProperty("value")).toList(); + + Assertions.assertThat(availableDirectives).as("Available directives") + .containsExactlyInAnyOrderElementsOf(expectedDirectives); } @Override diff --git a/src/org/labkey/test/util/core/admin/CspConfigHelper.java b/src/org/labkey/test/util/core/admin/CspConfigHelper.java index e458c97e41..27e135c706 100644 --- a/src/org/labkey/test/util/core/admin/CspConfigHelper.java +++ b/src/org/labkey/test/util/core/admin/CspConfigHelper.java @@ -41,7 +41,7 @@ private Connection getConnection() return _connectionSupplier.get(); } - public void setAllowedHosts(Map> allowedHosts) + public void setAllowedHosts(Map> allowedHosts) throws IOException, CommandException { StringBuilder existingValues = new StringBuilder(); for (Map.Entry> entry : allowedHosts.entrySet()) @@ -57,17 +57,10 @@ public void setAllowedHosts(Map> allowedHosts) postCommand.setParameters(Map.of( "saveAll", true, "existingValues", existingValues)); - try - { - postCommand.execute(getConnection(), null); - } - catch (IOException | CommandException e) - { - throw new RuntimeException(e); - } + postCommand.execute(getConnection(), null); } - public void clearAllowedHosts() + public void clearAllowedHosts() throws IOException, CommandException { setAllowedHosts(Collections.emptyMap()); } From 918c82c4dd08f3753680c0d8fdcaba4a70239e3d Mon Sep 17 00:00:00 2001 From: labkey-tchad Date: Thu, 12 Jun 2025 12:10:14 -0700 Subject: [PATCH 09/12] delete host test --- .../test/pages/admin/ExternalSourcesPage.java | 43 +++--- .../core/admin/CspResourceHostsTest.java | 123 ++++++++++++------ .../test/util/core/admin/CspConfigHelper.java | 38 ++++++ 3 files changed, 138 insertions(+), 66 deletions(-) diff --git a/src/org/labkey/test/pages/admin/ExternalSourcesPage.java b/src/org/labkey/test/pages/admin/ExternalSourcesPage.java index acc798b94f..0b6eb917df 100644 --- a/src/org/labkey/test/pages/admin/ExternalSourcesPage.java +++ b/src/org/labkey/test/pages/admin/ExternalSourcesPage.java @@ -1,5 +1,6 @@ package org.labkey.test.pages.admin; +import org.junit.Assert; import org.labkey.test.Locator; import org.labkey.test.Locators; import org.labkey.test.WebDriverWrapper; @@ -10,18 +11,13 @@ import org.labkey.test.pages.LabKeyPage; import org.labkey.test.util.LogMethod; import org.labkey.test.util.LoggedParam; +import org.labkey.test.util.core.admin.CspConfigHelper; import org.openqa.selenium.NotFoundException; import org.openqa.selenium.WebDriver; import org.openqa.selenium.WebElement; import java.util.ArrayList; -import java.util.Collections; -import java.util.HashMap; -import java.util.HashSet; import java.util.List; -import java.util.Map; -import java.util.Set; -import java.util.stream.Collectors; import java.util.stream.Stream; /** @@ -43,7 +39,7 @@ public static ExternalSourcesPage beginAt(WebDriverWrapper webDriverWrapper) @LogMethod public ExternalSourcesPage ensureHost(Directive directive, String host) { - if (!getExistingHosts().getOrDefault(directive, Collections.emptySet()).contains(host)) + if (!getExistingHosts().contains(new CspConfigHelper.AllowedHost(directive, host))) { return addHost(directive, host); } @@ -75,16 +71,18 @@ public List addHostExpectingError(@LoggedParam Directive directive, @Log clickAndWait(elementCache().addButton); clearCache(); - return getTexts(Locators.labkeyError.findElements(elementCache())); + List errorEls = Locators.labkeyError.findElements(elementCache()); + Assert.assertFalse("No errors found", errorEls.isEmpty()); + return getTexts(errorEls); } - public ExternalSourcesPage editExistingSource(Directive directive, String host, String newHost) + public ExternalSourcesPage editHost(Directive directive, String host, String newHost) { getExistingSourceRow(directive, host).getHostInput().set(newHost); return this; } - public ExternalSourcesPage deleteExistingSource(Directive directive, String host, String newHost) + public ExternalSourcesPage deleteHost(Directive directive, String host) { getExistingSourceRow(directive, host).clickDelete(); return this; @@ -113,18 +111,20 @@ public List saveChangesExpectingError() clickAndWait(elementCache().saveButton); clearCache(); - return getTexts(Locators.labkeyError.findElements(elementCache())); + List errorEls = Locators.labkeyError.findElements(elementCache()); + Assert.assertFalse("No errors found", errorEls.isEmpty()); + return getTexts(errorEls); } - public Map> getExistingHosts() + public List getExistingHosts() { - Map> existingHosts = new HashMap<>(); + List existingHosts = new ArrayList<>(); for (ExistingSourceRow row : elementCache().getExistingSourceRows()) { Directive directive = row.getDirective(); - String hostInput = row.getHost(); - existingHosts.computeIfAbsent(directive, d -> new HashSet<>()).add(hostInput); + String host = row.getHost(); + existingHosts.add(new CspConfigHelper.AllowedHost(directive, host)); } return existingHosts; @@ -151,14 +151,7 @@ protected List getExistingSourceRows() { if (existingSourceRows == null) { - List rows = existingValuesTable.getRows(); - List temp = new ArrayList<>(); - for (int i = 0; i < rows.size(); i++) - { - WebElement row = rows.get(i); - temp.add(new ExistingSourceRow(row, i)); - } - existingSourceRows = Collections.unmodifiableList(temp); + existingSourceRows = existingValuesTable.getRows().stream().map(ExistingSourceRow::new).toList(); } return existingSourceRows; } @@ -171,16 +164,14 @@ protected class ExistingSourceRow private final WebElement directiveInput; private final Input hostInput; private final WebElement deleteButton; - private final int rowIndex; private Directive directive; - ExistingSourceRow(WebElement row, int rowIndex) + ExistingSourceRow(WebElement row) { directiveInput = Locator.tag("input").withAttributeContaining("name", "directive").findWhenNeeded(row); hostInput = Input.Input(Locator.tag("input").withAttributeContaining("name", "host"), getDriver()).findWhenNeeded(row); deleteButton = Locator.lkButton("Delete").findWhenNeeded(row); - this.rowIndex = rowIndex; } Directive getDirective() diff --git a/src/org/labkey/test/tests/core/admin/CspResourceHostsTest.java b/src/org/labkey/test/tests/core/admin/CspResourceHostsTest.java index 290ed344be..bb0c5df520 100644 --- a/src/org/labkey/test/tests/core/admin/CspResourceHostsTest.java +++ b/src/org/labkey/test/tests/core/admin/CspResourceHostsTest.java @@ -1,9 +1,6 @@ package org.labkey.test.tests.core.admin; -import org.apache.commons.logging.Log; -import org.apache.commons.logging.LogFactory; import org.apache.hc.core5.http.HttpStatus; -import org.assertj.core.api.Assertions; import org.junit.Assert; import org.junit.Before; import org.junit.BeforeClass; @@ -16,21 +13,19 @@ import org.labkey.test.components.html.SelectWrapper; import org.labkey.test.pages.admin.ExternalSourcesPage; import org.labkey.test.pages.admin.ExternalSourcesPage.Directive; +import org.labkey.test.util.core.admin.CspConfigHelper.AllowedHost; import org.labkey.test.pages.core.admin.ShowAdminPage; import org.labkey.test.util.ApiPermissionsHelper; import org.labkey.test.util.core.admin.CspConfigHelper; -import java.io.IOException; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; -import java.util.HashMap; +import java.util.Iterator; import java.util.List; -import java.util.Map; -import java.util.Set; -import java.util.stream.Collectors; +import static org.assertj.core.api.Assertions.assertThat; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; import static org.labkey.test.util.PermissionsHelper.MemberType.user; @Category({Daily.class}) @@ -38,7 +33,6 @@ public class CspResourceHostsTest extends BaseWebDriverTest { private static final String APP_ADMIN = "csp_app_admin@cspresourcehoststest.test"; private static final String TROUBLESHOOTER = "csp_troubleshooter@cspresourcehoststest.test"; - private static final Log log = LogFactory.getLog(CspResourceHostsTest.class); private final CspConfigHelper _cspConfigHelper = new CspConfigHelper(this); @@ -83,11 +77,13 @@ public void testTroubleshooterPermissions() throws Exception ExternalSourcesPage externalSourcesPage = ExternalSourcesPage.beginAt(this); externalSourcesPage.addHost(directive, host); + assertElementPresent(addButton); assertElementPresent(saveButton); assertElementPresent(deleteButton); assertElementNotPresent(doneButton); - assertEquals("Defined directives", Map.of(directive, Set.of(host)), externalSourcesPage.getExistingHosts()); + List expected = List.of(new AllowedHost(directive, host)); + assertEquals("Defined directives", expected, externalSourcesPage.getExistingHosts()); impersonate(TROUBLESHOOTER); externalSourcesPage = ShowAdminPage.beginAt(this).clickAllowedExternalResourceHosts(); @@ -95,7 +91,7 @@ public void testTroubleshooterPermissions() throws Exception assertElementNotPresent(saveButton); assertElementNotPresent(deleteButton); assertElementPresent(doneButton); - assertEquals("Defined directives", Map.of(directive, Set.of(host)), externalSourcesPage.getExistingHosts()); + assertEquals("Defined directives", expected, externalSourcesPage.getExistingHosts()); clickAndWait(doneButton); assertEquals("Done button destination", "/admin-showAdmin.view", getCurrentRelativeURL()); @@ -116,7 +112,7 @@ public void testTroubleshooterPermissions() throws Exception public void testAddDuplicateHostsErrors() { String host1 = "https://labkey.org"; - Map> expectedDirectives = new HashMap<>(); + List expectedDirectives = new ArrayList<>(); impersonate(APP_ADMIN); ExternalSourcesPage externalSourcesPage = ExternalSourcesPage.beginAt(this); @@ -124,7 +120,7 @@ public void testAddDuplicateHostsErrors() log("Verify that multiple directives can have the same host"); for (Directive directive : Directive.values()) { - expectedDirectives.put(directive, Set.of(host1)); + expectedDirectives.add(new AllowedHost(directive, host1)); externalSourcesPage.addHost(directive, " " + host1 + " "); } @@ -133,19 +129,20 @@ public void testAddDuplicateHostsErrors() { String host1UpperCase = host1.toUpperCase(); List errors = externalSourcesPage.addHostExpectingError(directive, " " + host1UpperCase + " "); - Assertions.assertThat(errors).as("error count").hasSize(1); - Assertions.assertThat(errors.get(0)).contains("Duplicate values are not allowed.", directive.name(), host1UpperCase); + assertThat(errors).as("error count").hasSize(1); + assertThat(errors.get(0)).contains("Duplicate values are not allowed.", directive.name(), host1UpperCase); } - assertEquals("Defined directives", expectedDirectives, externalSourcesPage.getExistingHosts()); + assertThat(externalSourcesPage.getExistingHosts()).as("Defined directives") + .containsExactlyInAnyOrderElementsOf(expectedDirectives); } @Test - public void testEditDuplicateHostsErrors() throws Exception + public void testEditDuplicateHostsErrors() { String host1 = "https://labkey.org"; String host2 = "https://labkey.com"; - Map> expectedDirectives = new HashMap<>(); + List expectedDirectives = new ArrayList<>(); impersonate(APP_ADMIN); ExternalSourcesPage externalSourcesPage = ExternalSourcesPage.beginAt(this); @@ -153,7 +150,8 @@ public void testEditDuplicateHostsErrors() throws Exception log("Setup directives with multiple hosts"); for (Directive directive : Directive.values()) { - expectedDirectives.put(directive, Set.of(host1, host2)); + expectedDirectives.add(new AllowedHost(directive, host1)); + expectedDirectives.add(new AllowedHost(directive, host2)); externalSourcesPage.addHost(directive, " " + host1 + " "); externalSourcesPage.addHost(directive, " " + host2 + " "); } @@ -162,17 +160,18 @@ public void testEditDuplicateHostsErrors() throws Exception for (Directive directive : Directive.values()) { String host1UpperCase = host1.toUpperCase(); - externalSourcesPage.editExistingSource(directive, host2, " " + host1UpperCase + " "); + externalSourcesPage.editHost(directive, host2, " " + host1UpperCase + " "); List errors = externalSourcesPage.saveChangesExpectingError(); - Assertions.assertThat(errors).as("error count").hasSize(1); - Assertions.assertThat(errors.get(0)).contains("Duplicate values are not allowed.", directive.name(), host1); + assertThat(errors).as("error count").hasSize(1); + assertThat(errors.get(0)).contains("Duplicate values are not allowed.", directive.name(), host1); } - assertEquals("Defined directives", expectedDirectives, externalSourcesPage.getExistingHosts()); + assertThat(externalSourcesPage.getExistingHosts()).as("Defined directives") + .containsExactlyInAnyOrderElementsOf(expectedDirectives); } @Test - public void testAddInvalidHostsErrors() throws Exception + public void testAddInvalidHostsErrors() { String host1 = "https://labkey.org"; @@ -184,24 +183,24 @@ public void testAddInvalidHostsErrors() throws Exception { String badHost1 = host1 + ";"; List errors = externalSourcesPage.addHostExpectingError(directive, badHost1); - Assertions.assertThat(errors).as("error").containsExactly("Semicolons are not allowed in host names"); + assertThat(errors).as("error").containsExactly("Semicolons are not allowed in host names"); } log("Verify that each directive can't add blank hosts"); for (Directive directive : Directive.values()) { List errors = externalSourcesPage.addHostExpectingError(directive, ""); - Assertions.assertThat(errors).as("error").containsExactly("Host must not be blank"); + assertThat(errors).as("error").containsExactly("Host must not be blank"); } - assertEquals("Defined directives", Collections.emptyMap(), externalSourcesPage.getExistingHosts()); + assertEquals("Defined directives", Collections.emptyList(), externalSourcesPage.getExistingHosts()); } @Test - public void testEditInvalidHostsErrors() throws Exception + public void testEditInvalidHostsErrors() { - String host1 = "https://labkey.org"; - Map> expectedDirectives = new HashMap<>(); + String host = "https://labkey.org"; + List expectedDirectives = new ArrayList<>(); impersonate(APP_ADMIN); ExternalSourcesPage externalSourcesPage = ExternalSourcesPage.beginAt(this); @@ -209,28 +208,72 @@ public void testEditInvalidHostsErrors() throws Exception log("Setup directives with multiple hosts"); for (Directive directive : Directive.values()) { - expectedDirectives.put(directive, Set.of(host1)); - externalSourcesPage.addHost(directive, " " + host1 + " "); + expectedDirectives.add(new AllowedHost(directive, host)); + externalSourcesPage.addHost(directive, " " + host + " "); } log("Verify that directive can't be edited to have semicolon"); for (Directive directive : Directive.values()) { - String host1UpperCase = host1.toUpperCase(); - externalSourcesPage.editExistingSource(directive, host1, host1UpperCase + " ;"); + String host1UpperCase = host.toUpperCase(); + externalSourcesPage.editHost(directive, host, host1UpperCase + " ;"); List errors = externalSourcesPage.saveChangesExpectingError(); - Assertions.assertThat(errors).as("error").containsExactly("Semicolons are not allowed in host names"); + assertThat(errors).as("error").containsExactly("Semicolons are not allowed in host names"); } log("Verify that directive can't be edited to be blank"); for (Directive directive : Directive.values()) { - externalSourcesPage.editExistingSource(directive, host1, ""); + externalSourcesPage.editHost(directive, host, ""); List errors = externalSourcesPage.saveChangesExpectingError(); - Assertions.assertThat(errors).as("error").containsExactly("Host must not be blank"); + assertThat(errors).as("error").containsExactly("Host must not be blank"); + } + + assertThat(externalSourcesPage.getExistingHosts()).as("Defined directives") + .containsExactlyInAnyOrderElementsOf(expectedDirectives); + } + + @Test + public void testDeleteAvailableHosts() + { + String host1 = "https://labkey.org"; + String host2 = "https://labkey.com"; + List expectedDirectives = new ArrayList<>(); + + impersonate(APP_ADMIN); + ExternalSourcesPage externalSourcesPage = ExternalSourcesPage.beginAt(this); + + log("Setup directives with multiple hosts in a random order"); + for (Directive directive : Directive.values()) + { + expectedDirectives.add(new AllowedHost(directive, host1)); + expectedDirectives.add(new AllowedHost(directive, host2)); + } + Collections.shuffle(expectedDirectives); + for (AllowedHost allowedHost : expectedDirectives) + { + externalSourcesPage.addHost(allowedHost.getDirective(), allowedHost.getHost()); + } + + assertThat(externalSourcesPage.getExistingHosts()).as("Defined directives") + .containsExactlyInAnyOrderElementsOf(expectedDirectives); + + log("Delete directives in a random order"); + Collections.shuffle(expectedDirectives); + Iterator iterator = expectedDirectives.iterator(); + while (iterator.hasNext()) + { + AllowedHost allowedHost = iterator.next(); + iterator.remove(); + externalSourcesPage.deleteHost(allowedHost.getDirective(), allowedHost.getHost()); + + assertThat(externalSourcesPage.getExistingHosts()).as("Defined directives") + .containsExactlyInAnyOrderElementsOf(expectedDirectives); } - assertEquals("Defined directives", expectedDirectives, externalSourcesPage.getExistingHosts()); + assertThat(externalSourcesPage.getExistingHosts()) + .as("Defined directives after deleting all") + .isEmpty(); } @Test @@ -242,7 +285,7 @@ public void testAvailableDirectives() List availableDirectives = SelectWrapper.Select(Locator.id("newDirective")).find(getDriver()) .getOptions().stream().map(el -> el.getDomProperty("value")).toList(); - Assertions.assertThat(availableDirectives).as("Available directives") + assertThat(availableDirectives).as("Available directives") .containsExactlyInAnyOrderElementsOf(expectedDirectives); } diff --git a/src/org/labkey/test/util/core/admin/CspConfigHelper.java b/src/org/labkey/test/util/core/admin/CspConfigHelper.java index 27e135c706..a8ec0125ed 100644 --- a/src/org/labkey/test/util/core/admin/CspConfigHelper.java +++ b/src/org/labkey/test/util/core/admin/CspConfigHelper.java @@ -74,4 +74,42 @@ public static void debugCspWarnings() { Log4jUtils.setLogLevel("org.labkey.core.admin.AdminController.ContentSecurityPolicyReportAction", ManagerPage.LoggingLevel.DEBUG); } + + public static class AllowedHost + { + private final Directive _directive; + private final String _host; + + public AllowedHost(Directive directive, String host) + { + _directive = Objects.requireNonNull(directive); + _host = Objects.requireNonNull(host); + } + + public Directive getDirective() + { + return _directive; + } + + public String getHost() + { + return _host; + } + + @Override + public final boolean equals(Object o) + { + if (!(o instanceof AllowedHost that)) return false; + + return _directive == that._directive && _host.equals(that._host); + } + + @Override + public int hashCode() + { + int result = _directive.hashCode(); + result = 31 * result + _host.hashCode(); + return result; + } + } } From 086c1f35bf5225554ef56acea037637bcf8ea1e8 Mon Sep 17 00:00:00 2001 From: labkey-tchad Date: Thu, 12 Jun 2025 12:23:12 -0700 Subject: [PATCH 10/12] Clean up imports --- .../labkey/test/pages/admin/ExternalSourcesPage.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/org/labkey/test/pages/admin/ExternalSourcesPage.java b/src/org/labkey/test/pages/admin/ExternalSourcesPage.java index 0b6eb917df..25d1da4db3 100644 --- a/src/org/labkey/test/pages/admin/ExternalSourcesPage.java +++ b/src/org/labkey/test/pages/admin/ExternalSourcesPage.java @@ -11,7 +11,7 @@ import org.labkey.test.pages.LabKeyPage; import org.labkey.test.util.LogMethod; import org.labkey.test.util.LoggedParam; -import org.labkey.test.util.core.admin.CspConfigHelper; +import org.labkey.test.util.core.admin.CspConfigHelper.AllowedHost; import org.openqa.selenium.NotFoundException; import org.openqa.selenium.WebDriver; import org.openqa.selenium.WebElement; @@ -39,7 +39,7 @@ public static ExternalSourcesPage beginAt(WebDriverWrapper webDriverWrapper) @LogMethod public ExternalSourcesPage ensureHost(Directive directive, String host) { - if (!getExistingHosts().contains(new CspConfigHelper.AllowedHost(directive, host))) + if (!getExistingHosts().contains(new AllowedHost(directive, host))) { return addHost(directive, host); } @@ -116,15 +116,15 @@ public List saveChangesExpectingError() return getTexts(errorEls); } - public List getExistingHosts() + public List getExistingHosts() { - List existingHosts = new ArrayList<>(); + List existingHosts = new ArrayList<>(); for (ExistingSourceRow row : elementCache().getExistingSourceRows()) { Directive directive = row.getDirective(); String host = row.getHost(); - existingHosts.add(new CspConfigHelper.AllowedHost(directive, host)); + existingHosts.add(new AllowedHost(directive, host)); } return existingHosts; From de5455244bb29748dd4261b175cde19a315a29e0 Mon Sep 17 00:00:00 2001 From: labkey-tchad Date: Thu, 12 Jun 2025 12:42:40 -0700 Subject: [PATCH 11/12] Remove temp debug --- .../test/tests/AbstractKnitrReportTest.java | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/src/org/labkey/test/tests/AbstractKnitrReportTest.java b/src/org/labkey/test/tests/AbstractKnitrReportTest.java index 4a21d8247a..d40ece66e7 100644 --- a/src/org/labkey/test/tests/AbstractKnitrReportTest.java +++ b/src/org/labkey/test/tests/AbstractKnitrReportTest.java @@ -17,6 +17,7 @@ import org.junit.Assume; import org.junit.BeforeClass; +import org.labkey.remoteapi.CommandException; import org.labkey.test.BaseWebDriverTest; import org.labkey.test.Locator; import org.labkey.test.TestFileUtils; @@ -34,6 +35,7 @@ import org.openqa.selenium.WebElement; import java.io.File; +import java.io.IOException; import java.nio.file.Path; import java.util.Arrays; import java.util.List; @@ -73,13 +75,19 @@ public static void initProject() throws Exception } @LogMethod - protected void setupProject() throws Exception + protected void setupProject() { - CspConfigHelper.debugCspWarnings(); // TODO: temp debug on Teamcity Log4jUtils.setLogLevel("org.labkey.query.reports.ReportsController", ManagerPage.LoggingLevel.DEBUG); - new CspConfigHelper(this).setAllowedHosts(Map.of( - ExternalSourcesPage.Directive.Style, List.of("https://cdn.datatables.net"), - ExternalSourcesPage.Directive.Font, List.of("https://mathjax.rstudio.com"))); + try + { + new CspConfigHelper(this).setAllowedHosts(Map.of( + ExternalSourcesPage.Directive.Style, List.of("https://cdn.datatables.net"), + ExternalSourcesPage.Directive.Font, List.of("https://mathjax.rstudio.com"))); + } + catch (IOException | CommandException e) + { + throw new RuntimeException(e); + } _rReportHelper.ensureRConfig(isDocker()); From cc6bc75e087955f87ab2f943e816a189882c6784 Mon Sep 17 00:00:00 2001 From: labkey-tchad Date: Tue, 17 Jun 2025 11:34:25 -0700 Subject: [PATCH 12/12] Group related validation with checker --- .../core/admin/CspResourceHostsTest.java | 33 +++++++++---------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/src/org/labkey/test/tests/core/admin/CspResourceHostsTest.java b/src/org/labkey/test/tests/core/admin/CspResourceHostsTest.java index bb0c5df520..7b00b7fac4 100644 --- a/src/org/labkey/test/tests/core/admin/CspResourceHostsTest.java +++ b/src/org/labkey/test/tests/core/admin/CspResourceHostsTest.java @@ -13,10 +13,10 @@ import org.labkey.test.components.html.SelectWrapper; import org.labkey.test.pages.admin.ExternalSourcesPage; import org.labkey.test.pages.admin.ExternalSourcesPage.Directive; -import org.labkey.test.util.core.admin.CspConfigHelper.AllowedHost; import org.labkey.test.pages.core.admin.ShowAdminPage; import org.labkey.test.util.ApiPermissionsHelper; import org.labkey.test.util.core.admin.CspConfigHelper; +import org.labkey.test.util.core.admin.CspConfigHelper.AllowedHost; import java.util.ArrayList; import java.util.Arrays; @@ -70,30 +70,29 @@ public void testTroubleshooterPermissions() throws Exception { Directive directive = Directive.Connection; String host = "https://labkey.org"; - Locator addButton = Locator.lkButton("Add"); - Locator saveButton = Locator.lkButton("Save"); - Locator deleteButton = Locator.lkButton("Delete"); - Locator doneButton = Locator.lkButton("Done"); + String add = "Add"; + String save = "Save"; + String delete = "Delete"; + String done = "Done"; ExternalSourcesPage externalSourcesPage = ExternalSourcesPage.beginAt(this); externalSourcesPage.addHost(directive, host); + List expectedHosts = List.of(new AllowedHost(directive, host)); - assertElementPresent(addButton); - assertElementPresent(saveButton); - assertElementPresent(deleteButton); - assertElementNotPresent(doneButton); - List expected = List.of(new AllowedHost(directive, host)); - assertEquals("Defined directives", expected, externalSourcesPage.getExistingHosts()); + List buttons = getTexts(Locator.lkButton().findElements(getDriver())); + checker().verifyEqualsSorted("Form buttons", List.of(add, save, delete), buttons); + checker().verifyEquals("Defined directives", expectedHosts, externalSourcesPage.getExistingHosts()); + checker().screenShotIfNewError("site_admin_csp"); impersonate(TROUBLESHOOTER); externalSourcesPage = ShowAdminPage.beginAt(this).clickAllowedExternalResourceHosts(); - assertElementNotPresent(addButton); - assertElementNotPresent(saveButton); - assertElementNotPresent(deleteButton); - assertElementPresent(doneButton); - assertEquals("Defined directives", expected, externalSourcesPage.getExistingHosts()); - clickAndWait(doneButton); + buttons = getTexts(Locator.lkButton().findElements(getDriver())); + checker().verifyEqualsSorted("Form buttons", List.of(done), buttons); + checker().verifyEquals("Defined directives", expectedHosts, externalSourcesPage.getExistingHosts()); + checker().screenShotIfNewError("troubleshooter_csp"); + + clickAndWait(Locator.lkButton(done)); assertEquals("Done button destination", "/admin-showAdmin.view", getCurrentRelativeURL()); try