allura-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From brond...@apache.org
Subject [allura] 01/01: [#8353] update several github api usages & auth to latest way of doing it
Date Tue, 03 Mar 2020 21:11:08 GMT
This is an automated email from the ASF dual-hosted git repository.

brondsem pushed a commit to branch db/8353
in repository https://gitbox.apache.org/repos/asf/allura.git

commit a4445f676c5c52ca065061915077f50bb7420516
Author: Dave Brondsema <dave@brondsema.net>
AuthorDate: Tue Mar 3 16:10:57 2020 -0500

    [#8353] update several github api usages & auth to latest way of doing it
---
 Allura/allura/lib/helpers.py                       |  8 +++-
 Allura/development.ini                             |  2 +-
 ForgeImporters/forgeimporters/base.py              |  8 ++--
 ForgeImporters/forgeimporters/github/__init__.py   | 52 ++++++++++++++--------
 .../forgeimporters/github/tests/test_oauth.py      | 27 +++++++----
 .../tests/github/functional/test_github.py         | 11 ++---
 .../forgeimporters/tests/github/test_extractor.py  |  9 ++--
 7 files changed, 75 insertions(+), 42 deletions(-)

diff --git a/Allura/allura/lib/helpers.py b/Allura/allura/lib/helpers.py
index 13c14a8..5c0e7f8 100644
--- a/Allura/allura/lib/helpers.py
+++ b/Allura/allura/lib/helpers.py
@@ -1017,11 +1017,17 @@ def urlopen(url, retries=3, codes=(408, 500, 502, 503, 504), timeout=None):
                     url_string = url.get_full_url()  # if url is Request obj
                 except Exception:
                     url_string = url
+                if hasattr(e, 'filename') and url_string != e.filename:
+                    url_string += ' => {}'.format(e.filename)
                 if timeout is None:
                     timeout = socket.getdefaulttimeout()
+                if hasattr(e, 'fp'):
+                    body = e.fp.read()
+                else:
+                    body = ''
                 log.exception(
                     'Failed after %s retries on url with a timeout of %s: %s: %s',
-                    attempts, timeout, url_string, e)
+                    attempts, timeout, url_string, body[:250])
                 raise e
 
 
diff --git a/Allura/development.ini b/Allura/development.ini
index ad56a7a..cdaf2bf 100644
--- a/Allura/development.ini
+++ b/Allura/development.ini
@@ -449,7 +449,7 @@ disable_entry_points.allura.theme.override = responsive
 ;hidden_importers = trac-tickets
 
 ; GitHub importer keys.  For github ticket import, it is best to set
-; up an app at https://github.com/settings/applications  Use the root URL
+; up an app at https://github.com/settings/developers  Use the root URL
 ; of your Allura instance for both URLs, and enter client values here:
 ;github_importer.client_id =
 ;github_importer.client_secret =
diff --git a/ForgeImporters/forgeimporters/base.py b/ForgeImporters/forgeimporters/base.py
index f4e78c9..afbe7a9 100644
--- a/ForgeImporters/forgeimporters/base.py
+++ b/ForgeImporters/forgeimporters/base.py
@@ -171,10 +171,12 @@ class ProjectExtractor(object):
             self.get_page(page_name, **kw)
 
     @staticmethod
-    def urlopen(url, retries=3, codes=(408, 500, 502, 503, 504), timeout=120, **kw):
+    def urlopen(url, retries=3, codes=(408, 500, 502, 503, 504), timeout=120, unredirected_hdrs=None,
**kw):
         req = six.moves.urllib.request.Request(url, **kw)
-        req.add_header(
-            'User-Agent', 'Allura Data Importer (https://allura.apache.org/)')
+        if unredirected_hdrs:
+            for k, v in unredirected_hdrs.items():
+                req.add_unredirected_header(k, v)
+        req.add_header('User-Agent', 'Allura Data Importer (https://allura.apache.org/)')
         return h.urlopen(req, retries=retries, codes=codes, timeout=timeout)
 
     def get_page(self, page_name_or_url, parser=None, **kw):
diff --git a/ForgeImporters/forgeimporters/github/__init__.py b/ForgeImporters/forgeimporters/github/__init__.py
index 1b67d0a..a5e3ddb 100644
--- a/ForgeImporters/forgeimporters/github/__init__.py
+++ b/ForgeImporters/forgeimporters/github/__init__.py
@@ -75,10 +75,10 @@ class GitHubProjectExtractor(base.ProjectExtractor):
         super(GitHubProjectExtractor, self).__init__(*args, **kw)
 
     def add_token(self, url):
+        headers = {}
         if self.token:
-            glue = '&' if '?' in url else '?'
-            url += glue + 'access_token=' + self.token
-        return url
+            headers['Authorization'] = 'token {}'.format(self.token)
+        return url, headers
 
     def wait_for_limit_reset(self, headers):
         reset = headers.get('X-RateLimit-Reset')
@@ -89,10 +89,15 @@ class GitHubProjectExtractor(base.ProjectExtractor):
                  'Sleeping until %s UTC' % (limit, reset))
         time.sleep((reset - now).total_seconds())
 
