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

totoroliu · 2018-03-09T17:55:36Z

Hi,

I'm trying to develop for Jenkins-49486,
but this is my first time implementing a feature for a Jenkins plugin.
My plan is to update Approver field for each build
by doing TokenMacro.expandAll() for each of comma delimited user
from String ManualCondition.users at the end of each build.

After bring in RunListenerImpl.onCompleted(),
I'm able to expand Approver field when a build is copmleted.
However, now it's not only updating the build's Approver,
but also update the Jenkins job's configurations.
Hence, at the end of a build,
the job configuration's Approver variable
would be expanded to be hardcoded Approver accroding to my last build result.

I'm just wondering if any one could point out some direction for me.
Thanks!

dwnusbaum

Thanks for your PR! There are some issues in your current approach, but with some changes a feature like this might be able to work.

I am not exactly sure the best way to go about implementing this feature, but maybe one way to approach it would be to edit the methods in PromotionPermissionHelper.java to take an extra AbstractBuild<?,?> build parameter and use that to expand the values when checking for permissions. You'd also have to do something about ManualCondition#isInUsersList and ManualCondition#isInGroupList so that they would be able to handle variables in the users list correctly.

dwnusbaum · 2018-03-20T14:46:57Z

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

@@ -52,8 +59,9 @@
 * @author Peter Hayes
 */
 public class ManualCondition extends PromotionCondition {
-    private String users;
+    private static String users;


By making this static, you have forced all ManualConditions to use the same list of users, which is causing the issue noted in your email to jenkinsci-dev.

thank you very much for the direction!
I'll give it a try.

totoroliu · 2018-03-20T22:42:20Z

Hi @dwnusbaum,

I updated my pull-request based on your suggestion.
I did some initial testing,
and able to expand variable (in $XXX or ${XXX} form)

But I still miss one thing.
The promotion page (${BUILD_URL}/promotion)
that displays Manual Approval - Approvers field
would still shows the original form (the variable name).

How can I update the promotion page to show approvers
based on the result of expanded users?

totoroliu · 2018-03-20T23:20:05Z

@dwnusbaum

I updated src/main/resources/hudson/plugins/promoted_builds/conditions/ManualCondition/index.jelly
to use ${it.getUsersAsSet(pba.owner)} for promotion Manual Approval - Approvers field.

Since getUsersAsSet is set,
now approvers field would have 2 extra [ ].

I tried to use "${String.join(',', it.getUsersAsSet(pba.owner))}"
to join the set as a String,
but the dispaly result becomes empty.

How can I properly call Java function in Stapler?

I think once I got this issue resolved,
JENKINS-49486 should be ready to be pulled.

dwnusbaum · 2018-03-21T17:20:12Z

@totoroliu I'll try to take a look at the new changes sometime this week, thanks!

totoroliu · 2018-08-13T17:08:47Z

any update on this PR?

dwnusbaum

HI @totoroliu! I am sorry that I was not able to review this sooner. It looks ok in general to me, but I have a few comments.

The only big thing I think the PR still needs is a few tests for the new behavior to confirm that it works as expected and so that it doesn't regress in the future. You should be able to add new tests to ManualConditionTest.java that are very similar to the tests already in that file.

CC @oleg-nenashev: It would be great if you could take a look and confirm whether the feature/implementation make sense to you.

dwnusbaum · 2018-08-13T17:36:54Z

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

+            LOGGER.log(Level.FINER, "ManualCondition.getUsersAsSet(): user={0}", user);
+            if (user.startsWith("$")){
+                user = user.substring(1);
+                if (user.startsWith("{") && user.endsWith("}"))


I would prefer to allow either $NAME or ${NAME}, but not both. What happens if there is a user named $NAME? I don't think users can have $ in their name after a recent security fix, but I am not 100% sure.

the reason I allow both $NAME and ${NAME} because
Email-ext plugin and token-macro plugin support these 2 syntax.

But if you insist,
I could allow only one of the case,
and which form do you prefert to support?

If email-ext and token-macro support both, then it probably makes sense to support both here as well. I'd leave it the way you have it for now.

dwnusbaum · 2018-08-13T17:38:48Z

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

            // 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, AbstractBuild<?,?> build, @CheckForNull ManualCondition associatedCondition) {


It would be good to add either @CheckForNull or @Nonnull to the parameter for clarity (same for the other places where a parameter is added).

dwnusbaum · 2018-08-13T17:41:37Z

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

@@ -95,18 +98,28 @@ public ParameterDefinition getParameterDefinition(String name) {
        return null;
    }

-    public Set<String> getUsersAsSet() {
+    public Set<String> getUsersAsSet(AbstractBuild<?,?> build) {


Breaks binary compatibility, although I don't see any other callers based on a GitHub search of the jenkinsci organization, so it might not matter. We could always be safe and keep the previous method around as well, and have it call the new method passing null for the new parameter.

Same issue with isInUsersList and isInGroupList .

oleg-nenashev · 2018-08-13T18:21:54Z

will try to review it this week, ping me if I don't

dwnusbaum · 2018-08-13T18:45:20Z

Also just a note: For any job with a parameterized approver for its manual promotion condition, all Job/Build users would be able to allow anyone to promote the job. Previously, only Job/Configure users were able to edit the list and allow anyone to promote the job, so this feature definitely needs to be used carefully.

@totoroliu In your environment, which users would be configuring/building/promoting, and what permissions would they have? It would be good to make sure we are all on the same page and that the permission model makes sense for real use-cases.

totoroliu · 2018-08-13T19:07:16Z

I'm using LDAP authentication with Role-based permission control.
so far a couple admin users are using this feature.

dwnusbaum · 2018-08-13T19:30:35Z

Please don't force-push, it makes it difficult to review the PR since we cannot tell what changed each time and have to review everything from scratch. It's ok to have a ton of commits here, we can always squash-merge the PR if it gets messy.

Enable variable expansion for ManualCondition.users (approvers) by adding parameters 'AbstractBuild<?,?> build' to methods in PromotionPermissionHelper.java and ManualCondition.getUsersAsSet()

totoroliu · 2018-08-14T17:44:42Z

i'm sorry for the trouble.
i'll stop using force-push.

totoroliu · 2018-08-14T19:00:12Z

In ManualConditionTest.java,
how to set a build variable?

My plan is:
In testManualPromotionPermissionsViaWebClient(),
wc.login("promoter", "promoter"),
then set build variable approved_approver = "promoter",
and put ${approved_approver} as manual approver.

dwnusbaum · 2018-08-14T19:07:40Z

I think that you would replace a line like the following:

FreeStyleBuild b = j.assertBuildStatusSuccess(p.scheduleBuild2(0));

with the following (although I have not tested it):

FreeStyleBuild b = j.assertBuildStatusSuccess(p.scheduleBuild2(0, 
        new ParametersAction(new StringParameterValue("name", "value"))));

…n.users (approvers)

totoroliu · 2018-08-15T17:48:43Z

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

+            }
+            assertThat(waitForBuildByNumber(pp, 4).getResult(), equalTo(Result.SUCCESS));
+        }
+


@dwnusbaum
Do you know why this webclient test fail?

At a glance, no, but you could use a debugger and step through the test/plugin code to figure out why it is failing before removing it :)

oleg-nenashev

AFAICT there is a bug in the current logic implementation. Regarding the use-case itself, it looks reasonable though it makes the job developer responsible for all possible security concerns related to the variable resolution.

Before merging the PR, you need to ensure that the patch does not allow exposing sensitive environment variable values to the system log or web UI. From what I see, it does it in the current implementation

oleg-nenashev · 2018-08-17T09:29:15Z

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

+    }
+
+    public Set<String> getUsersAsSet(AbstractBuild<?,?> build) {
+        if (users == null || users.equals("") || build == null)


This condition is not correct from what I see.
When build is null, the result will be always empty.
E.g. getUsersAsSet() will be always returning empty list after the change

What sensitive environment variable is exposing right now?
Could you give me some examples?

dwnusbaum · 2018-08-17T20:45:47Z

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

        if (users == null || users.equals(""))
            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("$")){


You still need to null-check build somewhere before using it below. I think it makes sense to do it here: if (build != null && user.startsWith("$")). That way if the the build is null, we don't try to substitute parameterized names, and just treat them as actual usernames.

dwnusbaum · 2018-08-17T20:46:53Z

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

+                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}", 


