Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add the option to configure redirect uri #111

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add the option to configure redirect uri #111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
91e7b7f
Add the option to configure redirect uri
philiplrb Apr 2, 2019
938510d
Expand tabs to spaces
philiplrb Apr 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 56 additions & 12 deletions src/main/java/org/jenkinsci/plugins/GithubSecurityRealm.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ of this software and associated documentation files (the "Software"), to deal
public class GithubSecurityRealm extends AbstractPasswordBasedSecurityRealm implements UserDetailsService {
private static final String DEFAULT_WEB_URI = "https://github.com";
private static final String DEFAULT_API_URI = "https://api.github.com";
private static final String DEFAULT_REDIRECT_URI = "";
private static final String DEFAULT_ENTERPRISE_API_SUFFIX = "/api/v3";
private static final String DEFAULT_OAUTH_SCOPES = "read:org,user:email,repo";

Expand All @@ -116,6 +117,7 @@ public class GithubSecurityRealm extends AbstractPasswordBasedSecurityRealm impl
private Secret clientSecret;
private String oauthScopes;
private String[] myScopes;
private String redirectUri;

/**
* @param githubWebUri The URI to the root of the web UI for GitHub or GitHub Enterprise,
Expand All @@ -125,20 +127,23 @@ public class GithubSecurityRealm extends AbstractPasswordBasedSecurityRealm impl
* @param clientID The client ID for the created OAuth Application.
* @param clientSecret The client secret for the created GitHub OAuth Application.
* @param oauthScopes A comma separated list of OAuth Scopes to request access to.
* @param redirectUri An optional redirect URI to be used by GitHub.
*/
@DataBoundConstructor
public GithubSecurityRealm(String githubWebUri,
String githubApiUri,
String clientID,
String clientSecret,
String oauthScopes) {
String oauthScopes,
String redirectUri) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may cause upgrade issues. I'm not sure just a hunch. I'll test and let you know. From what I recall it's no longer a good coding practice to modify the constructors of configuration classes. @jenkinsci/code-reviewers keep me honest!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this needs to change, please give guidance on the approach you'd like to see.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See Constructor vs. setters. Is redirectUri a mandatory or optional parameter? If optional, it should be a @DataBoundSetter rather than a @DataBoundConstructor.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, missing a test that a serialized older version deserializes into newer version with default value. Which it should looking at marshal/unmarshal.

The reason not to change the constructor is to not break deployments where jenkins instance is initialized in groovy hooks. So at least keep the old constructer signature as well.

super();

this.githubWebUri = Util.fixEmptyAndTrim(githubWebUri);
this.githubApiUri = Util.fixEmptyAndTrim(githubApiUri);
this.clientID = Util.fixEmptyAndTrim(clientID);
setClientSecret(Util.fixEmptyAndTrim(clientSecret));
this.oauthScopes = Util.fixEmptyAndTrim(oauthScopes);
this.redirectUri = Util.fixEmptyAndTrim(redirectUri);
}

private GithubSecurityRealm() { }
Expand Down Expand Up @@ -188,6 +193,13 @@ private void setOauthScopes(String oauthScopes) {
this.oauthScopes = oauthScopes;
}

/**
* @param redirectUri the redirectUri to set
*/
private void setRedirectUri(String redirectUri) {
this.redirectUri = redirectUri;
}

