Unified exception when load() is called before save() (#67)

* Unified exception when load() is called before save()

* Go with an explicit PersistenceNotFound exception

* Caller has a much simpler pattern to handle read errors

* Change "where" to "location"

* Test cases for PersistentNotFound

Author: rayluo

msal_extensions/persistence.py

@@ -9,6 +9,7 @@
 import abc
 import os
 import errno
+import logging
 try:
     from pathlib import Path  # Built-in in Python 3
 except:
@@ -21,6 +22,9 @@
     ABC = abc.ABCMeta("ABC", (object,), {"__slots__": ()})  # type: ignore
 
 
+logger = logging.getLogger(__name__)
+
+
 def _mkdir_p(path):
     """Creates a directory, and any necessary parents.
 
@@ -41,6 +45,16 @@ def _mkdir_p(path):
             raise
 
 
+# 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(OSError):
+    def __init__(
+            self,
+            err_no=errno.ENOENT, message="Persistence not found", location=None):
+        super(PersistenceNotFound, self).__init__(err_no, message, location)
+
+
 class BasePersistence(ABC):
     """An abstract persistence defining the common interface of this family"""
 
@@ -55,12 +69,18 @@ def save(self, content):
     @abc.abstractmethod
     def load(self):
         # type: () -> str
-        """Load content from this persistence"""
+        """Load content from this persistence.
+
+        Could raise PersistenceNotFound if no save() was called before.
+        """
         raise NotImplementedError
 
     @abc.abstractmethod
     def time_last_modified(self):
-        """Get the last time when this persistence has been modified"""
+        """Get the last time when this persistence has been modified.
+
+        Could raise PersistenceNotFound if no save() was called before.
+        """
         raise NotImplementedError
 
     @abc.abstractmethod
@@ -87,11 +107,32 @@ def save(self, content):
     def load(self):
         # type: () -> str
         """Load content from this persistence"""
-        with open(self._location, 'r') as handle:
-            return handle.read()
+        try:
+            with open(self._location, 'r') as handle:
+                return handle.read()
+        except EnvironmentError as exp:  # EnvironmentError in Py 2.7 works across platform
+            if exp.errno == errno.ENOENT:
+                raise PersistenceNotFound(
+                    message=(
+                        "Persistence not initialized. "
+                        "You can recover by calling a save() first."),
+                    location=self._location,
+                    )
+            raise
+
 
     def time_last_modified(self):
-        return os.path.getmtime(self._location)
+        try:
+            return os.path.getmtime(self._location)
+        except EnvironmentError as exp:  # EnvironmentError in Py 2.7 works across platform
+            if exp.errno == errno.ENOENT:
+                raise PersistenceNotFound(
+                    message=(
+                        "Persistence not initialized. "
+                        "You can recover by calling a save() first."),
+                    location=self._location,
+                    )
+            raise
 
     def touch(self):
         """To touch this file-based persistence without writing content into it"""
@@ -115,13 +156,28 @@ def __init__(self, location, entropy=''):
 
     def save(self, content):
         # type: (str) -> None
+        data = self._dp_agent.protect(content)
         with open(self._location, 'wb+') as handle:
-            handle.write(self._dp_agent.protect(content))
+            handle.write(data)
 
     def load(self):
         # type: () -> str
-        with open(self._location, 'rb') as handle:
-            return self._dp_agent.unprotect(handle.read())
+        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(
+                    message=(
+                        "Persistence not initialized. "
+                        "You can recover by calling a save() first."),
+                    location=self._location,
+                    )
+            logger.exception(
+                "DPAPI error likely caused by file content not previously encrypted. "
+                "App developer should migrate by calling save(plaintext) first.")
+            raise
 
 
 class KeychainPersistence(BasePersistence):
@@ -136,9 +192,10 @@ def __init__(self, signal_location, service_name, account_name):
         """
         if not (service_name and account_name):  # It would hang on OSX
             raise ValueError("service_name and account_name are required")
-        from .osx import Keychain  # pylint: disable=import-outside-toplevel
+        from .osx import Keychain, KeychainError  # pylint: disable=import-outside-toplevel
         self._file_persistence = FilePersistence(signal_location)  # Favor composition
         self._Keychain = Keychain  # pylint: disable=invalid-name
+        self._KeychainError = KeychainError  # pylint: disable=invalid-name
         self._service_name = service_name
         self._account_name = account_name
 
@@ -150,8 +207,21 @@ def save(self, content):
 
     def load(self):
         with self._Keychain() as locker:
-            return locker.get_generic_password(
-                self._service_name, self._account_name)
+            try:
+                return locker.get_generic_password(
+                    self._service_name, self._account_name)
+            except self._KeychainError as ex:
+                if ex.exit_status == self._KeychainError.ITEM_NOT_FOUND:
+                    # This happens when a load() is called before a save().
+                    # We map it into cross-platform error for unified catching.
+                    raise PersistenceNotFound(
+                        location="Service:{} Account:{}".format(
+                            self._service_name, self._account_name),
+                        message=(
+                            "Keychain persistence not initialized. "
+                            "You can recover by call a save() first."),
+                        )
+                raise  # We do not intend to hide any other underlying exceptions
 
     def time_last_modified(self):
         return self._file_persistence.time_last_modified()
@@ -188,7 +258,14 @@ def save(self, content):
             self._file_persistence.touch()  # For time_last_modified()
 
     def load(self):
-        return self._agent.load()
+        data = self._agent.load()
+        if data is None:
+            # Lower level libsecret would return None when found nothing. Here
+            # in persistence layer, we convert it to a unified error for consistence.
+            raise PersistenceNotFound(message=(
+                "Keyring persistence not initialized. "
+                "You can recover by call a save() first."))
+        return data
 
     def time_last_modified(self):
         return self._file_persistence.time_last_modified()

msal_extensions/token_cache.py

@@ -2,14 +2,13 @@
 import os
 import warnings
 import time
-import errno
 import logging
 
 import msal
 
 from .cache_lock import CrossPlatLock
 from .persistence import (
-    _mkdir_p, FilePersistence,
+    _mkdir_p, PersistenceNotFound, FilePersistence,
     FilePersistenceWithDataProtection, KeychainPersistence)
 
 
@@ -35,10 +34,10 @@ def _reload_if_necessary(self):
             if self._last_sync < self._persistence.time_last_modified():
                 self.deserialize(self._persistence.load())
                 self._last_sync = time.time()
-        except EnvironmentError as exp:
-            if exp.errno != errno.ENOENT:
-                raise
-            # Otherwise, from cache's perspective, a nonexistent file is a NO-OP
+        except PersistenceNotFound:
+            # From cache's perspective, a nonexistent persistence is a NO-OP.
+            pass
+        # However, existing data unable to be decrypted will still be bubbled up.
 
     def modify(self, credential_type, old_entry, new_key_value_pairs=None):
         with CrossPlatLock(self._lock_location):

tests/test_persistence.py

@@ -26,22 +26,45 @@ def _test_persistence_roundtrip(persistence):
     persistence.save(payload)
     assert persistence.load() == payload
 
+def _test_nonexistent_persistence(persistence):
+    with pytest.raises(PersistenceNotFound):
+        persistence.load()
+    with pytest.raises(PersistenceNotFound):
+        persistence.time_last_modified()
+
 def test_file_persistence(temp_location):
     _test_persistence_roundtrip(FilePersistence(temp_location))
 
+def test_nonexistent_file_persistence(temp_location):
+    _test_nonexistent_persistence(FilePersistence(temp_location))
+
 @pytest.mark.skipif(
     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))
 