Level.INFO is visible by default in Jenkins, so I would move this and the other call to LOGGER.log to Level.FINER or similar.

dwnusbaum requested changes Mar 20, 2018

View reviewed changes

dwnusbaum changed the title ~~JENKINS-49486: Add class RunListenerImpl to pick up completions of a …~~ [JENKINS-49486] Add class RunListenerImpl to pick up completions of a … Mar 20, 2018

totoroliu force-pushed the JENKINS-49486-initial branch 3 times, most recently from 748f171 to e73582d Compare March 20, 2018 22:37

totoroliu force-pushed the JENKINS-49486-initial branch from e73582d to fbddb91 Compare March 20, 2018 23:14

totoroliu force-pushed the JENKINS-49486-initial branch from fbddb91 to 163f07c Compare March 20, 2018 23:54

totoroliu changed the title ~~[JENKINS-49486] Add class RunListenerImpl to pick up completions of a …~~ [JENKINS-49486] Enable variable expansion for ManualCondition.users (approvers) Mar 22, 2018

dwnusbaum self-requested a review April 5, 2018 13:19

totoroliu force-pushed the JENKINS-49486-initial branch from 163f07c to 5394488 Compare April 18, 2018 16:53

dwnusbaum reviewed Aug 13, 2018

View reviewed changes

