8000 Merge pull requests #107 #108 #109 · ojacques/github-oauth-plugin@f4a6494 · GitHub
[go: up one dir, main page]
Skip to content

Commit f4a6494

Browse files
samrocketmansamrocketman
committed
This octomerge simplifies merging multiple pull requests. They've all been tested in conjunction and don't cause any upgrade issues.
4 parents c5a53c3 + 962a063 + f941461 + 6c9f6d9 commit f4a6494

File tree

4 files changed

+45
-6
lines changed

4 files changed

+45
-6
lines changed

‎pom.xml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,7 @@
164164
<version>1.109</version>
165165
<extensions>true</extensions>
166166
<configuration>
167-
<compatibleSinceVersion>1.93</compatibleSinceVersion>
167+
<compatibleSinceVersion>0.3</compatibleSinceVersion>
168168
</configuration>
169169
</plugin>
170170
<plugin>

‎src/main/java/org/jenkinsci/plugins/GithubSecurityRealm.java

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@ of this software and associated documentation files (the "Software"), to deal
8888
import java.io.IOException;
8989
import java.net.InetSocketAddress;
9090
import java.net.Proxy;
91+
import java.security.SecureRandom;
9192
import java.util.Arrays;
9293
import java.util.HashSet;
9394
import java.util.Set;
@@ -337,6 +338,9 @@ public String getOauthScopes() {
337338

338339
public HttpResponse doCommenceLogin(StaplerRequest request, @QueryParameter String from, @Header("Referer") final String referer)
339340
throws IOException {
341+
// https://tools.ietf.org/html/rfc6749#section-10.10 dictates that probability that an attacker guesses the string
342+
// SHOULD be less than or equal to 2^(-160) and our Strings consist of 65 chars. (65^27 ~= 2^160)
343+
final String state = getSecureRandomString(27);
340344
String redirectOnFinish;
341345
if (from != null && Util.isSafeToRedirectTo(from)) {
342346
redirectOnFinish = from;
@@ -347,18 +351,19 @@ public HttpResponse doCommenceLogin(StaplerRequest request, @QueryParameter Stri
347351
}
348352

349353
request.getSession().setAttribute(REFERER_ATTRIBUTE, redirectOnFinish);
354+
request.getSession().setAttribute(STATE_ATTRIBUTE, state);
350355

351356
Set<String> scopes = new HashSet<>();
352357
for (GitHubOAuthScope s : getJenkins().getExtensionList(GitHubOAuthScope.class)) {
353358
scopes.addAll(s.getScopesToRequest());
354359
}
355360
String suffix="";
356361
if (!scopes.isEmpty()) {
357-
suffix = "&scope="+Util.join(scopes,",");
362+
suffix = "&scope="+Util.join(scopes,",")+"&state="+state;
358363
} else {
359364
// We need repo scope in order to access private repos
360365
// See https://developer.github.com/v3/oauth/#scopes
361-
suffix = "&scope=" + oauthScopes;
366+
suffix = "&scope=" + oauthScopes +"&state="+state;
362367
}
363368

364369
return new HttpRedirect(githubWebUri + "/login/oauth/authorize?client_id="
@@ -372,13 +377,27 @@ public HttpResponse doCommenceLogin(StaplerRequest request, @QueryParameter Stri
372377
public HttpResponse doFinishLogin(StaplerRequest request)
373378
throws IOException {
374379
String code = request.getParameter("code");
380+
String state = request.getParameter(STATE_ATTRIBUTE);
375381
String referer = (String)request.getSession().getAttribute(REFERER_ATTRIBUTE);
382+
String expectedState = (String)request.getSession().getAttribute(STATE_ATTRIBUTE);
376383

377384
if (code == null || code.trim().length() == 0) {
378385
Log.info("doFinishLogin: missing code.");
379386
return HttpResponses.redirectToContextRoot();
380387
}
381388

389+
if (state == null){
390+
Log.info("doFinishLogin: missing state parameter from Github response.");
391+
return HttpResponses.redirectToContextRoot();
392+
} else if (expectedState == null){
393+
Log.info("doFinishLogin: missing state parameter from user's session.");
394+
return HttpResponses.redirectToContextRoot();
395+
} else if (!state.equals(expectedState)){
396+
Log.info("state parameter value ["+state+"] does not match the expected one ["+expectedState+"]");
397+
return HttpResponses.redirectToContextRoot();
398+
}
399+
400+
382401
String accessToken = getAccessToken(code);
383402

384403
if (accessToken != null && accessToken.trim().length() > 0) {
@@ -460,6 +479,21 @@ private String getAccessToken(@Nonnull String code) throws IOException {
460479
return null;
461480
}
462481

482+
/**
483+
* Generates a random URL Safe String of n characters
484+
*/
485+
private String getSecureRandomString(int n) {
486+
if (n < 0){
487+
throw new IllegalArgumentException("Length must be a positive integer");
488+
}
489+
// See RFC3986
490+
final String urlSafeChars = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789.-_";
491+
final StringBuilder sb = new StringBuilder();
492+
for (int i = 0; i < n ; i++){
493+
sb.append(urlSafeChars.charAt(SECURE_RANDOM.nextInt(urlSafeChars.length())));
494+
}
495+
return sb.toString();
496+
}
463497
/**
464498
* Returns the proxy to be used when connecting to the given URI.
465499
*/
@@ -659,7 +693,7 @@ public UserDetails loadUserByUsername(String username)
659693

660694
Authentication token = SecurityContextHolder.getContext().getAuthentication();
661695

662-
if (token == null) {
696+
if (token == null || !username.equals(token.getPrincipal())) {
663697
if(localUser != null && GithubSecretStorage.contains(localUser)){
664698
String accessToken = GithubSecretStorage.retrieve(localUser);
665699
try {
@@ -789,6 +823,9 @@ static Jenkins getJenkins() {
789823
private static final Logger LOGGER = Logger.getLogger(GithubSecurityRealm.class.getName());
790824

791825
private static final String REFERER_ATTRIBUTE = GithubSecurityRealm.class.getName()+".referer";
826+
private static final String STATE_ATTRIBUTE = "state";
827+
828+
private static final SecureRandom SECURE_RANDOM = new SecureRandom();
792829

793830
/**
794831
* Asks for the password.

‎src/test/java/org/jenkinsci/plugins/GithubAccessTokenPropertySEC797Test.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -249,7 +249,8 @@ private void onUserTeams(HttpServletRequest req, HttpServletResponse resp) throw
249249

250250
private void onLoginOAuthAuthorize(HttpServletRequest req, HttpServletResponse resp) throws IOException {
251251
String code = "test";
252-
resp.sendRedirect(jenkinsRule.getURL() + "securityRealm/finishLogin?code=" + code);
252+
String state = req.getParameter("state");
253+
resp.sendRedirect(jenkinsRule.getURL() + "securityRealm/finishLogin?code=" + code + "&state=" + state);
253254
}
254255

255256
private void onLoginOAuthAccessToken(HttpServletRequest req, HttpServletResponse resp) throws IOException {

‎src/test/java/org/jenkinsci/plugins/GithubAccessTokenPropertyTest.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -245,7 +245,8 @@ private void onUserTeams(HttpServletRequest req, HttpServletResponse resp) throw
245245

246246
private void onLoginOAuthAuthorize(HttpServletRequest req, HttpServletResponse resp) throws IOException {
247247
String code = "test";
248-
resp.sendRedirect(jenkinsRule.getURL() + "securityRealm/finishLogin?code=" + code);
248+
String state = req.getParameter("state");
249+
resp.sendRedirect(jenkinsRule.getURL() + "securityRealm/finishLogin?code=" + code + "&state=" + state);
249250
}
250251

251252
private void onLoginOAuthAccessToken(HttpServletRequest req, HttpServletResponse resp) throws IOException {

0 commit comments

Comments
 (0)
0