Applied review suggestions.

- renamed function
- updated/added documentation
- properly paramterize test-cases

SEC-240 / CURA-8407

UM/CentralFileStorage.py

@@ -69,8 +69,8 @@ def store(cls, path: str, path_id: str, version: Version = Version("1.0.0"), mov
                 cls._unmoved_files[full_identifier] = path
             return
 
-        if not os.path.exists(cls.centralStorageLocation()):
-            os.makedirs(cls.centralStorageLocation())
+        if not os.path.exists(cls.getCentralStorageLocation()):
+            os.makedirs(cls.getCentralStorageLocation())
         if not os.path.exists(path):
             Logger.debug(f"{path_id} {str(version)} was already stored centrally or the provided path is not correct")
             return
@@ -129,10 +129,10 @@ def _getItemPath(cls, item_id: str, version: Version) -> str:
         :return: A path to store such an item.
         """
         item_name = item_id + "." + str(version)
-        return os.path.join(cls.centralStorageLocation(), item_name)
+        return os.path.join(cls.getCentralStorageLocation(), item_name)
 
     @classmethod
-    def centralStorageLocation(cls) -> str:
+    def getCentralStorageLocation(cls) -> str:
         """
         Gets a directory to store things in a version-neutral location.
         :return: A directory to store things centrally.

UM/Trust.py

@@ -42,6 +42,10 @@ class TrustBasics:
     __signature_filename_extension = ".signature"
     __root_signature_entry = "root_signature"
 
+    # Files that are cached should be removed before verifying:
+    __cached_folder_names = ["__pycache__"]
+    __cached_file_extentions = [".qmlc"]
+
     @staticmethod
     def defaultViolationHandler(message: str) -> None:
         """This violationHandler is called after any other handlers"""
@@ -261,17 +265,25 @@ def saveKeyPair(private_key: "RSAPrivateKeyWithSerialization", private_path: str
             Logger.logException("e", "Save private/public key to '{0}','{1}' failed.".format(private_path, public_path))
         return False
 
-    @staticmethod
-    def removeCached(path: str) -> bool:
+    @classmethod
+    def removeCached(cls, path: str) -> bool:
+        """ Removes any cached files and folders from a (folder) path (like __pycache__ and such).
+
+        This not only prevents people from messing with these hardly checkable things, but also makes the code have less
+        exceptions, since it can be assumed afterwards that the folder is in some sort of 'canonical' state.
+
+        :param path: The path to remove any cached files or folders from.
+        :return: Whether this operation succeeded.
+        """
         try:
             cache_folders_to_empty = []  # type: List[str]
             cache_files_to_remove = []  # type: List[str]
             for root, dirnames, filenames in os.walk(path, followlinks=True):
                 for dirname in dirnames:
-                    if dirname == "__pycache__":
+                    if dirname in cls.__cached_folder_names:
                         cache_folders_to_empty.append(os.path.join(root, dirname))
                 for filename in filenames:
-                    if Path(filename).suffix == ".qmlc":
+                    if Path(filename).suffix in cls.__cached_file_extentions:
                         cache_files_to_remove.append(os.path.join(root, filename))
             for cache_folder in cache_folders_to_empty:
                 for root, dirnames, filenames in os.walk(cache_folder, followlinks=True):
@@ -287,6 +299,12 @@ def removeCached(path: str) -> bool:
 
     @staticmethod
     def isPathInLocation(location: str, path: str) -> bool:
+        """ Whether a path is a sub-folder (or equal) of a given location (path).
+
+        :param location: The given path the other path should be a (sub-)folder of.
+        :param path: The presumptive (sub-)folder.
+        :return: True if path is equal to or matches but deeper than location, False otherwise.
+        """
         try:
             canonical_location = os.path.normpath(location)
             canonical_path = os.path.normpath(path)
@@ -461,12 +479,12 @@ def signedFolderPreStorageCheck(self, path: str) -> bool:
                     self._violation_handler(f"Manifest '{manifest_path}' is not properly self-signed in '{path}'.")
                     return False
 
-            # Check if the central storage file doesn't contain files that would be moved outside the plugin folder,
-            #   or files that would be moved to outside of the central storage location:
+            # Check if the central storage file doesn't contain files that would be moved outside the plugin folder, or
+            # files that would be moved to outside of the central storage location:
             with open(central_storage_filename, "r", encoding = "utf-8") as central_storage_file:
                 central_storage_list = json.loads(central_storage_file.read())
 
-                storage_location = CentralFileStorage.centralStorageLocation()
+                storage_location = CentralFileStorage.getCentralStorageLocation()
                 for file_to_move in central_storage_list:
 
                     # Any file is not from outside of the plugin:

tests/TestTrust.py

@@ -199,7 +199,7 @@ def test_signFolderAndVerify(self, init_trust):
         violation_callback.reset_mock()
 
         # * 'Central file storage'-enabled section *
-        with patch("UM.CentralFileStorage.CentralFileStorage.centralStorageLocation", MagicMock(return_value = central_storage_dir)):
+        with patch("UM.CentralFileStorage.CentralFileStorage.getCentralStorageLocation", MagicMock(return_value = central_storage_dir)):
 
             # Do some set-up (signing, moving files around with the central file storage):
             assert signFolder(private_path, folderpath_large, [], _passphrase)
@@ -245,14 +245,22 @@ def test_signNonexisting(self):
         private_key, public_key = TrustBasics.generateNewKeyPair()
         assert TrustBasics.getFileSignature("file-not-found", private_key) is None
 
-    def test_isPathInLocation(self):
-        assert TrustBasics.isPathInLocation(r"/a/b/c", r"/a/b/c/d")
-        assert TrustBasics.isPathInLocation(r"/a/b/c", r"/a/b/c/d/..")
-        assert not TrustBasics.isPathInLocation(r"/a/b/c", r"/a/b/c/d/../..")
-        assert not TrustBasics.isPathInLocation(r"/a/b/c", r"/a/b")
-        assert not TrustBasics.isPathInLocation(r"/a/b/c", r"/d/q/f")
-        assert TrustBasics.isPathInLocation(r"/a/b/c", r"/a/b/../b/c/d/../e")
-        assert TrustBasics.isPathInLocation(r"/a/b/../d/c", r"/a/d/c")
-        assert not TrustBasics.isPathInLocation(r"/a/b/../d/c", r"/a/d/c.txt")
-        assert not TrustBasics.isPathInLocation(r"/a/b/../d/c", r"/a/b/../b/c/d/../e")
-        assert not TrustBasics.isPathInLocation(r"/a/b/../d/c.txt", r"/a/d/c")
+    @pytest.mark.parametrize("location,subfolder", [
+        (r"/a/b/c", r"/a/b/c/d"),
+        (r"/a/b/c", r"/a/b/c/d/.."),
+        (r"/a/b/c", r"/a/b/../b/c/d/../e"),
+        (r"/a/b/../d/c", r"/a/d/c")
+    ])
+    def test_isPathInLocation(self, location, subfolder):
+        assert TrustBasics.isPathInLocation(location, subfolder)
+
+    @pytest.mark.parametrize("location,subfolder", [
+        (r"/a/b/c", r"/a/b/c/d/../.."),
+        (r"/a/b/c", r"/a/b"),
+        (r"/a/b/c", r"/d/q/f"),
+        (r"/a/b/../d/c", r"/a/d/c.txt"),
+        (r"/a/b/../d/c", r"/a/b/../b/c/d/../e"),
+        (r"/a/b/../d/c.txt", r"/a/d/c")
+    ])
+    def test_notIsPathInLocation(self, location, subfolder):
+        assert not TrustBasics.isPathInLocation(location, subfolder)