oleg-nenashev self-requested a review August 13, 2018 18:21

totoroliu force-pushed the JENKINS-49486-initial branch from 52a1215 to ba93574 Compare August 13, 2018 18:53

totoroliu force-pushed the JENKINS-49486-initial branch from ba93574 to a3e8f84 Compare August 13, 2018 19:28

JENKINS-49486:

40964fd

Enable variable expansion for ManualCondition.users (approvers) by adding parameters 'AbstractBuild<?,?> build' to methods in PromotionPermissionHelper.java and ManualCondition.getUsersAsSet()

totoroliu force-pushed the JENKINS-49486-initial branch from a3e8f84 to 40964fd Compare August 13, 2018 19:42

dwnusbaum self-requested a review August 13, 2018 21:23

JENKINS-49486: add test case for variable expansion in ManualConditio…

cb62791

…n.users (approvers)

totoroliu commented Aug 15, 2018

View reviewed changes

Rick Liu added 2 commits August 16, 2018 09:41

Remove failed test case

4ba97ce

Add back and fix the test case

287d420

oleg-nenashev requested changes Aug 17, 2018

View reviewed changes

Take out build==null condition in getUserAsSet

8a4433a

dwnusbaum reviewed Aug 17, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

totoroliu commented Mar 9, 2018 •

edited

Loading

dwnusbaum left a comment •

edited

Loading

dwnusbaum Mar 20, 2018 •

edited

Loading

totoroliu Mar 20, 2018

totoroliu commented Mar 20, 2018

totoroliu commented Mar 20, 2018

dwnusbaum commented Mar 21, 2018

totoroliu commented Aug 13, 2018

dwnusbaum left a comment

dwnusbaum Aug 13, 2018

totoroliu Aug 13, 2018

dwnusbaum Aug 13, 2018

dwnusbaum Aug 13, 2018

dwnusbaum Aug 13, 2018 •

edited

Loading

oleg-nenashev commented Aug 13, 2018

dwnusbaum commented Aug 13, 2018

totoroliu commented Aug 13, 2018

dwnusbaum commented Aug 13, 2018 •

edited

Loading

totoroliu commented Aug 14, 2018

totoroliu commented Aug 14, 2018

dwnusbaum commented Aug 14, 2018 •

edited

Loading

totoroliu Aug 15, 2018

dwnusbaum Aug 16, 2018 •

edited

Loading

oleg-nenashev left a comment

oleg-nenashev Aug 17, 2018

totoroliu Aug 17, 2018

dwnusbaum Aug 17, 2018

dwnusbaum Aug 17, 2018

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

Are you sure you want to change the base?

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

Conversation

totoroliu commented Mar 9, 2018 • edited Loading

dwnusbaum left a comment • edited Loading

Choose a reason for hiding this comment

dwnusbaum Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

totoroliu commented Mar 20, 2018

totoroliu commented Mar 20, 2018

dwnusbaum commented Mar 21, 2018

totoroliu commented Aug 13, 2018

dwnusbaum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwnusbaum Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

oleg-nenashev commented Aug 13, 2018

dwnusbaum commented Aug 13, 2018

totoroliu commented Aug 13, 2018

dwnusbaum commented Aug 13, 2018 • edited Loading

totoroliu commented Aug 14, 2018

totoroliu commented Aug 14, 2018

dwnusbaum commented Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

dwnusbaum Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

oleg-nenashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

totoroliu commented Mar 9, 2018 •

edited

Loading

dwnusbaum left a comment •

edited

Loading

dwnusbaum Mar 20, 2018 •

edited

Loading

dwnusbaum Aug 13, 2018 •

edited

Loading

dwnusbaum commented Aug 13, 2018 •

edited

Loading

dwnusbaum commented Aug 14, 2018 •

edited

Loading

dwnusbaum Aug 16, 2018 •

edited

Loading