Merge pull request #27 from ikedam/feature/JENKINS-35081_AdditionalCh…

…ange [JENKINS-35081] Additonal changes for #26
jenkinsci · Jan 26, 2017 · 627d9cb · 627d9cb
2 parents 4b8379e + c7c59a2
commit 627d9cb
Show file tree

Hide file tree

Showing 13 changed files with 420 additions and 132 deletions.
diff --git a/src/main/java/org/jenkinsci/plugins/authorizeproject/AuthorizeProjectProperty.java b/src/main/java/org/jenkinsci/plugins/authorizeproject/AuthorizeProjectProperty.java
@@ -24,7 +24,6 @@
 
 package org.jenkinsci.plugins.authorizeproject;
 
-import edu.umd.cs.findbugs.annotations.NonNull;
 import hudson.BulkChange;
 import hudson.DescriptorExtensionList;
 import hudson.Extension;
@@ -38,12 +37,8 @@
 import hudson.model.JobProperty;
 import hudson.model.JobPropertyDescriptor;
 import hudson.model.Queue;
-import hudson.security.ACL;
-import hudson.security.AccessControlled;
 import hudson.util.FormApply;
 import java.io.IOException;
-import java.io.InvalidObjectException;
-import java.io.ObjectStreamException;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
@@ -55,14 +50,12 @@
 import jenkins.model.Jenkins;
 import jenkins.model.TransientActionFactory;
 import net.sf.json.JSONObject;
-import org.acegisecurity.AccessDeniedException;
 import org.acegisecurity.Authentication;
 import org.jenkins.ui.icon.IconSpec;
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
 import org.kohsuke.stapler.DataBoundConstructor;
 import org.kohsuke.stapler.HttpResponse;
-import org.kohsuke.stapler.Stapler;
 import org.kohsuke.stapler.StaplerRequest;
 import org.kohsuke.stapler.interceptor.RequirePOST;
 
@@ -145,44 +138,14 @@ public static void setStrategyCritical() {
         Items.XSTREAM2.addCriticalField(AuthorizeProjectProperty.class, "strategy");
     }
 
-    /**
-     * If we are being deserialized outside of loading the initial jobs (or reloading) then we need to cross check
-     * the strategy permissions to defend against somebody trying to push a configuration relating to a user other
-     * than themselves.
-     *
-     * @return {@code this}
-     * @throws ObjectStreamException if the object cannot be deserialized.
-     * @see #setStrategyCritical()
-     */
-    private Object readResolve() throws ObjectStreamException {
-        if (strategy != null) {
-            Authentication authentication = Jenkins.getAuthentication();
-            if (authentication != ACL.SYSTEM) {
-                if (ProjectQueueItemAuthenticator.isConfigured()) {
-                    StaplerRequest request = Stapler.getCurrentRequest();
-                    AccessControlled context;
-                    if (request == null) {
-                        context = Jenkins.getActiveInstance();
-                    } else {
-                        Job job = request.findAncestorObject(Job.class);
-                        context = job == null ? Jenkins.getActiveInstance() : job;
-                    }
-                    try {
-                        strategy.checkConfigurePermission(context);
-                    } catch (AccessDeniedException e) {
-                        throw new InvalidObjectException(e.getMessage());
-                    }
-                }
-            }
-        }
-        return this;
-    }
-
     /**
      * {@inheritDoc}
      */
     @Override
     public JobProperty<?> reconfigure(StaplerRequest req, JSONObject form) throws Descriptor.FormException {
+        // This is called when the job configuration is submitted.
+        // authorize-project is preserved in job configuration pages.
+        // It is updated via AuthorizationAction instead.
         return strategy != null && ProjectQueueItemAuthenticator.isConfigured() ? this : null;
     }
 
