Merge pull request #227 from xuwaters/github-check-membership

GitHubOAuthenticator: check org_whitelist using check-membership api

oauthenticator/github.py

@@ -169,19 +169,30 @@ def authenticate(self, handler, data=None):
     def _check_organization_whitelist(self, org, username, access_token):
         http_client = AsyncHTTPClient()
         headers = _api_headers(access_token)
-        # Get all the members for organization 'org'
+        # Check membership of user `username` for organization `org` via api [check-membership](https://developer.github.com/v3/orgs/members/#check-membership)
         # With empty scope (even if authenticated by an org member), this
         #  will only yield public org members.  You want 'read:org' in order
         #  to be able to iterate through all members.
-        next_page = "%s://%s/orgs/%s/members" % (GITHUB_PROTOCOL, GITHUB_API, org)
-        while next_page:
-            req = HTTPRequest(next_page, method="GET", headers=headers)
-            resp = yield http_client.fetch(req)
-            resp_json = json.loads(resp.body.decode('utf8', 'replace'))
-            next_page = next_page_from_links(resp)
-            for entry in resp_json:
-                if username == entry['login']:
-                    return True
+        check_membership_url = "%s://%s/orgs/%s/members/%s" % (GITHUB_PROTOCOL, GITHUB_API, org, username)
+        req = HTTPRequest(check_membership_url, method="GET", headers=headers)
+        self.log.debug("Checking GitHub organization membership: %s in %s?", username, org)
+        resp = yield http_client.fetch(req, raise_error=False)
+        if resp.code == 204:
+            self.log.info("Allowing %s as member of %s", username, org)
+            return True
+        else:
+            try:
+                resp_json = json.loads(resp.body.decode('utf8', 'replace'))
+                message = resp_json.get('message', '')
+            except ValueError:
+                message = ''
+            self.log.debug(
+                "%s does not appear to be a member of %s (status=%s): %s",
+                username,
+                org,
+                resp.code,
+                message,
+            )
         return False
 
 

oauthenticator/tests/test_github.py

@@ -76,7 +76,7 @@ def team_members(paginate, request):
         team = member_regex.match(urlinfo.path).group(1)
 
         if team not in teams:
-            return HTTPResponse(400, request)
+            return HTTPResponse(request, 404)
 
         if not paginate:
             return [user_model(m) for m in teams[team]]
@@ -102,12 +102,30 @@ def team_members_paginated(team, page, urlinfo, response):
                         headers=HTTPHeaders(headers),
                         buffer=BytesIO(json.dumps(ret).encode('utf-8')))
 
+
+    membership_regex = re.compile(r'/orgs/(.*)/members/(.*)')
+
+    def team_membership(request):
+        urlinfo = urlparse(request.url)
+        urlmatch = membership_regex.match(urlinfo.path)
+        team = urlmatch.group(1)
+        username = urlmatch.group(2)
+        print('Request team = %s, username = %s' % (team, username))
+        if team not in teams:
+            print('Team not found: team = %s' %(team))
+            return HTTPResponse(request, 404)
+        if username not in teams[team]:
+            print('Member not found: team = %s, username = %s' %(team, username))
+            return HTTPResponse(request, 404)
+        return HTTPResponse(request, 204)
+
+
     ## Perform tests
 
     for paginate in (False, True):
-        client.hosts['api.github.com'].append(
-            (member_regex, functools.partial(team_members, paginate)),
-        )
+        client_hosts = client.hosts['api.github.com']
+        client_hosts.append((membership_regex, team_membership))
+        client_hosts.append((member_regex, functools.partial(team_members, paginate)))
 
         authenticator.github_organization_whitelist = ['blue']
 
@@ -130,5 +148,5 @@ def team_members_paginated(team, page, urlinfo, response):
         user = await authenticator.authenticate(handler)
         assert user['name'] == 'donut'
 
-        client.hosts['api.github.com'].pop()
-
+        client_hosts.pop()
+        client_hosts.pop()