Add configurable minimum permissions for triggers #60

pascal-hofmann · 2023-03-03T14:27:33Z

For private repositories people with Read role (aka pull permission) are seen as collaborators
by the GitHub API. This means, they were able trigger builds with older versions of the plugin.

This change fixes this, and also adds the option to limit triggering of builds to repository admins.

The existing allowUntrusted toggle is dropped from the UI.
This change adds a new drop-down Minimum Permissions on repository to trigger the build with these options:

Only users with admin permission
Only users that can push to the repository (default)
Allow untrusted users to trigger the build

Use-case for the "admin" part:

We have PR builds in our organization that need access to credentials with a high blast radius.
For repos with these builds all developers in the organisation have write access, but merging to master is restricted to admins (via .github/CODEOWNERS).
Allowing everyone to trigger builds is dangerous, as the Jenkinsfile and other scripts that are used in the build could be modified by people with bad intend / hacked accounts. So we also want to be able to limit triggering of builds to admins only.

pascal-hofmann · 2023-03-03T16:52:06Z

@bluesliverx It would be great to get this merged/released soon.

Our GitHub org has a lot of users, but we trust only a subset of them for certain repositories.

lprimak · 2023-03-03T23:54:30Z

@pascal-hofmann I am not sure I understand any reason for this change.
The "allow untrusted builds" no longer makes any sense, and creates a combinatorial explosion
with the dropdown box which makes no sense and is very confusing.

For example, "NONE" equals "allow untrusted builds" and wording is confusing.
"READ" makes no sense to me because if there are no "READ" permissions, the user wouldn't
be able to clone nor comment on the PR anyway.
"ADMIN" may make sense but it's really a stretch, I can't think of a single use case for it.

Am I not understanding something critical here?
What's the exact issue that triggered the need for this change?
Can you create a ticket that explains the issue in detail?

lprimak

See comments on this PR. It doesn't make sense to me

src/main/java/com/adobe/jenkins/github_pr_comment_build/TriggerPRReviewBranchProperty.java

pascal-hofmann · 2023-03-04T10:58:29Z

Hi,
thanks for the quick and honest feedback. I'll ~~create an issue with more details and will also~~ update this PR.

Edit: The existing issue types don't make much sense and request a lot of information that's not relevant. I'll update the PR description / commit message instead.

pascal-hofmann · 2023-03-04T11:28:59Z

@lprimak I updated the PR description to make the intent/reasoning behind this PR clearer.

PS: I converted the PR to a draft because I was not able to test the latest code changes yet. I'll also add an updated screenshot once this is done.

lprimak · 2023-03-04T17:39:41Z

Ok, I can see the issue with private repositories.
I think your solution should work and not break anything.
Please create a ticket nonetheless (you can delete the template if you wish)

pascal-hofmann · 2023-03-05T10:43:46Z

I opened #61 for this.

lprimak · 2023-03-16T01:44:46Z

@pascal-hofmann I already reviewed it (repeated code comments, etc)
I realize there is already repeated code, but I would rather not have any more repeated code on top of that.
Thank you

pascal-hofmann · 2023-03-16T08:16:10Z

~~@lprimak I just tried creating an abstract base class AbstractTriggerBranchProperty but I can also see this as selectable in the jenkins UI. Do you know how to get rid of this?~~

Edit: Found it and pushed the refactored version.

For private repositories people with `Read` role (aka `pull` permission) are seen as collaborators by the GitHub API. This means, they were able trigger builds with older versions of the plugin. This change fixes this, and also adds the option to limit triggering of builds to repository admins. The existing `allowUntrusted` toggle is dropped from the UI. This change adds a new drop-down `Minimum Permissions on repository to trigger the build` with these options: - Only users with admin permission - Only users that can push to the repository (default) - Allow untrusted users to trigger the build

lprimak · 2023-03-03T23:58:20Z

src/main/java/com/adobe/jenkins/github_pr_comment_build/TriggerPRUpdateBranchProperty.java

+         *
+         * @return the strategy options.
+         */
+        @NonNull


Repeated code. Needs refactor

lprimak · 2023-03-03T23:58:31Z

src/main/java/com/adobe/jenkins/github_pr_comment_build/TriggerPRCommentBranchProperty.java

+         *
+         * @return the strategy options.
+         */
+        @NonNull


Repeated code. Needs refactor

lprimak · 2023-03-04T17:35:39Z

src/main/java/com/adobe/jenkins/github_pr_comment_build/TriggerPRReviewBranchProperty.java

