[JENKINS-42509] authenticated team members should have read/build (#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.

Author: sgtcoolguy

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

@@ -26,12 +26,15 @@ of this software and associated documentation files (the "Software"), to deal
  */
 package org.jenkinsci.plugins;
 
+import com.google.common.base.Optional;
 import com.google.common.cache.Cache;
 import com.google.common.cache.CacheBuilder;
 import com.squareup.okhttp.OkHttpClient;
 import com.squareup.okhttp.OkUrlFactory;
 
+import hudson.security.Permission;
 import hudson.security.SecurityRealm;
+import hudson.model.Item;
 import jenkins.model.Jenkins;
 import org.acegisecurity.GrantedAuthority;
 import org.acegisecurity.GrantedAuthorityImpl;
@@ -92,23 +95,31 @@ public class GithubAuthenticationToken extends AbstractAuthenticationToken {
     private static final Cache<String, Set<String>> userOrganizationCache =
             CacheBuilder.newBuilder().expireAfterWrite(1, CACHE_EXPIRY).build();
 
-    private static final Cache<String, Set<String>> repositoryCollaboratorsCache =
-            CacheBuilder.newBuilder().expireAfterWrite(1, CACHE_EXPIRY).build();
-
     private static final Cache<String, Set<String>> repositoriesByUserCache =
             CacheBuilder.newBuilder().expireAfterWrite(1, CACHE_EXPIRY).build();
 
-    private static final Cache<String, Boolean> publicRepositoryCache =
+    private static final Cache<String, GithubUser> usersByIdCache =
             CacheBuilder.newBuilder().expireAfterWrite(1, CACHE_EXPIRY).build();
 
-    private static final Cache<String, GithubUser> usersByIdCache =
+    /**
+     * This cache is for repositories and is explicitly _not_ static because we
+     * want to store repo information per-user (and this class should be per-user).
+     * We potentially could hold a separe static cache for public repo info
+     * that applies to all users, but it wouldn't be able to contain user-specific
+     * details like exact permissions (read/write/admin).
+     *
+     * This representation of the repo holds details on whether the repo is
+     * public/private, as well as whether the current user has pull/push/admin
+     * access.
+     */
+    private final Cache<String, RepoRights> repositoryCache =
             CacheBuilder.newBuilder().expireAfterWrite(1, CACHE_EXPIRY).build();
 
     private final List<GrantedAuthority> authorities = new ArrayList<GrantedAuthority>();
 
     private static final GithubUser UNKNOWN_USER = new GithubUser(null);
 
-    /** Wrapper for cache **/
+    /** Wrappers for cache **/
     static class GithubUser {
         public final GHUser user;
 
@@ -117,6 +128,45 @@ public GithubUser(GHUser user) {
         }
     }
 
+    static class RepoRights {
+        public final boolean hasAdminAccess;
+        public final boolean hasPullAccess;
+        public final boolean hasPushAccess;
+        public final boolean isPrivate;
+
+        public RepoRights(GHRepository repo) {
+            if (repo != null) {
+                this.hasAdminAccess = repo.hasAdminAccess();
+                this.hasPullAccess = repo.hasPullAccess();
+                this.hasPushAccess = repo.hasPushAccess();
+                this.isPrivate = repo.isPrivate();
+            } else {
+                // assume null repo means we had no rights to view it
+                // so must be private
+                this.hasAdminAccess = false;
+                this.hasPullAccess = false;
+                this.hasPushAccess = false;
+                this.isPrivate = true;
+            }
+        }
+
+        public boolean hasAdminAccess() {
+          return this.hasAdminAccess;
+        }
+
+        public boolean hasPullAccess() {
+          return this.hasPullAccess;
+        }
+
+        public boolean hasPushAccess() {
+          return this.hasPushAccess;
+        }
+
+        public boolean isPrivate() {
+          return this.isPrivate;
+        }
+    }
+
     public GithubAuthenticationToken(final String accessToken, final String githubServer) throws IOException {
         super(new GrantedAuthority[] {});
 
@@ -170,7 +220,6 @@ public GithubAuthenticationToken(final String accessToken, final String githubSe
      */
     public static void clearCaches() {
         userOrganizationCache.invalidateAll();
-        repositoryCollaboratorsCache.invalidateAll();
         repositoriesByUserCache.invalidateAll();
         usersByIdCache.invalidateAll();
     }
@@ -193,14 +242,14 @@ public String getGithubServer() {
 
     public GitHub getGitHub() throws IOException {
         if (this.gh == null) {
-        	
+
             String host;
             try {
                 host = new URL(this.githubServer).getHost();
             } catch (MalformedURLException e) {
                 throw new IOException("Invalid GitHub API URL: " + this.githubServer, e);
             }
-        	
+
             OkHttpClient client = new OkHttpClient().setProxy(getProxy(host));
 
             this.gh = GitHubBuilder.fromEnvironment()
@@ -212,7 +261,7 @@ public GitHub getGitHub() throws IOException {
         }
         return gh;
     }
-    
+
     /**
      * Uses proxy if configured on pluginManager/advanced page
      *
@@ -289,8 +338,32 @@ public Set<String> call() throws Exception {
         }
     }
 
-    public boolean hasRepositoryPermission(final String repositoryName) {
-        return myRepositories().contains(repositoryName);
+    public boolean hasRepositoryPermission(String repositoryName, Permission permission) {
+        LOGGER.log(Level.FINEST, "Checking for permission: " + permission + " on repo: " + repositoryName + " for user: " + this.userName);
+        boolean isRepoOfMine = myRepositories().contains(repositoryName);
+        if (isRepoOfMine) {
+          return true;
+        }
+        // This is not my repository, nor is it a repository of an organization I belong to.
+        // Check what rights I have on the github repo.
+        RepoRights repository = loadRepository(repositoryName);
+        if (repository == null) {
+          return false;
+        }
+        // let admins do anything
+        if (repository.hasAdminAccess()) {
+          return true;
+        }
+        // WRITE or READ can Read/Build/View Workspace
+        if (permission.equals(Item.READ) || permission.equals(Item.BUILD) || permission.equals(Item.WORKSPACE)) {
+          return repository.hasPullAccess() || repository.hasPushAccess();
+        }
+        // WRITE can cancel builds or view config
+        if (permission.equals(Item.CANCEL) || permission.equals(Item.EXTENDED_READ)) {
+          return repository.hasPushAccess();
+        }
+        // Need ADMIN rights to do rest: configure, create, delete, discover, wipeout
+        return false;
     }
 
     public Set<String> myRepositories() {
@@ -328,23 +401,9 @@ public Set<String> listToNames(Collection<GHRepository> respositories) throws IO
         return names;
     }
 
-    public boolean isPublicRepository(final String repositoryName) {
-        try {
-            return publicRepositoryCache.get(repositoryName,
-                new Callable<Boolean>() {
-                    @Override
-                    public Boolean call() throws Exception {
-                        GHRepository repository = loadRepository(repositoryName);
-                        // We don't have access so it must not be public (it could be non-existant)
-                        return repository != null && !repository.isPrivate();
-                    }
-                }
-            );
-        } catch (ExecutionException e) {
-            LOGGER.log(Level.SEVERE, "an exception was thrown", e);
-            throw new RuntimeException("authorization failed for user = "
-                    + getName(), e);
-        }
+    public boolean isPublicRepository(String repositoryName) {
+        RepoRights repository = loadRepository(repositoryName);
+        return repository != null && !repository.isPrivate();
     }
 
     private static final Logger LOGGER = Logger
@@ -377,17 +436,26 @@ public GHOrganization loadOrganization(String organization) {
         return null;
     }
 
-    public GHRepository loadRepository(String repositoryName) {
-        try {
-            if (gh != null && isAuthenticated()) {
-                return getGitHub().getRepository(repositoryName);
-            }
-        } catch (IOException e) {
-            LOGGER.log(Level.WARNING,
-                    "Looks like a bad GitHub URL OR the Jenkins user does not have access to the repository{0}",
-                    repositoryName);
-        }
-        return null;
+    public RepoRights loadRepository(final String repositoryName) {
+      try {
+          if (gh != null && isAuthenticated() && (myRealm.hasScope("repo") || myRealm.hasScope("public_repo"))) {
+              return repositoryCache.get(repositoryName,
+                  new Callable<RepoRights>() {
+                      @Override
+                      public RepoRights call() throws Exception {
+                          GHRepository repo = getGitHub().getRepository(repositoryName);
+                          return new RepoRights(repo);
+                      }
+                  }
+              );
+          }
+      } catch (Exception e) {
+          LOGGER.log(Level.SEVERE, "an exception was thrown", e);
+          LOGGER.log(Level.WARNING,
+              "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.",
+              new Object[] { this.userName, repositoryName });
+      }
+      return null;
     }
 
     public GHTeam loadTeam(String organization, String team) {

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

@@ -273,7 +273,7 @@ public boolean hasRepositoryPermission(GithubAuthenticationToken authenticationT
                 authenticationToken.isPublicRepository(repositoryName)) {
             return true;
         } else {
-            return authenticationToken.hasRepositoryPermission(repositoryName);
+            return authenticationToken.hasRepositoryPermission(repositoryName, permission);
         }
     }
 

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

@@ -106,7 +106,7 @@ public class GithubSecurityRealm extends AbstractPasswordBasedSecurityRealm impl
     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_ENTERPRISE_API_SUFFIX = "/api/v3";
-    private static final String DEFAULT_OAUTH_SCOPES = "read:org,user:email";
+    private static final String DEFAULT_OAUTH_SCOPES = "read:org,user:email,repo";
 
     private String githubWebUri;
     private String githubApiUri;
@@ -566,7 +566,7 @@ public String getLoginUrl() {
     @Override
     protected String getPostLogOutUrl(StaplerRequest req, Authentication auth) {
         // if we just redirect to the root and anonymous does not have Overall read then we will start a login all over again.
-        // we are actually anonymous here as the security context has been cleared 
+        // we are actually anonymous here as the security context has been cleared
         Jenkins j = Jenkins.getInstance();
         assert j != null;
         if (j.hasPermission(Jenkins.READ)) {

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

@@ -92,6 +92,8 @@ public class GithubRequireOrganizationMembershipACLTest extends TestCase {
     @Mock
     private Jenkins jenkins;
 
+    private GitHub gh;
+
     @Mock
     private GithubSecurityRealm securityRealm;
 
@@ -101,7 +103,9 @@ public void setUp() throws Exception {
         PowerMockito.mockStatic(Jenkins.class);
         PowerMockito.when(Jenkins.getInstance()).thenReturn(jenkins);
         PowerMockito.when(jenkins.getSecurityRealm()).thenReturn(securityRealm);
-        PowerMockito.when(securityRealm.getOauthScopes()).thenReturn("read:org");
+        PowerMockito.when(securityRealm.getOauthScopes()).thenReturn("read:org,repo");
+        PowerMockito.when(securityRealm.hasScope("read:org")).thenReturn(true);
+        PowerMockito.when(securityRealm.hasScope("repo")).thenReturn(true);
     }
 
     private static final Permission VIEW_JOBSTATUS_PERMISSION = new Permission(Item.PERMISSIONS,
@@ -170,7 +174,7 @@ private GithubRequireOrganizationMembershipACL aclForWorkflowJob(WorkflowJob wor
     }
 
     private GHMyself mockGHMyselfAs(String username) throws IOException {
-        GitHub gh = PowerMockito.mock(GitHub.class);
+        gh = PowerMockito.mock(GitHub.class);
         GitHubBuilder builder = PowerMockito.mock(GitHubBuilder.class);
         PowerMockito.mockStatic(GitHub.class);
         PowerMockito.mockStatic(GitHubBuilder.class);
@@ -264,14 +268,6 @@ private MultiBranchProject mockMultiBranchProject(String url) {
         return multiBranchProject;
     }
 
-    private void mockGithubRepositoryWithCollaborators(GitHub mockGithub, String name, boolean isPrivate, List<String> collaboratorNames) throws IOException {
-        GHRepository ghRepository = PowerMockito.mock(GHRepository.class);
-        PowerMockito.when(mockGithub.getRepository(name)).thenReturn(ghRepository);
-        PowerMockito.when(ghRepository.isPrivate()).thenReturn(isPrivate);
-        Set<String> names = new HashSet(collaboratorNames);
-        PowerMockito.when(ghRepository.getCollaboratorNames()).thenReturn(names);
-    }
-
     @Test
     public void testCanReadAndBuildOneOfMyRepositories() throws IOException {
         GHMyself me = mockGHMyselfAs("Me");
@@ -296,6 +292,7 @@ public void testCanReadAndBuildOneOfMyRepositories() throws IOException {
 
     @Override
     protected void tearDown() throws Exception {
+        gh = null;
         super.tearDown();
         GithubAuthenticationToken.clearCaches();
     }
@@ -323,6 +320,37 @@ public void testCanReadAndBuildOrgRepositoryICollaborateOn() throws IOException
         assertTrue(workflowJobAcl.hasPermission(authenticationToken, Item.BUILD));
     }
 
+    @Test
+    public void testCanReadAndBuildOtherOrgPrivateRepositoryICollaborateOn() throws IOException {
+        GHMyself me = mockGHMyselfAs("Me");
+        mockReposFor(me, Arrays.asList("me/a-repo"));
+        mockOrgRepos(me, ImmutableMap.of("some-org", Arrays.asList("some-org/a-private-repo")));
+        GHRepository ghRepository = PowerMockito.mock(GHRepository.class);
+        PowerMockito.when(gh.getRepository("org-i-dont-belong-to/a-private-repo-i-collaborate-on")).thenReturn(ghRepository);
+        PowerMockito.when(ghRepository.isPrivate()).thenReturn(true);
+        PowerMockito.when(ghRepository.hasAdminAccess()).thenReturn(false);
+        PowerMockito.when(ghRepository.hasPushAccess()).thenReturn(false);
+        PowerMockito.when(ghRepository.hasPullAccess()).thenReturn(true);
+
+        // The user isn't part of "org-i-dont-belong-to"
+        String repoUrl = "https://github.com/org-i-dont-belong-to/a-private-repo-i-collaborate-on.git";
+        Project mockProject = mockProject(repoUrl);
+        MultiBranchProject mockMultiBranchProject = mockMultiBranchProject(repoUrl);
+        WorkflowJob mockWorkflowJob = mockWorkflowJob(repoUrl);
+        GithubRequireOrganizationMembershipACL workflowJobAcl = aclForWorkflowJob(mockWorkflowJob);
+        GithubRequireOrganizationMembershipACL multiBranchProjectAcl = aclForMultiBranchProject(mockMultiBranchProject);
+        GithubRequireOrganizationMembershipACL projectAcl = aclForProject(mockProject);
+
+        GithubAuthenticationToken authenticationToken = new GithubAuthenticationToken("accessToken", "https://api.github.com");
+
+        assertTrue(projectAcl.hasPermission(authenticationToken, Item.READ));
+        assertTrue(projectAcl.hasPermission(authenticationToken, Item.BUILD));
+        assertTrue(multiBranchProjectAcl.hasPermission(authenticationToken, Item.READ));
+        assertTrue(multiBranchProjectAcl.hasPermission(authenticationToken, Item.BUILD));
+        assertTrue(workflowJobAcl.hasPermission(authenticationToken, Item.READ));
+        assertTrue(workflowJobAcl.hasPermission(authenticationToken, Item.BUILD));
+    }
+
     @Test
     public void testCanNotReadOrBuildRepositoryIDoNotCollaborateOn() throws IOException {
         GHMyself me = mockGHMyselfAs("Me");

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

@@ -82,6 +82,6 @@ public void testDescriptorImplGetDefaultGithubApiUri() {
     @Test
     public void testDescriptorImplGetDefaultOauthScopes() {
         DescriptorImpl descriptor = new DescriptorImpl();
-        assertTrue("read:org,user:email".equals(descriptor.getDefaultOauthScopes()));
+        assertTrue("read:org,user:email,repo".equals(descriptor.getDefaultOauthScopes()));
     }
 }