diff --git a/.pylintrc b/.pylintrc index 101e3ed..0046729 100644 --- a/.pylintrc +++ b/.pylintrc @@ -2,6 +2,7 @@ good-names= logger disable= + consider-using-f-string, # For Python < 3.6 super-with-arguments, # For Python 2.x raise-missing-from, # For Python 2.x trailing-newlines, diff --git a/msal_extensions/persistence.py b/msal_extensions/persistence.py index 4c56cd5..c6fdad7 100644 --- a/msal_extensions/persistence.py +++ b/msal_extensions/persistence.py @@ -56,19 +56,31 @@ def _auto_hash(input_string): # We do not aim to wrap every os-specific exception. -# Here we define only the most common one, -# otherwise caller would need to catch os-specific persistence exceptions. -class PersistenceNotFound(IOError): # Use IOError rather than OSError as base, +# Here we standardize only the most common ones, +# otherwise caller would need to catch os-specific underlying exceptions. +class PersistenceError(IOError): # Use IOError rather than OSError as base, + """The base exception for persistence.""" # because historically an IOError was bubbled up and expected. # https://github.com/AzureAD/microsoft-authentication-extensions-for-python/blob/0.2.2/msal_extensions/token_cache.py#L38 # Now we want to maintain backward compatibility even when using Python 2.x # It makes no difference in Python 3.3+ where IOError is an alias of OSError. + def __init__(self, err_no=None, message=None, location=None): # pylint: disable=useless-super-delegation + super(PersistenceError, self).__init__(err_no, message, location) + + +class PersistenceNotFound(PersistenceError): """This happens when attempting BasePersistence.load() on a non-existent persistence instance""" def __init__(self, err_no=None, message=None, location=None): super(PersistenceNotFound, self).__init__( - err_no or errno.ENOENT, - message or "Persistence not found", - location) + err_no=errno.ENOENT, + message=message or "Persistence not found", + location=location) + +class PersistenceEncryptionError(PersistenceError): + """This could be raised by persistence.save()""" + +class PersistenceDecryptionError(PersistenceError): + """This could be raised by persistence.load()""" class BasePersistence(ABC): @@ -177,7 +189,13 @@ def __init__(self, location, entropy=''): def save(self, content): # type: (str) -> None - data = self._dp_agent.protect(content) + try: + data = self._dp_agent.protect(content) + except OSError as exception: + raise PersistenceEncryptionError( + err_no=getattr(exception, "winerror", None), # Exists in Python 3 on Windows + message="Encryption failed: {}. Consider disable encryption.".format(exception), + ) with os.fdopen(_open(self._location), 'wb+') as handle: handle.write(data) @@ -186,7 +204,6 @@ def load(self): try: with open(self._location, 'rb') as handle: data = handle.read() - return self._dp_agent.unprotect(data) except EnvironmentError as exp: # EnvironmentError in Py 2.7 works across platform if exp.errno == errno.ENOENT: raise PersistenceNotFound( @@ -199,6 +216,17 @@ def load(self): "DPAPI error likely caused by file content not previously encrypted. " "App developer should migrate by calling save(plaintext) first.") raise + try: + return self._dp_agent.unprotect(data) + except OSError as exception: + raise PersistenceDecryptionError( + err_no=getattr(exception, "winerror", None), # Exists in Python 3 on Windows + message="Decryption failed: {}. " + "App developer may consider this guidance: " + "https://github.com/AzureAD/microsoft-authentication-extensions-for-python/wiki/PersistenceDecryptionError" # pylint: disable=line-too-long + .format(exception), + location=self._location, + ) class KeychainPersistence(BasePersistence): diff --git a/msal_extensions/windows.py b/msal_extensions/windows.py index f1e8b59..03f2022 100644 --- a/msal_extensions/windows.py +++ b/msal_extensions/windows.py @@ -39,6 +39,15 @@ def raw(self): _MEMCPY(blob_buffer, pb_data, cb_data) return blob_buffer.raw +_err_description = { + # Keys came from real world observation, values came from winerror.h (http://errors (Microsoft internal)) + -2146893813: "Key not valid for use in specified state.", + -2146892987: "The requested operation cannot be completed. " + "The computer must be trusted for delegation and " + "the current user account must be configured to allow delegation. " + "See also https://docs.microsoft.com/en-us/windows/security/threat-protection/security-policy-settings/enable-computer-and-user-accounts-to-be-trusted-for-delegation", + 13: "The data is invalid", + } # This code is modeled from a StackOverflow question, which can be found here: # https://stackoverflow.com/questions/463832/using-dpapi-with-python @@ -82,7 +91,7 @@ def protect(self, message): _LOCAL_FREE(result.pbData) err_code = _GET_LAST_ERROR() - raise OSError(256, '', '', err_code) + raise OSError(None, _err_description.get(err_code), None, err_code) def unprotect(self, cipher_text): # type: (bytes) -> str @@ -111,4 +120,4 @@ def unprotect(self, cipher_text): finally: _LOCAL_FREE(result.pbData) err_code = _GET_LAST_ERROR() - raise OSError(256, '', '', err_code) + raise OSError(None, _err_description.get(err_code), None, err_code) diff --git a/tests/test_persistence.py b/tests/test_persistence.py index dfc0963..eab4048 100644 --- a/tests/test_persistence.py +++ b/tests/test_persistence.py @@ -9,11 +9,18 @@ from msal_extensions.persistence import * -is_running_on_travis_ci = bool( # (WTF) What-The-Finding: +def _is_env_var_defined(env_var): + return bool( # (WTF) What-The-Finding: # The bool(...) is necessary, otherwise skipif(...) would treat "true" as # string conditions and then raise an undefined "true" exception. # https://docs.pytest.org/en/latest/historical-notes.html#string-conditions - os.getenv("TRAVIS")) + os.getenv(env_var)) + + +# Note: If you use tox, remember to pass them through via tox.ini +# https://tox.wiki/en/latest/example/basic.html#passing-down-environment-variables +is_running_on_travis_ci = _is_env_var_defined("TRAVIS") +is_running_on_github_ci = _is_env_var_defined("GITHUB_ACTIONS") @pytest.fixture def temp_location(): @@ -42,7 +49,13 @@ def test_nonexistent_file_persistence(temp_location): is_running_on_travis_ci or not sys.platform.startswith('win'), reason="Requires Windows Desktop") def test_file_persistence_with_data_protection(temp_location): - _test_persistence_roundtrip(FilePersistenceWithDataProtection(temp_location)) + try: + _test_persistence_roundtrip(FilePersistenceWithDataProtection(temp_location)) + except PersistenceDecryptionError: + if is_running_on_github_ci or is_running_on_travis_ci: + logging.warning("DPAPI tends to fail on Windows VM. Run this on your desktop to double check.") + else: + raise @pytest.mark.skipif( is_running_on_travis_ci or not sys.platform.startswith('win'), diff --git a/tox.ini b/tox.ini index ecbab74..8a538bc 100644 --- a/tox.ini +++ b/tox.ini @@ -5,5 +5,7 @@ envlist = py27,py35,py36,py37,py38 deps = pytest passenv = TRAVIS + GITHUB_ACTIONS + commands = pytest