Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions ghcloneall.py
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,15 @@ def list_gists(self, user, pattern=None):
# - exclude private gists (if g['public'])
return sorted(map(Repo.from_gist, gists), key=attrgetter('name'))

def _verify_user_token(self, user):
# Verify that the user and token match
user_data, _ = get_json_and_links('https://api.github.com/user',
session=self.session)
if user_data.get('login') == user:
return
raise Error('The github_user specified ({}) '
'does not match the token used.'.format(user))

def list_repos(self, user=None, organization=None, pattern=None,
include_archived=False, include_forks=False,
include_private=True, include_disabled=True):
Expand All @@ -443,6 +452,7 @@ def list_repos(self, user=None, organization=None, pattern=None,
elif user and not organization:
owner = user
if include_private and self.has_auth_token:
self._verify_user_token(user)
# users/$name/repos does not include private repos, so
# we have to query for the repos owned by the current
# user instead. This only works if the current token
Expand Down
32 changes: 31 additions & 1 deletion tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ def raise_for_status(self):

class MockRequestGet:

user_endpoint = 'https://api.github.com/user'

def __init__(self):
self.responses = {}
self.not_found = MockResponse(
Expand All @@ -46,6 +48,15 @@ def __init__(self):
def update(self, responses):
self.responses.update(responses)

def set_user(self, user):
if user is None:
if self.user_endpoint in self.responses:
del self.responses[self.user_endpoint]
else:
self.responses[self.user_endpoint] = MockResponse(
json={'login': user},
)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to make the mock a bit smarter and return a response only when the request contains an Authorization header, failing with a 401 otherwise.

I think that would have caught the use case where you're now calling _verify_user_token() even if the user hasn't provided a token.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if the user is None, the response should be 401 instead of failing with an error because the mock wasn't completely configured?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just checked: get_json_and_links() converts all 4xx errors to an exception, so I don't think the distinction between 401 and 404 really matters for these tests.


def __call__(self, url, headers=None):
return self.responses.get(url, self.not_found)

Expand All @@ -55,6 +66,7 @@ def mock_requests_get(monkeypatch):
mock_get = MockRequestGet()
monkeypatch.setattr(requests, 'get', mock_get)
monkeypatch.setattr(requests.Session, 'get', mock_get)
mock_get.set_user(None)
return mock_get


Expand Down Expand Up @@ -1374,10 +1386,12 @@ def test_main_no_org_gists(monkeypatch, capsys):
)


def test_main_run_error_handling_with_private_token(monkeypatch, capsys):
def test_main_run_error_handling_with_private_token(
monkeypatch, mock_requests_get, capsys):
monkeypatch.setattr(sys, 'argv', [
'ghcloneall', '--user', 'mgedmin', '--github-token', 'xyzzy',
])
mock_requests_get.set_user('mgedmin')
with pytest.raises(SystemExit) as ctx:
ghcloneall.main()
assert str(ctx.value) == (
Expand Down Expand Up @@ -1427,6 +1441,7 @@ def test_main_run_with_token(monkeypatch, mock_requests_get, capsys):
'ghcloneall', '--user', 'mgedmin', '--concurrency=1',
'--github-token', 'fake-token',
])
mock_requests_get.set_user('mgedmin')
mock_requests_get.update(mock_multi_page_api_responses(
url='https://api.github.com/user/repos?affiliation=owner',
pages=[
Expand All @@ -1446,6 +1461,21 @@ def test_main_run_with_token(monkeypatch, mock_requests_get, capsys):
)


def test_main_run_with_mismatched_token(monkeypatch, mock_requests_get,
capsys):
monkeypatch.setattr(sys, 'argv', [
'ghcloneall', '--user', 'test_user', '--concurrency=1',
'--github-token', 'fake-token',
])
mock_requests_get.set_user('some-other-user')
with pytest.raises(SystemExit) as ctx:
ghcloneall.main()
assert str(ctx.value) == (
'The github_user specified (test_user) '
'does not match the token used.'
)


def test_main_run_private_without_token(monkeypatch, mock_requests_get,
capsys):
monkeypatch.setattr(sys, 'argv', [
Expand Down