-    def urlopen(self, url, **kw):
+    def urlopen(self, url, headers=None, **kw):
+        if headers is None:
+            headers = {}
         try:
-            resp = super(GitHubProjectExtractor, self).urlopen(
-                self.add_token(url), **kw)
+            url, auth_headers = self.add_token(url)
+            # need to use unredirected_hdrs for Authorization for APIs that redirect to an
AWS file asset which has
+            # separate authentication added automatically
+            resp = super(GitHubProjectExtractor, self).urlopen(url,
+                                                               headers=headers, unredirected_hdrs=auth_headers,
**kw)
         except six.moves.urllib.error.HTTPError as e:
             # GitHub will return 403 if rate limit exceeded.
             # We're checking for limit on every request below, but we still
@@ -109,7 +114,9 @@ class GitHubProjectExtractor(base.ProjectExtractor):
         return resp
 
     def check_readable(self):
-        resp = requests.head(self.add_token(self.get_page_url('project_info')), timeout=10)
+        url, headers = self.add_token(self.get_page_url('project_info'))
+        headers['User-Agent'] = 'Allura Data Importer (https://allura.apache.org/)'
+        resp = requests.head(url, headers=headers, timeout=10)
         return resp.status_code == 200
 
     def get_next_page_url(self, link):
@@ -179,18 +186,29 @@ class GitHubProjectExtractor(base.ProjectExtractor):
         return self.get_page('project_info').get('has_issues')
 
 
-def valid_access_token(access_token):
-    # https://developer.github.com/v3/oauth_authorizations/#check-an-authorization
+def oauth_app_basic_auth(config):
     client_id = config['github_importer.client_id']
     secret = config['github_importer.client_secret']
-    token_valid_resp = requests.get('https://api.github.com/applications/{}/tokens/{}'.format(client_id,
access_token),
-                                    auth=requests.auth.HTTPBasicAuth(client_id, secret),
-                                    timeout=10)
-    return token_valid_resp.status_code == 200
+    return requests.auth.HTTPBasicAuth(client_id, secret)
+
+
+def valid_access_token(access_token):
+    return access_token_details(access_token).status_code == 200
+
+
+def access_token_details(access_token):
+    # https://developer.github.com/v3/apps/oauth_applications/#check-a-token
+    client_id = config['github_importer.client_id']
+    url = 'https://api.github.com/applications/{}/token'.format(client_id)
+    return requests.post(url, auth=oauth_app_basic_auth(config), timeout=10, json=dict(
+        access_token=access_token,
+    ))
 
 
 class GitHubOAuthMixin(object):
-    '''Support for github oauth web application flow.'''
+    '''
+    Support for github oauth web application flow.  This is an "OAuth App" not a "GitHub
App"
+    '''
 
     def oauth_begin(self, scope=None):
         client_id = config.get('github_importer.client_id')
@@ -241,8 +259,6 @@ class GitHubOAuthMixin(object):
         token = c.user.get_tool_data('GitHubProjectImport', 'token')
         if not token:
             return False
-        url = 'https://api.github.com/?access_token={}'.format(token)
-        r = requests.head(url, timeout=10)
-        scopes = r.headers.get('X-OAuth-Scopes', '')
-        scopes = [s.strip() for s in scopes.split(',')]
+        r = access_token_details(token)
+        scopes = r.json()['scopes']
         return scope in scopes
diff --git a/ForgeImporters/forgeimporters/github/tests/test_oauth.py b/ForgeImporters/forgeimporters/github/tests/test_oauth.py
index 7e60b7e..563869c 100644
--- a/ForgeImporters/forgeimporters/github/tests/test_oauth.py
+++ b/ForgeImporters/forgeimporters/github/tests/test_oauth.py
@@ -43,20 +43,31 @@ class TestGitHubOAuthMixin(TestController, TestCase):
         c.user.get_tool_data.return_value = None
         self.assertFalse(self.mix.oauth_has_access('write:repo_hook'))
 
+    @patch.dict(config, {'github_importer.client_id': '123456',
+                         'github_importer.client_secret': 'deadbeef'})
     @patch('forgeimporters.github.requests')
