From 7d3aececce383af1f3b5ba2d3158abe964e9e01d Mon Sep 17 00:00:00 2001 From: Tamir Daniely Date: Fri, 5 Aug 2022 14:52:22 +0300 Subject: [PATCH] Add redirect_uri oauth param and configuration --- .../plugins/GithubSecurityRealm.java | 64 +++++++++++++++---- .../plugins/GithubSecurityRealm/config.jelly | 5 ++ .../webapp/help/realm/redirect-uri-help.html | 3 + .../plugins/GithubSecurityRealmTest.java | 19 +++++- 4 files changed, 76 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..1a70f240 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; @@ -85,6 +85,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 +117,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 +191,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 +258,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 +291,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 +306,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 +352,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 +383,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 +658,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 +762,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 +777,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