From 00b67016b606c36399314883a5bff53e0164b1f6 Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Sat, 11 Jul 2026 15:57:02 -0700 Subject: [PATCH 1/3] Added a Targeted MS Guest Access site-admin setting to turn off guest access for targetedms actions targeted by bots. * New admin-console page (GuestAccessSettingsAction) with a master switch, off by default, plus per-action checkboxes that require a login for guests on the targetedms pages commonly targeted by anonymous bots, so guest users can be sent to login during a traffic spike. * Gated showProtein, showPeptide, showMolecule, showCalibrationCurve (on by default), showPrecursorList, and four chart-image endpoints such as precursorChromatogramChart (off by default). * Stored on the root container and audited on change, link shown only when panoramapublic module is enabled. * Added a new TargetedMSGuestAccessTest. Co-Authored-By: Claude --- .../labkey/targetedms/GuestAccessManager.java | 183 ++++++++++++++ .../targetedms/TargetedMSController.java | 193 +++++++++++++++ .../labkey/targetedms/TargetedMSModule.java | 9 + .../targetedms/view/guestAccessSettings.jsp | 83 +++++++ .../targetedms/TargetedMSGuestAccessTest.java | 227 ++++++++++++++++++ 5 files changed, 695 insertions(+) create mode 100644 src/org/labkey/targetedms/GuestAccessManager.java create mode 100644 src/org/labkey/targetedms/view/guestAccessSettings.jsp create mode 100644 test/src/org/labkey/test/tests/targetedms/TargetedMSGuestAccessTest.java diff --git a/src/org/labkey/targetedms/GuestAccessManager.java b/src/org/labkey/targetedms/GuestAccessManager.java new file mode 100644 index 000000000..0c43aeb7d --- /dev/null +++ b/src/org/labkey/targetedms/GuestAccessManager.java @@ -0,0 +1,183 @@ +/* + * Copyright (c) 2026 LabKey Corporation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.labkey.targetedms; + +import org.jetbrains.annotations.NotNull; +import org.labkey.api.audit.AuditLogService; +import org.labkey.api.audit.provider.SiteSettingsAuditProvider; +import org.labkey.api.data.Container; +import org.labkey.api.data.ContainerManager; +import org.labkey.api.data.PropertyManager; +import org.labkey.api.data.PropertyManager.PropertyMap; +import org.labkey.api.data.PropertyManager.WritablePropertyMap; +import org.labkey.api.security.User; + +import java.util.ArrayList; +import java.util.EnumSet; +import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; + +/** + * Site-admin setting that can require a login for slow targetedms pages that get hit by bots. A master + * switch (off by default) plus one flag per action are stored as site properties on the root container. + * When the master switch is on, each flagged action sends a guest to the login page instead of rendering. + * When it is off, guests can open the pages as before. + * + * This setting does not change actions that are already require a login: + * PrecursorAllChromatogramsChartAction, MoleculePrecursorAllChromatogramsChartAction, ShowTransitionListAction + */ +public class GuestAccessManager +{ + // Site properties live in this category on the root container. + private static final String CATEGORY = "TargetedMSGuestAccess"; + private static final String MASTER_KEY = "masterEnabled"; + private static final String TRUE = Boolean.TRUE.toString(); + + /** + * The actions a site admin can choose to require a login. + * defaultChecked is used when nothing has been saved yet. + * + * The pages that draw many charts are checked by default. The single-chart image actions and the + * precursor table are offered too but are off by default, so guests keep seeing inline charts and lists + * during normal operation. An admin can also check those to block direct requests to them during an + * bot attack (they show up in high volume because the detail pages embed many of them). + */ + public enum RestrictableAction + { + showProtein("Protein details page (showProtein)", true), + showPeptide("Peptide details page (showPeptide)", true), + showMolecule("Small molecule details page (showMolecule)", true), + showCalibrationCurve("Calibration curves page (showCalibrationCurve)", true), + showPrecursorList("Precursor list page (showPrecursorList)", false), + showPeakAreas("Peak areas chart image (showPeakAreas)", false), + showRetentionTimesChart("Retention times chart image (showRetentionTimesChart)", false), + precursorChromatogramChart("Precursor chromatogram image (precursorChromatogramChart)", false), + groupChromatogramChart("Protein chromatogram image (groupChromatogramChart)", false); + + private final String _label; + private final boolean _defaultChecked; + + RestrictableAction(String label, boolean defaultChecked) + { + _label = label; + _defaultChecked = defaultChecked; + } + + public String getLabel() + { + return _label; + } + + public boolean isDefaultChecked() + { + return _defaultChecked; + } + } + + private GuestAccessManager() + { + } + + private static PropertyMap getProperties() + { + return PropertyManager.getProperties(ContainerManager.getRoot(), CATEGORY); + } + + /** True when the master switch is on. */ + public static boolean isMasterEnabled() + { + return TRUE.equals(getProperties().get(MASTER_KEY)); + } + + /** + * The saved (or default) checkbox state for an action, independent of the master toggle. Used to render + * the settings page. + */ + public static boolean isActionChecked(@NotNull RestrictableAction action) + { + String saved = getProperties().get(action.name()); + return saved == null ? action.isDefaultChecked() : TRUE.equals(saved); + } + + /** + * True when a guest opening this page should be sent to the login page: the master switch is on AND + * this page's checkbox is checked. This is the single method the actions consult. + */ + public static boolean isRestricted(@NotNull RestrictableAction action) + { + PropertyMap props = getProperties(); + if (!TRUE.equals(props.get(MASTER_KEY))) + return false; + String saved = props.get(action.name()); + return saved == null ? action.isDefaultChecked() : TRUE.equals(saved); + } + + /** The set of currently-checked actions (independent of the master toggle). */ + public static Set getCheckedActions() + { + Set checked = EnumSet.noneOf(RestrictableAction.class); + for (RestrictableAction action : RestrictableAction.values()) + { + if (isActionChecked(action)) + checked.add(action); + } + return checked; + } + + /** + * Persist the settings on the root container and, if anything changed, write a site-settings audit entry + * recording who changed it and the new state. + */ + public static void save(@NotNull User user, boolean masterEnabled, @NotNull Set checkedActions) + { + boolean oldMaster = isMasterEnabled(); + Set oldChecked = getCheckedActions(); + + // When the master switch is off, the per-action checkboxes are disabled on the settings page and + // do not post, so preserve the previously saved per-action selections rather than overwriting them. + Set newChecked = masterEnabled ? checkedActions : oldChecked; + + WritablePropertyMap props = PropertyManager.getWritableProperties(ContainerManager.getRoot(), CATEGORY, true); + props.put(MASTER_KEY, Boolean.toString(masterEnabled)); + for (RestrictableAction action : RestrictableAction.values()) + props.put(action.name(), Boolean.toString(newChecked.contains(action))); + props.save(); + + if (masterEnabled != oldMaster || !newChecked.equals(oldChecked)) + { + String comment = "Targeted MS Guest Access settings updated. Master switch: " + (masterEnabled ? "on" : "off") + + ". Actions requiring login for guests: " + describe(newChecked) + "."; + SiteSettingsAuditProvider.SiteSettingsAuditEvent event = + new SiteSettingsAuditProvider.SiteSettingsAuditEvent(ContainerManager.getRoot(), comment); + AuditLogService.get().addEvent(user, event); + } + } + + private static String describe(Set actions) + { + if (actions.isEmpty()) + return "(none)"; + // Report in enum declaration order for a stable, readable list. + List names = new ArrayList<>(); + for (RestrictableAction action : RestrictableAction.values()) + { + if (actions.contains(action)) + names.add(action.name()); + } + return names.stream().collect(Collectors.joining(", ")); + } +} diff --git a/src/org/labkey/targetedms/TargetedMSController.java b/src/org/labkey/targetedms/TargetedMSController.java index 1fbda6177..512657360 100644 --- a/src/org/labkey/targetedms/TargetedMSController.java +++ b/src/org/labkey/targetedms/TargetedMSController.java @@ -306,6 +306,7 @@ import java.util.Collections; import java.util.Comparator; import java.util.Date; +import java.util.EnumSet; import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashMap; @@ -589,6 +590,141 @@ public void addNavTrail(NavTree root) } } + // ------------------------------------------------------------------------ + // Site-admin settings page: require a login for guests on targetedms actions targeted by bots. + // ------------------------------------------------------------------------ + public static class GuestAccessSettingsForm + { + private boolean _masterEnabled; + private String[] _restrictedActions = new String[0]; + + public boolean isMasterEnabled() + { + return _masterEnabled; + } + + public void setMasterEnabled(boolean masterEnabled) + { + _masterEnabled = masterEnabled; + } + + public String[] getRestrictedActions() + { + return _restrictedActions; + } + + public void setRestrictedActions(String[] restrictedActions) + { + _restrictedActions = restrictedActions; + } + + /** The checked actions as an enum set, ignoring any unrecognized keys. */ + public Set getCheckedActions() + { + Set checked = EnumSet.noneOf(GuestAccessManager.RestrictableAction.class); + for (String name : _restrictedActions) + { + try + { + checked.add(GuestAccessManager.RestrictableAction.valueOf(name)); + } + catch (IllegalArgumentException ignored) + { + // Skip anything that is not a current action key + } + } + return checked; + } + } + + /** One row on the settings page: an action, its label, and whether its checkbox is currently checked. */ + public static class GuestAccessActionState + { + private final GuestAccessManager.RestrictableAction _action; + private final boolean _checked; + + public GuestAccessActionState(GuestAccessManager.RestrictableAction action, boolean checked) + { + _action = action; + _checked = checked; + } + + public String getName() + { + return _action.name(); + } + + public String getLabel() + { + return _action.getLabel(); + } + + public boolean isChecked() + { + return _checked; + } + } + + /** Model for the settings JSP: the master switch state plus one row per restrictable action. */ + public static class GuestAccessSettingsBean + { + private final boolean _masterEnabled; + private final List _actions; + + public GuestAccessSettingsBean() + { + _masterEnabled = GuestAccessManager.isMasterEnabled(); + _actions = new ArrayList<>(); + for (GuestAccessManager.RestrictableAction action : GuestAccessManager.RestrictableAction.values()) + _actions.add(new GuestAccessActionState(action, GuestAccessManager.isActionChecked(action))); + } + + public boolean isMasterEnabled() + { + return _masterEnabled; + } + + public List getActions() + { + return _actions; + } + } + + @RequiresPermission(ApplicationAdminPermission.class) + public class GuestAccessSettingsAction extends FormViewAction + { + @Override + public void validateCommand(GuestAccessSettingsForm target, Errors errors) + { + } + + @Override + public ModelAndView getView(GuestAccessSettingsForm form, boolean reshow, BindException errors) + { + return new JspView<>("/org/labkey/targetedms/view/guestAccessSettings.jsp", new GuestAccessSettingsBean(), errors); + } + + @Override + public boolean handlePost(GuestAccessSettingsForm form, BindException errors) + { + GuestAccessManager.save(getUser(), form.isMasterEnabled(), form.getCheckedActions()); + return true; + } + + @Override + public URLHelper getSuccessURL(GuestAccessSettingsForm form) + { + // Reshow the settings page with the saved values. + return new ActionURL(GuestAccessSettingsAction.class, getContainer()); + } + + @Override + public void addNavTrail(NavTree root) + { + urlProvider(AdminUrls.class).addAdminNavTrail(root, "Targeted MS Guest Access", getClass(), getContainer()); + } + } + // ------------------------------------------------------------------------ // Action to create a Raw Data tab // ------------------------------------------------------------------------ @@ -1971,6 +2107,8 @@ public class GroupChromatogramChartAction extends ExportAction @Override public ModelAndView getView(ChromatogramForm form, BindException errors) { + HtmlView loginGate = getGuestLoginGate(GuestAccessManager.RestrictableAction.showPeptide, getViewContext(), getContainer()); + if (loginGate != null) + return loginGate; + long peptideId = form.getId(); // peptide Id Peptide peptide = PeptideManager.getPeptide(getContainer(), peptideId); @@ -3123,6 +3267,10 @@ public static class ShowMoleculeAction extends SimpleViewAction { @@ -4529,6 +4704,13 @@ public ShowPrecursorListAction(ViewContext ctx) setViewContext(ctx); } + @Override + public ModelAndView getHtmlView(final RunDetailsForm form, BindException errors) throws Exception + { + HtmlView loginGate = getGuestLoginGate(GuestAccessManager.RestrictableAction.showPrecursorList, getViewContext(), getContainer()); + return loginGate != null ? loginGate : super.getHtmlView(form, errors); + } + @Override protected DocumentPrecursorsView createQueryView(RunDetailsForm form, BindException errors, boolean forExport, String dataRegion) { @@ -4703,6 +4885,13 @@ protected GroupComparisonView createQueryView( @RequiresPermission(ReadPermission.class) public class ShowCalibrationCurvesAction extends ShowRunSplitDetailsAction { + @Override + public ModelAndView getHtmlView(final RunDetailsForm form, BindException errors) throws Exception + { + HtmlView loginGate = getGuestLoginGate(GuestAccessManager.RestrictableAction.showCalibrationCurve, getViewContext(), getContainer()); + return loginGate != null ? loginGate : super.getHtmlView(form, errors); + } + @Override public String getDataRegionNamePeptide() { @@ -5508,6 +5697,10 @@ public class ShowProteinAction extends SimpleViewAction @Override public ModelAndView getView(final ProteinForm form, BindException errors) { + HtmlView loginGate = getGuestLoginGate(GuestAccessManager.RestrictableAction.showProtein, getViewContext(), getContainer()); + if (loginGate != null) + return loginGate; + PeptideGroup group = PeptideGroupManager.getPeptideGroup(getContainer(), form.getId()); if (group == null) { diff --git a/src/org/labkey/targetedms/TargetedMSModule.java b/src/org/labkey/targetedms/TargetedMSModule.java index 573a74790..c532fed13 100644 --- a/src/org/labkey/targetedms/TargetedMSModule.java +++ b/src/org/labkey/targetedms/TargetedMSModule.java @@ -37,6 +37,7 @@ import org.labkey.api.module.Module; import org.labkey.api.module.ModuleContext; import org.labkey.api.module.ModuleHtmlView; +import org.labkey.api.module.ModuleLoader; import org.labkey.api.module.ModuleProperty; import org.labkey.api.module.SpringModule; import org.labkey.api.pipeline.PipelineService; @@ -654,6 +655,14 @@ protected void startupAfterSpringConfig(ModuleContext moduleContext) ActionURL chromatogramURL = new ActionURL(TargetedMSController.ChromatogramCrawlerAction.class, ContainerManager.getRoot()); AdminConsole.addLink(AdminConsole.SettingsLinkType.Premium, "Targeted MS Chromatogram Crawler", chromatogramURL, ApplicationAdminPermission.class); + // Only offer the guest-access setting on servers where the PanoramaPublic module is active (e.g. PanoramaWeb), + // Use the "panoramapublic" string (not the module class) to avoid a dependency on that module. + if (ModuleLoader.getInstance().hasModule("panoramapublic")) + { + ActionURL guestAccessURL = new ActionURL(TargetedMSController.GuestAccessSettingsAction.class, ContainerManager.getRoot()); + AdminConsole.addLink(AdminConsole.SettingsLinkType.Premium, "Targeted MS Guest Access", guestAccessURL, ApplicationAdminPermission.class); + } + FileContentService fcs = FileContentService.get(); if(null != fcs) { diff --git a/src/org/labkey/targetedms/view/guestAccessSettings.jsp b/src/org/labkey/targetedms/view/guestAccessSettings.jsp new file mode 100644 index 000000000..c5f3b1fc2 --- /dev/null +++ b/src/org/labkey/targetedms/view/guestAccessSettings.jsp @@ -0,0 +1,83 @@ +<% +/* + * Copyright (c) 2026 LabKey Corporation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +%> +<%@ page import="org.labkey.api.view.HttpView" %> +<%@ page import="org.labkey.targetedms.TargetedMSController" %> +<%@ page import="org.labkey.targetedms.TargetedMSController.GuestAccessActionState" %> +<%@ page import="org.labkey.targetedms.TargetedMSController.GuestAccessSettingsBean" %> +<%@ taglib prefix="labkey" uri="http://www.labkey.org/taglib" %> +<%@ page extends="org.labkey.api.jsp.JspBase" %> +<% + GuestAccessSettingsBean bean = (GuestAccessSettingsBean) HttpView.currentView().getModelBean(); +%> +
+

+ Use this to require a login on some Panorama pages when a lot of bot traffic is hitting public + data. When the master switch below is on, each checked page requires a logged-in user, and a + guest is sent to the login page. When the master switch is off (the default), all pages + work normally and stay open to guests on public data. Leave this off during normal operation. +

+

+ This does not change the pages that always require a login for guests. +

+ + +
+ +
+ + + <% for (GuestAccessActionState state : bean.getActions()) { %> + + + + <% } %> +
+ +
+ +
+ <%=button("Save").submit(true)%> + +
+ + diff --git a/test/src/org/labkey/test/tests/targetedms/TargetedMSGuestAccessTest.java b/test/src/org/labkey/test/tests/targetedms/TargetedMSGuestAccessTest.java new file mode 100644 index 000000000..8d4330dd2 --- /dev/null +++ b/test/src/org/labkey/test/tests/targetedms/TargetedMSGuestAccessTest.java @@ -0,0 +1,227 @@ +/* + * Copyright (c) 2026 LabKey Corporation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.labkey.test.tests.targetedms; + +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.labkey.test.BaseWebDriverTest; +import org.labkey.test.Locator; +import org.labkey.test.TestTimeoutException; +import org.labkey.test.WebTestHelper; +import org.labkey.test.util.ApiPermissionsHelper; +import org.labkey.test.util.LogMethod; +import org.openqa.selenium.WebElement; + +import java.util.Map; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.labkey.test.util.PermissionsHelper.READER_ROLE; + +/** + * Tests the site-admin "Targeted MS Guest Access" setting (GuestAccessSettingsAction). + * + * With the master switch off (the default), guests can view the slow pages on public data as before. + * With the master switch on, each checked action requires a login. Unchecking an action lets guests use + * it again. + * + * Two kinds of action are covered, and they signal a blocked guest differently: + * - Detail pages (showProtein, showPeptide) show an inline login view at the same URL, whose text is + * "Login to view this data". + * - Chart-image endpoints (precursorChromatogramChart) cannot return that HTML, so they redirect a + * blocked guest to the standard login page. This is tested with a dummy id, because the gate runs + * before the id is looked up: a blocked guest lands on login.view, while an allowed guest stays on the + * chart URL (a "not found" page for the dummy id). + * + * This does not change the pages that always require a login for guests (e.g. the precursor + * "all chromatograms" page), which stay blocked no matter what the switch is set to. + */ +@Category({}) +@BaseWebDriverTest.ClassTimeout(minutes = 3) +public class TargetedMSGuestAccessTest extends TargetedMSTest +{ + private static final String SKY_FILE = "MRMer.zip"; + private static final String TARGET_PROTEIN = "YAL038W"; + private static final String TARGET_PEPTIDE = "LTSLNVVAGSDLR"; + private static final String LOGIN_MESSAGE = "Login to view this data"; + + // Action keys as stored/posted by the settings page (must match the GuestAccessManager enum names). + private static final String SHOW_PROTEIN = "showProtein"; + private static final String SHOW_PEPTIDE = "showPeptide"; + private static final String PRECURSOR_CHROMATOGRAM_CHART = "precursorChromatogramChart"; + + private static final Locator MASTER_SWITCH = Locator.id("tms-require-login-master"); + + // Captured during setup and read from the @Test method, which runs on a different instance than + // @BeforeClass setup, so these must be static (instance fields would be null in the test method). + private static String proteinUrl; // showProtein detail page + private static String peptideUrl; // showPeptide detail page + private static String chartUrl; // precursorChromatogramChart image endpoint (dummy id) + private static String requiresLoginUrl; // PrecursorAllChromatogramsChartAction, always blocked for guests + + @Override + protected String getProjectName() + { + return getClass().getSimpleName() + " Project"; + } + + @BeforeClass + public static void initProject() + { + TargetedMSGuestAccessTest init = getCurrentTest(); + init.doSetup(); + } + + @LogMethod + private void doSetup() + { + setupFolder(FolderType.Experiment); + importData(SKY_FILE); + + // Capture the URLs we will later request as a guest. + goToDashboard(); + clickAndWait(Locator.linkWithText(SKY_FILE)); + clickAndWait(Locator.linkWithText(TARGET_PROTEIN)); + proteinUrl = getCurrentRelativeURL(); + + // A chart-image endpoint in this same folder. A dummy id is enough because the login gate runs + // before the id is looked up. + chartUrl = WebTestHelper.buildURL("targetedms", getCurrentContainerPath(), PRECURSOR_CHROMATOGRAM_CHART, + Map.of("id", "1")); + + clickAndWait(Locator.linkWithText(TARGET_PEPTIDE)); + peptideUrl = getCurrentRelativeURL(); + + // Peptide -> precursor "all chromatograms" page, which always requires a login for guests. + clickAndWait(Locator.linkWithImage("TransitionGroupLib.png")); + requiresLoginUrl = getCurrentRelativeURL(); + + // Make the data public so a guest has read access. The guest-access gate is what we are testing, + // not the underlying read permission. + goToProjectHome(getProjectName()); + new ApiPermissionsHelper(this).setSiteGroupPermissions("Guests", READER_ROLE); + } + + @Override + protected void doCleanup(boolean afterTest) throws TestTimeoutException + { + // Leave the site-wide switch off so it can't leak into other tests' guest access. The framework + // already signs back in as the site admin before calling doCleanup, so we can just save. Best + // effort: don't let a failure here block the project deletion in super.doCleanup. + try + { + saveGuestAccessSettings(false); + } + catch (Exception ignored) + { + } + super.doCleanup(afterTest); + } + + @Test + public void testGuestAccessToggle() + { + // 1. Master OFF (default): guests can view the pages and the chart; the always-on page is blocked. + signOut(); + assertGuestAllowedPage(proteinUrl, TARGET_PROTEIN); + assertGuestAllowedPage(peptideUrl, TARGET_PEPTIDE); + assertGuestChartAllowed(chartUrl); + assertGuestPageBlocked(requiresLoginUrl); + + // 2. Master ON, gate the two pages and the chart endpoint: guests are sent to login on all three. + signIn(); + saveGuestAccessSettings(true, SHOW_PROTEIN, SHOW_PEPTIDE, PRECURSOR_CHROMATOGRAM_CHART); + signOut(); + assertGuestPageBlocked(proteinUrl); + assertGuestPageBlocked(peptideUrl); + assertGuestChartBlocked(chartUrl); + assertGuestPageBlocked(requiresLoginUrl); + + // 3. Gate only showPeptide: showProtein and the chart open back up, showPeptide stays blocked. + signIn(); + saveGuestAccessSettings(true, SHOW_PEPTIDE); + signOut(); + assertGuestAllowedPage(proteinUrl, TARGET_PROTEIN); + assertGuestChartAllowed(chartUrl); + assertGuestPageBlocked(peptideUrl); + assertGuestPageBlocked(requiresLoginUrl); // always-on page stays blocked regardless of the switch + + // 4. Master OFF again: back to fully open. + signIn(); + saveGuestAccessSettings(false); + signOut(); + assertGuestAllowedPage(proteinUrl, TARGET_PROTEIN); + assertGuestChartAllowed(chartUrl); + } + + /** + * Set the master switch and, when on, gate exactly the given action keys (any others are unchecked). + * Must be signed in as a site admin. The per-action checkboxes are disabled by the page's script until + * the master switch is on, so set the master switch first. + */ + @LogMethod + private void saveGuestAccessSettings(boolean masterEnabled, String... checkedActionKeys) + { + beginAt(WebTestHelper.buildURL("targetedms", "/", "guestAccessSettings")); + setCheckbox(MASTER_SWITCH, masterEnabled); + if (masterEnabled) + { + // Start from a known state: uncheck every action, then check just the requested ones. + for (WebElement box : Locator.tagWithClass("input", "tms-require-login-action").findElements(getDriver())) + { + if (box.isSelected()) + box.click(); + } + for (String key : checkedActionKeys) + setCheckbox(Locator.checkboxByNameAndValue("restrictedActions", key), true); + } + clickButton("Save"); + } + + /** A detail page a guest can view: no login view, and the expected content actually rendered. */ + private void assertGuestAllowedPage(String url, String expectedContent) + { + beginAt(url); + // The login view renders "Login" as a link, so the phrase is split by a tag in the HTML source. + // Check the rendered body text (as TargetedMSExperimentTest.verifyNoGuestAccessMessage does). + assertFalse("Guest was unexpectedly shown the login view at " + url, getBodyText().contains(LOGIN_MESSAGE)); + assertTextPresent(expectedContent); + } + + /** A detail page blocked for a guest: the inline login view is shown at the same URL. */ + private void assertGuestPageBlocked(String url) + { + beginAt(url); + assertTrue("Expected the guest login view at " + url, getBodyText().contains(LOGIN_MESSAGE)); + } + + /** A chart-image endpoint a guest can reach: not redirected to the login page. */ + private void assertGuestChartAllowed(String url) + { + beginAt(url); + assertFalse("Guest was unexpectedly redirected to login for " + url, + getCurrentRelativeURL().contains("login.view")); + } + + /** A chart-image endpoint blocked for a guest: redirected to the standard login page. */ + private void assertGuestChartBlocked(String url) + { + beginAt(url); + assertTrue("Expected a redirect to the login page for " + url + ", but landed on " + getCurrentRelativeURL(), + getCurrentRelativeURL().contains("login.view")); + } +} From 4fe53ce1fa1d6c963f6cb42c9eb8c9a7e2da1b73 Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Sat, 11 Jul 2026 20:46:52 -0700 Subject: [PATCH 2/3] Addressed self-review and Copilot findings on Guest Access setting * Gated showPrecursorList and showCalibrationCurves in getView so the login requirement covers their export/print URLs, not just the HTML page * Renamed the showCalibrationCurve key to showCalibrationCurves to match the endpoint, and null-guarded the settings form binding to avoid a NPE * Set a page title on the guest login-gate path for showPeptide/showMolecule (addNavTrail crumb was inside the _run guard), which the test framework requires * Reported the enforced action set (not the saved checkboxes) in the audit entry when the master switch is off * Extended the test: export-URL gate coverage, a real chart rendered from a live precursorChromInfoId, and snapshot/restore of the site-wide settings in setup/cleanup; fixed a Javadoc typo Co-Authored-By: Claude --- .../labkey/targetedms/GuestAccessManager.java | 24 ++-- .../targetedms/TargetedMSController.java | 22 ++-- .../targetedms/TargetedMSGuestAccessTest.java | 108 +++++++++++++++--- 3 files changed, 121 insertions(+), 33 deletions(-) diff --git a/src/org/labkey/targetedms/GuestAccessManager.java b/src/org/labkey/targetedms/GuestAccessManager.java index 0c43aeb7d..feeca3c0a 100644 --- a/src/org/labkey/targetedms/GuestAccessManager.java +++ b/src/org/labkey/targetedms/GuestAccessManager.java @@ -37,7 +37,7 @@ * When the master switch is on, each flagged action sends a guest to the login page instead of rendering. * When it is off, guests can open the pages as before. * - * This setting does not change actions that are already require a login: + * This setting does not change actions that already require a login: * PrecursorAllChromatogramsChartAction, MoleculePrecursorAllChromatogramsChartAction, ShowTransitionListAction */ public class GuestAccessManager @@ -61,12 +61,12 @@ public enum RestrictableAction showProtein("Protein details page (showProtein)", true), showPeptide("Peptide details page (showPeptide)", true), showMolecule("Small molecule details page (showMolecule)", true), - showCalibrationCurve("Calibration curves page (showCalibrationCurve)", true), - showPrecursorList("Precursor list page (showPrecursorList)", false), - showPeakAreas("Peak areas chart image (showPeakAreas)", false), - showRetentionTimesChart("Retention times chart image (showRetentionTimesChart)", false), - precursorChromatogramChart("Precursor chromatogram image (precursorChromatogramChart)", false), - groupChromatogramChart("Protein chromatogram image (groupChromatogramChart)", false); + showCalibrationCurves("Calibration curves page (showCalibrationCurves)", true), + showPrecursorList("Document details page (showPrecursorList)", false), + showPeakAreas("Peak areas chart (showPeakAreas)", false), + showRetentionTimesChart("Retention times chart (showRetentionTimesChart)", false), + precursorChromatogramChart("Precursor chromatogram (precursorChromatogramChart)", false), + groupChromatogramChart("Protein chromatogram (groupChromatogramChart)", false); private final String _label; private final boolean _defaultChecked; @@ -129,10 +129,13 @@ public static boolean isRestricted(@NotNull RestrictableAction action) /** The set of currently-checked actions (independent of the master toggle). */ public static Set getCheckedActions() { + // Read the property map once and reuse it rather than re-fetching per action. + PropertyMap props = getProperties(); Set checked = EnumSet.noneOf(RestrictableAction.class); for (RestrictableAction action : RestrictableAction.values()) { - if (isActionChecked(action)) + String saved = props.get(action.name()); + if (saved == null ? action.isDefaultChecked() : TRUE.equals(saved)) checked.add(action); } return checked; @@ -159,8 +162,11 @@ public static void save(@NotNull User user, boolean masterEnabled, @NotNull Set< if (masterEnabled != oldMaster || !newChecked.equals(oldChecked)) { + // When the master switch is off, no actions are enforced even though per-action selections are + // preserved, so report the effective (enforced) set rather than the saved checkboxes. + String enforced = masterEnabled ? describe(newChecked) : "(none - master switch off)"; String comment = "Targeted MS Guest Access settings updated. Master switch: " + (masterEnabled ? "on" : "off") - + ". Actions requiring login for guests: " + describe(newChecked) + "."; + + ". Actions requiring login for guests: " + enforced + "."; SiteSettingsAuditProvider.SiteSettingsAuditEvent event = new SiteSettingsAuditProvider.SiteSettingsAuditEvent(ContainerManager.getRoot(), comment); AuditLogService.get().addEvent(user, event); diff --git a/src/org/labkey/targetedms/TargetedMSController.java b/src/org/labkey/targetedms/TargetedMSController.java index 512657360..665f78feb 100644 --- a/src/org/labkey/targetedms/TargetedMSController.java +++ b/src/org/labkey/targetedms/TargetedMSController.java @@ -615,7 +615,7 @@ public String[] getRestrictedActions() public void setRestrictedActions(String[] restrictedActions) { - _restrictedActions = restrictedActions; + _restrictedActions = restrictedActions == null ? new String[0] : restrictedActions; } /** The checked actions as an enum set, ignoring any unrecognized keys. */ @@ -3246,9 +3246,12 @@ public ModelAndView getView(ChromatogramForm form, BindException errors) @Override public void addNavTrail(NavTree root) { + // Add the top-level crumb unconditionally so the page has a title even on the guest login-gate + // path, where getView returns early and _run is never set (the test framework flags a titleless + // action as a failure). + root.addChild("Targeted MS Runs", getShowListURL(getContainer())); if (null != _run) { - root.addChild("Targeted MS Runs", getShowListURL(getContainer())); root.addChild(_run.getDescription(), getShowRunURL(getContainer(), _run.getId())); root.addChild(_sequence); } @@ -3340,9 +3343,12 @@ public ModelAndView getView(ChromatogramForm form, BindException errors) @Override public void addNavTrail(NavTree root) { + // Add the top-level crumb unconditionally so the page has a title even on the guest login-gate + // path, where getView returns early and _run is never set (the test framework flags a titleless + // action as a failure). + root.addChild("Targeted MS Runs", getShowListURL(getContainer())); if (null != _run) { - root.addChild("Targeted MS Runs", getShowListURL(getContainer())); root.addChild(_run.getDescription(), getShowRunURL(getContainer(), _run.getId())); root.addChild(_customIonName); } @@ -4705,10 +4711,10 @@ public ShowPrecursorListAction(ViewContext ctx) } @Override - public ModelAndView getHtmlView(final RunDetailsForm form, BindException errors) throws Exception + public ModelAndView getView(RunDetailsForm form, BindException errors) throws Exception { HtmlView loginGate = getGuestLoginGate(GuestAccessManager.RestrictableAction.showPrecursorList, getViewContext(), getContainer()); - return loginGate != null ? loginGate : super.getHtmlView(form, errors); + return loginGate != null ? loginGate : super.getView(form, errors); } @Override @@ -4886,10 +4892,10 @@ protected GroupComparisonView createQueryView( public class ShowCalibrationCurvesAction extends ShowRunSplitDetailsAction { @Override - public ModelAndView getHtmlView(final RunDetailsForm form, BindException errors) throws Exception + public ModelAndView getView(RunDetailsForm form, BindException errors) throws Exception { - HtmlView loginGate = getGuestLoginGate(GuestAccessManager.RestrictableAction.showCalibrationCurve, getViewContext(), getContainer()); - return loginGate != null ? loginGate : super.getHtmlView(form, errors); + HtmlView loginGate = getGuestLoginGate(GuestAccessManager.RestrictableAction.showCalibrationCurves, getViewContext(), getContainer()); + return loginGate != null ? loginGate : super.getView(form, errors); } @Override diff --git a/test/src/org/labkey/test/tests/targetedms/TargetedMSGuestAccessTest.java b/test/src/org/labkey/test/tests/targetedms/TargetedMSGuestAccessTest.java index 8d4330dd2..b0c8d42e8 100644 --- a/test/src/org/labkey/test/tests/targetedms/TargetedMSGuestAccessTest.java +++ b/test/src/org/labkey/test/tests/targetedms/TargetedMSGuestAccessTest.java @@ -18,6 +18,8 @@ import org.junit.BeforeClass; import org.junit.Test; import org.junit.experimental.categories.Category; +import org.labkey.remoteapi.query.SelectRowsCommand; +import org.labkey.remoteapi.query.SelectRowsResponse; import org.labkey.test.BaseWebDriverTest; import org.labkey.test.Locator; import org.labkey.test.TestTimeoutException; @@ -26,6 +28,8 @@ import org.labkey.test.util.LogMethod; import org.openqa.selenium.WebElement; +import java.util.ArrayList; +import java.util.List; import java.util.Map; import static org.junit.Assert.assertFalse; @@ -43,9 +47,8 @@ * - Detail pages (showProtein, showPeptide) show an inline login view at the same URL, whose text is * "Login to view this data". * - Chart-image endpoints (precursorChromatogramChart) cannot return that HTML, so they redirect a - * blocked guest to the standard login page. This is tested with a dummy id, because the gate runs - * before the id is looked up: a blocked guest lands on login.view, while an allowed guest stays on the - * chart URL (a "not found" page for the dummy id). + * blocked guest to the standard login page. A blocked guest lands on login.view (the gate runs before + * the id is looked up), while an allowed guest renders the real chart at the same URL. * * This does not change the pages that always require a login for guests (e.g. the precursor * "all chromatograms" page), which stay blocked no matter what the switch is set to. @@ -62,6 +65,7 @@ public class TargetedMSGuestAccessTest extends TargetedMSTest // Action keys as stored/posted by the settings page (must match the GuestAccessManager enum names). private static final String SHOW_PROTEIN = "showProtein"; private static final String SHOW_PEPTIDE = "showPeptide"; + private static final String SHOW_PRECURSOR_LIST = "showPrecursorList"; private static final String PRECURSOR_CHROMATOGRAM_CHART = "precursorChromatogramChart"; private static final Locator MASTER_SWITCH = Locator.id("tms-require-login-master"); @@ -70,8 +74,15 @@ public class TargetedMSGuestAccessTest extends TargetedMSTest // @BeforeClass setup, so these must be static (instance fields would be null in the test method). private static String proteinUrl; // showProtein detail page private static String peptideUrl; // showPeptide detail page - private static String chartUrl; // precursorChromatogramChart image endpoint (dummy id) + private static String chartUrl; // precursorChromatogramChart image endpoint private static String requiresLoginUrl; // PrecursorAllChromatogramsChartAction, always blocked for guests + private static String precursorListUrl; // showPrecursorList HTML page (a QueryView action) + private static String precursorListExportUrl; // its TSV export URL (dispatched before the HTML view) + + // The site-wide Guest Access state as it was before this test, so doCleanup can restore it exactly (these + // are root-container properties shared across the whole server, not scoped to this test's folder). + private static boolean origMasterEnabled; + private static List origCheckedKeys; @Override protected String getProjectName() @@ -80,32 +91,49 @@ protected String getProjectName() } @BeforeClass - public static void initProject() + public static void initProject() throws Exception { TargetedMSGuestAccessTest init = getCurrentTest(); init.doSetup(); } @LogMethod - private void doSetup() + private void doSetup() throws Exception { + // Remember the site-wide Guest Access settings before touching anything, so doCleanup can put them + // back exactly as they were (they live on the root container and are shared across the whole server). + captureOriginalGuestAccessSettings(); + setupFolder(FolderType.Experiment); importData(SKY_FILE); // Capture the URLs we will later request as a guest. goToDashboard(); clickAndWait(Locator.linkWithText(SKY_FILE)); + + // The document's default page is the precursor list (showPrecursorList, a QueryView action). Capture + // it and its TSV export URL. + precursorListUrl = getCurrentRelativeURL(); + precursorListExportUrl = precursorListUrl + (precursorListUrl.contains("?") ? "&" : "?") + "exportType=exportRowsTsv"; + clickAndWait(Locator.linkWithText(TARGET_PROTEIN)); proteinUrl = getCurrentRelativeURL(); - // A chart-image endpoint in this same folder. A dummy id is enough because the login gate runs - // before the id is looked up. - chartUrl = WebTestHelper.buildURL("targetedms", getCurrentContainerPath(), PRECURSOR_CHROMATOGRAM_CHART, - Map.of("id", "1")); - clickAndWait(Locator.linkWithText(TARGET_PEPTIDE)); peptideUrl = getCurrentRelativeURL(); + // Look up a real precursorChromInfoId from the database and build the chart URL from it. A real id + // renders an actual chart in the master-off "allowed" case; the gate still runs before the id lookup, + // so the master-on case redirects to login regardless of the id. + SelectRowsCommand chromInfoQuery = new SelectRowsCommand("targetedms", "PrecursorChromInfo"); + chromInfoQuery.setColumns(List.of("Id")); + chromInfoQuery.setMaxRows(1); + SelectRowsResponse chromInfoRows = chromInfoQuery.execute(createDefaultConnection(), getCurrentContainerPath()); + assertFalse("Expected at least one PrecursorChromInfo row", chromInfoRows.getRows().isEmpty()); + Number chromInfoId = (Number) chromInfoRows.getRows().get(0).get("Id"); + chartUrl = WebTestHelper.buildURL("targetedms", getCurrentContainerPath(), PRECURSOR_CHROMATOGRAM_CHART, + Map.of("id", String.valueOf(chromInfoId.longValue()))); + // Peptide -> precursor "all chromatograms" page, which always requires a login for guests. clickAndWait(Locator.linkWithImage("TransitionGroupLib.png")); requiresLoginUrl = getCurrentRelativeURL(); @@ -116,15 +144,36 @@ private void doSetup() new ApiPermissionsHelper(this).setSiteGroupPermissions("Guests", READER_ROLE); } + /** Read the current site-wide Guest Access state from the settings page (must be signed in as site admin). */ + private void captureOriginalGuestAccessSettings() + { + beginAt(WebTestHelper.buildURL("targetedms", "/", "guestAccessSettings")); + origMasterEnabled = MASTER_SWITCH.findElement(getDriver()).isSelected(); + origCheckedKeys = new ArrayList<>(); + for (WebElement box : Locator.tagWithClass("input", "tms-require-login-action").findElements(getDriver())) + { + if (box.isSelected()) + origCheckedKeys.add(box.getAttribute("value")); + } + } + @Override protected void doCleanup(boolean afterTest) throws TestTimeoutException { - // Leave the site-wide switch off so it can't leak into other tests' guest access. The framework - // already signs back in as the site admin before calling doCleanup, so we can just save. Best - // effort: don't let a failure here block the project deletion in super.doCleanup. + // Restore the site-wide Guest Access settings to what they were before the test. These are root- + // container properties shared across the whole server, so a leftover value would leak into other + // tests. The framework already signs back in as the site admin before doCleanup. Best effort: don't + // let a failure here block the project deletion in super.doCleanup. try { - saveGuestAccessSettings(false); + if (origCheckedKeys != null) + { + // The per-action checkboxes only post while the master switch is on, so re-apply the original + // selections with it on, then set the master switch back to its original value. + saveGuestAccessSettings(true, origCheckedKeys.toArray(new String[0])); + if (!origMasterEnabled) + saveGuestAccessSettings(false); + } } catch (Exception ignored) { @@ -165,7 +214,22 @@ public void testGuestAccessToggle() saveGuestAccessSettings(false); signOut(); assertGuestAllowedPage(proteinUrl, TARGET_PROTEIN); + assertGuestAllowedPage(peptideUrl, TARGET_PEPTIDE); assertGuestChartAllowed(chartUrl); + + // 5. A QueryView action's export URL is a separate direct request, not a link reached through the HTML + // page: showPrecursorList.view?...&exportType=exportRowsTsv serves a TSV. QueryViewAction.getView + // dispatches that export branch before it would reach getHtmlView, so a client requesting the export + // URL directly (e.g. a crawler replaying or constructing it) bypasses a gate placed in getHtmlView. + // Gating showPrecursorList in getView closes that: a guest is sent to login for both the page and a + // direct export request. + assertGuestAllowedPage(precursorListUrl, TARGET_PROTEIN); + assertGuestNotLoginGated(precursorListExportUrl); + signIn(); + saveGuestAccessSettings(true, SHOW_PRECURSOR_LIST); + signOut(); + assertGuestPageBlocked(precursorListUrl); + assertGuestPageBlocked(precursorListExportUrl); } /** @@ -209,12 +273,24 @@ private void assertGuestPageBlocked(String url) assertTrue("Expected the guest login view at " + url, getBodyText().contains(LOGIN_MESSAGE)); } - /** A chart-image endpoint a guest can reach: not redirected to the login page. */ + /** A URL a guest is not login-gated on: the login view is not shown (the response may be data or not-found). */ + private void assertGuestNotLoginGated(String url) + { + beginAt(url); + assertFalse("Guest was unexpectedly shown the login view at " + url, getBodyText().contains(LOGIN_MESSAGE)); + } + + /** A chart-image endpoint a guest can reach: an actual chart image is served, not a login redirect. */ private void assertGuestChartAllowed(String url) { beginAt(url); assertFalse("Guest was unexpectedly redirected to login for " + url, getCurrentRelativeURL().contains("login.view")); + // Confirm a real chart image was served (not a not-found page). This also verifies the chart id is + // valid for this folder: a wrong id would render an HTML not-found page instead of an image. + String contentType = (String) executeScript("return document.contentType;"); + assertTrue("Expected a chart image at " + url + " but got content type " + contentType, + contentType != null && contentType.startsWith("image/")); } /** A chart-image endpoint blocked for a guest: redirected to the standard login page. */ From eec5567395e2e63592e43e704d04a12b13845b0f Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Sun, 12 Jul 2026 13:21:18 -0700 Subject: [PATCH 3/3] Follow-up on the "Targeted MS Guest Access" login toggle. - Gate ShowCalibrationCurveAction (the single-curve page bots hit), not the ShowCalibrationCurvesAction list. It gates in validate() to skip building the curve, and addNavTrail always adds a crumb so the login-gate path has a title. - RestrictableAction is now keyed by action class: actions gate via getClass() (forClass lookup), and the settings-page label comes from the registered action name, so a renamed action keeps working. Stored keys (enum name()) are unchanged. - TargetedMSGuestAccessTest sets the master switch off explicitly in step 1, and restores the site-wide settings in @After so it runs under clean=false. Co-Authored-By: Claude --- .../labkey/targetedms/GuestAccessManager.java | 79 +++++++++++++------ .../targetedms/TargetedMSController.java | 57 +++++++------ .../targetedms/TargetedMSGuestAccessTest.java | 59 +++++++------- 3 files changed, 117 insertions(+), 78 deletions(-) diff --git a/src/org/labkey/targetedms/GuestAccessManager.java b/src/org/labkey/targetedms/GuestAccessManager.java index feeca3c0a..383e541a1 100644 --- a/src/org/labkey/targetedms/GuestAccessManager.java +++ b/src/org/labkey/targetedms/GuestAccessManager.java @@ -16,9 +16,11 @@ package org.labkey.targetedms; import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; +import org.labkey.api.action.BaseViewAction; +import org.labkey.api.action.SpringActionController; import org.labkey.api.audit.AuditLogService; import org.labkey.api.audit.provider.SiteSettingsAuditProvider; -import org.labkey.api.data.Container; import org.labkey.api.data.ContainerManager; import org.labkey.api.data.PropertyManager; import org.labkey.api.data.PropertyManager.PropertyMap; @@ -29,7 +31,6 @@ import java.util.EnumSet; import java.util.List; import java.util.Set; -import java.util.stream.Collectors; /** * Site-admin setting that can require a login for slow targetedms pages that get hit by bots. A master @@ -48,44 +49,68 @@ public class GuestAccessManager private static final String TRUE = Boolean.TRUE.toString(); /** - * The actions a site admin can choose to require a login. - * defaultChecked is used when nothing has been saved yet. + * The actions a site admin can choose to require a login. Each entry is keyed by its action class. * * The pages that draw many charts are checked by default. The single-chart image actions and the - * precursor table are offered too but are off by default, so guests keep seeing inline charts and lists - * during normal operation. An admin can also check those to block direct requests to them during an - * bot attack (they show up in high volume because the detail pages embed many of them). + * precursor table are offered too but are off by default. An admin can also check those to block direct + * requests to them during a bot attack (they show up in high volume during an attack because the detail + * pages embed many of them). */ public enum RestrictableAction { - showProtein("Protein details page (showProtein)", true), - showPeptide("Peptide details page (showPeptide)", true), - showMolecule("Small molecule details page (showMolecule)", true), - showCalibrationCurves("Calibration curves page (showCalibrationCurves)", true), - showPrecursorList("Document details page (showPrecursorList)", false), - showPeakAreas("Peak areas chart (showPeakAreas)", false), - showRetentionTimesChart("Retention times chart (showRetentionTimesChart)", false), - precursorChromatogramChart("Precursor chromatogram (precursorChromatogramChart)", false), - groupChromatogramChart("Protein chromatogram (groupChromatogramChart)", false); - - private final String _label; + showProtein(TargetedMSController.ShowProteinAction.class, "Protein details page", true), + showPeptide(TargetedMSController.ShowPeptideAction.class, "Peptide details page", true), + showMolecule(TargetedMSController.ShowMoleculeAction.class, "Small molecule details page", true), + showCalibrationCurve(TargetedMSController.ShowCalibrationCurveAction.class, "Calibration curve details page", true), + showPrecursorList(TargetedMSController.ShowPrecursorListAction.class, "Document details page", false), + showPeakAreas(TargetedMSController.ShowPeakAreasAction.class, "Peak areas chart", false), + showRetentionTimesChart(TargetedMSController.ShowRetentionTimesChartAction.class, "Retention times chart", false), + precursorChromatogramChart(TargetedMSController.PrecursorChromatogramChartAction.class, "Precursor chromatogram", false), + groupChromatogramChart(TargetedMSController.GroupChromatogramChartAction.class, "Protein chromatogram", false); + + private final Class> _actionClass; + private final String _description; private final boolean _defaultChecked; - RestrictableAction(String label, boolean defaultChecked) + RestrictableAction(Class> actionClass, String description, boolean defaultChecked) { - _label = label; + _actionClass = actionClass; + _description = description; _defaultChecked = defaultChecked; } + /** + * The action's registered URL name (e.g. "showCalibrationCurve"), derived from the action class. + */ + private String getActionName() + { + return SpringActionController.getActionName(_actionClass); + } + + /** Settings-page label, e.g. "Calibration curve details page (showCalibrationCurve)". */ public String getLabel() { - return _label; + return _description + " (" + getActionName() + ")"; } - public boolean isDefaultChecked() + private boolean isDefaultChecked() { return _defaultChecked; } + + /** + * The restrictable action for this action class, or null if the class is not gated. + */ + @Nullable + public static RestrictableAction forClass(@NotNull Class> actionClass) + { + for (RestrictableAction action : values()) + { + if (action._actionClass.equals(actionClass)) + return action; + } + return null; + } } private GuestAccessManager() @@ -122,14 +147,16 @@ public static boolean isRestricted(@NotNull RestrictableAction action) PropertyMap props = getProperties(); if (!TRUE.equals(props.get(MASTER_KEY))) return false; + // saved true/false is the admin's explicit choice (an explicit uncheck is respected). Absent means + // never decided - e.g. an action added in a later release - so fall back to the action's default: + // a new default-checked action is gated once the master switch is on, without waiting for a re-save. String saved = props.get(action.name()); return saved == null ? action.isDefaultChecked() : TRUE.equals(saved); } /** The set of currently-checked actions (independent of the master toggle). */ - public static Set getCheckedActions() + private static Set getCheckedActions() { - // Read the property map once and reuse it rather than re-fetching per action. PropertyMap props = getProperties(); Set checked = EnumSet.noneOf(RestrictableAction.class); for (RestrictableAction action : RestrictableAction.values()) @@ -182,8 +209,8 @@ private static String describe(Set actions) for (RestrictableAction action : RestrictableAction.values()) { if (actions.contains(action)) - names.add(action.name()); + names.add(action.getActionName()); } - return names.stream().collect(Collectors.joining(", ")); + return String.join(", ", names); } } diff --git a/src/org/labkey/targetedms/TargetedMSController.java b/src/org/labkey/targetedms/TargetedMSController.java index 665f78feb..fbe3abd9a 100644 --- a/src/org/labkey/targetedms/TargetedMSController.java +++ b/src/org/labkey/targetedms/TargetedMSController.java @@ -53,6 +53,7 @@ import org.labkey.api.action.ApiResponse; import org.labkey.api.action.ApiSimpleResponse; import org.labkey.api.action.ApiUsageException; +import org.labkey.api.action.BaseViewAction; import org.labkey.api.action.ExportAction; import org.labkey.api.action.FormHandlerAction; import org.labkey.api.action.FormViewAction; @@ -2107,7 +2108,7 @@ public class GroupChromatogramChartAction extends ExportAction @Override public ModelAndView getView(ChromatogramForm form, BindException errors) { - HtmlView loginGate = getGuestLoginGate(GuestAccessManager.RestrictableAction.showPeptide, getViewContext(), getContainer()); + HtmlView loginGate = getGuestLoginGate(getClass(), getViewContext(), getContainer()); if (loginGate != null) return loginGate; @@ -3270,7 +3271,7 @@ public static class ShowMoleculeAction extends SimpleViewAction> actionClass, ViewContext context) + { + GuestAccessManager.RestrictableAction action = GuestAccessManager.RestrictableAction.forClass(actionClass); + return action != null && context.getUser().isGuest() && GuestAccessManager.isRestricted(action); + } + /** * Returns a login view when a guest should be sent to the login page for this action (the site-admin * master switch is on AND this action's checkbox is checked), otherwise null so the action runs as * normal. See {@link GuestAccessSettingsAction}. */ @Nullable - private static HtmlView getGuestLoginGate(GuestAccessManager.RestrictableAction action, ViewContext context, Container container) + private static HtmlView getGuestLoginGate(Class> actionClass, ViewContext context, Container container) { - if (context.getUser().isGuest() && GuestAccessManager.isRestricted(action)) - return getLoginView(context, container); - return null; + return isGuestGated(actionClass, context) ? getLoginView(context, container) : null; } /** * Same check as {@link #getGuestLoginGate}, but for the chart actions that write an image. Those cannot * return the HTML login view, so a restricted guest is redirected to the login page instead. */ - private static void redirectGuestToLoginForChart(GuestAccessManager.RestrictableAction action, ViewContext context, Container container) + private static void redirectGuestToLoginForChart(Class> actionClass, ViewContext context, Container container) { - if (context.getUser().isGuest() && GuestAccessManager.isRestricted(action)) + if (isGuestGated(actionClass, context)) throw new RedirectException(PageFlowUtil.urlProvider(LoginUrls.class).getLoginURL(container, context.getActionURL())); } @@ -4713,7 +4721,7 @@ public ShowPrecursorListAction(ViewContext ctx) @Override public ModelAndView getView(RunDetailsForm form, BindException errors) throws Exception { - HtmlView loginGate = getGuestLoginGate(GuestAccessManager.RestrictableAction.showPrecursorList, getViewContext(), getContainer()); + HtmlView loginGate = getGuestLoginGate(getClass(), getViewContext(), getContainer()); return loginGate != null ? loginGate : super.getView(form, errors); } @@ -4891,13 +4899,6 @@ protected GroupComparisonView createQueryView( @RequiresPermission(ReadPermission.class) public class ShowCalibrationCurvesAction extends ShowRunSplitDetailsAction { - @Override - public ModelAndView getView(RunDetailsForm form, BindException errors) throws Exception - { - HtmlView loginGate = getGuestLoginGate(GuestAccessManager.RestrictableAction.showCalibrationCurves, getViewContext(), getContainer()); - return loginGate != null ? loginGate : super.getView(form, errors); - } - @Override public String getDataRegionNamePeptide() { @@ -5703,7 +5704,7 @@ public class ShowProteinAction extends SimpleViewAction @Override public ModelAndView getView(final ProteinForm form, BindException errors) { - HtmlView loginGate = getGuestLoginGate(GuestAccessManager.RestrictableAction.showProtein, getViewContext(), getContainer()); + HtmlView loginGate = getGuestLoginGate(getClass(), getViewContext(), getContainer()); if (loginGate != null) return loginGate; @@ -8441,9 +8442,11 @@ public ShowCalibrationCurveAction() @Override public void addNavTrail(NavTree root) { + // Add the top-level crumb unconditionally so the page has a title even on the guest login-gate + // path, where validate() returns early and _run is never set. + root.addChild("Targeted MS Runs", getShowListURL(getContainer())); if (null != _run) { - root.addChild("Targeted MS Runs", getShowListURL(getContainer())); root.addChild(_run.getDescription(), getShowCalibrationCurvesURL(getContainer(), _run.getId())); if (_curvePlotView.getChart().getMolecule() != null) { @@ -8455,6 +8458,11 @@ public void addNavTrail(NavTree root) @Override public void validate(CalibrationCurveForm form, BindException errors) { + // Skip the expensive CalibrationCurveView construction below. Gating only in getView would be too late, + // because the expensive work is done here in validate(). + if (isGuestGated(getClass(), getViewContext())) + return; + _curvePlotView = new CalibrationCurveView(getUser(), getContainer(), form.getCalibrationCurveId()); CalibrationCurveEntity chart = _curvePlotView.getChart().getCalibrationCurveEntity(); //ensure that the experiment run is valid and exists within the current container @@ -8476,6 +8484,11 @@ protected QueryView createQueryView(CalibrationCurveForm form, BindException err @Override public ModelAndView getView(CalibrationCurveForm calibrationCurveForm, BindException errors) { + // A restricted guest is short-circuited here; validate() already skipped the expensive work. + HtmlView loginGate = getGuestLoginGate(getClass(), getViewContext(), getContainer()); + if (loginGate != null) + return loginGate; + CalibrationCurveChart chart = _curvePlotView.getChart(); GeneralMolecule molecule = chart.getMolecule(); if (molecule == null) diff --git a/test/src/org/labkey/test/tests/targetedms/TargetedMSGuestAccessTest.java b/test/src/org/labkey/test/tests/targetedms/TargetedMSGuestAccessTest.java index b0c8d42e8..95892d9aa 100644 --- a/test/src/org/labkey/test/tests/targetedms/TargetedMSGuestAccessTest.java +++ b/test/src/org/labkey/test/tests/targetedms/TargetedMSGuestAccessTest.java @@ -15,6 +15,7 @@ */ package org.labkey.test.tests.targetedms; +import org.junit.After; import org.junit.BeforeClass; import org.junit.Test; import org.junit.experimental.categories.Category; @@ -22,7 +23,6 @@ import org.labkey.remoteapi.query.SelectRowsResponse; import org.labkey.test.BaseWebDriverTest; import org.labkey.test.Locator; -import org.labkey.test.TestTimeoutException; import org.labkey.test.WebTestHelper; import org.labkey.test.util.ApiPermissionsHelper; import org.labkey.test.util.LogMethod; @@ -70,8 +70,7 @@ public class TargetedMSGuestAccessTest extends TargetedMSTest private static final Locator MASTER_SWITCH = Locator.id("tms-require-login-master"); - // Captured during setup and read from the @Test method, which runs on a different instance than - // @BeforeClass setup, so these must be static (instance fields would be null in the test method). + // Captured during setup and read from the @Test method. private static String proteinUrl; // showProtein detail page private static String peptideUrl; // showPeptide detail page private static String chartUrl; // precursorChromatogramChart image endpoint @@ -79,8 +78,10 @@ public class TargetedMSGuestAccessTest extends TargetedMSTest private static String precursorListUrl; // showPrecursorList HTML page (a QueryView action) private static String precursorListExportUrl; // its TSV export URL (dispatched before the HTML view) - // The site-wide Guest Access state as it was before this test, so doCleanup can restore it exactly (these - // are root-container properties shared across the whole server, not scoped to this test's folder). + // The site-wide Guest Access state as it was before this test, so the @After restore can put it back + // exactly (these are root-container properties shared across the whole server, not scoped to this test's + // folder). Restored in @After rather than doCleanup so it runs even when project cleanup is skipped + // (clean=false). private static boolean origMasterEnabled; private static List origCheckedKeys; @@ -100,8 +101,7 @@ public static void initProject() throws Exception @LogMethod private void doSetup() throws Exception { - // Remember the site-wide Guest Access settings before touching anything, so doCleanup can put them - // back exactly as they were (they live on the root container and are shared across the whole server). + // Remember the site-wide Guest Access settings before touching anything, so they can be restored after the test. captureOriginalGuestAccessSettings(); setupFolder(FolderType.Experiment); @@ -130,7 +130,7 @@ private void doSetup() throws Exception chromInfoQuery.setMaxRows(1); SelectRowsResponse chromInfoRows = chromInfoQuery.execute(createDefaultConnection(), getCurrentContainerPath()); assertFalse("Expected at least one PrecursorChromInfo row", chromInfoRows.getRows().isEmpty()); - Number chromInfoId = (Number) chromInfoRows.getRows().get(0).get("Id"); + Number chromInfoId = (Number) chromInfoRows.getRows().getFirst().get("Id"); chartUrl = WebTestHelper.buildURL("targetedms", getCurrentContainerPath(), PRECURSOR_CHROMATOGRAM_CHART, Map.of("id", String.valueOf(chromInfoId.longValue()))); @@ -157,34 +157,33 @@ private void captureOriginalGuestAccessSettings() } } - @Override - protected void doCleanup(boolean afterTest) throws TestTimeoutException + @After + public void restoreGuestAccessSettings() { - // Restore the site-wide Guest Access settings to what they were before the test. These are root- - // container properties shared across the whole server, so a leftover value would leak into other - // tests. The framework already signs back in as the site admin before doCleanup. Best effort: don't - // let a failure here block the project deletion in super.doCleanup. - try - { - if (origCheckedKeys != null) - { - // The per-action checkboxes only post while the master switch is on, so re-apply the original - // selections with it on, then set the master switch back to its original value. - saveGuestAccessSettings(true, origCheckedKeys.toArray(new String[0])); - if (!origMasterEnabled) - saveGuestAccessSettings(false); - } - } - catch (Exception ignored) - { - } - super.doCleanup(afterTest); + // Put the site-wide Guest Access settings back the way the test found them. This runs in @After, + // not doCleanup, so it still happens when project cleanup is skipped (clean=false). + if (origCheckedKeys == null) + return; // setup did not get far enough to capture the originals + + // The test signs out at the end, and the settings page requires a site admin, so sign back in first. + ensureSignedInAsPrimaryTestUser(); + + // The per-action checkboxes only post while the master switch is on, so re-apply the original + // selections with it on, then set the master switch back to its original value. + saveGuestAccessSettings(true, origCheckedKeys.toArray(new String[0])); + if (!origMasterEnabled) + saveGuestAccessSettings(false); } @Test public void testGuestAccessToggle() { - // 1. Master OFF (default): guests can view the pages and the chart; the always-on page is blocked. + // 1. Master OFF: establish the baseline explicitly rather than trusting the ambient site-wide state. + // The master switch is a root-container property shared across the whole server, so a prior aborted + // run could leave it on; set it off here (we are still signed in as the site admin) so the "guests + // can view" checks below are deterministic. Guests can then view the pages and the chart, while the + // always-on page stays blocked. + saveGuestAccessSettings(false); signOut(); assertGuestAllowedPage(proteinUrl, TARGET_PROTEIN); assertGuestAllowedPage(peptideUrl, TARGET_PEPTIDE);