[JENKINS-49486] Enable variable expansion for ManualCondition.users (approvers)

src/main/java/hudson/plugins/promoted_builds/PromotedBuildAction.java

@@ -155,7 +155,7 @@ public boolean canPromote(String processName) {
         if (process != null) {
             manualCondition = (ManualCondition) process.getPromotionCondition(ManualCondition.class.getName());
         }
-        return PromotionPermissionHelper.hasPermission(getProject(), manualCondition);
+        return PromotionPermissionHelper.hasPermission(getProject(), owner, manualCondition);
     }
 
     /**
@@ -239,7 +239,7 @@ public HttpResponse doForcePromotion(@QueryParameter("name") String name) throws
             throw new IllegalStateException("This project doesn't have the promotion criterion called "+name);
 
         ManualCondition manualCondition = (ManualCondition) p.getPromotionCondition(ManualCondition.class.getName());
-        PromotionPermissionHelper.checkPermission(getProject(), manualCondition);
+        PromotionPermissionHelper.checkPermission(getProject(), owner, manualCondition);
 
         p.promote(owner,new UserCause(),new ManualPromotionBadge());
 

src/main/java/hudson/plugins/promoted_builds/PromotionPermissionHelper.java

@@ -24,6 +24,7 @@
 
 package hudson.plugins.promoted_builds;
 
+import hudson.model.AbstractBuild;
 import hudson.model.AbstractProject;
 import hudson.plugins.promoted_builds.conditions.ManualCondition;
 import hudson.security.AccessDeniedException2;
@@ -40,19 +41,19 @@
 @Restricted(NoExternalUse.class)
 public class PromotionPermissionHelper {
 
-    public static void checkPermission(@Nonnull AbstractProject<?,?> target, @CheckForNull ManualCondition associatedCondition) {
-        if (!hasPermission(target, associatedCondition)) {
+    public static void checkPermission(@Nonnull AbstractProject<?,?> target, @Nonnull AbstractBuild<?,?> build, @CheckForNull ManualCondition associatedCondition) {
+        if (!hasPermission(target, build, associatedCondition)) {
             // TODO: Give a more accurate error message if the user has Promotion.PROMOTE but is not in the list of approvers.
             throw new AccessDeniedException2(Jenkins.getAuthentication(), Promotion.PROMOTE);
         }
     }
 
-    public static boolean hasPermission(@Nonnull AbstractProject<?,?> target, @CheckForNull ManualCondition associatedCondition) {
+    public static boolean hasPermission(@Nonnull AbstractProject<?,?> target, @Nonnull AbstractBuild<?,?> build, @CheckForNull ManualCondition associatedCondition) {
         if (associatedCondition == null) {
             return target.hasPermission(Promotion.PROMOTE);
-        } else if (associatedCondition.getUsersAsSet().isEmpty()) {
+        } else if (associatedCondition.getUsersAsSet(build).isEmpty()) {
             return target.hasPermission(Promotion.PROMOTE);
-        } else if (associatedCondition.isInUsersList() || associatedCondition.isInGroupList()) {
+        } else if (associatedCondition.isInUsersList(build) || associatedCondition.isInGroupList(build)) {
             // Explicitly listed users do not need Promotion/Promote permissions.
             return true;
         } else {

src/main/java/hudson/plugins/promoted_builds/Status.java

@@ -375,7 +375,7 @@ public boolean canBuild() {
         }
 
         ManualCondition manualCondition = (ManualCondition) process.getPromotionCondition(ManualCondition.class.getName());
-        return PromotionPermissionHelper.hasPermission(target.getProject(), manualCondition);
+        return PromotionPermissionHelper.hasPermission(target.getProject(), target, manualCondition);
     }
 
     /**
@@ -399,7 +399,7 @@ public void doBuild(StaplerRequest req, StaplerResponse rsp) throws IOException,
 
         ManualCondition manualCondition = (ManualCondition) process.getPromotionCondition(ManualCondition.class.getName());     
         // TODO: Use PromotionPermissionHelper.checkPermission instead, but consider issues with backwards compatibility.
-        if (!PromotionPermissionHelper.hasPermission(target.getProject(), manualCondition)) {
+        if (!PromotionPermissionHelper.hasPermission(target.getProject(), target, manualCondition)) {
             return;
         }
 

src/main/java/hudson/plugins/promoted_builds/conditions/ManualCondition.java

@@ -31,6 +31,8 @@
 import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 import javax.annotation.CheckForNull;
 import javax.annotation.Nonnull;
 
@@ -54,6 +56,7 @@
 public class ManualCondition extends PromotionCondition {
     private String users;
     private List<ParameterDefinition> parameterDefinitions = new ArrayList<ParameterDefinition>();
+    private static final Logger LOGGER = Logger.getLogger(ManualCondition.class.getName());
 
     /**
      * 
@@ -96,17 +99,31 @@ public ParameterDefinition getParameterDefinition(String name) {
     }
 
     public Set<String> getUsersAsSet() {
-        if (users == null || users.equals(""))
+        return getUsersAsSet(null);
+    }
+
+    public Set<String> getUsersAsSet(AbstractBuild<?,?> build) {
+        if (users == null || users.equals("") || build == null)
             return Collections.emptySet();
 
         Set<String> set = new HashSet<String>();
         for (String user : users.split(",")) {
             user = user.trim();
 
-            if (user.trim().length() > 0)
+            LOGGER.log(Level.FINER, "ManualCondition.getUsersAsSet(): user={0}", user);
+            if (user.startsWith("$")){
+                user = user.substring(1);
+                if (user.startsWith("{") && user.endsWith("}"))
+                    user = user.substring(1, user.length() -1);
+                LOGGER.log(Level.INFO, "ManualCondition.getUsersAsSet(): build.getBuildVariableResolver().resolve(user=${0})={1}", 
+                        new Object [] {user, build.getBuildVariableResolver().resolve(user)});
+                user = build.getBuildVariableResolver().resolve(user);
+            }
+            if (user != null && user.trim().length() > 0)
                 set.add(user);
         }
 
+        LOGGER.log(Level.INFO, "ManualCondition.getUsersAsSet(): return set={0}", set);
         return set;
     }
 
@@ -129,7 +146,7 @@ public PromotionBadge isMet(PromotionProcess promotionProcess, AbstractBuild<?,?
      * approved.
      */
     public boolean canApprove(PromotionProcess promotionProcess, AbstractBuild<?,?> build) {
-        if (!PromotionPermissionHelper.hasPermission(build.getProject(), this)) {
+        if (!PromotionPermissionHelper.hasPermission(build.getProject(), build, this)) {
             return false;
         }
 
@@ -150,16 +167,24 @@ public boolean canApprove(PromotionProcess promotionProcess, AbstractBuild<?,?>
      * @since 2.24 
      */
     public boolean isInUsersList() {
+        return isInUsersList(null);
+    }
+
+    public boolean isInUsersList(AbstractBuild<?,?> build) {
         // Current user must be in users list or users list is empty
-        Set<String> usersSet = getUsersAsSet();
+        Set<String> usersSet = getUsersAsSet(build);
         return usersSet.contains(Hudson.getAuthentication().getName());
     }
 
     /*
      * Check if user is a member of a groups as listed in the user / group field
      */
     public boolean isInGroupList() {
-        Set<String> groups = getUsersAsSet();
+        return isInGroupList(null);
+    }
+
+    public boolean isInGroupList(AbstractBuild<?,?> build) {
+        Set<String> groups = getUsersAsSet(build);
         GrantedAuthority[] authorities = Hudson.getAuthentication().getAuthorities();
         for (GrantedAuthority authority : authorities) {
             if (groups.contains(authority.getAuthority()))

src/main/resources/hudson/plugins/promoted_builds/conditions/ManualCondition/index.jelly

@@ -4,7 +4,7 @@
     <f:form method="post" action="promotionProcess/${h.encode(p.name)}/promotionCondition/hudson.plugins.promoted_builds.conditions.ManualCondition/approve" name="approve">
       <j:if test="${!it.usersAsSet.isEmpty()}">
         <f:entry title="${%Approvers}" description="${%List of users or groups that can approve this promotion}">
-          <f:textbox value="${it.users}" readonly="true"/>
+          <f:textbox value="${it.getUsersAsSet(pba.owner)}" readonly="true"/>
 	</f:entry>
       </j:if>
       <j:if test="${it.canApprove(p, pba.owner)}">

src/test/java/hudson/plugins/promoted_builds/conditions/ManualConditionTest.java

@@ -2,14 +2,17 @@
 
 import hudson.ExtensionList;
 import hudson.model.AbstractProject;
+import hudson.model.Cause;
 import hudson.model.FreeStyleBuild;
 import hudson.model.Item;
+import hudson.model.ParametersAction;
 import hudson.model.ParameterDefinition;
 import hudson.model.ParameterValue;
 import hudson.model.Result;
 import hudson.model.Descriptor;
 import hudson.model.FreeStyleProject;
 import hudson.model.StringParameterDefinition;
+import hudson.model.StringParameterValue;
 import hudson.model.User;
 import hudson.plugins.promoted_builds.JobPropertyImpl;
 import hudson.plugins.promoted_builds.PromotedBuildAction;
@@ -263,6 +266,25 @@ public void testManualPromotionPermissions() throws Exception {
                     approval, notNullValue());
         }
 
+        {
+            // Approvers specified in varaible, user is approver, but does not have Promotion/Promote
+            cond.setUsers("${approver}");
+            List<ParameterValue> paramsList = new ArrayList<ParameterValue>();
+            paramsList.add(new StringParameterValue("approver", "non-promoter"));
+            //FreeStyleBuild b = j.assertBuildStatusSuccess(p.scheduleBuild2(0));
+            FreeStyleBuild b = j.assertBuildStatusSuccess(
+                    p.scheduleBuild2(
+                            0,
+                            new Cause.RemoteCause("test-host", ""),
+                            new ParametersAction(paramsList)));
+            SecurityContext previous = ACL.impersonate(User.get("non-promoter").impersonate());
+            j.assertBuildStatusSuccess(cond.approve(b, pp, Collections.EMPTY_LIST));
+            ACL.impersonate(previous.getAuthentication());
+            ManualApproval approval = b.getAction(ManualApproval.class);
+            assertThat("If users are specified in variable, then users in that list should be able to approve even without Promotion/Promote permissions",
+                    approval, notNullValue());
+        }
+
         {
             // Approvers specified, user is not approver, but does have Promotion/Promote
             cond.setUsers("non-promoter");
@@ -283,7 +305,14 @@ public void testManualPromotionPermissionsViaWebClient() throws Exception {
         PromotionProcess pp = addPromotionProcess(p, "foo");
         WebClient wc = j.createWebClient();
 
-        FreeStyleBuild b = j.assertBuildStatusSuccess(p.scheduleBuild2(0));
+        List<ParameterValue> paramsList = new ArrayList<ParameterValue>();
+        paramsList.add(new StringParameterValue("approver", "promoter"));
+        //FreeStyleBuild b = j.assertBuildStatusSuccess(p.scheduleBuild2(0));
+        FreeStyleBuild b = j.assertBuildStatusSuccess(
+                p.scheduleBuild2(
+                        0,
+                        new Cause.RemoteCause("test-host", ""),
+                        new ParametersAction(paramsList)));
         ManualCondition cond = new ManualCondition();
         pp.conditions.add(cond);
         j.assertBuildStatusSuccess(cond.approve(b, pp, Collections.EMPTY_LIST));
@@ -322,13 +351,26 @@ public void testManualPromotionPermissionsViaWebClient() throws Exception {
             assertThat(waitForBuildByNumber(pp, 3).getResult(), equalTo(Result.SUCCESS));
         }
 
+        {
+            // Re-execute promotion as specified user in varaible without Promotion/Promote
+            cond.setUsers("${approver}");
+            wc.login("non-promoter", "non-promoter");
+            try {
+                wc.getPage(b, String.format("promotion/%s/build?json={}", pp.getName()));
+                fail();
+            } catch (FailingHttpStatusCodeException e) {
+                assertThat(e.getStatusCode(), equalTo(404)); // Redirect after the build is broken.
+            }
+            assertThat(waitForBuildByNumber(pp, 4).getResult(), equalTo(Result.SUCCESS));
+        }
+
         {
             // Re-execute promotion as unspecified user with Promotion/Promote
             cond.setUsers("non-promoter");
             wc.login("promoter", "promoter");
             // Status#doBuild does a bare `return;` without scheduling the build in this case, which is why we use goTo with "" for the MIME type.
             wc.goTo(String.format("job/%s/%d/promotion/%s/build?json={}", p.getName(), b.getNumber(), pp.getName()), "");
-            assertThat(pp.getBuildByNumber(4), nullValue());
+            assertThat(pp.getBuildByNumber(5), nullValue());
         }
     }