Skip to content

Commit c4159ef

Browse files
i386i386
committed
Reduce overall calls to Github
* Always lookup the user locally before asking Github * lazyily lookup authorities if needed. * Requires 2.7.1 for User.getUserById
1 parent f332e34 commit c4159ef

File tree

4 files changed

+101
-52
lines changed

4 files changed

+101
-52
lines changed

‎pom.xml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414

1515
<properties>
1616
<!-- Baseline Jenkins version you use to build the plugin. Users must have this version or newer to run. -->
17-
<jenkins.version>1.612</jenkins.version>
17+
<jenkins.version>2.7.1</jenkins.version>
1818
<!-- Java Level to use. Java 7 required when using core >= 1.612 -->
1919
<java.level>7</java.level>
2020
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>

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

Lines changed: 63 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -91,11 +91,22 @@ public class GithubAuthenticationToken extends AbstractAuthenticationToken {
9191
private static final Cache<String, Boolean> publicRepositoryCache =
9292
CacheBuilder.newBuilder().expireAfterWrite(1, CACHE_EXPIRY).build();
9393

94-
private static final Cache<String, GHUser> usersByIdCache =
94+
private static final Cache<String, GithubUser> usersByIdCache =
9595
CacheBuilder.newBuilder().expireAfterWrite(1, CACHE_EXPIRY).build();
9696

9797
private final List<GrantedAuthority> authorities = new ArrayList<GrantedAuthority>();
9898

99+
private static final GithubUser UNKNOWN_USER = new GithubUser(null);
100+
101+
/** Wrapper for cache **/
102+
static class GithubUser {
103+
public final GHUser user;
104+
105+
public GithubUser(GHUser user) {
106+
this.user = user;
107+
}
108+
}
109+
99110
public GithubAuthenticationToken(final String accessToken, final String githubServer) throws IOException {
100111
super(new GrantedAuthority[] {});
101112

@@ -300,18 +311,20 @@ public Boolean call() throws Exception {
300311
.getLogger(GithubAuthenticationToken.class.getName());
301312

302313
public GHUser loadUser(String username) throws IOException {
303-
GHUser user;
314+
GithubUser user;
304315
try {
305316
user = usersByIdCache.get(username);
306317
if (gh != null && user == null && isAuthenticated()) {
307-
user = getGitHub().getUser(username);
308-
usersByIdCache.put(user.getLogin(), user);
318+
GHUser ghUser = getGitHub().getUser(username);
319+
user = new GithubUser(ghUser);
320+
usersByIdCache.put(username, user);
309321
}
310322
} catch (IOException | ExecutionException e) {
311323
LOGGER.log(Level.FINEST, e.getMessage(), e);
312-
user = null;
324+
user = UNKNOWN_USER;
325+
usersByIdCache.put(username, UNKNOWN_USER);
313326
}
314-
return user;
327+
return user != null ? user.user : null;
315328
}
316329

317330
public GHOrganization loadOrganization(String organization) {
@@ -352,51 +365,55 @@ public GHTeam loadTeam(String organization, String team) {
352365
public GithubOAuthUserDetails getUserDetails(String username) throws IOException {
353366
GHUser user = loadUser(username);
354367
if (user != null) {
355-
List<GrantedAuthority> groups = new ArrayList<GrantedAuthority>();
356-
try {
357-
GHPersonSet<GHOrganization> orgs;
358-
if(myRealm == null) {
359-
Jenkins jenkins = Jenkins.getInstance();
360-
if (jenkins == null) {
361-
throw new IllegalStateException("Jenkins not started");
362-
}
363-
myRealm = (GithubSecurityRealm) jenkins.getSecurityRealm();
364-
}
365-
//Search for scopes that allow fetching team membership. This is documented online.
366-
//https://developer.github.com/v3/orgs/#list-your-organizations
367-
//https://developer.github.com/v3/orgs/teams/#list-user-teams
368-
if(this.userName.equals(username) && (myRealm.hasScope("read:org") || myRealm.hasScope("admin:org") || myRealm.hasScope("user") || myRealm.hasScope("repo"))) {
369-
//This allows us to search for private organization membership.
370-
orgs = getMyself().getAllOrganizations();
371-
} else {
372-
//This searches for public organization membership.
373-
orgs = user.getOrganizations();
368+
return new GithubOAuthUserDetails(user.getLogin(), this);
369+
}
370+
return null;
371+
}
372+
373+
public GrantedAuthority[] getGrantedAuthorities(GHUser user) {
374+
List<GrantedAuthority> groups = new ArrayList<GrantedAuthority>();
375+
try {
376+
GHPersonSet<GHOrganization> orgs;
377+
if(myRealm == null) {
378+
Jenkins jenkins = Jenkins.getInstance();
379+
if (jenkins == null) {
380+
throw new IllegalStateException("Jenkins not started");
374381
}
375-
for (GHOrganization ghOrganization : orgs) {
376-
String orgLogin = ghOrganization.getLogin();
377-
LOGGER.log(Level.FINE, "Fetch teams for user " + username + " in organization " + orgLogin);
378-
groups.add(new GrantedAuthorityImpl(orgLogin));
379-
try {
380-
if (!getMyself().isMemberOf(ghOrganization)) {
381-
continue;
382-
}
383-
Map<String, GHTeam> teams = ghOrganization.getTeams();
384-
for (Map.Entry<String, GHTeam> entry : teams.entrySet()) {
385-
GHTeam team = entry.getValue();
386-
if (team.hasMember(user)) {
387-
groups.add(new GrantedAuthorityImpl(orgLogin + GithubOAuthGroupDetails.ORG_TEAM_SEPARATOR
388-
+ team));
389-
}
382+
myRealm = (GithubSecurityRealm) jenkins.getSecurityRealm();
383+
}
384+
//Search for scopes that allow fetching team membership. This is documented online.
385+
//https://developer.github.com/v3/orgs/#list-your-organizations
386+
//https://developer.github.com/v3/orgs/teams/#list-user-teams
387+
if(this.userName.equals(user.getLogin()) && (myRealm.hasScope("read:org") || myRealm.hasScope("admin:org") || myRealm.hasScope("user") || myRealm.hasScope("repo"))) {
388+
//This allows us to search for private organization membership.
389+
orgs = getMyself().getAllOrganizations();
390+
} else {
391+
//This searches for public organization membership.
392+
orgs = user.getOrganizations();
393+
}
394+
for (GHOrganization ghOrganization : orgs) {
395+
String orgLogin = ghOrganization.getLogin();
396+
LOGGER.log(Level.FINE, "Fetch teams for user " + user.getLogin() + " in organization " + orgLogin);
397+
groups.add(new GrantedAuthorityImpl(orgLogin));
398+
try {
399+
if (!getMyself().isMemberOf(ghOrganization)) {
400+
continue;
401+
}
402+
Map<String, GHTeam> teams = ghOrganization.getTeams();
403+
for (Map.Entry<String, GHTeam> entry : teams.entrySet()) {
404+
GHTeam team = entry.getValue();
405+
if (team.hasMember(user)) {
406+
groups.add(new GrantedAuthorityImpl(orgLogin + GithubOAuthGroupDetails.ORG_TEAM_SEPARATOR
407+
+ team));
390408
}
391-
} catch (IOException | Error ignore) {
392-
LOGGER.log(Level.FINEST, "not enough rights to list teams from " + orgLogin, ignore);
393409
}
410+
} catch (IOException | Error ignore) {
411+
LOGGER.log(Level.FINEST, "not enough rights to list teams from " + orgLogin, ignore);
394412
}
395-
} catch(IOException e) {
396-
LOGGER.log(Level.FINE, e.getMessage(), e);
397413
}
398-
return new GithubOAuthUserDetails(user, groups.toArray(new GrantedAuthority[groups.size()]));
414+
} catch(IOException e) {
415+
LOGGER.log(Level.FINE, e.getMessage(), e);
399416
}
400-
return null;
417+
return groups.toArray(new GrantedAuthority[groups.size()]);
401418
}
402419
}

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

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,21 +3,46 @@
33
*/
44
package org.jenkinsci.plugins;
55

6+
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
67
import org.acegisecurity.GrantedAuthority;
78
import org.acegisecurity.userdetails.User;
89
import org.acegisecurity.userdetails.UserDetails;
910
import org.kohsuke.github.GHUser;
1011

12+
import javax.annotation.Nonnull;
13+
import java.io.IOException;
14+
1115
/**
1216
* @author Mike
1317
*
1418
*/
19+
@SuppressFBWarnings("EQ_DOESNT_OVERRIDE_EQUALS")
1520
public class GithubOAuthUserDetails extends User implements UserDetails {
1621

1722
private static final long serialVersionUID = 1L;
1823

19-
public GithubOAuthUserDetails(GHUser user, GrantedAuthority[] authorities) {
20-
super(user.getLogin(), "", true, true, true, true, authorities);
24+
private final GithubAuthenticationToken authenticationToken;
25+
26+
public GithubOAuthUserDetails(@Nonnull String login, @Nonnull GrantedAuthority[] authorities) {
27+
super(login, "", true, true, true, true, authorities);
28+
this.authenticationToken = null;
2129
}
2230

31+
public GithubOAuthUserDetails(@Nonnull String login, @Nonnull GithubAuthenticationToken authenticationToken) {
32+
super(login, "", true, true, true, true, null);
33+
this.authenticationToken = authenticationToken;
34+
}
35+
36+
@Override
37+
public GrantedAuthority[] getAuthorities() {
38+
if (super.getAuthorities() == null) {
39+
try {
40+
GHUser user = authenticationToken.loadUser(getUsername());
41+
setAuthorities(authenticationToken.getGrantedAuthorities(user));
42+
} catch (IOException e) {
43+
throw new RuntimeException(e);
44+
}
45+
}
46+
return super.getAuthorities();
47+
}
2348
}

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

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,6 @@ of this software and associated documentation files (the "Software"), to deal
6767
import org.kohsuke.github.GHMyself;
6868
import org.kohsuke.github.GHOrganization;
6969
import org.kohsuke.github.GHTeam;
70-
import org.kohsuke.github.GHUser;
7170
import org.kohsuke.stapler.DataBoundConstructor;
7271
import org.kohsuke.stapler.Header;
7372
import org.kohsuke.stapler.HttpRedirect;
@@ -393,7 +392,7 @@ public HttpResponse doFinishLogin(StaplerRequest request)
393392
}
394393
}
395394

396-
fireAuthenticated(new GithubOAuthUserDetails(self, auth.getAuthorities()));
395+
fireAuthenticated(new GithubOAuthUserDetails(self.getLogin(), auth.getAuthorities()));
397396
} else {
398397
Log.info("Github did not return an access token.");
399398
}
@@ -580,7 +579,6 @@ public DescriptorImpl getDescriptor() {
580579
@Override
581580
public UserDetails loadUserByUsername(String username)
582581
throws UsernameNotFoundException, DataAccessException {
583-
GHUser user = null;
584582

585583
Authentication token = SecurityContextHolder.getContext().getAuthentication();
586584

@@ -596,6 +594,15 @@ public UserDetails loadUserByUsername(String username)
596594
throw new UserMayOrMayNotExistException("Unexpected authentication type: " + token);
597595
}
598596

597+
/**
598+
* Always lookup the local user first. If we can't resolve it then we can burn an API request to Github for this user
599+
* Taken from hudson.security.HudsonPrivateSecurityRealm#loadUserByUsername(java.lang.String)
600+
*/
601+
User localUser = User.getById(username, false);
602+
if (localUser != null) {
603+
return new GithubOAuthUserDetails(username, authToken);
604+
}
605+
599606
try {
600607
GithubOAuthUserDetails userDetails = authToken.getUserDetails(username);
601608
if (userDetails == null)

0 commit comments

Comments
 (0)