TDL-13115: Added code change for checking minimal permissions (#123)

hpatel41 · KAllan357 · web-flow · commit 021cfdd67c82 · 2021-06-10T16:46:12.000-04:00
* TDL-13115: Added code change for checking minimal permissions

* Enhanced some code

* Removed nested try-catch for error code handling

* Updated some error message

Co-authored-by: Kyle Allan &lt;KAllan357@gmail.com&gt;
diff --git a/tap_github/__init__.py b/tap_github/__init__.py
@@ -97,7 +97,7 @@ class RateLimitExceeded(GithubException):
     },
     404: {
         "raise_exception": NotFoundException,
-        "message": "The resource you have specified cannot be found."
+        "message": "The resource you have specified cannot be found"
     },
     409: {
         "raise_exception": ConflictError,
@@ -164,25 +164,32 @@ def get_bookmark(state, repo, stream_name, bookmark_key, start_date):
         return start_date
     return None
 
-def raise_for_error(resp):
+def raise_for_error(resp, source):
+
+    content_length = len(resp.content)
+    if content_length == 0:
+        # There is nothing we can do here since Github has neither sent
+        # us a 2xx response nor a response content.
+        return
+
+    error_code = resp.status_code
     try:
-        resp.raise_for_status()
-    except (requests.HTTPError, requests.ConnectionError) as error:
-        try:
-            error_code = resp.status_code
-            try:
-                response_json = resp.json()
-            except Exception:
-                response_json = {}
-
-            message = "HTTP-error-code: {}, Error: {}".format(
-                error_code, ERROR_CODE_EXCEPTION_MAPPING.get(error_code, {}).get("message", "Unknown Error") if response_json == {} else response_json)
-
-            exc = ERROR_CODE_EXCEPTION_MAPPING.get(error_code, {}).get("raise_exception", GithubException)
-            raise exc(message) from None
-
-        except (ValueError, TypeError):
-            raise GithubException(error) from None
+        response_json = resp.json()
+    except Exception:
+        response_json = {}
+
+    if error_code == 404:
+        details = ERROR_CODE_EXCEPTION_MAPPING.get(error_code).get("message")
+        if source == "teams":
+            details += ' or it is a personal account repository'
+        message = "HTTP-error-code: 404, Error: {}. Please refer \'{}\' for more details.".format(details, response_json.get("documentation_url"))
+    else:
+        message = "HTTP-error-code: {}, Error: {}".format(
+            error_code, ERROR_CODE_EXCEPTION_MAPPING.get(error_code, {}).get("message", "Unknown Error") if response_json == {} else response_json)
+
+    exc = ERROR_CODE_EXCEPTION_MAPPING.get(error_code, {}).get("raise_exception", GithubException)
+    raise exc(message) from None
+
 def calculate_seconds(epoch):
     current = time.time()
     return int(round((epoch - current), 0))
@@ -204,7 +211,7 @@ def authed_get(source, url, headers={}):
         session.headers.update(headers)
         resp = session.request(method='get', url=url)
         if resp.status_code != 200:
-            raise_for_error(resp)
+            raise_for_error(resp, source)
             return None
         else:
             timer.tags[metrics.Tag.http_status_code] = resp.status_code
@@ -318,15 +325,7 @@ def verify_repo_access(url_for_repo, repo):
         message = "HTTP-error-code: 404, Error: Please check the repository name \'{}\' or you do not have sufficient permissions to access this repository.".format(repo)
         raise NotFoundException(message) from None
 
-def verify_org_access(url_for_org, org):
-    try:
-        authed_get("verifying organization access", url_for_org)
-    except NotFoundException:
-        # throwing user-friendly error message as it shows "Not Found" message
-        message = "HTTP-error-code: 404, Error: \'{}\' is not an organization.".format(org)
-        raise NotFoundException(message) from None
-
-def verify_access_for_repo_org(config):
+def verify_access_for_repo(config):
 
     access_token = config['access_token']
     session.headers.update({'authorization': 'token ' + access_token, 'per_page': '1', 'page': '1'})
@@ -335,18 +334,14 @@ def verify_access_for_repo_org(config):
 
     for repo in repositories:
         logger.info("Verifying access of repository: %s", repo)
-        org = repo.split('/')[0]
 
-        url_for_repo = "https://api.github.com/repos/{}/collaborators".format(repo)
-        url_for_org = "https://api.github.com/orgs/{}/teams".format(org)
+        url_for_repo = "https://api.github.com/repos/{}/commits".format(repo)
 
         # Verifying for Repo access
         verify_repo_access(url_for_repo, repo)
-        # Verifying for Org access
-        verify_org_access(url_for_org, org)
 
 def do_discover(config):
-    verify_access_for_repo_org(config)
+    verify_access_for_repo(config)
     catalog = get_catalog()
     # dump catalog
     print(json.dumps(catalog, indent=2))
diff --git a/tests/unittests/test_exception_handling.py b/tests/unittests/test_exception_handling.py
@@ -9,6 +9,7 @@ def __init__(self, status_code, json, raise_error, headers={'X-RateLimit-Remaini
         self.raise_error = raise_error
         self.text = json
         self.headers = headers
+        self.content = "github"
 
     def raise_for_status(self):
         if not self.raise_error:
@@ -49,12 +50,21 @@ def test_403_error(self, mocked_request):
             self.assertEquals(str(e), "HTTP-error-code: 403, Error: User doesn't have permission to access the resource.")
     
     def test_404_error(self, mocked_request):
-        mocked_request.return_value = get_response(404, raise_error = True)
+        json = {"message": "Not Found", "documentation_url": "https:/docs.github.com/"}
+        mocked_request.return_value = get_response(404, json = json, raise_error = True)
 
         try:
             tap_github.authed_get("", "")
         except tap_github.NotFoundException as e:
-            self.assertEquals(str(e), "HTTP-error-code: 404, Error: The resource you have specified cannot be found.")
+            self.assertEquals(str(e), "HTTP-error-code: 404, Error: The resource you have specified cannot be found. Please refer '{}' for more details.".format(json.get("documentation_url")))
+
+    def test_404_error_for_teams(self, mocked_request):
+        json = {"message": "Not Found", "documentation_url": "https:/docs.github.com/"}
+
+        try:
+            tap_github.raise_for_error(get_response(404, json = json, raise_error = True), "teams")
+        except tap_github.NotFoundException as e:
+            self.assertEquals(str(e), "HTTP-error-code: 404, Error: The resource you have specified cannot be found or it is a personal account repository. Please refer '{}' for more details.".format(json.get("documentation_url")))
 
     def test_500_error(self, mocked_request):
         mocked_request.return_value = get_response(500, raise_error = True)
diff --git a/tests/unittests/test_formatting_dates.py b/tests/unittests/test_formatting_dates.py
@@ -7,6 +7,7 @@ class Mockresponse:
     def __init__(self, resp, not_list=False):
         self.not_list = not_list
         self.json_data = resp
+        self.content = "github"
     
     def json(self):
         if self.not_list:
@@ -34,7 +35,7 @@ def test_due_on_none_without_state(self, mocked_request):
         init_state = {}
         repo_path = "singer-io/tap-github"
 
-        final_state = tap_github.get_all_issue_milestones({}, repo_path, init_state, {})
+        final_state = tap_github.get_all_issue_milestones({}, repo_path, init_state, {}, "")
         # as we will get 1 record and initial bookmark is empty, checking that if bookmark exists in state file returned
         self.assertTrue(final_state["bookmarks"][repo_path]["issue_milestones"]["since"])
 
@@ -51,7 +52,7 @@ def test_due_on_none_with_state(self, mocked_request):
         init_state = {'bookmarks': {'singer-io/tap-github': {'issue_milestones': {'since': '2021-05-05T07:20:36.887412Z'}}}}
         init_bookmark = singer.utils.strptime_to_utc(init_state["bookmarks"][repo_path]["issue_milestones"]["since"])
 
-        final_state = tap_github.get_all_issue_milestones({}, repo_path, init_state, {})
+        final_state = tap_github.get_all_issue_milestones({}, repo_path, init_state, {}, "")
         last_bookmark = singer.utils.strptime_to_utc(final_state["bookmarks"][repo_path]["issue_milestones"]["since"])
         # as we will get 1 record, final bookmark will be greater than initial bookmark
         self.assertGreater(last_bookmark, init_bookmark)
@@ -70,7 +71,7 @@ def test_due_on_not_none_1(self, mocked_request):
         init_state = {'bookmarks': {'singer-io/tap-github': {'issue_milestones': {'since': '2021-05-05T07:20:36.887412Z'}}}}
         init_bookmark = singer.utils.strptime_to_utc(init_state["bookmarks"][repo_path]["issue_milestones"]["since"])
 
-        final_state = tap_github.get_all_issue_milestones({}, repo_path, init_state, {})
+        final_state = tap_github.get_all_issue_milestones({}, repo_path, init_state, {}, "")
         last_bookmark = singer.utils.strptime_to_utc(final_state["bookmarks"][repo_path]["issue_milestones"]["since"])
         # as we will get 1 record, final bookmark will be greater than initial bookmark
         self.assertGreater(last_bookmark, init_bookmark)
@@ -88,7 +89,7 @@ def test_due_on_not_none_2(self, mocked_request):
         init_state = {'bookmarks': {'singer-io/tap-github': {'issue_milestones': {'since': '2021-05-08T07:20:36.887412Z'}}}}
         init_bookmark = init_state["bookmarks"][repo_path]["issue_milestones"]["since"]
 
-        final_state = tap_github.get_all_issue_milestones({}, repo_path, init_state, {})
+        final_state = tap_github.get_all_issue_milestones({}, repo_path, init_state, {}, "")
         # as we will get 0 records, initial and final bookmark will be same
         self.assertEquals(init_bookmark, final_state["bookmarks"][repo_path]["issue_milestones"]["since"])
 
@@ -111,7 +112,7 @@ def test_data_containing_both_values(self, mocked_write_record, mocked_request):
         init_state = {'bookmarks': {'singer-io/tap-github': {'issue_milestones': {'since': '2021-05-08T07:20:36.887412Z'}}}}
         init_bookmark = singer.utils.strptime_to_utc(init_state["bookmarks"][repo_path]["issue_milestones"]["since"])
 
-        final_state = tap_github.get_all_issue_milestones({}, repo_path, init_state, {})
+        final_state = tap_github.get_all_issue_milestones({}, repo_path, init_state, {}, "")
         last_bookmark = singer.utils.strptime_to_utc(final_state["bookmarks"][repo_path]["issue_milestones"]["since"])
         # as we will get 2 record, final bookmark will be greater than initial bookmark
         self.assertGreater(last_bookmark, init_bookmark)
diff --git a/tests/unittests/test_key_error.py b/tests/unittests/test_key_error.py
@@ -5,6 +5,7 @@
 class Mockresponse:
     def __init__(self, resp):
         self.json_data = resp
+        self.content = "github"
 
     def json(self):
         return [(self.json_data)]
@@ -35,7 +36,7 @@ def test_slug_sub_stream_selected_slug_selected(self, mocked_team_members, mocke
             "breadcrumb": [ "properties", "name"],
             "metadata": {"inclusion": "available"}
         }]
-        tap_github.get_all_teams(schemas, "tap-github", {}, mdata)
+        tap_github.get_all_teams(schemas, "tap-github", {}, mdata, "")
         self.assertEquals(mocked_team_members.call_count, 1)
 
     @mock.patch("tap_github.__init__.get_all_team_members")
@@ -58,7 +59,7 @@ def test_slug_sub_stream_not_selected_slug_selected(self, mocked_team_members, m
             "breadcrumb": [ "properties", "name"],
             "metadata": {"inclusion": "available"}
         }]
-        tap_github.get_all_teams(schemas, "tap-github", {}, mdata)
+        tap_github.get_all_teams(schemas, "tap-github", {}, mdata, "")
         self.assertEquals(mocked_team_members.call_count, 0)
 
     @mock.patch("tap_github.__init__.get_all_team_members")
@@ -81,7 +82,7 @@ def test_slug_sub_stream_selected_slug_not_selected(self, mocked_team_members, m
             "breadcrumb": [ "properties", "name"],
             "metadata": {"inclusion": "available"}
         }]
