diff --git a/docs/changes.rst b/docs/changes.rst index 4a10b86..4427b8a 100644 --- a/docs/changes.rst +++ b/docs/changes.rst @@ -39,12 +39,18 @@ Released: not yet * Added missing exception handling to 'Keyring.set_password()' and improved exception chaining in 'Keyring.get_password()' and 'Keyring.delete_password()'. +* Fixed an UnboundLocalError when the temporary file could not be created or + written during 'EasyVault.encrypt()' / 'decrypt()'. + **Enhancements:** * Added a 'easy-vault delete-password' command that deletes the password for a vault file in the keyring service. Added a corresponding 'Keyring.delete_password()' method. (issues #33 and #35) +* Improved error messages when writing vault file during 'EasyVault.encrypt()' + / 'decrypt()'. + * Test: Improved test coverage of Keyring class. **Cleanup:** diff --git a/easy_vault/_easy_vault.py b/easy_vault/_easy_vault.py index cf182b1..0049649 100644 --- a/easy_vault/_easy_vault.py +++ b/easy_vault/_easy_vault.py @@ -538,22 +538,29 @@ def write_file(filepath, data): tfpath = tfp.name except (OSError, IOError) as exc: new_exc = EasyVaultFileError( - "Cannot open temporary file {tfn} for writing: {exc}". - format(tfn=tfp.name, exc=exc)) + "Cannot open temporary file for writing: {exc}". + format(exc=exc)) new_exc.__cause__ = None raise new_exc # EasyVaultFileError + # On Windows, the temp file may be on a different drive than the + # original file, so os.rename() cannot be used and we copy/delete instead. try: - # On Windows, the temp file may be on a different drive than the - # original file, so os.rename() cannot be used. shutil.copy(tfpath, filepath) - os.remove(tfpath) except (OSError, IOError) as exc: new_exc = EasyVaultFileError( - "Cannot rename temporary file {tfn} to vault file {fn}: {exc}". + "Cannot copy temporary file {tfn} to vault file {fn}: {exc}". format(tfn=tfpath, fn=filepath, exc=exc)) new_exc.__cause__ = None raise new_exc # EasyVaultFileError + try: + os.remove(tfpath) + except (OSError, IOError) as exc: + new_exc = EasyVaultFileError( + "Cannot remove temporary file {tfn}: {exc}". + format(tfn=tfpath, exc=exc)) + new_exc.__cause__ = None + raise new_exc # EasyVaultFileError def chunks(iterable, size): diff --git a/tests/unittest/test_easy_vault.py b/tests/unittest/test_easy_vault.py index bef9acd..db7fd83 100644 --- a/tests/unittest/test_easy_vault.py +++ b/tests/unittest/test_easy_vault.py @@ -15,15 +15,25 @@ """ from __future__ import absolute_import, print_function +try: + from unittest import mock +except ImportError: + import mock import pytest import six +import yaml from easy_vault import EasyVault, EasyVaultFileError, EasyVaultEncryptError, \ - EasyVaultDecryptError + EasyVaultDecryptError, EasyVaultYamlError from ..utils.simplified_test_function import simplified_test_function from ..utils.vault_utils import saved_file, assert_files_bytes_equal, \ assert_files_text_equal, assert_content_text_equal +if six.PY2: + BUILTINS_OPEN = '__builtin__.open' +else: + BUILTINS_OPEN = 'builtins.open' + TEST_VAULT_DECRYPTED = 'tests/testfiles/vault_decrypted.yml' TEST_VAULT_ENCRYPTED = 'tests/testfiles/vault_encrypted.yml' TEST_VAULT_ERR_MONIKER = 'tests/testfiles/vault_err_moniker.yml' @@ -316,6 +326,36 @@ def test_vault_encrypt(testcase, filepath, password): assert vault.is_encrypted() +@pytest.mark.parametrize( + "patched_function", + [ + 'tempfile.NamedTemporaryFile', + 'shutil.copy', + 'os.remove', + ] +) +@pytest.mark.parametrize( + "patched_exc", + [IOError, OSError] +) +def test_vault_encrypt_fail(patched_exc, patched_function): + """ + Test function for EasyVault.encrypt() where a function raises an exception. + """ + filepath = TEST_VAULT_DECRYPTED + password = TEST_VAULT_PASSWORD + + # Since saved_file() depends on a functioning open(), we need to patch + # open() after using saved_file(). + with saved_file(filepath): + with mock.patch(patched_function, side_effect=patched_exc): + vault = EasyVault(filepath, password) + with pytest.raises(EasyVaultFileError): + + # The code to be tested + vault.encrypt() + + TESTCASES_VAULT_DECRYPT = [ # Testcases for EasyVault.decrypt() @@ -394,6 +434,36 @@ def test_vault_decrypt(testcase, filepath, password, exp_filepath): assert_files_text_equal(filepath, exp_filepath) +@pytest.mark.parametrize( + "patched_function", + [ + 'tempfile.NamedTemporaryFile', + 'shutil.copy', + 'os.remove', + ] +) +@pytest.mark.parametrize( + "patched_exc", + [IOError, OSError] +) +def test_vault_decrypt_fail(patched_exc, patched_function): + """ + Test function for EasyVault.decrypt() where a function raises an exception. + """ + filepath = TEST_VAULT_ENCRYPTED + password = TEST_VAULT_PASSWORD + + # Since saved_file() depends on a functioning open(), we need to patch + # open() after using saved_file(). + with saved_file(filepath): + with mock.patch(patched_function, side_effect=patched_exc): + vault = EasyVault(filepath, password) + with pytest.raises(EasyVaultFileError): + + # The code to be tested + vault.decrypt() + + TESTCASES_VAULT_GET = [ # Testcases for EasyVault.get_*() @@ -548,3 +618,170 @@ def test_vault_get_yaml(testcase, filepath, password, exp_filepath): assert_files_bytes_equal(filepath, saved_filepath) assert isinstance(act_yaml, (dict, list)) + + +@pytest.mark.parametrize( + "failing_open", + [0, 1] +) +@pytest.mark.parametrize( + "filepath", + [TEST_VAULT_ENCRYPTED, TEST_VAULT_DECRYPTED] +) +@pytest.mark.parametrize( + "open_exc", + [IOError, OSError] +) +def test_vault_get_bytes_fail(open_exc, filepath, failing_open): + """ + Test function for EasyVault.get_bytes() that fails because open() on vault + file raises an exception. + """ + password = TEST_VAULT_PASSWORD + + # Since saved_file() depends on a functioning open(), we need to patch + # open() after using saved_file(). + with saved_file(filepath): + + # The following ideally would be a simple counter. In Python 3, that + # is possible with the 'nonlocal' statement, but since we support + # Python 2, we have the counter in a dict to avoid the rebinding. + local_dict = dict() + local_dict['count'] = 0 + + builtins_open = open + + def new_open(*args, **kwargs): + """Mock wrapper for open()""" + if local_dict['count'] == failing_open: + raise open_exc() + local_dict['count'] += 1 + return builtins_open(*args, **kwargs) + + with mock.patch(BUILTINS_OPEN, new_open): + + vault = EasyVault(filepath, password) + with pytest.raises(EasyVaultFileError): + + # The code to be tested + vault.get_bytes() + + +@pytest.mark.parametrize( + "failing_open", + [0, 1] +) +@pytest.mark.parametrize( + "filepath", + [TEST_VAULT_ENCRYPTED, TEST_VAULT_DECRYPTED] +) +@pytest.mark.parametrize( + "open_exc", + [IOError, OSError] +) +def test_vault_get_text_fail(open_exc, filepath, failing_open): + """ + Test function for EasyVault.get_text() that fails because open() on vault + file raises an exception. + """ + password = TEST_VAULT_PASSWORD + + # Since saved_file() depends on a functioning open(), we need to patch + # open() after using saved_file(). + with saved_file(filepath): + + # The following ideally would be a simple counter. In Python 3, that + # is possible with the 'nonlocal' statement, but since we support + # Python 2, we have the counter in a dict to avoid the rebinding. + local_dict = dict() + local_dict['count'] = 0 + + builtins_open = open + + def new_open(*args, **kwargs): + """Mock wrapper for open()""" + if local_dict['count'] == failing_open: + raise open_exc() + local_dict['count'] += 1 + return builtins_open(*args, **kwargs) + + with mock.patch(BUILTINS_OPEN, new_open): + + vault = EasyVault(filepath, password) + with pytest.raises(EasyVaultFileError): + + # The code to be tested + vault.get_text() + + +@pytest.mark.parametrize( + "failing_open", + [0, 1] +) +@pytest.mark.parametrize( + "filepath", + [TEST_VAULT_ENCRYPTED, TEST_VAULT_DECRYPTED] +) +@pytest.mark.parametrize( + "open_exc", + [IOError, OSError] +) +def test_vault_get_yaml_fail1(open_exc, filepath, failing_open): + """ + Test function for EasyVault.get_yaml() that fails because open() on vault + file raises an exception. + """ + password = TEST_VAULT_PASSWORD + + # Since saved_file() depends on a functioning open(), we need to patch + # open() after using saved_file(). + with saved_file(filepath): + + # The following ideally would be a simple counter. In Python 3, that + # is possible with the 'nonlocal' statement, but since we support + # Python 2, we have the counter in a dict to avoid the rebinding. + local_dict = dict() + local_dict['count'] = 0 + + builtins_open = open + + def new_open(*args, **kwargs): + """Mock wrapper for open()""" + if local_dict['count'] == failing_open: + raise open_exc() + local_dict['count'] += 1 + return builtins_open(*args, **kwargs) + + with mock.patch(BUILTINS_OPEN, new_open): + + vault = EasyVault(filepath, password) + with pytest.raises(EasyVaultFileError): + + # The code to be tested + vault.get_yaml() + + +@pytest.mark.parametrize( + "filepath", + [TEST_VAULT_ENCRYPTED, TEST_VAULT_DECRYPTED] +) +@pytest.mark.parametrize( + "yaml_load_exc", + [yaml.YAMLError] +) +def test_vault_get_yaml_fail2(yaml_load_exc, filepath): + """ + Test function for EasyVault.get_yaml() that fails because yaml.safe_load() + on vault file raises an exception. + """ + password = TEST_VAULT_PASSWORD + + # Since saved_file() depends on a functioning open(), we need to patch + # open() after using saved_file(). + with saved_file(filepath): + with mock.patch.object(yaml, 'safe_load', side_effect=yaml_load_exc): + vault = EasyVault(filepath, password) + with pytest.raises(EasyVaultYamlError): + + # The code to be tested + vault.get_yaml()