Skip to content

Commit bb9d1ef

Browse files
samrocketmansamrocketman
authored
Merge pull request jenkinsci#115 from jenkinsci/bugfix-JENKINS-57154
[JENKINS-57154] Fix configureSecurity HTTP 403 err
2 parents 6165ab2 + 65f0d3d commit bb9d1ef

File tree

3 files changed

+36
-26
lines changed

3 files changed

+36
-26
lines changed

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

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,13 +32,14 @@ of this software and associated documentation files (the "Software"), to deal
3232
import com.squareup.okhttp.OkHttpClient;
3333
import com.squareup.okhttp.OkUrlFactory;
3434

35+
import hudson.model.Item;
3536
import hudson.security.Permission;
3637
import hudson.security.SecurityRealm;
37-
import hudson.model.Item;
3838
import jenkins.model.Jenkins;
3939
import org.acegisecurity.GrantedAuthority;
4040
import org.acegisecurity.GrantedAuthorityImpl;
4141
import org.acegisecurity.providers.AbstractAuthenticationToken;
42+
import org.acegisecurity.userdetails.UsernameNotFoundException;
4243
import org.kohsuke.github.GHMyself;
4344
import org.kohsuke.github.GHOrganization;
4445
import org.kohsuke.github.GHPersonSet;
@@ -198,7 +199,9 @@ public GithubAuthenticationToken(final String accessToken, final String githubSe
198199

199200
this.me = loadMyself(accessToken);
200201

201-
assert this.me!=null;
202+
if(this.me == null) {
203+
throw new UsernameNotFoundException("Token not valid");
204+
}
202205

203206
setAuthenticated(true);
204207

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

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -24,11 +24,12 @@
2424
package org.jenkinsci.plugins;
2525

2626
import hudson.model.User;
27-
import org.jfree.util.Log;
2827

28+
import java.io.IOException;
29+
import java.util.logging.Level;
30+
import java.util.logging.Logger;
2931
import javax.annotation.CheckForNull;
3032
import javax.annotation.Nonnull;
31-
import java.io.IOException;
3233

3334
public class GithubSecretStorage {
3435

@@ -43,20 +44,25 @@ public static boolean contains(@Nonnull User user) {
4344
public static @CheckForNull String retrieve(@Nonnull User user) {
4445
GithubAccessTokenProperty property = user.getProperty(GithubAccessTokenProperty.class);
4546
if (property == null) {
46-
Log.debug("Cache miss for username: " + user.getId());
47+
LOGGER.log(Level.FINE, "Cache miss for username: " + user.getId());
4748
return null;
4849
} else {
49-
Log.debug("Token retrieved using cache for username: " + user.getId());
50+
LOGGER.log(Level.FINE, "Token retrieved using cache for username: " + user.getId());
5051
return property.getAccessToken().getPlainText();
5152
}
5253
}
5354

5455
public static void put(@Nonnull User user, @Nonnull String accessToken) {
55-
Log.debug("Populating the cache for username: " + user.getId());
56+
LOGGER.log(Level.FINE, "Populating the cache for username: " + user.getId());
5657
try {
5758
user.addProperty(new GithubAccessTokenProperty(accessToken));
5859
} catch (IOException e) {
59-
Log.warn("Received an exception when trying to add the GitHub access token to the user: " + user.getId(), e);
60+
LOGGER.log(Level.WARNING, "Received an exception when trying to add the GitHub access token to the user: " + user.getId(), e);
6061
}
6162
}
63+
64+
/**
65+
* Logger for debugging purposes.
66+
*/
67+
private static final Logger LOGGER = Logger.getLogger(GithubSecretStorage.class.getName());
6268
}

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

Lines changed: 19 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,6 @@ of this software and associated documentation files (the "Software"), to deal
6868
import org.apache.http.impl.client.CloseableHttpClient;
6969
import org.apache.http.impl.client.HttpClients;
7070
import org.apache.http.util.EntityUtils;
71-
import org.jfree.util.Log;
7271
import org.kohsuke.args4j.Option;
7372
import org.kohsuke.github.GHEmail;
7473
import org.kohsuke.github.GHMyself;
@@ -92,6 +91,7 @@ of this software and associated documentation files (the "Software"), to deal
9291
import java.util.Arrays;
9392
import java.util.HashSet;
9493
import java.util.Set;
94+
import java.util.logging.Level;
9595
import java.util.logging.Logger;
9696
import javax.annotation.Nonnull;
9797
import javax.annotation.Nullable;
@@ -382,18 +382,18 @@ public HttpResponse doFinishLogin(StaplerRequest request)
382382
String expectedState = (String)request.getSession().getAttribute(STATE_ATTRIBUTE);
383383

384384
if (code == null || code.trim().length() == 0) {
385-
Log.info("doFinishLogin: missing code.");
385+
LOGGER.info("doFinishLogin: missing code.");
386386
return HttpResponses.redirectToContextRoot();
387387
}
388388

389389
if (state == null){
390-
Log.info("doFinishLogin: missing state parameter from Github response.");
390+
LOGGER.info("doFinishLogin: missing state parameter from Github response.");
391391
return HttpResponses.redirectToContextRoot();
392392
} else if (expectedState == null){
393-
Log.info("doFinishLogin: missing state parameter from user's session.");
393+
LOGGER.info("doFinishLogin: missing state parameter from user's session.");
394394
return HttpResponses.redirectToContextRoot();
395395
} else if (!state.equals(expectedState)){
396-
Log.info("state parameter value ["+state+"] does not match the expected one ["+expectedState+"]");
396+
LOGGER.info("state parameter value ["+state+"] does not match the expected one ["+expectedState+"]");
397397
return HttpResponses.redirectToContextRoot();
398398
}
399399

@@ -445,7 +445,7 @@ public HttpResponse doFinishLogin(StaplerRequest request)
445445
// or modifications in organizations will be not reflected when using API Token, due to that caching
446446
// SecurityListener.fireLoggedIn(self.getLogin());
447447
} else {
448-
Log.info("Github did not return an access token.");
448+
LOGGER.info("Github did not return an access token.");
449449
}
450450

451451
if (referer!=null) return HttpResponses.redirectTo(referer);
@@ -458,7 +458,7 @@ private String getAccessToken(@Nonnull String code) throws IOException {
458458
try (CloseableHttpClient httpClient = HttpClients.createDefault()) {
459459
HttpPost httpost = new HttpPost(githubWebUri
460460
+ "/login/oauth/access_token?" + "client_id=" + clientID + "&"
461-
+ "client_secret=" + clientSecret + "&" + "code=" + code);
461+
+ "client_secret=" + clientSecret.getPlainText() + "&" + "code=" + code);
462462
HttpHost proxy = getProxy(httpost);
463463
if (proxy != null) {
464464
RequestConfig requestConfig = RequestConfig.custom().setProxy(proxy).build();
@@ -693,17 +693,17 @@ public UserDetails loadUserByUsername(String username)
693693

694694
Authentication token = SecurityContextHolder.getContext().getAuthentication();
695695

696-
if (token == null || !username.equals(token.getPrincipal())) {
697-
if(localUser != null && GithubSecretStorage.contains(localUser)){
696+
try {
697+
if(localUser != null && GithubSecretStorage.contains(localUser)) {
698698
String accessToken = GithubSecretStorage.retrieve(localUser);
699-
try {
700-
token = new GithubAuthenticationToken(accessToken, getGithubApiUri());
701-
} catch (IOException e) {
702-
throw new UserMayOrMayNotExistException("Could not connect to GitHub API server, target URL = " + getGithubApiUri(), e);
703-
}
704-
SecurityContextHolder.getContext().setAuthentication(token);
705-
}else{
706-
throw new UserMayOrMayNotExistException("Could not get auth token.");
699+
token = new GithubAuthenticationToken(accessToken, getGithubApiUri());
700+
}
701+
} catch(IOException | UsernameNotFoundException e) {
702+
if(e instanceof IOException) {
703+
throw new UserMayOrMayNotExistException("Could not connect to GitHub API server, target URL = " + getGithubApiUri(), e);
704+
} else {
705+
// user not found so continuing normally re-using the current context holder
706+
LOGGER.log(Level.FINE, "Attempted to impersonate " + username + " but token in user property was invalid.");
707707
}
708708
}
709709

@@ -725,8 +725,9 @@ public UserDetails loadUserByUsername(String username)
725725

726726
try {
727727
GithubOAuthUserDetails userDetails = authToken.getUserDetails(username);
728-
if (userDetails == null)
728+
if (userDetails == null) {
729729
throw new UsernameNotFoundException("Unknown user: " + username);
730+
}
730731

731732
// Check the username is not an homonym of an organization
732733
GHOrganization ghOrg = authToken.loadOrganization(username);

0 commit comments

Comments
 (0)