-        tap_github.get_all_teams(schemas, "tap-github", {}, mdata)
+        tap_github.get_all_teams(schemas, "tap-github", {}, mdata, "")
         self.assertEquals(mocked_team_members.call_count, 1)
 
     @mock.patch("tap_github.__init__.get_all_team_members")
@@ -104,7 +105,7 @@ def test_slug_sub_stream_not_selected_slug_not_selected(self, mocked_team_member
             "breadcrumb": [ "properties", "name"],
             "metadata": {"inclusion": "available"}
         }]
-        tap_github.get_all_teams(schemas, "tap-github", {}, mdata)
+        tap_github.get_all_teams(schemas, "tap-github", {}, mdata, "")
         self.assertEquals(mocked_team_members.call_count, 0)
 
 @mock.patch("tap_github.__init__.authed_get_all_pages")
@@ -130,7 +131,7 @@ def test_user_not_selected_in_stargazers(self, mocked_write_records, mocked_requ
           "breadcrumb": ["properties", "starred_at"],
           "metadata": {"inclusion": "available"}
         }]
-        tap_github.get_all_stargazers(schemas, "tap-github", {}, mdata)
+        tap_github.get_all_stargazers(schemas, "tap-github", {}, mdata, "")
         self.assertEquals(mocked_write_records.call_count, 1)
 
     @mock.patch("singer.write_record")
