From b7f27ae50d3978d5d754e4a4d280ef730c63e7e0 Mon Sep 17 00:00:00 2001 From: Kyle Fazzari Date: Mon, 18 May 2020 13:40:59 -0700 Subject: [PATCH 01/16] Support authentication `vcs_base.load_url()` currently doesn't support authentication. Add support for both basic and token-based authentication by parsing netrc-formatted files. Use `appdirs` to support vcstool-specific authentication files for both the user and the system (user takes precedence). Signed-off-by: Kyle Fazzari --- .travis.yml | 2 +- test/test_base.py | 252 ++++++++++++++++++++++++++++++++++++ vcstool/clients/vcs_base.py | 118 ++++++++++++++++- 3 files changed, 370 insertions(+), 2 deletions(-) create mode 100644 test/test_base.py diff --git a/.travis.yml b/.travis.yml index 838e5019..5b3f6129 100644 --- a/.travis.yml +++ b/.travis.yml @@ -12,7 +12,7 @@ matrix: install: # newer versions of PyYAML dropped support for Python 3.4 - if [ $TRAVIS_PYTHON_VERSION == "3.4" ]; then pip install PyYAML==5.2; fi - - pip install coverage flake8 flake8-docstrings flake8-import-order pytest PyYAML + - pip install appdirs coverage flake8 flake8-docstrings flake8-import-order pytest PyYAML mock script: - PYTHONPATH=`pwd` pytest -s -v test notifications: diff --git a/test/test_base.py b/test/test_base.py new file mode 100644 index 00000000..7db9bb51 --- /dev/null +++ b/test/test_base.py @@ -0,0 +1,252 @@ +import os +import shutil +import tempfile +import textwrap +import unittest + +try: + from urllib.error import HTTPError +except ImportError: + from urllib2 import HTTPError + + +try: + from unittest import mock +except ImportError: + import mock + +from vcstool.clients import vcs_base + + +class TestBase(unittest.TestCase): + + def setUp(self): + self.default_auth_dir = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, self.default_auth_dir) + self.user_auth_dir = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, self.user_auth_dir) + self.system_auth_dir = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, self.system_auth_dir) + + self._previous_home = os.getenv("HOME") + os.environ["HOME"] = self.default_auth_dir + + patcher = mock.patch( + 'vcstool.clients.vcs_base.appdirs.user_config_dir', + return_value=self.user_auth_dir) + patcher.start() + self.addCleanup(patcher.stop) + + patcher = mock.patch( + 'vcstool.clients.vcs_base.appdirs.site_config_dir', + return_value=self.system_auth_dir) + patcher.start() + self.addCleanup(patcher.stop) + + def tearDown(self): + if self._previous_home: + os.environ["HOME"] = self._previous_home + else: + del os.environ["HOME"] + + @mock.patch('vcstool.clients.vcs_base.urlopen', autospec=True) + @mock.patch( + 'vcstool.clients.vcs_base._authenticated_urlopen', autospec=True) + def test_load_url_calls_urlopen( + self, authenticated_urlopen_mock, urlopen_mock): + urlopen_read_mock = urlopen_mock.return_value.read + + vcs_base.load_url('example.com', timeout=123) + + urlopen_mock.assert_called_once_with('example.com', timeout=123) + urlopen_read_mock.assert_called_once_with() + self.assertFalse(authenticated_urlopen_mock.mock_calls) + + @mock.patch('vcstool.clients.vcs_base.urlopen', autospec=True) + @mock.patch( + 'vcstool.clients.vcs_base._authenticated_urlopen', autospec=True) + def test_load_url_calls_authenticated_urlopen( + self, authenticated_urlopen_mock, urlopen_mock): + for code in (401, 404): + urlopen_mock.side_effect = [ + HTTPError(None, code, None, None, None)] + urlopen_read_mock = urlopen_mock.return_value.read + + vcs_base.load_url('example.com', timeout=123) + + urlopen_mock.assert_called_once_with('example.com', timeout=123) + self.assertFalse(urlopen_read_mock.mock_calls) + + authenticated_urlopen_mock.assert_called_once_with( + 'example.com', timeout=123) + + authenticated_urlopen_mock.reset_mock() + urlopen_mock.reset_mock() + + def test_netrc_open_no_such_file(self): + try: + self.assertEqual(vcs_base._authenticated_urlopen( + 'https://example.com'), None) + except Exception: + self.fail( + 'The lack of a .netrc file should not result in an exception') + + def test_netrc_file_precedence(self): + machine = 'example.com' + + default_auth_file_path = os.path.join(self.default_auth_dir, '.netrc') + user_auth_file_path = os.path.join( + self.user_auth_dir, vcs_base._AUTHENTICATION_CONFIGURATION_FILE) + system_auth_file_path = os.path.join( + self.system_auth_dir, vcs_base._AUTHENTICATION_CONFIGURATION_FILE) + + for path in ( + default_auth_file_path, user_auth_file_path, + system_auth_file_path): + _create_netrc_file(path, textwrap.dedent('''\ + machine %s + password %s + ''' % (machine, path))) + + credentials = vcs_base._credentials_for_machine(machine) + self.assertIsNotNone(credentials) + self.assertEqual(len(credentials), 3) + self.assertEqual(credentials[2], default_auth_file_path) + + # Remove default auth file and assert that the user auth file is used + os.unlink(default_auth_file_path) + credentials = vcs_base._credentials_for_machine(machine) + self.assertIsNotNone(credentials) + self.assertEqual(len(credentials), 3) + self.assertEqual(credentials[2], user_auth_file_path) + + # Remove user auth file and assert that the system auth file is used + os.unlink(user_auth_file_path) + credentials = vcs_base._credentials_for_machine(machine) + self.assertIsNotNone(credentials) + self.assertEqual(len(credentials), 3) + self.assertEqual(credentials[2], system_auth_file_path) + + # Remove system auth file and assert that no creds are found + os.unlink(system_auth_file_path) + self.assertIsNone(vcs_base._credentials_for_machine(machine)) + + def test_netrc_file_skip_errors(self): + machine = 'example.com' + + default_auth_file_path = os.path.join(self.default_auth_dir, '.netrc') + user_auth_file_path = os.path.join( + self.user_auth_dir, vcs_base._AUTHENTICATION_CONFIGURATION_FILE) + + _create_netrc_file(default_auth_file_path, 'skip-me-invalid') + + _create_netrc_file(user_auth_file_path, textwrap.dedent('''\ + machine %s + password %s + ''' % (machine, user_auth_file_path))) + + credentials = vcs_base._credentials_for_machine(machine) + self.assertIsNotNone(credentials) + self.assertEqual(len(credentials), 3) + self.assertEqual(credentials[2], user_auth_file_path) + + def test_auth_parts(self): + user_auth_file_path = os.path.join( + self.user_auth_dir, vcs_base._AUTHENTICATION_CONFIGURATION_FILE) + user_auth_file_part_path = os.path.join( + self.user_auth_dir, + vcs_base._AUTHENTICATION_CONFIGURATION_PARTS_DIR, 'test.conf') + os.makedirs(os.path.dirname(user_auth_file_part_path)) + + auth_machine = 'auth.example.com' + parts_machine = 'parts.example.com' + + for path in (user_auth_file_path, user_auth_file_part_path): + _create_netrc_file(path, textwrap.dedent('''\ + machine %s + password %s + ''' % (auth_machine, path))) + with open(user_auth_file_part_path, 'a') as f: + f.write('machine %s\n' % parts_machine) + f.write('password %s\n' % path) + + credentials = vcs_base._credentials_for_machine(auth_machine) + self.assertIsNotNone(credentials) + self.assertEqual(len(credentials), 3) + self.assertEqual(credentials[2], user_auth_file_path) + + credentials = vcs_base._credentials_for_machine(parts_machine) + self.assertIsNotNone(credentials) + self.assertEqual(len(credentials), 3) + self.assertEqual(credentials[2], user_auth_file_part_path) + + @mock.patch('vcstool.clients.vcs_base.urlopen', autospec=True) + @mock.patch('vcstool.clients.vcs_base.build_opener', autospec=True) + def test_authenticated_urlopen_basic_auth( + self, build_opener_mock, urlopen_mock): + open_mock = build_opener_mock.return_value.open + + machine = 'example.com' + _create_netrc_file( + os.path.join(self.default_auth_dir, '.netrc'), + textwrap.dedent('''\ + machine %s + login username + password password + ''' % machine)) + + url = 'https://%s/foo/bar' % machine + vcs_base._authenticated_urlopen(url) + + self.assertFalse(urlopen_mock.mock_calls) + + class _HTTPBasicAuthHandlerMatcher(object): + def __init__(self, test): + self.test = test + + def __eq__(self, other): + manager = other.passwd + self.test.assertEqual( + manager.find_user_password(None, 'example.com'), + ('username', 'password')) + return True + + build_opener_mock.assert_called_once_with( + _HTTPBasicAuthHandlerMatcher(self)) + open_mock.assert_called_once_with(url, timeout=None) + + @mock.patch('vcstool.clients.vcs_base.urlopen', autospec=True) + @mock.patch('vcstool.clients.vcs_base.build_opener', autospec=True) + def test_authenticated_urlopen_token_auth( + self, build_opener_mock, urlopen_mock): + machine = 'example.com' + _create_netrc_file( + os.path.join(self.default_auth_dir, '.netrc'), + textwrap.dedent('''\ + machine %s + password password + ''' % machine)) + + url = 'https://%s/foo/bar' % machine + vcs_base._authenticated_urlopen(url) + + self.assertFalse(build_opener_mock.mock_calls) + + class _RequestMatcher(object): + def __init__(self, test): + self.test = test + + def __eq__(self, other): + self.test.assertEqual(other.get_full_url(), url) + self.test.assertEqual( + other.get_header('Private-token'), 'password') + return True + + urlopen_mock.assert_called_once_with( + _RequestMatcher(self), timeout=None) + + +def _create_netrc_file(path, contents): + with open(path, 'w') as f: + f.write(contents) + os.chmod(path, 0o600) diff --git a/vcstool/clients/vcs_base.py b/vcstool/clients/vcs_base.py index 19b6e96a..3f4f0ca7 100644 --- a/vcstool/clients/vcs_base.py +++ b/vcstool/clients/vcs_base.py @@ -1,3 +1,7 @@ +import errno +import glob +import logging +import netrc import os import socket import subprocess @@ -5,6 +9,10 @@ try: from urllib.request import Request from urllib.request import urlopen + from urllib.request import HTTPPasswordMgrWithDefaultRealm + from urllib.request import HTTPBasicAuthHandler + from urllib.request import build_opener + from urllib.parse import urlparse from urllib.error import HTTPError from urllib.error import URLError except ImportError: @@ -12,12 +20,24 @@ from urllib2 import Request from urllib2 import URLError from urllib2 import urlopen + from urllib2 import HTTPPasswordMgrWithDefaultRealm + from urllib2 import HTTPBasicAuthHandler + from urllib2 import build_opener + from urlparse import urlparse try: from shutil import which # noqa except ImportError: from vcstool.compat.shutil import which # noqa +import appdirs + +_AUTHENTICATION_CONFIGURATION_FILE = "auth.conf" +_AUTHENTICATION_CONFIGURATION_PARTS_DIR = "auth.conf.d" +_APPDIRS_PROJECT_NAME = 'vcstool' + +logger = logging.getLogger(__name__) + class VcsClientBase(object): @@ -93,7 +113,12 @@ def load_url(url, retry=2, retry_period=1, timeout=10): try: fh = urlopen(url, timeout=timeout) except HTTPError as e: - if e.code == 503 and retry: + if e.code in (401, 404): + # Try again, but with authentication + fh = _authenticated_urlopen(url, timeout=timeout) + if fh is not None: + return fh.read() + elif e.code == 503 and retry: time.sleep(retry_period) return load_url( url, retry=retry - 1, retry_period=retry_period, @@ -132,3 +157,94 @@ def test_url(url, retry=2, retry_period=1, timeout=10): timeout=timeout) raise URLError(str(e) + ' (%s)' % url) return response + + +def _authenticated_urlopen(uri, timeout=None): + machine = urlparse(uri).netloc + if not machine: + return None + + credentials = _credentials_for_machine(machine) + if credentials is None: + logger.warning('No credentials found for "%s"' % machine) + return None + + (username, account, password) = credentials + + # If we have both a username and a password, use basic auth + if username and password: + password_manager = HTTPPasswordMgrWithDefaultRealm() + password_manager.add_password(None, machine, username, password) + auth_handler = HTTPBasicAuthHandler(password_manager) + opener = build_opener(auth_handler) + return opener.open(uri, timeout=timeout) + + # If we have only a password, use token auth + elif password: + request = Request(uri) + request.add_header('PRIVATE-TOKEN', password) + return urlopen(request, timeout=timeout) + + return None + + +def _credentials_for_machine(machine): + # First check the default .netrc file, if any-- it takes precedence over + # everything else + credentials = _credentials_for_machine_in_file(None, machine) + if credentials: + return credentials + + # If that file either didn't exist or didn't match for the machine, + # check the user auth directory for vcstool + credentials = _credentials_for_machine_in_dir( + appdirs.user_config_dir(_APPDIRS_PROJECT_NAME), machine) + if credentials: + return credentials + + # Finally, check the system-wide auth directory for vcstool + return _credentials_for_machine_in_dir( + appdirs.site_config_dir(_APPDIRS_PROJECT_NAME), machine) + + +def _credentials_for_machine_in_dir(directory, machine): + # The idea here is similar to how Debian handles authenticated apt repos: + # https://manpages.debian.org/testing/apt/apt_auth.conf.5.en.html + + # Check the auth.conf file first + auth_file_path = os.path.join( + directory, _AUTHENTICATION_CONFIGURATION_FILE) + credentials = _credentials_for_machine_in_file(auth_file_path, machine) + if credentials: + return credentials + + # If that file either didn't exist or didn't match for the machine, check + # the .conf files in the configuration parts dir + configuration_parts_dir = os.path.join( + directory, _AUTHENTICATION_CONFIGURATION_PARTS_DIR) + auth_files = glob.glob(os.path.join(configuration_parts_dir, '*.conf')) + for auth_file in sorted(auth_files): + auth_file_path = os.path.join(configuration_parts_dir, auth_file) + credentials = _credentials_for_machine_in_file(auth_file_path, machine) + if credentials: + return credentials + + # Nothing matched + return None + + +def _credentials_for_machine_in_file(filename, machine): + credentials = None + try: + credentials = netrc.netrc(file=filename).authenticators(machine) + except EnvironmentError as e: + # Don't error just because the file didn't exist or we didn't have + # permission to access it. Catching this situation this way to be + # compatible with python 2 and 3. + if e.errno not in (errno.ENOENT, errno.EACCES): + raise + except netrc.NetrcParseError: + # If this file had issues, don't error out so we can try fallbacks + pass + + return credentials From 1cdc397997a4c9abd38717fb7cc08a4ada95d58a Mon Sep 17 00:00:00 2001 From: Kyle Fazzari Date: Fri, 22 May 2020 11:39:37 -0700 Subject: [PATCH 02/16] Refactor authentication fallback return/raise pattern Signed-off-by: Kyle Fazzari --- test/test_base.py | 2 +- vcstool/clients/vcs_base.py | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/test/test_base.py b/test/test_base.py index 7db9bb51..a6234b32 100644 --- a/test/test_base.py +++ b/test/test_base.py @@ -69,7 +69,7 @@ def test_load_url_calls_authenticated_urlopen( self, authenticated_urlopen_mock, urlopen_mock): for code in (401, 404): urlopen_mock.side_effect = [ - HTTPError(None, code, None, None, None)] + HTTPError(None, code, 'test', None, None)] urlopen_read_mock = urlopen_mock.return_value.read vcs_base.load_url('example.com', timeout=123) diff --git a/vcstool/clients/vcs_base.py b/vcstool/clients/vcs_base.py index 3f4f0ca7..f42a700f 100644 --- a/vcstool/clients/vcs_base.py +++ b/vcstool/clients/vcs_base.py @@ -110,21 +110,21 @@ def run_command(cmd, cwd, env=None): def load_url(url, retry=2, retry_period=1, timeout=10): + fh = None try: fh = urlopen(url, timeout=timeout) except HTTPError as e: + e.msg += ' (%s)' % url if e.code in (401, 404): # Try again, but with authentication fh = _authenticated_urlopen(url, timeout=timeout) - if fh is not None: - return fh.read() elif e.code == 503 and retry: time.sleep(retry_period) return load_url( url, retry=retry - 1, retry_period=retry_period, timeout=timeout) - e.msg += ' (%s)' % url - raise + if fh is None: + raise except URLError as e: if isinstance(e.reason, socket.timeout) and retry: time.sleep(retry_period) From 92a3bdf8e4183469bad475e67a0a1af4d8695855 Mon Sep 17 00:00:00 2001 From: Kyle Fazzari Date: Fri, 22 May 2020 11:45:11 -0700 Subject: [PATCH 03/16] Add appdirs dependency Signed-off-by: Kyle Fazzari --- setup.py | 2 +- stdeb.cfg | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/setup.py b/setup.py index 6c97fd80..dad693d8 100755 --- a/setup.py +++ b/setup.py @@ -6,7 +6,7 @@ from setuptools import setup from vcstool import __version__ -install_requires = ['PyYAML', 'setuptools'] +install_requires = ['appdirs', 'PyYAML', 'setuptools'] if sys.version_info[0] == 2 and sys.version_info[1] < 7: install_requires.append('argparse') diff --git a/stdeb.cfg b/stdeb.cfg index d6fdb42e..f5ce7159 100644 --- a/stdeb.cfg +++ b/stdeb.cfg @@ -1,6 +1,6 @@ [DEFAULT] -Depends: python-argparse, python-setuptools, python-yaml -Depends3: python3-setuptools, python3-yaml +Depends: python-appdirs, python-argparse, python-setuptools, python-yaml +Depends3: python3-appdirs, python3-setuptools, python3-yaml Conflicts: python3-vcstool Conflicts3: python-vcstool Suite: xenial yakkety zesty artful bionic cosmic disco eoan jessie stretch buster From 91f6ad0daafb9ebed18cd60f9fe8849331232208 Mon Sep 17 00:00:00 2001 From: Kyle Fazzari Date: Fri, 22 May 2020 13:31:55 -0700 Subject: [PATCH 04/16] Make _authenticated_urlopen retryable Signed-off-by: Kyle Fazzari --- test/test_base.py | 68 +++++++++++++++++++++++++++++++++++-- vcstool/clients/vcs_base.py | 61 +++++++++++++++++++-------------- 2 files changed, 102 insertions(+), 27 deletions(-) diff --git a/test/test_base.py b/test/test_base.py index a6234b32..409a02c8 100644 --- a/test/test_base.py +++ b/test/test_base.py @@ -213,7 +213,7 @@ def __eq__(self, other): build_opener_mock.assert_called_once_with( _HTTPBasicAuthHandlerMatcher(self)) - open_mock.assert_called_once_with(url, timeout=None) + open_mock.assert_called_once_with(url, timeout=10) @mock.patch('vcstool.clients.vcs_base.urlopen', autospec=True) @mock.patch('vcstool.clients.vcs_base.build_opener', autospec=True) @@ -243,7 +243,71 @@ def __eq__(self, other): return True urlopen_mock.assert_called_once_with( - _RequestMatcher(self), timeout=None) + _RequestMatcher(self), timeout=10) + + @mock.patch('vcstool.clients.vcs_base.urlopen', autospec=True) + def test_load_url_retries(self, urlopen_mock): + urlopen_read_mock = urlopen_mock.return_value.read + urlopen_mock.side_effect = [ + HTTPError(None, 503, 'test1', None, None), + HTTPError(None, 503, 'test2', None, None), + HTTPError(None, 503, 'test3', None, None), + ] + + with self.assertRaisesRegex(HTTPError, 'test3'): + vcs_base.load_url('example.com') + + self.assertEqual(len(urlopen_mock.mock_calls), 3) + urlopen_mock.assert_has_calls([ + mock.call('example.com', timeout=10), + mock.call('example.com', timeout=10), + mock.call('example.com', timeout=10), + ]) + self.assertFalse(urlopen_read_mock.mock_calls) + + @mock.patch('vcstool.clients.vcs_base.urlopen', autospec=True) + def test_load_url_retries_authenticated(self, urlopen_mock): + urlopen_read_mock = urlopen_mock.return_value.read + urlopen_mock.side_effect = [ + HTTPError(None, 401, 'test1', None, None), + HTTPError(None, 503, 'test2', None, None), + HTTPError(None, 503, 'test3', None, None), + HTTPError(None, 503, 'test4', None, None), + ] + + machine = 'example.com' + _create_netrc_file( + os.path.join(self.default_auth_dir, '.netrc'), + textwrap.dedent('''\ + machine %s + password password + ''' % machine)) + + url = 'https://%s/foo/bar' % machine + + with self.assertRaisesRegex(HTTPError, 'test4'): + vcs_base.load_url(url) + + self.assertEqual(len(urlopen_mock.mock_calls), 4) + + class _RequestMatcher(object): + def __init__(self, test): + self.test = test + + def __eq__(self, other): + self.test.assertEqual(other.get_full_url(), url) + self.test.assertEqual( + other.get_header('Private-token'), 'password') + return True + + urlopen_mock.assert_has_calls([ + mock.call(url, timeout=10), + mock.call(_RequestMatcher(self), timeout=10), + mock.call(_RequestMatcher(self), timeout=10), + mock.call(_RequestMatcher(self), timeout=10), + ]) + self.assertFalse(urlopen_read_mock.mock_calls) + def _create_netrc_file(path, contents): diff --git a/vcstool/clients/vcs_base.py b/vcstool/clients/vcs_base.py index f42a700f..5091c37f 100644 --- a/vcstool/clients/vcs_base.py +++ b/vcstool/clients/vcs_base.py @@ -1,4 +1,5 @@ import errno +import functools import glob import logging import netrc @@ -112,26 +113,17 @@ def run_command(cmd, cwd, env=None): def load_url(url, retry=2, retry_period=1, timeout=10): fh = None try: - fh = urlopen(url, timeout=timeout) + fh = _retryable_urlopen(url, timeout=timeout) except HTTPError as e: - e.msg += ' (%s)' % url if e.code in (401, 404): # Try again, but with authentication fh = _authenticated_urlopen(url, timeout=timeout) - elif e.code == 503 and retry: - time.sleep(retry_period) - return load_url( - url, retry=retry - 1, retry_period=retry_period, - timeout=timeout) if fh is None: + e.msg += ' (%s)' % url raise except URLError as e: - if isinstance(e.reason, socket.timeout) and retry: - time.sleep(retry_period) - return load_url( - url, retry=retry - 1, retry_period=retry_period, - timeout=timeout) raise URLError(str(e) + ' (%s)' % url) + return fh.read() @@ -140,33 +132,52 @@ def test_url(url, retry=2, retry_period=1, timeout=10): request.get_method = lambda: 'HEAD' try: - response = urlopen(request) + response = _retryable_urlopen(request) except HTTPError as e: - if e.code == 503 and retry: - time.sleep(retry_period) - return test_url( - url, retry=retry - 1, retry_period=retry_period, - timeout=timeout) e.msg += ' (%s)' % url raise except URLError as e: - if isinstance(e.reason, socket.timeout) and retry: - time.sleep(retry_period) - return test_url( - url, retry=retry - 1, retry_period=retry_period, - timeout=timeout) raise URLError(str(e) + ' (%s)' % url) return response -def _authenticated_urlopen(uri, timeout=None): +def _urlopen_retry(f): + @functools.wraps(f) + def _retryable_function(url, retry=2, retry_period=1, timeout=10): + retry += 1 + + while True: + try: + retry -= 1 + return f(url, timeout=timeout) + except HTTPError as e: + if e.code != 503 or retry <= 0: + raise + except URLError as e: + if not isinstance(e.reason, socket.timeout) or retry <= 0: + raise + + if retry > 0: + time.sleep(retry_period) + else: + break + + return _retryable_function + + +@_urlopen_retry +def _retryable_urlopen(url, timeout=10): + return urlopen(url, timeout=timeout) + + +@_urlopen_retry +def _authenticated_urlopen(uri, timeout=10): machine = urlparse(uri).netloc if not machine: return None credentials = _credentials_for_machine(machine) if credentials is None: - logger.warning('No credentials found for "%s"' % machine) return None (username, account, password) = credentials From f3286273b5150016e991a51173e3ab6cfa2e13fd Mon Sep 17 00:00:00 2001 From: Kyle Fazzari Date: Fri, 22 May 2020 14:20:35 -0700 Subject: [PATCH 05/16] Support testing authenticated URLs Signed-off-by: Kyle Fazzari --- test/test_base.py | 62 +++++++++++++++++++++++++++-- vcstool/clients/vcs_base.py | 79 +++++++++++++++++++++---------------- 2 files changed, 104 insertions(+), 37 deletions(-) diff --git a/test/test_base.py b/test/test_base.py index 409a02c8..980858a0 100644 --- a/test/test_base.py +++ b/test/test_base.py @@ -78,7 +78,56 @@ def test_load_url_calls_authenticated_urlopen( self.assertFalse(urlopen_read_mock.mock_calls) authenticated_urlopen_mock.assert_called_once_with( - 'example.com', timeout=123) + 'example.com', retry=2, retry_period=1, timeout=123) + + authenticated_urlopen_mock.reset_mock() + urlopen_mock.reset_mock() + + @mock.patch('vcstool.clients.vcs_base.urlopen', autospec=True) + @mock.patch( + 'vcstool.clients.vcs_base._authenticated_urlopen', autospec=True) + def test_test_url_calls_urlopen( + self, authenticated_urlopen_mock, urlopen_mock): + url = 'http://example.com' + vcs_base.test_url(url, timeout=123) + + class _RequestMatcher(object): + def __init__(self, test): + self.test = test + + def __eq__(self, other): + self.test.assertEqual(other.get_full_url(), url) + return True + + urlopen_mock.assert_called_once_with( + _RequestMatcher(self), timeout=123) + self.assertFalse(authenticated_urlopen_mock.mock_calls) + + @mock.patch('vcstool.clients.vcs_base.urlopen', autospec=True) + @mock.patch( + 'vcstool.clients.vcs_base._authenticated_urlopen', autospec=True) + def test_test_url_calls_authenticated_urlopen( + self, authenticated_urlopen_mock, urlopen_mock): + for code in (401, 404): + urlopen_mock.side_effect = [ + HTTPError(None, code, 'test', None, None)] + + url = 'http://example.com' + vcs_base.test_url(url, timeout=123) + + class _RequestMatcher(object): + def __init__(self, test): + self.test = test + + def __eq__(self, other): + self.test.assertEqual(other.get_full_url(), url) + return True + + urlopen_mock.assert_called_once_with( + _RequestMatcher(self), timeout=123) + + authenticated_urlopen_mock.assert_called_once_with( + _RequestMatcher(self), retry=2, retry_period=1, timeout=123) authenticated_urlopen_mock.reset_mock() urlopen_mock.reset_mock() @@ -211,9 +260,17 @@ def __eq__(self, other): ('username', 'password')) return True + class _RequestMatcher(object): + def __init__(self, test): + self.test = test + + def __eq__(self, other): + self.test.assertEqual(other.get_full_url(), url) + return True + build_opener_mock.assert_called_once_with( _HTTPBasicAuthHandlerMatcher(self)) - open_mock.assert_called_once_with(url, timeout=10) + open_mock.assert_called_once_with(_RequestMatcher(self), timeout=10) @mock.patch('vcstool.clients.vcs_base.urlopen', autospec=True) @mock.patch('vcstool.clients.vcs_base.build_opener', autospec=True) @@ -309,7 +366,6 @@ def __eq__(self, other): self.assertFalse(urlopen_read_mock.mock_calls) - def _create_netrc_file(path, contents): with open(path, 'w') as f: f.write(contents) diff --git a/vcstool/clients/vcs_base.py b/vcstool/clients/vcs_base.py index 5091c37f..0e2af096 100644 --- a/vcstool/clients/vcs_base.py +++ b/vcstool/clients/vcs_base.py @@ -113,11 +113,12 @@ def run_command(cmd, cwd, env=None): def load_url(url, retry=2, retry_period=1, timeout=10): fh = None try: - fh = _retryable_urlopen(url, timeout=timeout) + fh = _urlopen_retry(retry, retry_period)(urlopen)(url, timeout=timeout) except HTTPError as e: if e.code in (401, 404): # Try again, but with authentication - fh = _authenticated_urlopen(url, timeout=timeout) + fh = _authenticated_urlopen( + url, retry=retry, retry_period=retry_period, timeout=timeout) if fh is None: e.msg += ' (%s)' % url raise @@ -132,47 +133,56 @@ def test_url(url, retry=2, retry_period=1, timeout=10): request.get_method = lambda: 'HEAD' try: - response = _retryable_urlopen(request) + response = _urlopen_retry(retry, retry_period)(urlopen)( + request, timeout=timeout) except HTTPError as e: - e.msg += ' (%s)' % url - raise + if e.code in (401, 404): + # Try again, but with authentication + response = _authenticated_urlopen( + request, retry=retry, retry_period=retry_period, + timeout=timeout) + if response is None: + e.msg += ' (%s)' % url + raise except URLError as e: raise URLError(str(e) + ' (%s)' % url) return response -def _urlopen_retry(f): - @functools.wraps(f) - def _retryable_function(url, retry=2, retry_period=1, timeout=10): - retry += 1 +def _urlopen_retry(retry, retry_period): + def _retry_decorator(f): + @functools.wraps(f) + def _retryable_function(*args, **kwargs): + nonlocal retry + retry += 1 - while True: - try: - retry -= 1 - return f(url, timeout=timeout) - except HTTPError as e: - if e.code != 503 or retry <= 0: - raise - except URLError as e: - if not isinstance(e.reason, socket.timeout) or retry <= 0: - raise - - if retry > 0: - time.sleep(retry_period) - else: - break + while True: + try: + retry -= 1 + return f(*args, **kwargs) + except HTTPError as e: + if e.code != 503 or retry <= 0: + raise + except URLError as e: + if not isinstance(e.reason, socket.timeout) or retry <= 0: + raise - return _retryable_function + if retry > 0: + time.sleep(retry_period) + else: + break + return _retryable_function + return _retry_decorator -@_urlopen_retry -def _retryable_urlopen(url, timeout=10): - return urlopen(url, timeout=timeout) +def _authenticated_urlopen(uri, retry=2, retry_period=1, timeout=10): + if isinstance(uri, str): + request = Request(uri) + else: + request = uri -@_urlopen_retry -def _authenticated_urlopen(uri, timeout=10): - machine = urlparse(uri).netloc + machine = urlparse(request.full_url).netloc if not machine: return None @@ -188,13 +198,14 @@ def _authenticated_urlopen(uri, timeout=10): password_manager.add_password(None, machine, username, password) auth_handler = HTTPBasicAuthHandler(password_manager) opener = build_opener(auth_handler) - return opener.open(uri, timeout=timeout) + return _urlopen_retry(retry, retry_period)(opener.open)( + request, timeout=timeout) # If we have only a password, use token auth elif password: - request = Request(uri) request.add_header('PRIVATE-TOKEN', password) - return urlopen(request, timeout=timeout) + return _urlopen_retry(retry, retry_period)(urlopen)( + request, timeout=timeout) return None From bdc0d09043cd372a3c4088d330283b92ad9c8273 Mon Sep 17 00:00:00 2001 From: Kyle Fazzari Date: Fri, 22 May 2020 14:22:52 -0700 Subject: [PATCH 06/16] Use older assertRaisesRegexp Signed-off-by: Kyle Fazzari --- test/test_base.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/test_base.py b/test/test_base.py index 980858a0..88bf474b 100644 --- a/test/test_base.py +++ b/test/test_base.py @@ -311,7 +311,7 @@ def test_load_url_retries(self, urlopen_mock): HTTPError(None, 503, 'test3', None, None), ] - with self.assertRaisesRegex(HTTPError, 'test3'): + with self.assertRaisesRegexp(HTTPError, 'test3'): vcs_base.load_url('example.com') self.assertEqual(len(urlopen_mock.mock_calls), 3) @@ -342,7 +342,7 @@ def test_load_url_retries_authenticated(self, urlopen_mock): url = 'https://%s/foo/bar' % machine - with self.assertRaisesRegex(HTTPError, 'test4'): + with self.assertRaisesRegexp(HTTPError, 'test4'): vcs_base.load_url(url) self.assertEqual(len(urlopen_mock.mock_calls), 4) From 6b2b87b3319df1fb997c0be0725a4d4d684d89f2 Mon Sep 17 00:00:00 2001 From: Kyle Fazzari Date: Fri, 22 May 2020 14:38:37 -0700 Subject: [PATCH 07/16] Get rid of assertRaisesRegex* completely Signed-off-by: Kyle Fazzari --- test/test_base.py | 8 ++++++-- vcstool/clients/vcs_base.py | 12 ++++++------ 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/test/test_base.py b/test/test_base.py index 88bf474b..4d9f4187 100644 --- a/test/test_base.py +++ b/test/test_base.py @@ -311,9 +311,11 @@ def test_load_url_retries(self, urlopen_mock): HTTPError(None, 503, 'test3', None, None), ] - with self.assertRaisesRegexp(HTTPError, 'test3'): + with self.assertRaises(HTTPError) as e: vcs_base.load_url('example.com') + self.assertTrue('test3' in str(e.exception)) + self.assertEqual(len(urlopen_mock.mock_calls), 3) urlopen_mock.assert_has_calls([ mock.call('example.com', timeout=10), @@ -342,9 +344,11 @@ def test_load_url_retries_authenticated(self, urlopen_mock): url = 'https://%s/foo/bar' % machine - with self.assertRaisesRegexp(HTTPError, 'test4'): + with self.assertRaises(HTTPError) as e: vcs_base.load_url(url) + self.assertTrue('test4' in str(e.exception)) + self.assertEqual(len(urlopen_mock.mock_calls), 4) class _RequestMatcher(object): diff --git a/vcstool/clients/vcs_base.py b/vcstool/clients/vcs_base.py index 0e2af096..26170fcc 100644 --- a/vcstool/clients/vcs_base.py +++ b/vcstool/clients/vcs_base.py @@ -153,21 +153,21 @@ def _urlopen_retry(retry, retry_period): def _retry_decorator(f): @functools.wraps(f) def _retryable_function(*args, **kwargs): - nonlocal retry - retry += 1 + local_retry = retry + 1 while True: try: - retry -= 1 + local_retry -= 1 return f(*args, **kwargs) except HTTPError as e: - if e.code != 503 or retry <= 0: + if e.code != 503 or local_retry <= 0: raise except URLError as e: - if not isinstance(e.reason, socket.timeout) or retry <= 0: + if (not isinstance(e.reason, socket.timeout) or + local_retry <= 0): raise - if retry > 0: + if local_retry > 0: time.sleep(retry_period) else: break From 7ea0db984fc633042dab6c09a86a0b7a0feb09c3 Mon Sep 17 00:00:00 2001 From: Kyle Fazzari Date: Fri, 22 May 2020 14:44:32 -0700 Subject: [PATCH 08/16] Use get_full_url() Signed-off-by: Kyle Fazzari --- vcstool/clients/vcs_base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vcstool/clients/vcs_base.py b/vcstool/clients/vcs_base.py index 26170fcc..4fd5a621 100644 --- a/vcstool/clients/vcs_base.py +++ b/vcstool/clients/vcs_base.py @@ -182,7 +182,7 @@ def _authenticated_urlopen(uri, retry=2, retry_period=1, timeout=10): else: request = uri - machine = urlparse(request.full_url).netloc + machine = urlparse(request.get_full_url()).netloc if not machine: return None From 157967f6a666dba42d465ee56ade92da459dc1d8 Mon Sep 17 00:00:00 2001 From: Kyle Fazzari Date: Fri, 22 May 2020 14:47:41 -0700 Subject: [PATCH 09/16] Get rid of functools Signed-off-by: Kyle Fazzari --- vcstool/clients/vcs_base.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/vcstool/clients/vcs_base.py b/vcstool/clients/vcs_base.py index 4fd5a621..2af3751c 100644 --- a/vcstool/clients/vcs_base.py +++ b/vcstool/clients/vcs_base.py @@ -1,5 +1,4 @@ import errno -import functools import glob import logging import netrc @@ -151,7 +150,6 @@ def test_url(url, retry=2, retry_period=1, timeout=10): def _urlopen_retry(retry, retry_period): def _retry_decorator(f): - @functools.wraps(f) def _retryable_function(*args, **kwargs): local_retry = retry + 1 From fc361a2287a953e9a274d1b2d37fa6fc3360ad8e Mon Sep 17 00:00:00 2001 From: Kyle Fazzari Date: Tue, 26 May 2020 13:25:58 -0700 Subject: [PATCH 10/16] Document authentication behavior in README Signed-off-by: Kyle Fazzari --- README.rst | 38 +++++++++++++++++++++++++++++++++++++ vcstool/clients/vcs_base.py | 12 +++++++++--- 2 files changed, 47 insertions(+), 3 deletions(-) diff --git a/README.rst b/README.rst index a068eb51..7e04aea9 100644 --- a/README.rst +++ b/README.rst @@ -138,6 +138,44 @@ The set of repositories to operate on can optionally be restricted by the type: If the command should work on multiple repositories make sure to pass only generic arguments which work for all of these repository types. +Access repositories that require authentication +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Vcstool supports authenticated access to repositories by parsing files with a netrc-like format. +It will first look for login information in the `~/.netrc` file, used by ftp, git, and similar programs if it exists. +If it doesn't exist (or doesn't contain login infomation for the URL in question), it moves to vcstool-specific locations. +In these locations vcstool will look for credentials in either an `auth.conf` file, or `.conf` files inside an `auth.conf.d` directory. +These locations are (in order of precedence): + +1. User config directory. + - On Linux, abides by XDG spec: `~/.config/vcstool/` + - On macOS: `~/Library/Application Support/vcstool/` + - On Windows: `C:\\Users\\\\AppData\\Local\\vcstool\\vcstool\\` +2. Site config directory. + - On Linux: `vcstool/` subdirectory in any directory within `$XDG_CONFIG_DIRS`, or `/etc/xdg/vcstool/` if unset + - On macOS: `/Library/Application Support/vcstool/` + - On Windows: `C:\\ProgramData\\vcstool\\vcstool\\` + +The netrc-like format consists of a few different tokens separated by whitespace (spaces, tabs, or newlines): + +- `machine `: Credentials are retrieved by matching the repository URL to this token +- `login `: Login username +- `password `: Login password + +For example, to access private github repositories:: + + machine github.com + login mylogin + password myaccesstoken + +Accessing private gitlab repositories looks similar, although no `login` is required:: + + machine gitlab.com + password myaccesstoken + +A word of caution: these files are not encrypted. +Ensure that their permissions do not exceed that which is required. + How to install vcstool? ======================= diff --git a/vcstool/clients/vcs_base.py b/vcstool/clients/vcs_base.py index 2af3751c..ae36727b 100644 --- a/vcstool/clients/vcs_base.py +++ b/vcstool/clients/vcs_base.py @@ -222,9 +222,15 @@ def _credentials_for_machine(machine): if credentials: return credentials - # Finally, check the system-wide auth directory for vcstool - return _credentials_for_machine_in_dir( - appdirs.site_config_dir(_APPDIRS_PROJECT_NAME), machine) + # Finally, check the system-wide auth directories for vcstool, in order + auth_path = appdirs.site_config_dir(_APPDIRS_PROJECT_NAME, multipath=True) + for site_config_dir in auth_path.split(':'): + credentials = _credentials_for_machine_in_dir( + site_config_dir, machine) + if credentials: + return credentials + + return None def _credentials_for_machine_in_dir(directory, machine): From 037b2252ab00ba03e3e132bfdc5cfad615c52ad3 Mon Sep 17 00:00:00 2001 From: Dirk Thomas Date: Wed, 10 Jun 2020 21:04:06 -0700 Subject: [PATCH 11/16] insert mock in alphabetical order --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 5b3f6129..313cedec 100644 --- a/.travis.yml +++ b/.travis.yml @@ -12,7 +12,7 @@ matrix: install: # newer versions of PyYAML dropped support for Python 3.4 - if [ $TRAVIS_PYTHON_VERSION == "3.4" ]; then pip install PyYAML==5.2; fi - - pip install appdirs coverage flake8 flake8-docstrings flake8-import-order pytest PyYAML mock + - pip install appdirs coverage flake8 flake8-docstrings flake8-import-order mock pytest PyYAML script: - PYTHONPATH=`pwd` pytest -s -v test notifications: From 723abeecb56ba6b4e47bdaa9396836d0985e944f Mon Sep 17 00:00:00 2001 From: Dirk Thomas Date: Wed, 10 Jun 2020 21:05:59 -0700 Subject: [PATCH 12/16] use single quotes --- test/test_base.py | 8 ++++---- vcstool/clients/vcs_base.py | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/test/test_base.py b/test/test_base.py index 4d9f4187..91574085 100644 --- a/test/test_base.py +++ b/test/test_base.py @@ -28,8 +28,8 @@ def setUp(self): self.system_auth_dir = tempfile.mkdtemp() self.addCleanup(shutil.rmtree, self.system_auth_dir) - self._previous_home = os.getenv("HOME") - os.environ["HOME"] = self.default_auth_dir + self._previous_home = os.getenv('HOME') + os.environ['HOME'] = self.default_auth_dir patcher = mock.patch( 'vcstool.clients.vcs_base.appdirs.user_config_dir', @@ -45,9 +45,9 @@ def setUp(self): def tearDown(self): if self._previous_home: - os.environ["HOME"] = self._previous_home + os.environ['HOME'] = self._previous_home else: - del os.environ["HOME"] + del os.environ['HOME'] @mock.patch('vcstool.clients.vcs_base.urlopen', autospec=True) @mock.patch( diff --git a/vcstool/clients/vcs_base.py b/vcstool/clients/vcs_base.py index ae36727b..4c0871e8 100644 --- a/vcstool/clients/vcs_base.py +++ b/vcstool/clients/vcs_base.py @@ -32,8 +32,8 @@ import appdirs -_AUTHENTICATION_CONFIGURATION_FILE = "auth.conf" -_AUTHENTICATION_CONFIGURATION_PARTS_DIR = "auth.conf.d" +_AUTHENTICATION_CONFIGURATION_FILE = 'auth.conf' +_AUTHENTICATION_CONFIGURATION_PARTS_DIR = 'auth.conf.d' _APPDIRS_PROJECT_NAME = 'vcstool' logger = logging.getLogger(__name__) From 3c2d0ab937f63705817fe373522ce88fa44461c0 Mon Sep 17 00:00:00 2001 From: Dirk Thomas Date: Wed, 10 Jun 2020 21:11:54 -0700 Subject: [PATCH 13/16] correct case for proper names --- README.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.rst b/README.rst index 7e04aea9..021c41d6 100644 --- a/README.rst +++ b/README.rst @@ -162,13 +162,13 @@ The netrc-like format consists of a few different tokens separated by whitespace - `login `: Login username - `password `: Login password -For example, to access private github repositories:: +For example, to access private GitHub repositories:: machine github.com login mylogin password myaccesstoken -Accessing private gitlab repositories looks similar, although no `login` is required:: +Accessing private GitLab repositories looks similar, although no `login` is required:: machine gitlab.com password myaccesstoken From de57cd46188a3176635b762f98ceddc8d0a37052 Mon Sep 17 00:00:00 2001 From: Dirk Thomas Date: Wed, 10 Jun 2020 22:25:47 -0700 Subject: [PATCH 14/16] style only --- vcstool/clients/vcs_base.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/vcstool/clients/vcs_base.py b/vcstool/clients/vcs_base.py index 4c0871e8..a9579ecc 100644 --- a/vcstool/clients/vcs_base.py +++ b/vcstool/clients/vcs_base.py @@ -161,8 +161,10 @@ def _retryable_function(*args, **kwargs): if e.code != 503 or local_retry <= 0: raise except URLError as e: - if (not isinstance(e.reason, socket.timeout) or - local_retry <= 0): + if ( + not isinstance(e.reason, socket.timeout) or + local_retry <= 0 + ): raise if local_retry > 0: From f06fdceebf9981d6161c657287b4f52956fa53ab Mon Sep 17 00:00:00 2001 From: Kyle Fazzari Date: Mon, 15 Jun 2020 13:01:48 -0700 Subject: [PATCH 15/16] Specify that this feature is for .tar and .zip Signed-off-by: Kyle Fazzari --- README.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/README.rst b/README.rst index 021c41d6..6a24e54b 100644 --- a/README.rst +++ b/README.rst @@ -141,7 +141,8 @@ If the command should work on multiple repositories make sure to pass only gener Access repositories that require authentication ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -Vcstool supports authenticated access to repositories by parsing files with a netrc-like format. +The various version control systems supported by vcstool support authenticated access on their own (e.g. SSH public keys, .netrc files, etc.). +For .tar or .zip repositories, vcstool supports authenticated access by parsing files with a netrc-like format. It will first look for login information in the `~/.netrc` file, used by ftp, git, and similar programs if it exists. If it doesn't exist (or doesn't contain login infomation for the URL in question), it moves to vcstool-specific locations. In these locations vcstool will look for credentials in either an `auth.conf` file, or `.conf` files inside an `auth.conf.d` directory. From 45d012df4585a9ce2ea8392caa35e6d4cfd4f400 Mon Sep 17 00:00:00 2001 From: Kyle Fazzari Date: Mon, 15 Jun 2020 13:13:55 -0700 Subject: [PATCH 16/16] Compare against Request type instead of str For python 2 compatibility. Signed-off-by: Kyle Fazzari --- vcstool/clients/vcs_base.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/vcstool/clients/vcs_base.py b/vcstool/clients/vcs_base.py index a9579ecc..b392eb1a 100644 --- a/vcstool/clients/vcs_base.py +++ b/vcstool/clients/vcs_base.py @@ -177,10 +177,10 @@ def _retryable_function(*args, **kwargs): def _authenticated_urlopen(uri, retry=2, retry_period=1, timeout=10): - if isinstance(uri, str): - request = Request(uri) - else: + if isinstance(uri, Request): request = uri + else: + request = Request(uri) machine = urlparse(request.get_full_url()).netloc if not machine: