Refactoring: code that handles I/O to a file is moved to a separate class

core/src/main/java/jenkins/security/admin/AdminCallableMonitor.java

@@ -29,12 +29,12 @@ public class AdminCallableMonitor extends AdministrativeMonitor {
 
     @Override
     public boolean isActivated() {
-        return rule.hasRejection();
+        return !rule.rejected.get().isEmpty();
     }
 
     @Override
     public String getDisplayName() {
-        return "Slave \u2192 Master Command Whitelisting";
+        return "Slave \u2192 Master Access Control";
     }
 
     // bind this to URL

core/src/main/java/jenkins/security/admin/AdminWhitelistRule.java

@@ -1,15 +1,11 @@
 package jenkins.security.admin;
 
-import com.google.common.collect.ImmutableSet;
 import hudson.CopyOnWrite;
 import hudson.Extension;
 import hudson.FilePath;
 import hudson.Util;
-import hudson.init.Initializer;
 import hudson.util.HttpResponses;
-import hudson.util.TextFile;
 import jenkins.model.Jenkins;
-import jenkins.security.CallableWhitelist;
 import org.jenkinsci.remoting.Role;
 import org.jenkinsci.remoting.RoleSensitive;
 import org.kohsuke.stapler.HttpResponse;
@@ -21,19 +17,12 @@ import org.kohsuke.stapler.interceptor.RequirePOST;
 import javax.inject.Inject;
 import java.io.File;
 import java.io.IOException;
-import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Enumeration;
-import java.util.Iterator;
-import java.util.LinkedHashSet;
 import java.util.List;
-import java.util.Set;
-import java.util.logging.Level;
 import java.util.logging.Logger;
 
-import static hudson.init.InitMilestone.*;
-
 /**
  * Rules of whitelisting for {@link RoleSensitive} objects and {@link FilePath}s.
  *
@@ -47,14 +36,22 @@ public class AdminWhitelistRule implements StaplerProxy {
     /**
      * Ones that we rejected but want to run by admins.
      */
-    private final Set<Class> rejected = Collections.synchronizedSet(new LinkedHashSet<Class>());
+    public final CallableRejectionConfig rejected;
 
-    @CopyOnWrite
-    private volatile Set<String> whitelisted = Collections.emptySet();
+    public final CallableWhitelistConfig whitelisted;
 
     @CopyOnWrite
     private final List<FilePathRule> filePathRules = Collections.emptyList();
 
+    public AdminWhitelistRule() {
+        // while this file is not a secret, write access to this file is dangerous,
+        // so put this in the better-protected part of $JENKINS_HOME, which is in secrets/
+        this.whitelisted = new CallableWhitelistConfig(
+                new File(jenkins.getRootDir(),"secrets/whitelisted-callables.txt"));
+        this.rejected = new CallableRejectionConfig(
+                new File(jenkins.getRootDir(),"secrets/rejected-callables.txt"),
+                whitelisted);
+    }
 
     public boolean checkFileAccess(String op, File path) throws SecurityException {
         String pathStr = null;
@@ -86,75 +83,10 @@ public class AdminWhitelistRule implements StaplerProxy {
             return true;    // whitelisted by admin
 
         // otherwise record the problem and refuse to execute that
-        boolean changed = rejected.add(subject.getClass());
-        if (changed) {
-            saveRejected();
-        }
-
+        rejected.report(subject.getClass());
         return false;
     }
 
-    /**
-     * Returns the currently rejected callables.
-     */
-    public List<RejectedCallable> getRejectedList() {
-        List<RejectedCallable> l = new ArrayList<RejectedCallable>();
-        synchronized (rejected) {
-            for (Class c : rejected) {
-                l.add(new RejectedCallable(c));
-            }
-        }
-        return l;
-    }
-
-    public boolean hasRejection() {
-        return !rejected.isEmpty();
-    }
-
-    /**
-     * Persists {@link #rejected}
-     */
-    private void saveRejected() {
-        try {
-            synchronized (rejected) {
-                StringBuilder buf = new StringBuilder();
-                for (Class c : rejected) {
-                    if (buf.length()>0) buf.append('\n');
-                    buf.append(c.getName());
-                }
-                getRejectedFile().write(buf.toString());
-            }
-        } catch (IOException e) {
-            LOGGER.log(Level.WARNING, "Failed to persist "+getRejectedFile(), e);
-        }
-    }
-
-    public TextFile getRejectedFile() {
-        // while this file is not a secret, write access to this file is dangerous,
-        // so put this in the better-protected part of $JENKINS_HOME, which is in secrets/
-        return new TextFile(new File(jenkins.getRootDir(),"secrets/rejected-callables.txt"));
-    }
-
-    private void appendWhiteList(String addition) throws IOException {
-        String s = getWhitelistFile().read();
-        if (!s.endsWith("\n"))
-            s += "\n";
-        s+= addition;
-
-        setWhiteList(s);
-    }
-
-    private void setWhiteList(String newWhiteList) throws IOException {
-        jenkins.checkPermission(Jenkins.ADMINISTER);
-
-        getWhitelistFile().write(newWhiteList);
-        loadWhitelist();
-    }
-
-    public TextFile getWhitelistFile() {
-        return new TextFile(new File(jenkins.getRootDir(),"secrets/whitelisted-callables.txt"));
-    }
-
     @RequirePOST
     public HttpResponse doSubmit(StaplerRequest req) throws IOException {
         jenkins.checkPermission(Jenkins.ADMINISTER);
@@ -171,7 +103,7 @@ public class AdminWhitelistRule implements StaplerProxy {
             }
         }
 
-        setWhiteList(whitelist);
+        whitelisted.set(whitelist);
 
         return HttpResponses.redirectToDot();
     }
@@ -181,15 +113,13 @@ public class AdminWhitelistRule implements StaplerProxy {
      */
     @RequirePOST
     public HttpResponse doApproveAll() throws IOException {
-        synchronized (rejected) {
-            StringBuilder buf = new StringBuilder();
-            for (Class c : rejected) {
-                buf.append(c.getName()).append('\n');
-            }
-            appendWhiteList(buf.toString());
-
-            return HttpResponses.ok();
+        StringBuilder buf = new StringBuilder();
+        for (Class c : rejected.get()) {
+            buf.append(c.getName()).append('\n');
         }
+        whitelisted.append(buf.toString());
+
+        return HttpResponses.ok();
     }
 
     /**
@@ -197,59 +127,10 @@ public class AdminWhitelistRule implements StaplerProxy {
      */
     @RequirePOST
     public HttpResponse doApprove(@QueryParameter String value) throws IOException {
-        appendWhiteList(value);
+        whitelisted.append(value);
         return HttpResponses.ok();
     }
 
-    @Initializer(after=EXTENSIONS_AUGMENTED,fatal=false)
-    public static void init() throws IOException {
-        CallableWhitelist.all().get(AdminCallableWhitelist.class).load();
-    }
-
-    public void load() throws IOException {
-        loadRejected();
-        loadWhitelist();
-    }
-
-    private void loadRejected() throws IOException {
-        if (getRejectedFile().exists()) {
-            for (String line : getRejectedFile().read().split("\n")) {
-                try {
-                    Class<?> c = jenkins.pluginManager.uberClassLoader.loadClass(line.trim());
-                    rejected.add(c);
-                } catch (ClassNotFoundException e) {
-                    // no longer present in the system?
-                }
-            }
-        }
-    }
-
-    private void loadWhitelist() throws IOException {
-        if (getWhitelistFile().exists()) {
-            List<String> whitelist = new ArrayList<String>();
-            for (String line : getWhitelistFile().read().split("\n")) {
-                whitelist.add(line.trim());
-            }
-            this.whitelisted = ImmutableSet.copyOf(whitelist);
-        }
-
-        // remove whitelisted ones from the reject list
-        boolean changed = false;
-        synchronized (rejected) {
-            Iterator<Class> itr = rejected.iterator();
-            while (itr.hasNext()) {
-                Class next = itr.next();
-                if (whitelisted.contains(next.getName())) {
-                    itr.remove();
-                    changed = true;
-                }
-            }
-        }
-
-        if (changed)
-            saveRejected();
-    }
-
     /**
      * Restricts the access to administrator.
      */

core/src/main/java/jenkins/security/admin/CallableRejectionConfig.java

@@ -0,0 +1,78 @@
+package jenkins.security.admin;
+
+import com.google.common.collect.ImmutableSet;
+import jenkins.model.Jenkins;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import java.util.logging.Level;
+import java.util.logging.Logger;
+
+/**
+ * Text file that lists whitelisted callables.
+ *
+ * @author Kohsuke Kawaguchi
+ */
+class CallableRejectionConfig extends ConfigFile<Class,Set<Class>> {
+    private final CallableWhitelistConfig whitelist;
+
+    CallableRejectionConfig(File file, CallableWhitelistConfig whitelist) {
+        super(file);
+        this.whitelist = whitelist;
+    }
+
+    @Override
+    protected Set<Class> create() {
+        return new HashSet<Class>();
+    }
+
+    @Override
+    protected Set<Class> readOnly(Set<Class> base) {
+        return ImmutableSet.copyOf(base);
+    }
+
+    @Override
+    protected Class parse(String line) {
+        try {
+            line = line.trim();
+            if (whitelist.contains(line))   return null;    // already whitelisted
+
+            return Jenkins.getInstance().pluginManager.uberClassLoader.loadClass(line);
+        } catch (ClassNotFoundException e) {
+            // no longer present in the system?
+            return null;
+        }
+    }
+
+    /**
+     * This method gets called every time we see a new type of callable that we reject,
+     * so that we can persist the list.
+     */
+    void report(Class c) {
+        if (!get().contains(c)) {
+            try {
+                append(c.getName());
+            } catch (IOException e) {
+                LOGGER.log(Level.WARNING, "Failed to persist " + file, e);
+            }
+        }
+    }
+
+    /**
+     * Return the object that helps the UI rendering by providing the details.
+     */
+    public List<RejectedCallable> describe() {
+        List<RejectedCallable> l = new ArrayList<RejectedCallable>();
+        for (Class c : get()) {
+            l.add(new RejectedCallable(c));
+        }
+        return l;
+    }
+
+
+    private static final Logger LOGGER = Logger.getLogger(CallableRejectionConfig.class.getName());
+}

core/src/main/java/jenkins/security/admin/CallableWhitelistConfig.java

@@ -0,0 +1,39 @@
+package jenkins.security.admin;
+
+import com.google.common.collect.ImmutableSet;
+import hudson.Util;
+import org.jenkinsci.remoting.RoleSensitive;
+
+import java.io.File;
+import java.util.HashSet;
+import java.util.Set;
+
+/**
+ * Set of fully-qualified {@link RoleSensitive} (mostly Callable) class names that are whitelisted by admin.
+ *
+ * @author Kohsuke Kawaguchi
+ */
+class CallableWhitelistConfig extends ConfigFile<String,Set<String>> {
+    CallableWhitelistConfig(File file) {
+        super(file);
+    }
+
+    @Override
+    protected Set<String> create() {
+        return new HashSet<String>();
+    }
+
+    @Override
+    protected Set<String> readOnly(Set<String> base) {
+        return ImmutableSet.copyOf(base);
+    }
+
+    @Override
+    protected String parse(String line) {
+        return Util.fixEmptyAndTrim(line);
+    }
+
+    public boolean contains(String name) {
+        return get().contains(name);
+    }
+}

core/src/main/java/jenkins/security/admin/ConfigFile.java

@@ -0,0 +1,73 @@
+package jenkins.security.admin;
+
+import hudson.CopyOnWrite;
+import hudson.util.TextFile;
+import jenkins.model.Jenkins;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.Collection;
+
+/**
+ * Abstraction of a line-by-line configuration text file that gets parsed into some in-memory data form.
+ *
+ * @author Kohsuke Kawaguchi
+ */
+abstract class ConfigFile<T,COL extends Collection<T>> extends TextFile {
+    @CopyOnWrite
+    private volatile COL parsed;
+
+    public ConfigFile(File file) {
+        super(file);
+        load();
+    }
+
+    protected abstract COL create();
+    protected abstract COL readOnly(COL base);
+
+    public synchronized void load() {
+        COL result = create();
+
+        if (exists()) {
+            for (String line : lines()) {
+                if (line.startsWith("#")) continue;   // comment
+                T r = parse(line);
+                if (r != null)
+                    result.add(r);
+            }
+        }
+
+        parsed = readOnly(result);
+    }
+
+    protected abstract T parse(String line);
+
+    public synchronized void set(String newContent) throws IOException {
+        Jenkins.getInstance().checkPermission(Jenkins.ADMINISTER);
+
+        write(newContent);
+        load();
+    }
+
+    public synchronized void append(String additional) throws IOException {
+        String s = read();
+        if (!s.endsWith("\n"))
+            s += "\n";
+        s+= additional;
+
+        set(s);
+    }
+
+    public COL get() {
+        // load upon the first use
+        if (parsed==null) {
+            synchronized (this) {
+                if (parsed==null)
+                    load();
+            }
+        }
+        return parsed;
+    }
+
+
+}