@@ -14,12 +19,13 @@
 */
 public class TriggerPRReviewBranchProperty extends BranchProperty {
    private boolean allowUntrusted;


Needs to be removed along with it's getters and setters

lprimak · 2023-03-04T17:35:53Z

src/main/java/com/adobe/jenkins/github_pr_comment_build/TriggerPRReviewBranchProperty.java

@@ -30,6 +36,18 @@ public void setAllowUntrusted(boolean allowUntrusted) {
        this.allowUntrusted = allowUntrusted;


Needs to be removed

lprimak · 2023-03-16T15:19:39Z

Can we just remove "allow unstrusted" from all the code instead of deprecating it?

pascal-hofmann · 2023-03-16T16:44:14Z

I‘m not sure what will happen with existing configurations if I do that. I thought this way things just continue to work as expected. If that’s not desired I can remove it completely.

What do you prefer?

lprimak · 2023-03-16T16:46:10Z

@bluesliverx what do you think?

pascal-hofmann · 2023-04-14T14:04:11Z

Hey,
is there any update on this? Can I help with getting this merged?

bluesliverx · 2023-04-17T18:07:45Z

Sorry, finally getting back to this project. After looking through the code, I believe it is done the correct way. While the allowUntrusted flag still shows up in the code, it is marked deprecated and the value is handled correctly to set the new value. I like this quite a bit, thanks for the improvement!

bluesliverx · 2023-04-17T18:09:37Z

I'm just trying to get the PR to build and then I'll merge. Also, there is a new label type that was just barely merged. I'll work on getting this same functionality added to that after it builds/merges. Unless you beat me to it and then by all means add it to the label trigger (#62) @pascal-hofmann

bluesliverx · 2023-04-17T18:49:43Z

This has been released in 96.v9ff13b69dd66.

lprimak suggested changes Mar 3, 2023

View reviewed changes

lprimak reviewed Mar 3, 2023

View reviewed changes

src/main/java/com/adobe/jenkins/github_pr_comment_build/TriggerPRReviewBranchProperty.java Outdated Show resolved Hide resolved

pascal-hofmann force-pushed the configurable-minimum-permissions branch 3 times, most recently from 54ef976 to a2fe5b6 Compare March 4, 2023 11:27

pascal-hofmann marked this pull request as draft March 4, 2023 11:29

pascal-hofmann force-pushed the configurable-minimum-permissions branch 2 times, most recently from 75fde26 to c8a12d2 Compare March 4, 2023 13:46

pascal-hofmann force-pushed the configurable-minimum-permissions branch from c8a12d2 to 92c36fd Compare March 4, 2023 18:14

pascal-hofmann force-pushed the configurable-minimum-permissions branch 2 times, most recently from 9d1174a to f95d24e Compare March 6, 2023 14:52

pascal-hofmann marked this pull request as ready for review March 6, 2023 23:45

pascal-hofmann requested a review from lprimak March 15, 2023 20:05

pascal-hofmann force-pushed the configurable-minimum-permissions branch from f95d24e to a69c6b6 Compare March 16, 2023 08:43

pascal-hofmann force-pushed the configurable-minimum-permissions branch from a69c6b6 to 00d64c4 Compare March 16, 2023 08:48

lprimak approved these changes Mar 16, 2023

View reviewed changes

bluesliverx merged commit e96c665 into jenkinsci:master Apr 17, 2023

bluesliverx mentioned this pull request Apr 17, 2023

Add minimum permissions check for label trigger #63

Merged

pascal-hofmann deleted the configurable-minimum-permissions branch April 17, 2023 18:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add configurable minimum permissions for triggers #60

Add configurable minimum permissions for triggers #60

pascal-hofmann commented Mar 3, 2023 •

edited

Loading

pascal-hofmann commented Mar 3, 2023

lprimak commented Mar 3, 2023

lprimak left a comment

pascal-hofmann commented Mar 4, 2023 •

edited

Loading

pascal-hofmann commented Mar 4, 2023 •

edited

Loading

lprimak commented Mar 4, 2023

pascal-hofmann commented Mar 5, 2023

lprimak commented Mar 16, 2023

pascal-hofmann commented Mar 16, 2023 •

edited

Loading

lprimak Mar 3, 2023

lprimak Mar 3, 2023

lprimak Mar 4, 2023

lprimak Mar 4, 2023

lprimak commented Mar 16, 2023

pascal-hofmann commented Mar 16, 2023

lprimak commented Mar 16, 2023

pascal-hofmann commented Apr 14, 2023

bluesliverx commented Apr 17, 2023

bluesliverx commented Apr 17, 2023

bluesliverx commented Apr 17, 2023

		@@ -30,6 +36,18 @@ public void setAllowUntrusted(boolean allowUntrusted) {
		this.allowUntrusted = allowUntrusted;

Add configurable minimum permissions for triggers #60

Add configurable minimum permissions for triggers #60

Conversation

pascal-hofmann commented Mar 3, 2023 • edited Loading

pascal-hofmann commented Mar 3, 2023

lprimak commented Mar 3, 2023

lprimak left a comment

Choose a reason for hiding this comment

pascal-hofmann commented Mar 4, 2023 • edited Loading

pascal-hofmann commented Mar 4, 2023 • edited Loading

lprimak commented Mar 4, 2023

pascal-hofmann commented Mar 5, 2023

lprimak commented Mar 16, 2023

pascal-hofmann commented Mar 16, 2023 • edited Loading

lprimak Mar 3, 2023

Choose a reason for hiding this comment

lprimak Mar 3, 2023

Choose a reason for hiding this comment

lprimak Mar 4, 2023

Choose a reason for hiding this comment

lprimak Mar 4, 2023

Choose a reason for hiding this comment

lprimak commented Mar 16, 2023

pascal-hofmann commented Mar 16, 2023

lprimak commented Mar 16, 2023

pascal-hofmann commented Apr 14, 2023

bluesliverx commented Apr 17, 2023

bluesliverx commented Apr 17, 2023

bluesliverx commented Apr 17, 2023

pascal-hofmann commented Mar 3, 2023 •

edited

Loading

pascal-hofmann commented Mar 4, 2023 •

edited

Loading

pascal-hofmann commented Mar 4, 2023 •

edited

Loading

pascal-hofmann commented Mar 16, 2023 •

edited

Loading