Merge pull request #2 from daniel-beck/system-read-admin-monitors

More Javadoc to help implementers, simplify code a bit

core/src/main/java/hudson/model/AdministrativeMonitor.java

@@ -75,6 +75,14 @@ import org.kohsuke.stapler.interceptor.RequirePOST;
  * </dd>
  * </dl>
  *
+ * <h3>Use with System Read permission</h3>
+ * <p>
+ *     By default administrative monitors are visible only to users with Administer permission.
+ *     Users with {@link Jenkins#SYSTEM_READ} permission can access administrative monitors that override {@link #getRequiredPermission()}.
+ *     Care needs to be taken to ensure users with that permission don't have access to actions modifying system state.
+ *     For more details, see {@link #getRequiredPermission()}.
+ * </p>
+ *
  * @author Kohsuke Kawaguchi
  * @since 1.273
  * @see Jenkins#administrativeMonitors
@@ -156,15 +164,27 @@ public abstract class AdministrativeMonitor extends AbstractModelObject implemen
     }
 
     /**
-     * Required permission to view this admin monitor
-     *
+     * Required permission to view this admin monitor.
+     * By default {@link Jenkins#ADMINISTER}, but {@link Jenkins#SYSTEM_READ} is also supported.
+     * <p>
+     *     Changing this permission check to return {@link Jenkins#SYSTEM_READ} will make the active
+     *     administrative monitor appear on {@code manage.jelly} and on the globally visible
+     *     {@link jenkins.management.AdministrativeMonitorsDecorator} to users without Administer permission.
+     *     {@link #doDisable(StaplerRequest, StaplerResponse)} will still always require Administer permission.
+     * </p>
+     * <p>
+     *     Implementers need to ensure that {@code doAct} and other web methods perform necessary permission checks:
+     *     Users with System Read permissions are expected to be limited to read-only access.
+     *     Form UI elements that change system state, e.g. toggling a feature on or off, need to be hidden from users
+     *     lacking Administer permission.
+     * </p>
      */
     public Permission getRequiredPermission() {
         return Jenkins.ADMINISTER;
     }
 
     /**
-     * Requires ADMINISTER permission for any operation in here.
+     * Ensure that URLs in this administrative monitor are only accessible to users with {@link #getRequiredPermission()}.
      */
     @Restricted(NoExternalUse.class)
     public Object getTarget() {

core/src/main/java/jenkins/management/AdministrativeMonitorsDecorator.java

@@ -24,7 +24,6 @@
 package jenkins.management;
 
 import hudson.Extension;
-import hudson.Functions;
 import hudson.diagnosis.ReverseProxySetupMonitor;
 import hudson.model.AdministrativeMonitor;
 import hudson.model.PageDecorator;
@@ -38,8 +37,6 @@ import org.kohsuke.stapler.Ancestor;
 import org.kohsuke.stapler.Stapler;
 import org.kohsuke.stapler.StaplerRequest;
 
-import javax.servlet.ServletException;
-import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
@@ -91,11 +88,9 @@ public class AdministrativeMonitorsDecorator extends PageDecorator {
     /**
      * Whether the administrative monitors notifier should be shown.
      * @return true iff the administrative monitors notifier should be shown.
-     * @throws IOException
-     * @throws ServletException
      */
-    public boolean shouldDisplay() throws IOException, ServletException {
-        if (!Functions.hasPermission(Jenkins.SYSTEM_READ)) {
+    public boolean shouldDisplay() {
+        if (!Jenkins.get().hasPermission(Jenkins.SYSTEM_READ)) {
             return false;
         }
 

core/src/main/java/jenkins/model/Jenkins.java

@@ -2198,7 +2198,8 @@ public class Jenkins extends AbstractCIBase implements DirectlyModifiableTopLeve
     }
 
     /**
-     * Returns the enabled and activated administrative monitors.
+     * Returns the enabled and activated administrative monitors accessible to the current user.
+     *
      * @since 2.64
      */
     public List<AdministrativeMonitor> getActiveAdministrativeMonitors() {
@@ -2207,7 +2208,7 @@ public class Jenkins extends AbstractCIBase implements DirectlyModifiableTopLeve
         }
         return administrativeMonitors.stream().filter(m -> {
             try {
-                return m.isEnabled() && m.isActivated() && Jenkins.get().hasPermission(m.getRequiredPermission());
+                return Jenkins.get().hasPermission(m.getRequiredPermission()) && m.isEnabled() && m.isActivated();
             } catch (Throwable x) {
                 LOGGER.log(Level.WARNING, null, x);
                 return false;