/**
* Checks the security realm for a GitHub OAuth scope.
* @param scope A scope to check for in the security realm.
Expand Down Expand Up @@ -246,6 +258,13 @@ public void marshal(Object source, HierarchicalStreamWriter writer,
writer.setValue(realm.getOauthScopes());
writer.endNode();

writer.startNode("redirectUri");
String redirectUriValue = DEFAULT_REDIRECT_URI;
if (null != realm.getRedirectUri()) {
redirectUriValue = realm.getRedirectUri();
}
writer.setValue(redirectUriValue);
writer.endNode();
}

public Object unmarshal(HierarchicalStreamReader reader,
Expand All @@ -272,11 +291,14 @@ public Object unmarshal(HierarchicalStreamReader reader,
realm.setGithubApiUri(DEFAULT_API_URI);
}

if (realm.getRedirectUri() == null) {
realm.setRedirectUri(DEFAULT_REDIRECT_URI);
}

return realm;
}

private void setValue(GithubSecurityRealm realm, String node,
String value) {
private void setValue(GithubSecurityRealm realm, String node, String value) {
if (node.toLowerCase().equals("clientid")) {
realm.setClientID(value);
} else if (node.toLowerCase().equals("clientsecret")) {
Expand All @@ -291,6 +313,8 @@ private void setValue(GithubSecurityRealm realm, String node,
realm.setGithubApiUri(value);
} else if (node.toLowerCase().equals("oauthscopes")) {
realm.setOauthScopes(value);
} else if (node.toLowerCase().equals("redirecturi")) {
realm.setRedirectUri(value);
} else {
throw new ConversionException("Invalid node value = " + node);
}
Expand Down Expand Up @@ -335,6 +359,13 @@ public String getOauthScopes() {
return oauthScopes;
}

/**
* @return the redirectUri
*/
public String getRedirectUri() {
return redirectUri;
}

