Provide actionable messages for 2 dpapi errors

Add underlying OSError info into Persistence exceptions

Refer to the PersistenceDecryptionError wiki page

Update msal_extensions/windows.py

Co-authored-by: Jiashuo Li <[email protected]>

Ignore consider-using-f-string, once and for all

Author: rayluo

Diff for: .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 for: 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 for: 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 for: 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 for: tox.ini

@@ -5,5 +5,7 @@ envlist = py27,py35,py36,py37,py38
 deps = pytest
 passenv =
     TRAVIS
+    GITHUB_ACTIONS
+
 commands =
     pytest