-    def test_oauth_has_access_no_headers(self, req):
-        c.user.get_tool_data.return_value = 'token'
+    def test_oauth_has_access_no(self, req):
+        c.user.get_tool_data.return_value = 'some-token'
         self.assertFalse(self.mix.oauth_has_access('write:repo_hook'))
-        req.head.assert_called_once_with('https://api.github.com/?access_token=token', timeout=10)
+        call_args = req.post.call_args[0]
+        self.assertEqual(call_args, ('https://api.github.com/applications/123456/token',))
+        call_kwargs = req.post.call_args[1]
+        assert call_kwargs['auth']
+        self.assertEqual(call_kwargs['json'], {'access_token': 'some-token'})
 
+    @patch.dict(config, {'github_importer.client_id': '123456',
+                         'github_importer.client_secret': 'deadbeef'})
     @patch('forgeimporters.github.requests')
-    def test_oauth_has_access_with_headers(self, req):
-        c.user.get_tool_data.return_value = 'token'
-        req.head.return_value.headers = {'X-OAuth-Scopes': ''}
+    def test_oauth_has_access_yes(self, req):
+        c.user.get_tool_data.return_value = 'some-token'
+
+        req.post.return_value.json.return_value = {'scopes': []}
         self.assertFalse(self.mix.oauth_has_access('write:repo_hook'))
-        req.head.return_value.headers = {'X-OAuth-Scopes': 'some, other:scopes'}
+
+        req.post.return_value.json.return_value = {'scopes': ['some', 'other:scopes']}
         self.assertFalse(self.mix.oauth_has_access('write:repo_hook'))
-        req.head.return_value.headers = {'X-OAuth-Scopes': 'write:repo_hook, user'}
+
+        req.post.return_value.json.return_value = {'scopes': ['write:repo_hook', 'user']}
         self.assertTrue(self.mix.oauth_has_access('write:repo_hook'))
 
     @patch.dict(config, {'github_importer.client_id': '123456',
diff --git a/ForgeImporters/forgeimporters/tests/github/functional/test_github.py b/ForgeImporters/forgeimporters/tests/github/functional/test_github.py
index 1c0eb4d..f6b7ad5 100644
--- a/ForgeImporters/forgeimporters/tests/github/functional/test_github.py
+++ b/ForgeImporters/forgeimporters/tests/github/functional/test_github.py
@@ -83,13 +83,14 @@ class TestGitHubOAuth(TestController):
         user = M.User.by_username('test-admin')
         assert_equal(user.get_tool_data('GitHubProjectImport', 'token'), 'abc')
 
-        with patch('forgeimporters.github.requests.get') as valid_access_token_get:
-            valid_access_token_get.return_value = Mock(status_code=200)
+        with patch('forgeimporters.github.requests.post') as valid_access_token_post:
+            valid_access_token_post.return_value = Mock(status_code=200)
             r = self.app.get('/p/import_project/github/')
 
         # token in user data, so oauth isn't triggered
         assert_equal(r.status_int, 200)
 
-        valid_access_token_get.assert_called_once_with('https://api.github.com/applications/client_id/tokens/abc',
-                                                       auth=requests.auth.HTTPBasicAuth('client_id',
'secret'),
-                                                       timeout=10)
+        valid_access_token_post.assert_called_once_with('https://api.github.com/applications/client_id/token',
+                                                        auth=requests.auth.HTTPBasicAuth('client_id',
'secret'),
+                                                        json={'access_token': 'abc'},
+                                                        timeout=10)
diff --git a/ForgeImporters/forgeimporters/tests/github/test_extractor.py b/ForgeImporters/forgeimporters/tests/github/test_extractor.py
index 617e56b..2b44a40 100644
--- a/ForgeImporters/forgeimporters/tests/github/test_extractor.py
+++ b/ForgeImporters/forgeimporters/tests/github/test_extractor.py
@@ -150,12 +150,9 @@ class TestGitHubProjectExtractor(TestCase):
         e = github.GitHubProjectExtractor('test_project', user=user)
         e.urlopen(url)
         request = urlopen.call_args[0][0]
-        self.assertEqual(request.get_full_url(), url + '?access_token=abc')
-
-        url = 'https://github.com/u/p/?p=1'
-        e.urlopen(url)
-        request = urlopen.call_args[0][0]
-        self.assertEqual(request.get_full_url(), url + '&access_token=abc')
+        self.assertEqual(request.get_full_url(), url)
+        assert request.headers['User-agent']
+        self.assertEqual(request.unredirected_hdrs['Authorization'], 'token abc')
 
     @patch('forgeimporters.base.h.urlopen')
     @patch('forgeimporters.github.time.sleep')


Mime
View raw message