@@ -153,5 +154,5 @@ def test_user_selected_in_stargazers(self, mocked_write_records, mocked_request)
           "breadcrumb": ["properties", "starred_at"],
           "metadata": {"inclusion": "available"}
         }]
-        tap_github.get_all_stargazers(schemas, "tap-github", {}, mdata)
+        tap_github.get_all_stargazers(schemas, "tap-github", {}, mdata, "")
         self.assertEquals(mocked_write_records.call_count, 1)
diff --git a/tests/unittests/test_stargazers_full_table.py b/tests/unittests/test_stargazers_full_table.py
@@ -9,6 +9,6 @@ def test_stargazers_without_query_params(self, mocked_request):
 
         schemas = {"stargazers": "None"}
 
-        tap_github.get_all_stargazers(schemas, "tap-github", {}, {})
+        tap_github.get_all_stargazers(schemas, "tap-github", {}, {}, "")
 
         mocked_request.assert_called_with(mock.ANY, "https://api.github.com/repos/tap-github/stargazers", mock.ANY)
diff --git a/tests/unittests/test_verify_access.py b/tests/unittests/test_verify_access.py
@@ -9,6 +9,7 @@ def __init__(self, status_code, json, raise_error, headers={'X-RateLimit-Remaini
         self.raise_error = raise_error
         self.text = json
         self.headers = headers
+        self.content = "github"
 
     def raise_for_status(self):
         if not self.raise_error:
@@ -26,7 +27,7 @@ def get_response(status_code, json={}, raise_error=False):
 class TestCredentials(unittest.TestCase):
 
     def test_repo_not_found(self, mocked_request):
-        json = {"message": "Not Found", "documentation_url": "https:/"}
+        json = {"message": "Not Found", "documentation_url": "https:/docs.github.com/"}
         mocked_request.return_value = get_response(404, json, True)
 
         try:
@@ -43,49 +44,14 @@ def test_repo_bad_request(self, mocked_request):
             self.assertEquals(str(e), "HTTP-error-code: 400, Error: The request is missing or has a bad parameter.")
 
     def test_repo_bad_creds(self, mocked_request):
-        json = {"message": "Bad credentials", "documentation_url": "https://docs.github.com/rest"}
+        json = {"message": "Bad credentials", "documentation_url": "https://docs.github.com/"}
         mocked_request.return_value = get_response(401, json, True)
 
         try:
             tap_github.verify_repo_access("", "repo")
         except tap_github.BadCredentialsException as e:
             self.assertEquals(str(e), "HTTP-error-code: 401, Error: {}".format(json))
 
-    def test_org_not_found(self, mocked_request):
-        json = {"message": "Not Found", "documentation_url": "https:/"}
-        mocked_request.return_value = get_response(404, json, True)
-
-        try:
-            tap_github.verify_org_access("", "personal-repo")
-        except tap_github.NotFoundException as e:
-            self.assertEquals(str(e), "HTTP-error-code: 404, Error: 'personal-repo' is not an organization.")
-
-    def test_org_bad_request(self, mocked_request):
-        mocked_request.return_value = get_response(400, raise_error = True)
-
-        try:
-            tap_github.verify_org_access("", "personal-repo")
-        except tap_github.BadRequestException as e:
-            self.assertEquals(str(e), "HTTP-error-code: 400, Error: The request is missing or has a bad parameter.")
-
-    def test_org_forbidden(self, mocked_request):
-        json = {'message': 'Must have admin rights to Repository.', 'documentation_url': 'https://docs.github.com/rest/reference/'}
-        mocked_request.return_value = get_response(403, json, True)
-
-        try:
-            tap_github.verify_org_access("", "personal-repo")
-        except tap_github.AuthException as e:
-            self.assertEquals(str(e), "HTTP-error-code: 403, Error: {}".format(json))
-
-    def test_org_bad_creds(self, mocked_request):
-        json = {"message": "Bad credentials", "documentation_url": "https://docs.github.com/rest"}
-        mocked_request.return_value = get_response(401, json, True)
-
-        try:
-            tap_github.verify_org_access("", "personal-repo")
-        except tap_github.BadCredentialsException as e:
-            self.assertEquals(str(e), "HTTP-error-code: 401, Error: {}".format(json))
-
     @mock.patch("tap_github.get_catalog")
     def test_discover_valid_creds(self, mocked_get_catalog, mocked_request):
         mocked_request.return_value = get_response(200)
@@ -97,7 +63,7 @@ def test_discover_valid_creds(self, mocked_get_catalog, mocked_request):
 
     @mock.patch("tap_github.get_catalog")
     def test_discover_not_found(self, mocked_get_catalog, mocked_request):
-        json = {"message": "Not Found", "documentation_url": "https:/"}
+        json = {"message": "Not Found", "documentation_url": "https:/docs.github.com/"}
         mocked_request.return_value = get_response(404, json, True)
         mocked_get_catalog.return_value = {}
 
@@ -120,7 +86,7 @@ def test_discover_bad_request(self, mocked_get_catalog, mocked_request):
 
     @mock.patch("tap_github.get_catalog")
     def test_discover_bad_creds(self, mocked_get_catalog, mocked_request):
-        json = {"message":"Bad credentials","documentation_url":"https://docs.github.com/rest"}
+        json = {"message":"Bad credentials","documentation_url":"https://docs.github.com/"}
         mocked_request.return_value = get_response(401, json, True)
         mocked_get_catalog.return_value = {}
 
@@ -132,7 +98,7 @@ def test_discover_bad_creds(self, mocked_get_catalog, mocked_request):
 
     @mock.patch("tap_github.get_catalog")
     def test_discover_forbidden(self, mocked_get_catalog, mocked_request):
-        json = {'message': 'Must have admin rights to Repository.', 'documentation_url': 'https://docs.github.com/rest/reference/'}
+        json = {'message': 'Must have admin rights to Repository.', 'documentation_url': 'https://docs.github.com/'}
         mocked_request.return_value = get_response(403, json, True)
         mocked_get_catalog.return_value = {}
 
@@ -145,19 +111,16 @@ def test_discover_forbidden(self, mocked_get_catalog, mocked_request):
 
 @mock.patch("tap_github.logger.info")
 @mock.patch("tap_github.verify_repo_access")
-@mock.patch("tap_github.verify_org_access")
 class TestRepoCallCount(unittest.TestCase):
-    def test_repo_call_count(self, mocked_org, mocked_repo, mocked_logger_info):
+    def test_repo_call_count(self, mocked_repo, mocked_logger_info):
         """
             Here 3 repos are given,
             so tap will check creds for all 3 repos
         """
-        mocked_org.return_value = None
         mocked_repo.return_value = None
 
         config = {"access_token": "access_token", "repository": "org1/repo1 org1/repo2 org2/repo1"}
-        tap_github.verify_access_for_repo_org(config)
+        tap_github.verify_access_for_repo(config)
 
         self.assertEquals(mocked_logger_info.call_count, 3)
-        self.assertEquals(mocked_org.call_count, 3)
         self.assertEquals(mocked_repo.call_count, 3)
