Add redirect_uri and related configuration

jenkinsci · Aug 5, 2022 · 82101ad · 82101ad
1 parent db7b3ba
commit 82101ad
Show file tree

Hide file tree

Showing 4 changed files with 77 additions and 15 deletions.
diff --git 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
@@ -23,5 +23,10 @@
         <f:entry title="OAuth Scope(s)" field="oauthScopes" help="/plugin/github-oauth/help/realm/oauth-scopes-help.html">
             <f:textbox default="${descriptor.getDefaultOauthScopes()}" />
         </f:entry>
+
+        <f:entry title="Redirect URI" field="redirectUri" help="/plugin/github-oauth/help/realm/redirect-uri-help.html">
+            <f:textbox default="${descriptor.getDefaultRequestUri()}" />
+        </f:entry>
+
     </f:section>
 </j:jelly>
diff --git a/src/main/webapp/help/realm/redirect-uri-help.html b/src/main/webapp/help/realm/redirect-uri-help.html
@@ -0,0 +1,3 @@
+<div>
+An optional redirect URI to be used by GitHub.  See <a href="https://docs.github.com/en/developers/apps/building-github-apps/identifying-and-authorizing-users-for-github-apps">https://docs.github.com/en/developers/apps/building-github-apps/identifying-and-authorizing-users-for-github-apps</a>.
+</div>
diff --git 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