Follow-up on the "Targeted MS Guest Access" login toggle.

vagisha · claude · vagisha · commit eec5567395e2 · 2026-07-12T13:21:18.000-07:00
- 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 <noreply@anthropic.com>
diff --git 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<? extends BaseViewAction<?>> _actionClass;
+        private final String _description;
         private final boolean _defaultChecked;
 
-        RestrictableAction(String label, boolean defaultChecked)
+        RestrictableAction(Class<? extends BaseViewAction<?>> 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<? extends BaseViewAction<?>> 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<RestrictableAction> getCheckedActions()
+    private static Set<RestrictableAction> getCheckedActions()
     {
-        // Read the property map once and reuse it rather than re-fetching per action.
         PropertyMap props = getProperties();
         Set<RestrictableAction> checked = EnumSet.noneOf(RestrictableAction.class);
         for (RestrictableAction action : RestrictableAction.values())
@@ -182,8 +209,8 @@ private static String describe(Set<RestrictableAction> 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
@@ -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<GroupChromatogram
         @Override
         public void export(GroupChromatogramForm form, HttpServletResponse response, BindException errors) throws Exception
         {
-            redirectGuestToLoginForChart(GuestAccessManager.RestrictableAction.groupChromatogramChart, getViewContext(), getContainer());
+            redirectGuestToLoginForChart(getClass(), getViewContext(), getContainer());
 
             PeptideGroup group = PeptideGroupManager.getPeptideGroup(getContainer(), form.getGroupId());
             if (group == null)
@@ -2145,7 +2146,7 @@ public class PrecursorChromatogramChartAction extends ExportAction<ChromatogramF
         @Override
         public void export(ChromatogramForm form, HttpServletResponse response, BindException errors) throws Exception
         {
-            redirectGuestToLoginForChart(GuestAccessManager.RestrictableAction.precursorChromatogramChart, getViewContext(), getContainer());
+            redirectGuestToLoginForChart(getClass(), getViewContext(), getContainer());
 
             PrecursorChromInfo pChromInfo = PrecursorManager.getPrecursorChromInfo(getContainer(), form.getId());
             if (pChromInfo == null)
@@ -3169,7 +3170,7 @@ public class ShowPeptideAction extends SimpleViewAction<ChromatogramForm>
         @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<ChromatogramForm
         @Override
         public ModelAndView getView(ChromatogramForm form, BindException errors)
         {
-            HtmlView loginGate = getGuestLoginGate(GuestAccessManager.RestrictableAction.showMolecule, getViewContext(), getContainer());
+            HtmlView loginGate = getGuestLoginGate(getClass(), getViewContext(), getContainer());
             if (loginGate != null)
                 return loginGate;
 
@@ -3793,7 +3794,7 @@ public void validate(SummaryChartForm form, BindException errors)
         @Override
         public void export(SummaryChartForm form, HttpServletResponse response, BindException errors) throws Exception
         {
-            redirectGuestToLoginForChart(GuestAccessManager.RestrictableAction.showPeakAreas, getViewContext(), getContainer());
+            redirectGuestToLoginForChart(getClass(), getViewContext(), getContainer());
 
             JFreeChart chart;
             if (form.isAsProteomics())
@@ -3851,7 +3852,7 @@ public void validate(SummaryChartForm form, BindException errors)
         @Override
         public void export(SummaryChartForm form, HttpServletResponse response, BindException errors) throws Exception
         {
-            redirectGuestToLoginForChart(GuestAccessManager.RestrictableAction.showRetentionTimesChart, getViewContext(), getContainer());
+            redirectGuestToLoginForChart(getClass(), getViewContext(), getContainer());
 
             if (form.getValue() == null)
                 form.setValue("All");
@@ -4673,26 +4674,33 @@ private static HtmlView getLoginView(ViewContext context, Container container)
                         AuthenticationManager.isRegistrationEnabled() ? registerLink : "")));
     }
 
+    /**
+     * True when a guest should be sent to login for this action: master switch on AND this action checked.
+     */
+    private static boolean isGuestGated(Class<? extends BaseViewAction<?>> 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<? extends BaseViewAction<?>> 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<? extends BaseViewAction<?>> 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<CalibrationCurvesView>
     {
-        @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<ProteinForm>
         @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
