10000 [JENKINS-42509] authenticated team members should have read/build (#91) · jpigree/github-oauth-plugin@7a4539f · GitHub
[go: up one dir, main page]
Skip to content

Commit 7a4539f

Browse files
sgtcoolguysamrocketman
sgtcoolguy
authored and
samrocketman
committed
[JENKINS-42509] authenticated team members should have read/build (jenkinsci#91)
* [JENKINS-42509] authenticated team members should have read/build permissions when using Github Committer Authorization Strategy On private repositories of which the user is not an owner, not a member of the owning organization - check for admin/push/pull permissions on the repository to determine permissions on the Jenkisn item. * - Use a cache for loading repositories.- Guard against even trying to load repositories unless we have either the "repo" or "public_repo" oauth scopes. - Add "repo" to the default set of oauth scopes requested. * Add a wrapper POJO for storing GHRepository rights per-user in our cache. Make the repo cache an instance cache since it's specific to a user. Remove a coupel unnecessary final designations on method paramters.
1 parent 8a4c2bb commit 7a4539f

File tree

5 files changed

+150
-54
lines changed

5 files changed

+150
-54
lines changed

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

Lines changed: 108 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -26,12 +26,15 @@ of this software and associated documentation files (the "Software"), to deal
2626
*/
2727
package org.jenkinsci.plugins;
2828

29+
import com.google.common.base.Optional;
2930
import com.google.common.cache.Cache;
3031
import com.google.common.cache.CacheBuilder;
3132
import com.squareup.okhttp.OkHttpClient;
3233
import com.squareup.okhttp.OkUrlFactory;
3334

35+
import hudson.security.Permission;
3436
import hudson.security.SecurityRealm;
37+
import hudson.model.Item;
3538
import jenkins.model.Jenkins;
3639
import org.acegisecurity.GrantedAuthority;
3740
import org.acegisecurity.GrantedAuthorityImpl;
@@ -92,23 +95,31 @@ public class GithubAuthenticationToken extends AbstractAuthenticationToken {
9295
private static final Cache<String, Set<String>> userOrganizationCache =
9396
CacheBuilder.newBuilder().expireAfterWrite(1, CACHE_EXPIRY).build();
9497

95-
private static final Cache<String, Set<String>> repositoryCollaboratorsCache =
96-
CacheBuilder.newBuilder().expireAfterWrite(1, CACHE_EXPIRY).build();
97-
9898
private static final Cache<String, Set<String>> repositoriesByUserCache =
9999
CacheBuilder.newBuilder().expireAfterWrite(1, CACHE_EXPIRY).build();
100100

101-
private static final Cache<String, Boolean> publicRepositoryCache =
101+
private static final Cache<String, GithubUser> usersByIdCache =
102102
CacheBuilder.newBuilder().expireAfterWrite(1, CACHE_EXPIRY).build();
103103

104-
private static final Cache<String, GithubUser> usersByIdCache =
104+
/**
105+
* This cache is for repositories and is explicitly _not_ static because we
106+
* want to store repo information per-user (and this class should be per-user).
107+
* We potentially could hold a separe static cache for public repo info
108+
* that applies to all users, but it wouldn't be able to contain user-specific
109+
* details like exact permissions (read/write/admin).
110+
*
111+
* This representation of the repo holds details on whether the repo is
112+
* public/private, as well as whether the current user has pull/push/admin
113+
* access.
114+
*/
115+
private final Cache<String, RepoRights> repositoryCache =
105116
CacheBuilder.newBuilder().expireAfterWrite(1, CACHE_EXPIRY).build();
106117

107118
private final List<GrantedAuthority> authorities = new ArrayList<GrantedAuthority>();
108119

109120
private static final GithubUser UNKNOWN_USER = new GithubUser(null);
110121

111-
/** Wrapper for cache **/
122+
/** Wrappers for cache **/
112123
static class GithubUser {
113124
public final GHUser user;
114125

@@ -117,6 +128,45 @@ public GithubUser(GHUser user) {
117128
}
118129
}
119130

131+
static class RepoRights {
132+
public final boolean hasAdminAccess;
133+
public final boolean hasPullAccess;
134+
public final boolean hasPushAccess;
135+
public final boolean isPrivate;
136+
137+
public RepoRights(GHRepository repo) {
138+
if (repo != null) {
139+
this.hasAdminAccess = repo.hasAdminAccess();
140+
this.hasPullAccess = repo.hasPullAccess();
141+
this.hasPushAccess = repo.hasPushAccess();
142+
this.isPrivate = repo.isPrivate();
143+
} else {
144+
// assume null repo means we had no rights to view it
145+
// so must be private
146+
this.hasAdminAccess = false;
147+
this.hasPullAccess = false;
148+
this.hasPushAccess = false;
149+
this.isPrivate = true;
150+
}
151+
}
152+
153+
public boolean hasAdminAccess() {
154+
return this.hasAdminAccess;
155+
}
156+
157+
public boolean hasPullAccess() {
158+
return this.hasPullAccess;
159+
}
160+
161+
public boolean hasPushAccess() {
162+
return this.hasPushAccess;
163+
}
164+
165+
public boolean isPrivate() {
166+
return this.isPrivate;
167+
}
168+
}
169+
120170
public GithubAuthenticationToken(final String accessToken, final String githubServer) throws IOException {
121171
super(new GrantedAuthority[] {});
122172

@@ -170,7 +220,6 @@ public GithubAuthenticationToken(final String accessToken, final String githubSe
170220
*/
171221
public static void clearCaches() {
172222
userOrganizationCache.invalidateAll();
173-
repositoryCollaboratorsCache.invalidateAll();
174223
repositoriesByUserCache.invalidateAll();
175224
usersByIdCache.invalidateAll();
176225
}
@@ -193,14 +242,14 @@ public String getGithubServer() {
193242

194243
public GitHub getGitHub() throws IOException {
195244
if (this.gh == null) {
196-
245+
197246
String host;
198247
try {
199248
host = new URL(this.githubServer).getHost();
200249
} catch (MalformedURLException e) {
201250
throw new IOException("Invalid GitHub API URL: " + this.githubServer, e);
202251
}
203-
252+
204253
OkHttpClient client = new OkHttpClient().setProxy(getProxy(host));
205254

206255
this.gh = GitHubBuilder.fromEnvironment()
@@ -212,7 +261,7 @@ public GitHub getGitHub() throws IOException {
212261
}
213262
return gh;
214263
}
215-
264+
216265
/**
217266
* Uses proxy if configured on pluginManager/advanced page
218267
*
@@ -289,8 +338,32 @@ public Set<String> call() throws Exception {
289338
}
290339
}
291340

292-
public boolean hasRepositoryPermission(final String repositoryName) {
293-
return myRepositories().contains(repositoryName);
341+
public boolean hasRepositoryPermission(String repositoryName, Permission permission) {
342+
LOGGER.log(Level.FINEST, "Checking for permission: " + permission + " on repo: " + repositoryName + " for user: " + this.userName);
343+
boolean isRepoOfMine = myRepositories().contains(repositoryName);
344+
if (isRepoOfMine) {
345+
return true;
346+
}
347+
// This is not my repository, nor is it a repository of an organization I belong to.
348+
// Check what rights I have on the github repo.
349+
RepoRights repository = loadRepository(repositoryName);
350+
if (repository == null) {
351+
return false;
352+
}
353+
// let admins do anything
354+
if (repository.hasAdminAccess()) {
355+
return true;
356+
}
357+
// WRITE or READ can Read/Build/View Workspace
358+
if (permission.equals(Item.READ) || permission.equals(Item.BUILD) || permission.equals(Item.WORKSPACE)) {
359+
return repository.hasPullAccess() || repository.hasPushAccess();
360+
}
361+
// WRITE can cancel builds or view config
362+
if (permission.equals(Item.CANCEL) || permission.equals(Item.EXTENDED_READ)) {
363+
return repository.hasPushAccess();
364+
}
365+
// Need ADMIN rights to do rest: configure, create, delete, discover, wipeout
366+
return false;
294367
}
295368

296369
public Set<String> myRepositories() {
@@ -328,23 +401,9 @@ public Set<String> listToNames(Collection<GHRepository> respositories) throws IO
328401
return names;
329402
}
330403

331-
public boolean isPublicRepository(final String repositoryName) {
332-
try {
333-
return publicRepositoryCache.get(repositoryName,
334-
new Callable<Boolean>() {
335-
@Override
336-
public Boolean call() throws Exception {
337-
GHRepository repository = loadRepository(repositoryName);
338-
// We don't have access so it must not be public (it could be non-existant)
339-
return repository != null && !repository.isPrivate();
340-
}
341-
}
342-
);
343-
} catch (ExecutionException e) {
344-
LOGGER.log(Level.SEVERE, "an exception was thrown", e);
345-
throw new RuntimeException("authorization failed for user = "
346-
+ getName(), e);
347-
}
404+
public boolean isPublicRepository(String repositoryName) {
405+
RepoRights repository = loadRepository(repositoryName);
406+
return repository != null && !repository.isPrivate();
348407
}
349408

350409
private static final Logger LOGGER = Logger
@@ -377,17 +436,26 @@ public GHOrganization loadOrganization(String organization) {
377436
return null;
378437
}
379438

380-
public GHRepository loadRepository(String repositoryName) {
381-
try {
382-
if (gh != null && isAuthenticated()) {
383-
return getGitHub().getRepository(repositoryName);
384-
}
385-
} catch (IOException e) {
386-
LOGGER.log(Level.WARNING,
387-
"Looks like a bad GitHub URL OR the Jenkins user does not have access to the repository{0}",
388-
repositoryName);
389-
}
390-
return null;
439+
public RepoRights loadRepository(final String repositoryName) {
440+
try {
441+
if (gh != null && isAuthenticated() && (myRealm.hasScope("repo") || myRealm.hasScope("public_repo"))) {
442+
return repositoryCache.get(repositoryName,
443+
new Callable<RepoRights>() {
444+
@Override
445+
public RepoRights call() throws Exception {
446+
GHRepository repo = getGitHub().getRepository(repositoryName);
447+
return new RepoRights(repo);
448+
}
449+
}
450+
);
451+
}
452+
} catch (Exception e) {
453+
LOGGER.log(Level.SEVERE, "an exception was thrown", e);
454+
LOGGER.log(Level.WARNING,
455+
"Looks like a bad GitHub URL OR the Jenkins user {0} does not have access to the repository {1}. May need to add 'repo' or 'public_repo' to the list of oauth scopes requested.",
456+
new Object[] { this.userName, repositoryName });
457+
}
458+
return null;
391459
}
392460

393461
public GHTeam loadTeam(String organization, String team) {

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -273,7 +273,7 @@ public boolean hasRepositoryPermission(GithubAuthenticationToken authenticationT
273273
authenticationToken.isPublicRepository(repositoryName)) {
274274
return true;
275275
} else {
276-
return authenticationToken.hasRepositoryPermission(repositoryName);
276+
return authenticationToken.hasRepositoryPermission(repositoryName, permission);
277277
}
278278
}
279279

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ public class GithubSecurityRealm extends AbstractPasswordBasedSecurityRealm impl
106106
private static final String DEFAULT_WEB_URI = "https://github.com";
107107
private static final String DEFAULT_API_URI = "https://api.github.com";
108108
private static final String DEFAULT_ENTERPRISE_API_SUFFIX = "/api/v3";
109-
private static final String DEFAULT_OAUTH_SCOPES = "read:org,user:email";
109+
private static final String DEFAULT_OAUTH_SCOPES = "read:org,user:email,repo";
110110

111111
private String githubWebUri;
112112
private String githubApiUri;
@@ -566,7 +566,7 @@ public String getLoginUrl() {
566566
@Override
567567
protected String getPostLogOutUrl(StaplerRequest req, Authentication auth) {
568568
// if we just redirect to the root and anonymous does not have Overall read then we will start a login all over again.
569-
// we are actually anonymous here as the security context has been cleared
569+
// we are actually anonymous here as the security context has been cleared
570570
Jenkins j = Jenkins.getInstance();
571571
assert j != null;
572572
if (j.hasPermission(Jenkins.READ)) {

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

Lines changed: 38 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,8 @@ public class GithubRequireOrganizationMembershipACLTest extends TestCase {
9292
@Mock
9393
private Jenkins jenkins;
9494

95+
private GitHub gh;
96+
9597
@Mock
9698
private GithubSecurityRealm securityRealm;
9799

@@ -101,7 +103,9 @@ public void setUp() throws Exception {
101103
PowerMockito.mockStatic(Jenkins.class);
102104
PowerMockito.when(Jenkins.getInstance()).thenReturn(jenkins);
103105
PowerMockito.when(jenkins.getSecurityRealm()).thenReturn(securityRealm);
104-
PowerMockito.when(securityRealm.getOauthScopes()).thenReturn("read:org");
106+
PowerMockito.when(securityRealm.getOauthScopes()).thenReturn("read:org,repo");
107+
PowerMockito.when(securityRealm.hasScope("read:org")).thenReturn(true);
108+
PowerMockito.when(securityRealm.hasScope("repo")).thenReturn(true);
105109
}
106110

107111
private static final Permission VIEW_JOBSTATUS_PERMISSION = new Permission(Item.PERMISSIONS,
@@ -170,7 +174,7 @@ private GithubRequireOrganizationMembershipACL aclForWorkflowJob(WorkflowJob wor
170174
}
171175

172176
private GHMyself mockGHMyselfAs(String username) throws IOException {
173-
GitHub gh = PowerMockito.mock(GitHub.class);
177+
gh = PowerMockito.mock(GitHub.class);
174178
GitHubBuilder builder = PowerMockito.mock(GitHubBuilder.class);
175179
PowerMockito.mockStatic(GitHub.class);
176180
PowerMockito.mockStatic(GitHubBuilder.class);
@@ -264,14 +268,6 @@ private MultiBranchProject mockMultiBranchProject(String url) {
264268
return multiBranchProject;
265269
}
266270

267-
private void mockGithubRepositoryWithCollaborators(GitHub mockGithub, String name, boolean isPrivate, List<String> collaboratorNames) throws IOException {
268-
GHRepository ghRepository = PowerMockito.mock(GHRepository.class);
269-
PowerMockito.when(mockGithub.getRepository(name)).thenReturn(ghRepository);
270-
PowerMockito.when(ghRepository.isPrivate()).thenReturn(isPrivate);
271-
Set<String> names = new HashSet(collaboratorNames);
272-
PowerMockito.when(ghRepository.getCollaboratorNames()).thenReturn(names);
273-
}
274-
275271
@Test
276272
public void testCanReadAndBuildOneOfMyRepositories() throws IOException {
277273
GHMyself me = mockGHMyselfAs("Me");
@@ -296,6 +292,7 @@ public void testCanReadAndBuildOneOfMyRepositories() throws IOException {
296292

297293
@Override
298294
protected void tearDown() throws Exception {
295+
gh = null;
299296
super.tearDown();
300297
GithubAuthenticationToken.clearCaches();
301298
}
@@ -323,6 +320,37 @@ public void testCanReadAndBuildOrgRepositoryICollaborateOn() throws IOException
323320
assertTrue(workflowJobAcl.hasPermission(authenticationToken, Item.BUILD));
324321
}
325322

323+
@Test
324+
public void testCanReadAndBuildOtherOrgPrivateRepositoryICollaborateOn() throws IOException {
325+
GHMyself me = mockGHMyselfAs("Me");
326+
mockReposFor(me, Arrays.asList("me/a-repo"));
327+
mockOrgRepos(me, ImmutableMap.of("some-org", Arrays.asList("some-org/a-private-repo")));
328+
GHRepository ghRepository = PowerMockito.mock(GHRepository.class);
329+
PowerMockito.when(gh.getRepository("org-i-dont-belong-to/a-private-repo-i-collaborate-on")).thenReturn(ghRepository);
330+
PowerMockito.when(ghRepository.isPrivate()).thenReturn(true);
331+
PowerMockito.when(ghRepository.hasAdminAccess()).thenReturn(false);
332+
PowerMockito.when(ghRepository.hasPushAccess()).thenReturn(false);
333+
PowerMockito.when(ghRepository.hasPullAccess()).thenReturn(true);
334+
335+
// The user isn't part of "org-i-dont-belong-to"
336+
String repoUrl = "https://github.com/org-i-dont-belong-to/a-private-repo-i-collaborate-on.git";
337+
Project mockProject = mockProject(repoUrl);
338+
MultiBranchProject mockMultiBranchProject = mockMultiBranchProject(repoUrl);
339+
WorkflowJob mockWorkflowJob = mockWorkflowJob(repoUrl);
340+
GithubRequireOrganizationMembershipACL workflowJobAcl = aclForWorkflowJob(mockWorkflowJob);
341+
GithubRequireOrganizationMembershipACL multiBranchProjectAcl = aclForMultiBranchProject(mockMultiBranchProject);
342+
GithubRequireOrganizationMembershipACL projectAcl = aclForProject(mockProject);
343+
344+
GithubAuthenticationToken authenticationToken = new GithubAuthenticationToken("accessToken", "https://api.github.com");
345+
346+
assertTrue(projectAcl.hasPermission(authenticationToken, Item.READ));
347+
assertTrue(projectAcl.hasPermission(authenticationToken, Item.BUILD));
348+
assertTrue(multiBranchProjectAcl.hasPermission(authenticationToken, Item.READ));
349+
assertTrue(multiBranchProjectAcl.hasPermission(authenticationToken, Item.BUILD));
350+
assertTrue(workflowJobAcl.hasPermission(authenticationToken, Item.READ));
351+
assertTrue(workflowJobAcl.hasPermission(authenticationToken, Item.BUILD));
352+
}
353+
326354
@Test
327355
public void testCanNotReadOrBuildRepositoryIDoNotCollaborateOn() throws IOException {
328356
GHMyself me = mockGHMyselfAs("Me");

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,6 @@ public void testDescriptorImplGetDefaultGithubApiUri() {
8282
@Test
8383
public void testDescriptorImplGetDefaultOauthScopes() {
8484
DescriptorImpl descriptor = new DescriptorImpl();
85-
assertTrue("read:org,user:email".equals(descriptor.getDefaultOauthScopes()));
85+
assertTrue("read:org,user:email,repo".equals(descriptor.getDefaultOauthScopes()));
8686
}
8787
}

0 commit comments

Comments
 (0)
0