From 82101ad4217c73503603123f0c58cdb6dcf7a660 Mon Sep 17 00:00:00 2001 From: Tamir Daniely Date: Fri, 5 Aug 2022 15:05:07 +0300 Subject: [PATCH 01/10] Add redirect_uri and related configuration --- .../plugins/GithubSecurityRealm.java | 65 +++++++++++++++---- .../plugins/GithubSecurityRealm/config.jelly | 5 ++ .../webapp/help/realm/redirect-uri-help.html | 3 + .../plugins/GithubSecurityRealmTest.java | 19 +++++- 4 files changed, 77 insertions(+), 15 deletions(-) create mode 100644 src/main/webapp/help/realm/redirect-uri-help.html diff --git a/src/main/java/org/jenkinsci/plugins/GithubSecurityRealm.java b/src/main/java/org/jenkinsci/plugins/GithubSecurityRealm.java index 803fd1c5..c6c706d5 100644 --- a/src/main/java/org/jenkinsci/plugins/GithubSecurityRealm.java +++ b/src/main/java/org/jenkinsci/plugins/GithubSecurityRealm.java @@ -62,7 +62,7 @@ of this software and associated documentation files (the "Software"), to deal import org.apache.http.client.CredentialsProvider; import org.apache.http.client.config.RequestConfig; import org.apache.http.client.methods.HttpPost; -import org.apache.http.client.methods.HttpUriRequest; +import org.apache.http.client.utils.URIBuilder; import org.apache.http.impl.client.BasicCredentialsProvider; import org.apache.http.impl.client.CloseableHttpClient; import org.apache.http.impl.client.HttpClientBuilder; @@ -73,6 +73,7 @@ of this software and associated documentation files (the "Software"), to deal import org.kohsuke.github.GHOrganization; import org.kohsuke.github.GHTeam; import org.kohsuke.stapler.DataBoundConstructor; +import org.kohsuke.stapler.DataBoundSetter; import org.kohsuke.stapler.Header; import org.kohsuke.stapler.HttpRedirect; import org.kohsuke.stapler.HttpResponse; @@ -85,6 +86,8 @@ of this software and associated documentation files (the "Software"), to deal import java.io.IOException; import java.net.InetSocketAddress; import java.net.Proxy; +import java.net.URISyntaxException; +import java.nio.charset.StandardCharsets; import java.security.SecureRandom; import java.util.Arrays; import java.util.HashSet; @@ -115,6 +118,8 @@ public class GithubSecurityRealm extends AbstractPasswordBasedSecurityRealm impl private Secret clientSecret; private String oauthScopes; private String[] myScopes; + @NonNull + private String redirectUri = ""; /** * @param githubWebUri The URI to the root of the web UI for GitHub or GitHub Enterprise, @@ -187,6 +192,15 @@ private void setOauthScopes(String oauthScopes) { this.oauthScopes = oauthScopes; } + /** + * @param redirectUri the redirectUri to set + */ + @DataBoundSetter + public void setRedirectUri(String redirectUri) { + if (null == redirectUri) redirectUri = ""; + this.redirectUri = redirectUri; + } + /** * Checks the security realm for a GitHub OAuth scope. * @param scope A scope to check for in the security realm. @@ -245,6 +259,10 @@ public void marshal(Object source, HierarchicalStreamWriter writer, writer.setValue(realm.getOauthScopes()); writer.endNode(); + writer.startNode("redirectUri"); + writer.setValue(realm.getRedirectUri()); + writer.endNode(); + } public Object unmarshal(HierarchicalStreamReader reader, @@ -274,8 +292,7 @@ public Object unmarshal(HierarchicalStreamReader reader, return realm; } - private void setValue(GithubSecurityRealm realm, String node, - String value) { + private void setValue(GithubSecurityRealm realm, String node, String value) { if (node.equalsIgnoreCase("clientid")) { realm.setClientID(value); } else if (node.equalsIgnoreCase("clientsecret")) { @@ -290,6 +307,8 @@ private void setValue(GithubSecurityRealm realm, String node, realm.setGithubApiUri(value); } else if (node.equalsIgnoreCase("oauthscopes")) { realm.setOauthScopes(value); + } else if (node.equalsIgnoreCase("redirecturi")) { + realm.setRedirectUri(value); } else { throw new ConversionException("Invalid node value = " + node); } @@ -334,11 +353,21 @@ public String getOauthScopes() { return oauthScopes; } + /** + * @return the redirectUri + */ + @NonNull + public String getRedirectUri() { + return redirectUri; + } + public HttpResponse doCommenceLogin(StaplerRequest request, @QueryParameter String from, @Header("Referer") final String referer) - throws IOException { + throws IOException, URISyntaxException { // https://tools.ietf.org/html/rfc6749#section-10.10 dictates that probability that an attacker guesses the string // SHOULD be less than or equal to 2^(-160) and our Strings consist of 65 chars. (65^27 ~= 2^160) final String state = getSecureRandomString(27); + + // This is to go back to the current page after login, not the oauth callback String redirectOnFinish; if (from != null && Util.isSafeToRedirectTo(from)) { redirectOnFinish = from; @@ -355,17 +384,17 @@ public HttpResponse doCommenceLogin(StaplerRequest request, @QueryParameter Stri for (GitHubOAuthScope s : Jenkins.get().getExtensionList(GitHubOAuthScope.class)) { scopes.addAll(s.getScopesToRequest()); } - String suffix=""; - if (!scopes.isEmpty()) { - suffix = "&scope="+Util.join(scopes,",")+"&state="+state; - } else { - // We need repo scope in order to access private repos - // See https://developer.github.com/v3/oauth/#scopes - suffix = "&scope=" + oauthScopes +"&state="+state; + + URIBuilder builder = new URIBuilder(githubWebUri, StandardCharsets.UTF_8); + builder.setPath("/login/oauth/authorize"); + builder.setParameter("client_id", clientID); + builder.setParameter("scope", scopes.isEmpty() ? oauthScopes : Util.join(scopes,",")); + builder.setParameter("state", state); + if (!redirectUri.isEmpty()) { + builder.setParameter("redirect_uri", redirectUri); } - return new HttpRedirect(githubWebUri + "/login/oauth/authorize?client_id=" - + clientID + suffix); + return new HttpRedirect(builder.toString()); } /** @@ -630,6 +659,12 @@ public String getDefaultOauthScopes() { return DEFAULT_OAUTH_SCOPES; } + public String getDefaultRequestUri() { + // Intentionally making this default in UI and not in code + // to preserve behaviour for existing groovy init & JCasc setups + return Jenkins.get().getRootUrl() + "securityRealm/finishLogin"; + } + public DescriptorImpl() { super(); // TODO Auto-generated constructor stub @@ -728,7 +763,8 @@ public boolean equals(Object object){ this.getGithubApiUri().equals(obj.getGithubApiUri()) && this.getClientID().equals(obj.getClientID()) && this.getClientSecret().equals(obj.getClientSecret()) && - this.getOauthScopes().equals(obj.getOauthScopes()); + this.getOauthScopes().equals(obj.getOauthScopes()) && + this.getRedirectUri().equals(obj.getRedirectUri()); } else { return false; } @@ -742,6 +778,7 @@ public int hashCode() { .append(this.getClientID()) .append(this.getClientSecret()) .append(this.getOauthScopes()) + .append(this.getRedirectUri()) .toHashCode(); } diff --git a/src/main/resources/org/jenkinsci/plugins/GithubSecurityRealm/config.jelly b/src/main/resources/org/jenkinsci/plugins/GithubSecurityRealm/config.jelly index 5008251d..9cd44860 100644 --- a/src/main/resources/org/jenkinsci/plugins/GithubSecurityRealm/config.jelly +++ b/src/main/resources/org/jenkinsci/plugins/GithubSecurityRealm/config.jelly @@ -23,5 +23,10 @@ + + + + + diff --git a/src/main/webapp/help/realm/redirect-uri-help.html b/src/main/webapp/help/realm/redirect-uri-help.html new file mode 100644 index 00000000..c779283d --- /dev/null +++ b/src/main/webapp/help/realm/redirect-uri-help.html @@ -0,0 +1,3 @@ +
+An optional redirect URI to be used by GitHub. See https://docs.github.com/en/developers/apps/building-github-apps/identifying-and-authorizing-users-for-github-apps. +
diff --git a/src/test/java/org/jenkinsci/plugins/GithubSecurityRealmTest.java b/src/test/java/org/jenkinsci/plugins/GithubSecurityRealmTest.java index e6cd935a..1938cc78 100644 --- a/src/test/java/org/jenkinsci/plugins/GithubSecurityRealmTest.java +++ b/src/test/java/org/jenkinsci/plugins/GithubSecurityRealmTest.java @@ -52,7 +52,24 @@ public void testEquals_false() { GithubSecurityRealm a = new GithubSecurityRealm("http://jenkins.acme.com", "http://jenkins.acme.com/api/v3", "someid", "somesecret", "read:org"); GithubSecurityRealm b = new GithubSecurityRealm("http://jenkins.acme.com", "http://jenkins.acme.com/api/v3", "someid", "somesecret", "read:org,repo"); assertNotEquals(a, b); - assertNotEquals("", a); + } + + @Test + public void testEqualsRedirectUri() { + GithubSecurityRealm _default = new GithubSecurityRealm("http://jenkins.acme.com", "http://jenkins.acme.com/api/v3", "someid", "somesecret", "read:org"); + GithubSecurityRealm empty = new GithubSecurityRealm("http://jenkins.acme.com", "http://jenkins.acme.com/api/v3", "someid", "somesecret", "read:org"); + empty.setRedirectUri(""); + GithubSecurityRealm sameValue1 = new GithubSecurityRealm("http://jenkins.acme.com", "http://jenkins.acme.com/api/v3", "someid", "somesecret", "read:org"); + sameValue1.setRedirectUri("http://jenkins1.awesomecorp.com"); + GithubSecurityRealm sameValue2 = new GithubSecurityRealm("http://jenkins.acme.com", "http://jenkins.acme.com/api/v3", "someid", "somesecret", "read:org"); + sameValue2.setRedirectUri("http://jenkins1.awesomecorp.com"); + GithubSecurityRealm otherValue = new GithubSecurityRealm("http://jenkins.acme.com", "http://jenkins.acme.com/api/v3", "someid", "somesecret", "read:org"); + otherValue.setRedirectUri("http://jenkins1.notsogoodcorp.com"); + assertEquals(_default, empty); + assertEquals(sameValue1, sameValue2); + assertNotEquals(sameValue1, _default); + assertNotEquals(sameValue1, empty); + assertNotEquals(sameValue1, otherValue); } @Test From 29a8567e6a1189057f75010acbe468f8ca02af8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Brinkop?= Date: Tue, 18 Oct 2022 16:13:45 +0200 Subject: [PATCH 02/10] Add support for using jenkins agents that require authentication --- .../plugins/GithubAuthorizationStrategy.java | 7 +++++ ...ithubRequireOrganizationMembershipACL.java | 26 +++++++++++++++++++ .../GithubAuthorizationStrategy/config.jelly | 4 +++ .../help/auth/agent-user-name-help.html | 3 +++ .../GithubAuthorizationStrategyTest.java | 8 +++--- ...bRequireOrganizationMembershipACLTest.java | 1 + 6 files changed, 45 insertions(+), 4 deletions(-) create mode 100644 src/main/webapp/help/auth/agent-user-name-help.html diff --git a/src/main/java/org/jenkinsci/plugins/GithubAuthorizationStrategy.java b/src/main/java/org/jenkinsci/plugins/GithubAuthorizationStrategy.java index b9927aa6..7a1d303f 100644 --- a/src/main/java/org/jenkinsci/plugins/GithubAuthorizationStrategy.java +++ b/src/main/java/org/jenkinsci/plugins/GithubAuthorizationStrategy.java @@ -55,6 +55,7 @@ public class GithubAuthorizationStrategy extends AuthorizationStrategy { @DataBoundConstructor public GithubAuthorizationStrategy(String adminUserNames, + String agentUserName, boolean authenticatedUserReadPermission, boolean useRepositoryPermissions, boolean authenticatedUserCreateJobPermission, @@ -66,6 +67,7 @@ public GithubAuthorizationStrategy(String adminUserNames, super(); rootACL = new GithubRequireOrganizationMembershipACL(adminUserNames, + agentUserName, organizationNames, authenticatedUserReadPermission, useRepositoryPermissions, @@ -140,6 +142,10 @@ public String getAdminUserNames() { return StringUtils.join(rootACL.getAdminUserNameList().iterator(), ", "); } + public String getAgentUserName() { + return rootACL.getAgentUserName(); + } + /** * @return isUseRepositoryPermissions * @see org.jenkinsci.plugins.GithubRequireOrganizationMembershipACL#isUseRepositoryPermissions() @@ -208,6 +214,7 @@ public boolean equals(Object object){ GithubAuthorizationStrategy obj = (GithubAuthorizationStrategy) object; return this.getOrganizationNames().equals(obj.getOrganizationNames()) && this.getAdminUserNames().equals(obj.getAdminUserNames()) && + this.getAgentUserName().equals(obj.getAgentUserName()) && this.isUseRepositoryPermissions() == obj.isUseRepositoryPermissions() && this.isAuthenticatedUserCreateJobPermission() == obj.isAuthenticatedUserCreateJobPermission() && this.isAuthenticatedUserReadPermission() == obj.isAuthenticatedUserReadPermission() && diff --git a/src/main/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACL.java b/src/main/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACL.java index a823c017..a33a005f 100644 --- a/src/main/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACL.java +++ b/src/main/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACL.java @@ -64,6 +64,7 @@ public class GithubRequireOrganizationMembershipACL extends ACL { private final List organizationNameList; private final List adminUserNameList; + private final String agentUserName; private final boolean authenticatedUserReadPermission; private final boolean useRepositoryPermissions; private final boolean authenticatedUserCreateJobPermission; @@ -102,6 +103,11 @@ public boolean hasPermission(@NonNull Authentication a, @NonNull Permission perm return true; } + // Grant agent permissions to agent user + if (candidateName.equalsIgnoreCase(agentUserName) && checkAgentUserPermission(permission)) { + return true; + } + // Are they trying to read? if (checkReadPermission(permission)) { // if we support authenticated read return early @@ -153,6 +159,11 @@ else if (testBuildPermission(permission) && isInWhitelistedOrgs(authenticationTo return true; } + // Grant agent permissions to agent user + if (authenticatedUserName.equalsIgnoreCase(agentUserName) && checkAgentUserPermission(permission)) { + return true; + } + if (authenticatedUserName.equals("anonymous")) { if (checkJobStatusPermission(permission) && allowAnonymousJobStatusPermission) { return true; @@ -239,6 +250,14 @@ private boolean checkReadPermission(@NonNull Permission permission) { || id.equals("hudson.model.Item.Read")); } + private boolean checkAgentUserPermission(@NonNull Permission permission) { + String id = permission.getId(); + return (checkReadPermission(permission) + || id.equals("hudson.model.Computer.Create") + || id.equals("hudson.model.Computer.Connect") + || id.equals("hudson.model.Computer.Configure")); + } + private boolean checkJobStatusPermission(@NonNull Permission permission) { return permission.getId().equals("hudson.model.Item.ViewStatus"); } @@ -280,6 +299,7 @@ private String getRepositoryName() { } public GithubRequireOrganizationMembershipACL(String adminUserNames, + String agentUserName, String organizationNames, boolean authenticatedUserReadPermission, boolean useRepositoryPermissions, @@ -298,6 +318,7 @@ public GithubRequireOrganizationMembershipACL(String adminUserNames, this.allowAnonymousReadPermission = allowAnonymousReadPermission; this.allowAnonymousJobStatusPermission = allowAnonymousJobStatusPermission; this.adminUserNameList = new LinkedList<>(); + this.agentUserName = agentUserName; String[] parts = adminUserNames.split(","); @@ -319,6 +340,7 @@ public GithubRequireOrganizationMembershipACL(String adminUserNames, public GithubRequireOrganizationMembershipACL cloneForProject(AbstractItem item) { return new GithubRequireOrganizationMembershipACL( this.adminUserNameList, + this.agentUserName, this.organizationNameList, this.authenticatedUserReadPermission, this.useRepositoryPermissions, @@ -331,6 +353,7 @@ public GithubRequireOrganizationMembershipACL cloneForProject(AbstractItem item) } public GithubRequireOrganizationMembershipACL(List adminUserNameList, + String agentUserName, List organizationNameList, boolean authenticatedUserReadPermission, boolean useRepositoryPermissions, @@ -343,6 +366,7 @@ public GithubRequireOrganizationMembershipACL(List adminUserNameList, super(); this.adminUserNameList = adminUserNameList; + this.agentUserName = agentUserName; this.organizationNameList = organizationNameList; this.authenticatedUserReadPermission = authenticatedUserReadPermission; this.useRepositoryPermissions = useRepositoryPermissions; @@ -362,6 +386,8 @@ public List getAdminUserNameList() { return adminUserNameList; } + public String getAgentUserName() { return agentUserName; } + public boolean isUseRepositoryPermissions() { return useRepositoryPermissions; } diff --git a/src/main/resources/org/jenkinsci/plugins/GithubAuthorizationStrategy/config.jelly b/src/main/resources/org/jenkinsci/plugins/GithubAuthorizationStrategy/config.jelly index 2455a404..abd722d1 100644 --- a/src/main/resources/org/jenkinsci/plugins/GithubAuthorizationStrategy/config.jelly +++ b/src/main/resources/org/jenkinsci/plugins/GithubAuthorizationStrategy/config.jelly @@ -8,6 +8,10 @@ + + + + diff --git a/src/main/webapp/help/auth/agent-user-name-help.html b/src/main/webapp/help/auth/agent-user-name-help.html new file mode 100644 index 00000000..0f5a4093 --- /dev/null +++ b/src/main/webapp/help/auth/agent-user-name-help.html @@ -0,0 +1,3 @@ +
+If you are using a cluster of Jenkins agents (e.g. using the swarm plugin) this is the user that is used for authenticating agents. This user will receive the overall read rights as well as rights to create, connect and configure agents. +
\ No newline at end of file diff --git a/src/test/java/org/jenkinsci/plugins/GithubAuthorizationStrategyTest.java b/src/test/java/org/jenkinsci/plugins/GithubAuthorizationStrategyTest.java index f5307fbe..3c4a956a 100644 --- a/src/test/java/org/jenkinsci/plugins/GithubAuthorizationStrategyTest.java +++ b/src/test/java/org/jenkinsci/plugins/GithubAuthorizationStrategyTest.java @@ -32,14 +32,14 @@ of this software and associated documentation files (the "Software"), to deal public class GithubAuthorizationStrategyTest { @Test public void testEquals_true() { - GithubAuthorizationStrategy a = new GithubAuthorizationStrategy("", false, true, false, "", false, false, false, false); - GithubAuthorizationStrategy b = new GithubAuthorizationStrategy("", false, true, false, "", false, false, false, false); + GithubAuthorizationStrategy a = new GithubAuthorizationStrategy("", "", false, true, false, "", false, false, false, false); + GithubAuthorizationStrategy b = new GithubAuthorizationStrategy("", "", false, true, false, "", false, false, false, false); assertEquals(a, b); } @Test public void testEquals_false() { - GithubAuthorizationStrategy a = new GithubAuthorizationStrategy("", false, true, false, "", false, false, false, false); - GithubAuthorizationStrategy b = new GithubAuthorizationStrategy("", false, false, false, "", false, false, false, false); + GithubAuthorizationStrategy a = new GithubAuthorizationStrategy("", "", false, true, false, "", false, false, false, false); + GithubAuthorizationStrategy b = new GithubAuthorizationStrategy("", "", false, false, false, "", false, false, false, false); assertNotEquals(a, b); assertNotEquals("", a); } diff --git a/src/test/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACLTest.java b/src/test/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACLTest.java index c0e99bec..a4ed6fa3 100644 --- a/src/test/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACLTest.java +++ b/src/test/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACLTest.java @@ -132,6 +132,7 @@ private void mockJenkins(MockedStatic mockedJenkins) { private GithubRequireOrganizationMembershipACL createACL() { return new GithubRequireOrganizationMembershipACL( "admin", + "agent", "myOrg", authenticatedUserReadPermission, useRepositoryPermissions, From a1d6e49c3249019fde0caf552e5e6b902e6fb014 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Brinkop?= Date: Fri, 21 Oct 2022 10:24:44 +0200 Subject: [PATCH 03/10] Update read permissions for agent user --- .../plugins/GithubRequireOrganizationMembershipACL.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACL.java b/src/main/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACL.java index a33a005f..8d16b237 100644 --- a/src/main/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACL.java +++ b/src/main/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACL.java @@ -252,7 +252,7 @@ private boolean checkReadPermission(@NonNull Permission permission) { private boolean checkAgentUserPermission(@NonNull Permission permission) { String id = permission.getId(); - return (checkReadPermission(permission) + return (id.equals("hudson.model.Hudson.Read") || id.equals("hudson.model.Computer.Create") || id.equals("hudson.model.Computer.Connect") || id.equals("hudson.model.Computer.Configure")); From a1c5082850606406515ad748e3ad930d0d570202 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Brinkop?= Date: Fri, 21 Oct 2022 10:48:04 +0200 Subject: [PATCH 04/10] Add test cases for agent user rights --- ...bRequireOrganizationMembershipACLTest.java | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/test/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACLTest.java b/src/test/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACLTest.java index a4ed6fa3..2b36a81d 100644 --- a/src/test/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACLTest.java +++ b/src/test/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACLTest.java @@ -60,6 +60,7 @@ of this software and associated documentation files (the "Software"), to deal import java.util.Collections; import java.util.List; +import hudson.model.Computer; import hudson.model.Hudson; import hudson.model.Item; import hudson.model.Messages; @@ -555,6 +556,30 @@ public void testCannotReadRepositoryWithInvalidRepoUrl() throws IOException { } } + @Test + public void testAgentUserCanCreateConnectAndConfigureAgents() { + GithubAuthenticationToken authenticationToken = Mockito.mock(GithubAuthenticationToken.class); + Mockito.when(authenticationToken.isAuthenticated()).thenReturn(true); + Mockito.when(authenticationToken.getName()).thenReturn("agent"); + GithubRequireOrganizationMembershipACL acl = createACL(); + + assertTrue(acl.hasPermission(authenticationToken, Computer.CREATE)); + assertTrue(acl.hasPermission(authenticationToken, Computer.CONFIGURE)); + assertTrue(acl.hasPermission(authenticationToken, Computer.CONNECT)); + } + + @Test + public void testAuthenticatedCanNotCreateConnectAndConfigureAgents() { + GithubAuthenticationToken authenticationToken = Mockito.mock(GithubAuthenticationToken.class); + Mockito.when(authenticationToken.isAuthenticated()).thenReturn(true); + Mockito.when(authenticationToken.getName()).thenReturn("authenticated"); + GithubRequireOrganizationMembershipACL acl = createACL(); + + assertFalse(acl.hasPermission(authenticationToken, Computer.CREATE)); + assertFalse(acl.hasPermission(authenticationToken, Computer.CONFIGURE)); + assertFalse(acl.hasPermission(authenticationToken, Computer.CONNECT)); + } + @Test public void testAnonymousCanViewJobStatusWhenGranted() { this.allowAnonymousJobStatusPermission = true; From 5af8fc8c6fc80a2368d860d2e304ee5b52e6b10b Mon Sep 17 00:00:00 2001 From: "Brinkop Andre (uib05464)" Date: Tue, 18 Jul 2023 13:21:45 +0200 Subject: [PATCH 05/10] Add javadoc to agentUserName setter --- .../org/jenkinsci/plugins/GithubAuthorizationStrategy.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/main/java/org/jenkinsci/plugins/GithubAuthorizationStrategy.java b/src/main/java/org/jenkinsci/plugins/GithubAuthorizationStrategy.java index 7a1d303f..7378d7c0 100644 --- a/src/main/java/org/jenkinsci/plugins/GithubAuthorizationStrategy.java +++ b/src/main/java/org/jenkinsci/plugins/GithubAuthorizationStrategy.java @@ -142,6 +142,10 @@ public String getAdminUserNames() { return StringUtils.join(rootACL.getAdminUserNameList().iterator(), ", "); } + /** + * @return agentUserName + * @see GithubRequireOrganizationMembershipACL#getAgentUserName() + */ public String getAgentUserName() { return rootACL.getAgentUserName(); } From 75ea97eec5560539e26bddf1784e4639a271934f Mon Sep 17 00:00:00 2001 From: "Brinkop Andre (uib05464)" Date: Tue, 18 Jul 2023 13:45:22 +0200 Subject: [PATCH 06/10] Make agentUserName property optional to avoid a breaking change --- .../plugins/GithubAuthorizationStrategy.java | 12 ++++++++++-- .../GithubRequireOrganizationMembershipACL.java | 15 ++++++++------- .../plugins/GithubAuthorizationStrategyTest.java | 8 ++++---- ...ithubRequireOrganizationMembershipACLTest.java | 5 +++-- 4 files changed, 25 insertions(+), 15 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/GithubAuthorizationStrategy.java b/src/main/java/org/jenkinsci/plugins/GithubAuthorizationStrategy.java index 7378d7c0..e3767625 100644 --- a/src/main/java/org/jenkinsci/plugins/GithubAuthorizationStrategy.java +++ b/src/main/java/org/jenkinsci/plugins/GithubAuthorizationStrategy.java @@ -30,6 +30,7 @@ of this software and associated documentation files (the "Software"), to deal import org.jenkinsci.plugins.workflow.job.WorkflowJob; import org.jenkinsci.plugins.workflow.multibranch.BranchJobProperty; import org.kohsuke.stapler.DataBoundConstructor; +import org.kohsuke.stapler.DataBoundSetter; import java.util.Collection; import java.util.Collections; @@ -55,7 +56,6 @@ public class GithubAuthorizationStrategy extends AuthorizationStrategy { @DataBoundConstructor public GithubAuthorizationStrategy(String adminUserNames, - String agentUserName, boolean authenticatedUserReadPermission, boolean useRepositoryPermissions, boolean authenticatedUserCreateJobPermission, @@ -67,7 +67,6 @@ public GithubAuthorizationStrategy(String adminUserNames, super(); rootACL = new GithubRequireOrganizationMembershipACL(adminUserNames, - agentUserName, organizationNames, authenticatedUserReadPermission, useRepositoryPermissions, @@ -142,6 +141,15 @@ public String getAdminUserNames() { return StringUtils.join(rootACL.getAdminUserNameList().iterator(), ", "); } + /** Set the agent username. We use a setter instead of a constructor to make this an optional field + * to avoid a breaking change. + * @see org.jenkinsci.plugins.GithubRequireOrganizationMembershipACL#setAgentUserName(String) + */ + @DataBoundSetter + public void setAgentUserName(String agentUserName) { + rootACL.setAgentUserName(agentUserName); + } + /** * @return agentUserName * @see GithubRequireOrganizationMembershipACL#getAgentUserName() diff --git a/src/main/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACL.java b/src/main/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACL.java index 8d16b237..1c1bce38 100644 --- a/src/main/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACL.java +++ b/src/main/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACL.java @@ -64,7 +64,7 @@ public class GithubRequireOrganizationMembershipACL extends ACL { private final List organizationNameList; private final List adminUserNameList; - private final String agentUserName; + private String agentUserName; private final boolean authenticatedUserReadPermission; private final boolean useRepositoryPermissions; private final boolean authenticatedUserCreateJobPermission; @@ -299,7 +299,6 @@ private String getRepositoryName() { } public GithubRequireOrganizationMembershipACL(String adminUserNames, - String agentUserName, String organizationNames, boolean authenticatedUserReadPermission, boolean useRepositoryPermissions, @@ -318,7 +317,6 @@ public GithubRequireOrganizationMembershipACL(String adminUserNames, this.allowAnonymousReadPermission = allowAnonymousReadPermission; this.allowAnonymousJobStatusPermission = allowAnonymousJobStatusPermission; this.adminUserNameList = new LinkedList<>(); - this.agentUserName = agentUserName; String[] parts = adminUserNames.split(","); @@ -335,12 +333,12 @@ public GithubRequireOrganizationMembershipACL(String adminUserNames, } this.item = null; + this.agentUserName = ""; // Initially blank - populated by a setter since this field is optional } public GithubRequireOrganizationMembershipACL cloneForProject(AbstractItem item) { - return new GithubRequireOrganizationMembershipACL( + GithubRequireOrganizationMembershipACL acl = new GithubRequireOrganizationMembershipACL( this.adminUserNameList, - this.agentUserName, this.organizationNameList, this.authenticatedUserReadPermission, this.useRepositoryPermissions, @@ -350,10 +348,11 @@ public GithubRequireOrganizationMembershipACL cloneForProject(AbstractItem item) this.allowAnonymousReadPermission, this.allowAnonymousJobStatusPermission, item); + acl.setAgentUserName(agentUserName); + return acl; } public GithubRequireOrganizationMembershipACL(List adminUserNameList, - String agentUserName, List organizationNameList, boolean authenticatedUserReadPermission, boolean useRepositoryPermissions, @@ -366,7 +365,6 @@ public GithubRequireOrganizationMembershipACL(List adminUserNameList, super(); this.adminUserNameList = adminUserNameList; - this.agentUserName = agentUserName; this.organizationNameList = organizationNameList; this.authenticatedUserReadPermission = authenticatedUserReadPermission; this.useRepositoryPermissions = useRepositoryPermissions; @@ -386,6 +384,9 @@ public List getAdminUserNameList() { return adminUserNameList; } + public void setAgentUserName(String agentUserName) { + this.agentUserName = agentUserName; + } public String getAgentUserName() { return agentUserName; } public boolean isUseRepositoryPermissions() { diff --git a/src/test/java/org/jenkinsci/plugins/GithubAuthorizationStrategyTest.java b/src/test/java/org/jenkinsci/plugins/GithubAuthorizationStrategyTest.java index 3c4a956a..f5307fbe 100644 --- a/src/test/java/org/jenkinsci/plugins/GithubAuthorizationStrategyTest.java +++ b/src/test/java/org/jenkinsci/plugins/GithubAuthorizationStrategyTest.java @@ -32,14 +32,14 @@ of this software and associated documentation files (the "Software"), to deal public class GithubAuthorizationStrategyTest { @Test public void testEquals_true() { - GithubAuthorizationStrategy a = new GithubAuthorizationStrategy("", "", false, true, false, "", false, false, false, false); - GithubAuthorizationStrategy b = new GithubAuthorizationStrategy("", "", false, true, false, "", false, false, false, false); + GithubAuthorizationStrategy a = new GithubAuthorizationStrategy("", false, true, false, "", false, false, false, false); + GithubAuthorizationStrategy b = new GithubAuthorizationStrategy("", false, true, false, "", false, false, false, false); assertEquals(a, b); } @Test public void testEquals_false() { - GithubAuthorizationStrategy a = new GithubAuthorizationStrategy("", "", false, true, false, "", false, false, false, false); - GithubAuthorizationStrategy b = new GithubAuthorizationStrategy("", "", false, false, false, "", false, false, false, false); + GithubAuthorizationStrategy a = new GithubAuthorizationStrategy("", false, true, false, "", false, false, false, false); + GithubAuthorizationStrategy b = new GithubAuthorizationStrategy("", false, false, false, "", false, false, false, false); assertNotEquals(a, b); assertNotEquals("", a); } diff --git a/src/test/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACLTest.java b/src/test/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACLTest.java index 2b36a81d..6a7a9b99 100644 --- a/src/test/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACLTest.java +++ b/src/test/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACLTest.java @@ -131,9 +131,8 @@ private void mockJenkins(MockedStatic mockedJenkins) { new GrantedAuthority[]{new GrantedAuthorityImpl("anonymous")}); private GithubRequireOrganizationMembershipACL createACL() { - return new GithubRequireOrganizationMembershipACL( + GithubRequireOrganizationMembershipACL acl = new GithubRequireOrganizationMembershipACL( "admin", - "agent", "myOrg", authenticatedUserReadPermission, useRepositoryPermissions, @@ -142,6 +141,8 @@ private GithubRequireOrganizationMembershipACL createACL() { allowAnonymousCCTrayPermission, allowAnonymousReadPermission, allowAnonymousJobStatusPermission); + acl.setAgentUserName("agent"); + return acl; } private GithubRequireOrganizationMembershipACL aclForProject(Project project) { From afea89548284c68a7d24889db21e22b6d21160f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Brinkop?= Date: Tue, 18 Jul 2023 13:47:15 +0200 Subject: [PATCH 07/10] Add log output for agent user permission check Co-authored-by: Andreas Nygard --- .../plugins/GithubRequireOrganizationMembershipACL.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/main/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACL.java b/src/main/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACL.java index 1c1bce38..5bc0a192 100644 --- a/src/main/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACL.java +++ b/src/main/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACL.java @@ -105,6 +105,7 @@ public boolean hasPermission(@NonNull Authentication a, @NonNull Permission perm // Grant agent permissions to agent user if (candidateName.equalsIgnoreCase(agentUserName) && checkAgentUserPermission(permission)) { + log.finest("Granting Agent Connect rights to user " + candidateName); return true; } @@ -161,6 +162,7 @@ else if (testBuildPermission(permission) && isInWhitelistedOrgs(authenticationTo // Grant agent permissions to agent user if (authenticatedUserName.equalsIgnoreCase(agentUserName) && checkAgentUserPermission(permission)) { + log.finest("Granting Agent Connect rights to user " + authenticatedUserName); return true; } From 67648384d8842da8b0817908394145f1d5b2147b Mon Sep 17 00:00:00 2001 From: "Brinkop Andre (uib05464)" Date: Tue, 18 Jul 2023 13:52:47 +0200 Subject: [PATCH 08/10] Make agent user permission check type-safe --- .../GithubRequireOrganizationMembershipACL.java | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACL.java b/src/main/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACL.java index 5bc0a192..4659c4a0 100644 --- a/src/main/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACL.java +++ b/src/main/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACL.java @@ -26,6 +26,7 @@ of this software and associated documentation files (the "Software"), to deal */ package org.jenkinsci.plugins; +import hudson.model.*; import org.acegisecurity.Authentication; import org.jenkinsci.plugins.github_branch_source.GitHubSCMSource; import org.jenkinsci.plugins.workflow.job.WorkflowJob; @@ -41,10 +42,6 @@ of this software and associated documentation files (the "Software"), to deal import edu.umd.cs.findbugs.annotations.NonNull; import edu.umd.cs.findbugs.annotations.Nullable; -import hudson.model.AbstractItem; -import hudson.model.AbstractProject; -import hudson.model.Describable; -import hudson.model.Item; import hudson.plugins.git.GitSCM; import hudson.plugins.git.UserRemoteConfig; import hudson.security.ACL; @@ -254,10 +251,10 @@ private boolean checkReadPermission(@NonNull Permission permission) { private boolean checkAgentUserPermission(@NonNull Permission permission) { String id = permission.getId(); - return (id.equals("hudson.model.Hudson.Read") - || id.equals("hudson.model.Computer.Create") - || id.equals("hudson.model.Computer.Connect") - || id.equals("hudson.model.Computer.Configure")); + return (id.equals(Hudson.READ) + || id.equals(Computer.CREATE) + || id.equals(Computer.CONNECT) + || id.equals(Computer.CONFIGURE)); } private boolean checkJobStatusPermission(@NonNull Permission permission) { From bd9637bce93991d17b972aa742ac987066fbeda8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Brinkop?= Date: Tue, 18 Jul 2023 13:54:05 +0200 Subject: [PATCH 09/10] Update src/main/webapp/help/auth/agent-user-name-help.html Co-authored-by: Andreas Nygard --- src/main/webapp/help/auth/agent-user-name-help.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/webapp/help/auth/agent-user-name-help.html b/src/main/webapp/help/auth/agent-user-name-help.html index 0f5a4093..eee865b0 100644 --- a/src/main/webapp/help/auth/agent-user-name-help.html +++ b/src/main/webapp/help/auth/agent-user-name-help.html @@ -1,3 +1,3 @@
-If you are using a cluster of Jenkins agents (e.g. using the swarm plugin) this is the user that is used for authenticating agents. This user will receive the overall read rights as well as rights to create, connect and configure agents. +If you are using inbound Jenkins agents, this is the user that is used for authenticating agents. This user will receive rights to create, connect and configure agents.
\ No newline at end of file From 2feb79bd4397f277088e60d950a92e449f932a70 Mon Sep 17 00:00:00 2001 From: "Brinkop Andre (uib05464)" Date: Tue, 18 Jul 2023 14:02:48 +0200 Subject: [PATCH 10/10] Fix permission type in checkAgentUserPermission function --- .../plugins/GithubRequireOrganizationMembershipACL.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACL.java b/src/main/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACL.java index 4659c4a0..a7c8d81d 100644 --- a/src/main/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACL.java +++ b/src/main/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACL.java @@ -250,11 +250,10 @@ private boolean checkReadPermission(@NonNull Permission permission) { } private boolean checkAgentUserPermission(@NonNull Permission permission) { - String id = permission.getId(); - return (id.equals(Hudson.READ) - || id.equals(Computer.CREATE) - || id.equals(Computer.CONNECT) - || id.equals(Computer.CONFIGURE)); + return permission.equals(Hudson.READ) + || permission.equals(Computer.CREATE) + || permission.equals(Computer.CONNECT) + || permission.equals(Computer.CONFIGURE); } private boolean checkJobStatusPermission(@NonNull Permission permission) {