@@ -296,7 +259,7 @@ public String getIconFileName() {
          */
         @Override
         public String getDisplayName() {
-            return "Authorization";
+            return Messages.AuthorizationAction_DisplayName();
         }
 
         /**
@@ -323,10 +286,9 @@ public String getIconClassName() {
          * @throws ServletException when things go wrong.
          */
         @RequirePOST
-        @NonNull
+        @Nonnull
         @Restricted(NoExternalUse.class)
-        @SuppressWarnings("unused") // stapler web method binding
-        public synchronized HttpResponse doAuthorize(@NonNull StaplerRequest req) throws IOException, ServletException {
+        public synchronized HttpResponse doAuthorize(@Nonnull StaplerRequest req) throws IOException, ServletException {
             job.checkPermission(Job.CONFIGURE);
             JSONObject json = req.getSubmittedForm();
             JSONObject o = json.optJSONObject(getPropertyDescriptor().getJsonSafeClassName());
@@ -357,7 +319,8 @@ public synchronized HttpResponse doAuthorize(@NonNull StaplerRequest req) throws
      *
      * @since 1.3.0
      */
-    @Extension(ordinal = Double.MAX_VALUE / 2)
+    @SuppressWarnings("rawtypes")
+    @Extension(ordinal = Double.MAX_VALUE / 2)  // close to the top
     public static class TransientActionFactoryImpl extends TransientActionFactory<Job> {
 
         /**

diff --git a/src/main/java/org/jenkinsci/plugins/authorizeproject/AuthorizeProjectStrategy.java b/src/main/java/org/jenkinsci/plugins/authorizeproject/AuthorizeProjectStrategy.java
@@ -32,12 +32,18 @@
 import hudson.model.Descriptor;
 import hudson.model.Job;
 import hudson.model.Queue;
+import hudson.security.ACL;
 import hudson.security.AccessControlled;
+
+import java.io.InvalidObjectException;
+import java.io.ObjectStreamException;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import jenkins.model.Jenkins;
 import org.acegisecurity.AccessDeniedException;
 import org.acegisecurity.Authentication;
+import org.kohsuke.stapler.Stapler;
+import org.kohsuke.stapler.StaplerRequest;
 
 /**
  * Extension point to define a new strategy to authorize builds configured in project configuration pages.
@@ -110,23 +116,95 @@ public AuthorizeProjectStrategyDescriptor getDescriptor() {
      * @throws AccessDeniedException if the current user is not allowed to reconfigure the specified job
      * @since 1.3.0
      */
-    public final void checkConfigurePermission(AccessControlled context) {
-        if (!hasConfigurePermission(context)) {
-            throw new AccessDeniedException(Messages.AuthorizeProjectStrategy_UserNotAuthorized(
+    public final void checkJobConfigurePermission(AccessControlled context) {
+        if (!hasJobConfigurePermission(context)) {
+            throw new AccessDeniedException(Messages.AuthorizeProjectStrategy_UserNotAuthorizedForJob(
                     Jenkins.getAuthentication().getName()
             ));
         }
     }
 
     /**
      * Tests if the job can be reconfigured by the current user when this strategy is the configured strategy.
+     * Users with {@link Jenkins#ADMINISTER} permission skips this check.
      *
      * @param context the context of the job
      * @return {@code true} if and only if the current user is allowed to reconfigure the specified job.
      * @since 1.3.0
      */
-    public boolean hasConfigurePermission(AccessControlled context) {
+    public boolean hasJobConfigurePermission(AccessControlled context) {
         return true;
     }
 
+    /**
+     * Checks that the authorization can be configured by the current user.
+     *
+     * @param context the context of the job
+     * @throws AccessDeniedException if the current user is not allowed to configure this authorization
+     * @since 1.3.0
+     */
+    public final void checkAuthorizationConfigurePermission(AccessControlled context) {
+        if (!hasAuthorizationConfigurePermission(context)) {
+            throw new AccessDeniedException(Messages.AuthorizeProjectStrategy_UserNotAuthorized(
+                    Jenkins.getAuthentication().getName()
+            ));
+        }
+    }
+
+    /**
+     * Tests if the authorization can be configured by the current user.
+     * Users with {@link Jenkins#ADMINISTER} permission skips this check.
+     *
+     * @param context the context of the job
+     * @return {@code true} if and only if the current user is allowed to configure this authorization.
+     * @since 1.3.0
+     */
+    public boolean hasAuthorizationConfigurePermission(AccessControlled context) {
+        return true;
+    }
+
+    /**
+     * If we are being deserialized outside of loading the initial jobs (or reloading) then we need to cross check
+     * the strategy permissions to defend against somebody trying to push a configuration relating to a user other
+     * than themselves.
+     *
+     * @return {@code this}
+     * @throws ObjectStreamException if the object cannot be deserialized.
+     * @see AuthorizeProjectProperty#setStrategyCritical()
+     */
+    protected Object readResolve() throws ObjectStreamException {
+        checkUnsecuredConfiguration();
+        return this;
+    }
+
+    private void checkUnsecuredConfiguration() throws ObjectStreamException {
+        Authentication authentication = Jenkins.getAuthentication();
+        if (authentication == ACL.SYSTEM) {
+            // It is considered to initial loading or reloading.
+            return;
+        }
+        if (!ProjectQueueItemAuthenticator.isConfigured()) {
+            return;
+        }
+        if (Jenkins.getActiveInstance().hasPermission(Jenkins.ADMINISTER)) {
+            // allows any configurations by system administrators.
+            // It may not be allowed even if the user is an administrator of the job.
+            return;
+        }
+        StaplerRequest request = Stapler.getCurrentRequest();
+        AccessControlled context = (request != null) ? request.findAncestorObject(AccessControlled.class) : null;
+        if (context == null) {
+            context = Jenkins.getActiveInstance();
+        }
+        try {
+            checkJobConfigurePermission(context);
+        } catch (AccessDeniedException e) {
+            throw new InvalidObjectException(e.getMessage());
+        }
+        try {
+            checkAuthorizationConfigurePermission(context);
+        } catch (AccessDeniedException e) {
+            throw new InvalidObjectException(e.getMessage());
+        }
+    }
 }
diff --git a/src/main/java/org/jenkinsci/plugins/authorizeproject/AuthorizeProjectStrategyDescriptor.java b/src/main/java/org/jenkinsci/plugins/authorizeproject/AuthorizeProjectStrategyDescriptor.java
@@ -125,16 +125,4 @@ public boolean isEnabledByDefault() {
     public boolean isApplicableToGlobal() {
         return true;
     }
-
-    protected void prepareNewInstance(StaplerRequest req) {
-
-    }
-
-    /**
-     *
-     */
-    protected void finalizeNewInstance(StaplerRequest req) {
-
-    }
-
 }
diff --git a/src/main/java/org/jenkinsci/plugins/authorizeproject/ConfigurationPermissionEnforcer.java b/src/main/java/org/jenkinsci/plugins/authorizeproject/ConfigurationPermissionEnforcer.java
@@ -4,7 +4,12 @@
 import hudson.model.Job;
 import hudson.model.JobProperty;
 import hudson.model.JobPropertyDescriptor;
+import hudson.security.AccessControlled;
+import jenkins.model.Jenkins;
 import net.sf.json.JSONObject;
+
+import javax.annotation.CheckForNull;
+
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
 import org.kohsuke.stapler.DataBoundConstructor;
@@ -46,17 +51,38 @@ public String getDisplayName() {
         @Override
         public JobProperty<?> newInstance(StaplerRequest req, JSONObject formData) throws FormException {
             Job<?,?> job = req.findAncestorObject(Job.class);
-            if (job != null) {
-                AuthorizeProjectProperty property = job.getProperty(AuthorizeProjectProperty.class);
-                if (property != null && ProjectQueueItemAuthenticator.isConfigured()) {
-                    AuthorizeProjectStrategy strategy = property.getStrategy();
-                    if (strategy != null) {
-                        strategy.checkConfigurePermission(job);
-                    }
-                }
-            }
+            AccessControlled context = req.findAncestorObject(AccessControlled.class);
+            checkConfigurePermission(job, context);
             // we don't actually return a job property... just want to be called on every form submission.
             return null;
         }
+
+        private void checkConfigurePermission(@CheckForNull Job<?, ?> job, @CheckForNull AccessControlled context) {
+            if (job == null) {
+                return;
+            }
+            if (context == null) {
+                // this should not happen.
+                context = job;
+            }
+            if (Jenkins.getActiveInstance().hasPermission(Jenkins.ADMINISTER)) {
+                // allows any configurations by system administrators.
+                // It may not be allowed even if the user is an administrator of the job,
+                // 
+                return;
+            }
+            AuthorizeProjectProperty property = job.getProperty(AuthorizeProjectProperty.class);
+            if (property == null) {
+                return;
+            }
+            if (!ProjectQueueItemAuthenticator.isConfigured()) {
+                return;
+            }
+            AuthorizeProjectStrategy strategy = property.getStrategy();
+            if (strategy == null) {
+                return;
+            }
+            strategy.checkJobConfigurePermission(context);
+        }
     }
 }
diff --git a/src/main/java/org/jenkinsci/plugins/authorizeproject/ProjectQueueItemAuthenticator.java b/src/main/java/org/jenkinsci/plugins/authorizeproject/ProjectQueueItemAuthenticator.java
@@ -41,10 +41,8 @@
 import net.sf.json.JSONObject;
 
 import org.acegisecurity.Authentication;
-import org.kohsuke.stapler.DataBoundConstructor;
 import org.kohsuke.stapler.StaplerRequest;
 
-import jenkins.model.Jenkins;
 import jenkins.security.QueueItemAuthenticatorConfiguration;
 import jenkins.security.QueueItemAuthenticatorDescriptor;
 import jenkins.security.QueueItemAuthenticator;
@@ -67,7 +65,7 @@ public ProjectQueueItemAuthenticator(Map<String,Boolean> strategyEnabledMap) {
         this.strategyEnabledMap = strategyEnabledMap;
     }
 
-    public Object readResolve() {
+    protected Object readResolve() {
         if(strategyEnabledMap == null) {
             return new ProjectQueueItemAuthenticator(Collections.<String, Boolean>emptyMap());
         }