diff --git a/src/main/java/hudson/plugins/promoted_builds/PromotedBuildAction.java b/src/main/java/hudson/plugins/promoted_builds/PromotedBuildAction.java index a1f4b20e..656d0d10 100644 --- a/src/main/java/hudson/plugins/promoted_builds/PromotedBuildAction.java +++ b/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()); diff --git a/src/main/java/hudson/plugins/promoted_builds/PromotionPermissionHelper.java b/src/main/java/hudson/plugins/promoted_builds/PromotionPermissionHelper.java index 9be534da..26d06839 100644 --- a/src/main/java/hudson/plugins/promoted_builds/PromotionPermissionHelper.java +++ b/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 { diff --git a/src/main/java/hudson/plugins/promoted_builds/Status.java b/src/main/java/hudson/plugins/promoted_builds/Status.java index e4493ff6..7f1a01d3 100644 --- a/src/main/java/hudson/plugins/promoted_builds/Status.java +++ b/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; } diff --git a/src/main/java/hudson/plugins/promoted_builds/conditions/ManualCondition.java b/src/main/java/hudson/plugins/promoted_builds/conditions/ManualCondition.java index 6bdeab90..aba604ac 100644 --- a/src/main/java/hudson/plugins/promoted_builds/conditions/ManualCondition.java +++ b/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 parameterDefinitions = new ArrayList(); + private static final Logger LOGGER = Logger.getLogger(ManualCondition.class.getName()); /** * @@ -96,6 +99,10 @@ public ParameterDefinition getParameterDefinition(String name) { } public Set getUsersAsSet() { + return getUsersAsSet(null); + } + + public Set getUsersAsSet(AbstractBuild build) { if (users == null || users.equals("")) return Collections.emptySet(); @@ -103,10 +110,20 @@ public Set getUsersAsSet() { 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 build) { - if (!PromotionPermissionHelper.hasPermission(build.getProject(), this)) { + if (!PromotionPermissionHelper.hasPermission(build.getProject(), build, this)) { return false; } @@ -150,8 +167,12 @@ 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 usersSet = getUsersAsSet(); + Set usersSet = getUsersAsSet(build); return usersSet.contains(Hudson.getAuthentication().getName()); } @@ -159,7 +180,11 @@ public boolean isInUsersList() { * Check if user is a member of a groups as listed in the user / group field */ public boolean isInGroupList() { - Set groups = getUsersAsSet(); + return isInGroupList(null); + } + + public boolean isInGroupList(AbstractBuild build) { + Set groups = getUsersAsSet(build); GrantedAuthority[] authorities = Hudson.getAuthentication().getAuthorities(); for (GrantedAuthority authority : authorities) { if (groups.contains(authority.getAuthority())) diff --git a/src/main/resources/hudson/plugins/promoted_builds/conditions/ManualCondition/index.jelly b/src/main/resources/hudson/plugins/promoted_builds/conditions/ManualCondition/index.jelly index bf15935d..fc5abc55 100644 --- a/src/main/resources/hudson/plugins/promoted_builds/conditions/ManualCondition/index.jelly +++ b/src/main/resources/hudson/plugins/promoted_builds/conditions/ManualCondition/index.jelly @@ -4,7 +4,7 @@ - + diff --git a/src/test/java/hudson/plugins/promoted_builds/conditions/ManualConditionTest.java b/src/test/java/hudson/plugins/promoted_builds/conditions/ManualConditionTest.java index df36d9ca..a4462bd0 100644 --- a/src/test/java/hudson/plugins/promoted_builds/conditions/ManualConditionTest.java +++ b/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 paramsList = new ArrayList(); + 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 paramsList = new ArrayList(); + 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))); 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()); } }