+@pytest.mark.skipif(
+    is_running_on_travis_ci or not sys.platform.startswith('win'),
+    reason="Requires Windows Desktop")
+def test_nonexistent_file_persistence_with_data_protection(temp_location):
+    _test_nonexistent_persistence(FilePersistenceWithDataProtection(temp_location))
+
 @pytest.mark.skipif(
     not sys.platform.startswith('darwin'),
     reason="Requires OSX. Whether running on TRAVIS CI does not seem to matter.")
 def test_keychain_persistence(temp_location):
     _test_persistence_roundtrip(KeychainPersistence(
         temp_location, "my_service_name", "my_account_name"))
 
+@pytest.mark.skipif(
+    not sys.platform.startswith('darwin'),
+    reason="Requires OSX. Whether running on TRAVIS CI does not seem to matter.")
+def test_nonexistent_keychain_persistence(temp_location):
+    random_service_name = random_account_name = str(id(temp_location))
+    _test_nonexistent_persistence(
+        KeychainPersistence(temp_location, random_service_name, random_account_name))
+
 @pytest.mark.skipif(
     is_running_on_travis_ci or not sys.platform.startswith('linux'),
     reason="Requires Linux Desktop. Headless or SSH session won't work.")
@@ -52,3 +75,14 @@ def test_libsecret_persistence(temp_location):
         {"my_attr_1": "foo", "my_attr_2": "bar"},
         ))
 
+@pytest.mark.skipif(
+    is_running_on_travis_ci or not sys.platform.startswith('linux'),
+    reason="Requires Linux Desktop. Headless or SSH session won't work.")
+def test_nonexistent_libsecret_persistence(temp_location):
+    random_schema_name = random_value = str(id(temp_location))
+    _test_nonexistent_persistence(LibsecretPersistence(
+        temp_location,
+        random_schema_name,
+        {"my_attr_1": random_value, "my_attr_2": random_value},
+        ))
+