public HttpResponse doCommenceLogin(StaplerRequest request, @QueryParameter String from, @Header("Referer") final String referer)
throws IOException {
String redirectOnFinish;
Expand All @@ -352,15 +383,19 @@ public HttpResponse doCommenceLogin(StaplerRequest request, @QueryParameter Stri
for (GitHubOAuthScope s : getJenkins().getExtensionList(GitHubOAuthScope.class)) {
scopes.addAll(s.getScopesToRequest());
}
String suffix="";
String suffix = "";
if (!scopes.isEmpty()) {
suffix = "&scope="+Util.join(scopes,",");
suffix = "&scope=" + Util.join(scopes, ",");
} else {
// We need repo scope in order to access private repos
// See https://developer.github.com/v3/oauth/#scopes
suffix = "&scope=" + oauthScopes;
}

if (null != redirectUri && !redirectUri.isEmpty()) {
suffix += "&redirect_uri=" + redirectUri;
}

return new HttpRedirect(githubWebUri + "/login/oauth/authorize?client_id="
+ clientID + suffix);
}
Expand Down Expand Up @@ -712,14 +747,22 @@ public UserDetails loadUserByUsername(String username)
* @return true if the objects are the same instance and configuration.
*/
@Override
public boolean equals(Object object){
if(object instanceof GithubSecurityRealm) {
public boolean equals(Object object) {
if (object instanceof GithubSecurityRealm) {
GithubSecurityRealm obj = (GithubSecurityRealm) object;
return this.getGithubWebUri().equals(obj.getGithubWebUri()) &&
this.getGithubApiUri().equals(obj.getGithubApiUri()) &&
this.getClientID().equals(obj.getClientID()) &&
this.getClientSecret().equals(obj.getClientSecret()) &&
this.getOauthScopes().equals(obj.getOauthScopes());
String redirectUri2 = this.getRedirectUri();
if (null == redirectUri2) {
redirectUri2 = DEFAULT_REDIRECT_URI;
}
String redirectUri3 = obj.getRedirectUri();
if (null == redirectUri3) {
redirectUri3 = DEFAULT_REDIRECT_URI;
}
return this.getGithubWebUri().equals(obj.getGithubWebUri())
&& this.getGithubApiUri().equals(obj.getGithubApiUri())
&& this.getClientID().equals(obj.getClientID())
&& this.getClientSecret().equals(obj.getClientSecret())
&& this.getOauthScopes().equals(obj.getOauthScopes()) && redirectUri2.equals(redirectUri3);
} else {
return false;
}
Expand All @@ -733,6 +776,7 @@ public int hashCode() {
.append(this.getClientID())
.append(this.getClientSecret())
.append(this.getOauthScopes())
.append(this.getRedirectUri())
.toHashCode();
}

Expand Down
5 changes: 5 additions & 0 deletions src/main/resources/org/jenkinsci/plugins/GithubSecurityRealm/config.jelly
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 />
</f:entry>

</f:section>
</j:jelly>
3 changes: 3 additions & 0 deletions src/main/webapp/help/realm/redirect-uri-help.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
<div>
An optional redirect URI to be used by GitHub. See <a href="https://developer.github.com/enterprise/2.10/apps/building-oauth-apps/authorization-options-for-oauth-apps/#redirect-urls">https://developer.github.com/enterprise/2.10/apps/building-oauth-apps/authorization-options-for-oauth-apps/#redirect-urls</a>.
</div>
4 changes: 3 additions & 1 deletion src/test/java/org/jenkinsci/plugins/GithubAccessTokenPropertySEC797Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,13 +270,15 @@ private void setupRealm() {
String clientID = "xxx";
String clientSecret = "yyy";
String oauthScopes = "read:org";
String redirectUri = "";

GithubSecurityRealm githubSecurityRealm = new GithubSecurityRealm(
githubWebUri,
githubApiUri,
clientID,
clientSecret,
oauthScopes
oauthScopes,
redirectUri
);

j.jenkins.setSecurityRealm(githubSecurityRealm);
Expand Down
4 changes: 3 additions & 1 deletion src/test/java/org/jenkinsci/plugins/GithubAccessTokenPropertyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,13 +266,15 @@ private void setupRealm(){
String clientID = "xxx";
String clientSecret = "yyy";
String oauthScopes = "read:org";
String redirectUri = "";

GithubSecurityRealm githubSecurityRealm = new GithubSecurityRealm(
githubWebUri,
githubApiUri,
clientID,
clientSecret,
oauthScopes
oauthScopes,
redirectUri
);

j.jenkins.setSecurityRealm(githubSecurityRealm);
Expand Down
28 changes: 22 additions & 6 deletions src/test/java/org/jenkinsci/plugins/GithubSecurityRealmTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ of this software and associated documentation files (the "Software"), to deal
import org.jvnet.hudson.test.JenkinsRule;

import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;


Expand All @@ -40,30 +41,39 @@ public class GithubSecurityRealmTest {

@Test
public void testEquals_true() {
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");
GithubSecurityRealm a = new GithubSecurityRealm("http://jenkins.acme.com", "http://jenkins.acme.com/api/v3", "someid", "somesecret", "read:org", "https://build-dev.intuit.com/test/securityRealm/finishLogin");
GithubSecurityRealm b = new GithubSecurityRealm("http://jenkins.acme.com", "http://jenkins.acme.com/api/v3", "someid", "somesecret", "read:org", "https://build-dev.intuit.com/test/securityRealm/finishLogin");
assertTrue(a.equals(b));
}

@Test
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");
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", "");
assertFalse(a.equals(b));
assertFalse(a.equals(""));
}

@Test
public void testEqualsRedirectUri_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", "https://build-dev.intuit.com/test/securityRealm/finishLogin");
assertFalse(a.equals(b));
assertFalse(a.equals(""));
}


@Test
public void testHasScope_true() {
GithubSecurityRealm a = new GithubSecurityRealm("http://jenkins.acme.com", "http://jenkins.acme.com/api/v3", "someid", "somesecret", "read:org,user,user:email");
GithubSecurityRealm a = new GithubSecurityRealm("http://jenkins.acme.com", "http://jenkins.acme.com/api/v3", "someid", "somesecret", "read:org,user,user:email", "");
assertTrue(a.hasScope("user"));
assertTrue(a.hasScope("read:org"));
assertTrue(a.hasScope("user:email"));
}

@Test
public void testHasScope_false() {
GithubSecurityRealm a = new GithubSecurityRealm("http://jenkins.acme.com", "http://jenkins.acme.com/api/v3", "someid", "somesecret", "read:org,user,user:email");
GithubSecurityRealm a = new GithubSecurityRealm("http://jenkins.acme.com", "http://jenkins.acme.com/api/v3", "someid", "somesecret", "read:org,user,user:email", "");
assertFalse(a.hasScope("somescope"));
}

Expand All @@ -84,4 +94,10 @@ public void testDescriptorImplGetDefaultOauthScopes() {
DescriptorImpl descriptor = new DescriptorImpl();
assertTrue("read:org,user:email,repo".equals(descriptor.getDefaultOauthScopes()));
}

@Test
public void testHashCode() {
GithubSecurityRealm a = new GithubSecurityRealm("http://jenkins.acme.com", "http://jenkins.acme.com/api/v3", "someid", "somesecret", "read:org", "https://build-dev.intuit.com/test/securityRealm/finishLogin");
assertNotNull(a.hashCode());
}
}