Merge pull request OpenMined#8069 from khoaguin/mongo-permission-mode…

…l-tests

Tests for MongoDB permissions

packages/syft/src/syft/store/kv_document_store.py

@@ -450,6 +450,7 @@ def _delete(
                 ActionObjectWRITE(uid=qk.value, credentials=credentials)
             ):
                 _obj = self.data.pop(qk.value)
+                self.permissions.pop(qk.value)
                 self._delete_unique_keys_for(_obj)
                 self._delete_search_keys_for(_obj)
                 return Ok(SyftSuccess(message="Deleted"))

packages/syft/src/syft/store/mongo_client.py

@@ -73,7 +73,7 @@ class MongoStoreClientConfig(StoreClientConfig):
             Controls how long (in milliseconds) the driver will wait to find an available, appropriate
             server to carry out a database operation; while it is waiting, multiple server monitoring
             operations may be carried out, each controlled by `connectTimeoutMS`.
-            Defaults to ``30000`` (30 seconds).
+            Defaults to ``120000`` (120 seconds).
         `waitQueueTimeoutMS`: (integer or None)
             How long (in milliseconds) a thread will wait for a socket from the pool if the pool
             has no free sockets. Defaults to ``None`` (no timeout).
@@ -108,7 +108,7 @@ class MongoStoreClientConfig(StoreClientConfig):
     timeoutMS: int = 0
     socketTimeoutMS: int = 0
     connectTimeoutMS: int = 20000
-    serverSelectionTimeoutMS: int = 30000
+    serverSelectionTimeoutMS: int = 120000
     waitQueueTimeoutMS: Optional[int] = None
     heartbeatFrequencyMS: int = 10000
     appname: str = "pysyft"

packages/syft/src/syft/store/mongo_document_store.py

@@ -3,6 +3,7 @@
 from typing import Dict
 from typing import List
 from typing import Optional
+from typing import Set
 from typing import Type
 
 # third party
@@ -229,6 +230,8 @@ def _set(
         add_permissions: Optional[List[ActionObjectPermission]] = None,
         ignore_duplicates: bool = False,
     ) -> Result[SyftObject, str]:
+        # TODO: Refactor this function since now it's doing both set and
+        # update at the same time
         write_permission = ActionObjectWRITE(uid=obj.id, credentials=credentials)
         can_write = self.has_permission(write_permission)
 
@@ -379,21 +382,37 @@ def _get_all_from_store(
     def _delete(
         self, credentials: SyftVerifyKey, qk: QueryKey, has_permission: bool = False
     ) -> Result[SyftSuccess, Err]:
+        if not (
+            has_permission
+            or self.has_permission(
+                ActionObjectWRITE(uid=qk.value, credentials=credentials)
+            )
+        ):
+            return Err(f"You don't have permission to delete object with qk: {qk}")
+
         collection_status = self.collection
         if collection_status.is_err():
             return collection_status
         collection: MongoCollection = collection_status.ok()
 
-        if has_permission or self.has_permission(
-            ActionObjectWRITE(uid=qk.value, credentials=credentials)
-        ):
-            qks = QueryKeys(qks=qk)
-            result = collection.delete_one(filter=qks.as_dict_mongo)
-
-            if result.deleted_count == 1:
-                return Ok(SyftSuccess(message="Deleted"))
+        collection_permissions_status = self.permissions
+        if collection_permissions_status.is_err():
+            return collection_permissions_status
+        collection_permissions: MongoCollection = collection_permissions_status.ok()
 
-        return Err(f"Failed to delete object with qk: {qk}")
+        qks = QueryKeys(qks=qk)
+        # delete the object
+        result = collection.delete_one(filter=qks.as_dict_mongo)
+        # delete the object's permission
+        result_permission = collection_permissions.delete_one(filter=qks.as_dict_mongo)
+        if result.deleted_count == 1 and result_permission.deleted_count == 1:
+            return Ok(SyftSuccess(message="Object and its permission are deleted"))
+        elif result.deleted_count == 0:
+            return Err(f"Failed to delete object with qk: {qk}")
+        else:
+            return Err(
+                f"Object with qk: {qk} was deleted, but failed to delete its corresponding permission"
+            )
 
     def has_permission(self, permission: ActionObjectPermission) -> bool:
         """Check if the permission is inside the permission collection"""
@@ -402,17 +421,17 @@ def has_permission(self, permission: ActionObjectPermission) -> bool:
             return False
         collection_permissions: MongoCollection = collection_permissions_status.ok()
 
-        # TODO: fix for other admins
-        if self.root_verify_key.verify == permission.credentials.verify:
-            return True
-
         permissions: Optional[Dict] = collection_permissions.find_one(
             {"_id": permission.uid}
         )
 
         if permissions is None:
             return False
 
+        # TODO: fix for other admins
+        if self.root_verify_key.verify == permission.credentials.verify:
+            return True
+
         if permission.permission_string in permissions["permissions"]:
             return True
 
@@ -441,7 +460,7 @@ def add_permission(self, permission: ActionObjectPermission) -> Result[None, Err
             {"_id": permission.uid}
         )
         if permissions is None:
-            # Permission doesn't exits, add a new one
+            # Permission doesn't exist, add a new one
             collection_permissions.insert_one(
                 {
                     "_id": permission.uid,
@@ -450,7 +469,7 @@ def add_permission(self, permission: ActionObjectPermission) -> Result[None, Err
             )
         else:
             # update the permissions with the new permission string
-            permission_strings: set = permissions["permissions"]
+            permission_strings: Set = permissions["permissions"]
             permission_strings.add(permission.permission_string)
             collection_permissions.update_one(
                 {"_id": permission.uid}, {"$set": {"permissions": permission_strings}}
@@ -472,11 +491,18 @@ def remove_permission(
         )
         if permissions is None:
             return Err(f"permission with UID {permission.uid} not found!")
-        permissions_strings: set = permissions["permissions"]
-        permissions_strings.remove(permission.permission_string)
-        collection_permissions.update_one(
-            {"_id": permission.uid}, {"$set": {"permissions": permissions_strings}}
-        )
+        permissions_strings: Set = permissions["permissions"]
+        if permission.permission_string in permissions_strings:
+            permissions_strings.remove(permission.permission_string)
+            if len(permissions_strings) > 0:
+                collection_permissions.update_one(
+                    {"_id": permission.uid},
+                    {"$set": {"permissions": permissions_strings}},
+                )
+            else:
+                collection_permissions.delete_one({"_id": permission.uid})
+        else:
+            return Err(f"the permission {permission.permission_string} does not exist!")
 
     def take_ownership(
         self, uid: UID, credentials: SyftVerifyKey

packages/syft/tests/conftest.py

@@ -46,6 +46,11 @@ def guest_client(worker):
     return worker.guest_client
 
 
+@pytest.fixture(autouse=True)
+def guest_verify_key(worker):
+    return worker.guest_client.credentials.verify_key
+
+
 @pytest.fixture(autouse=True)
 def guest_domain_client(root_domain_client):
     return root_domain_client.guest()

packages/syft/tests/syft/stores/kv_document_store_test.py

@@ -101,7 +101,7 @@ def test_kv_store_partition_delete(
 
     assert len(kv_store_partition.all(root_verify_key).ok()) == len(objs)
 
-    # random object
+    # can't delete a random object since it was not added
     obj = MockSyftObject(data="bogus")
     key = kv_store_partition.settings.store_key.with_obj(obj)
     res = kv_store_partition.delete(root_verify_key, key)
@@ -114,10 +114,13 @@ def test_kv_store_partition_delete(
         res = kv_store_partition.delete(root_verify_key, key)
         assert res.is_ok()
         assert len(kv_store_partition.all(root_verify_key).ok()) == len(objs) - idx - 1
+        # check that the corresponding permissions were also deleted
+        assert len(kv_store_partition.data) == len(kv_store_partition.permissions)
 
         res = kv_store_partition.delete(root_verify_key, key)
         assert res.is_err()
         assert len(kv_store_partition.all(root_verify_key).ok()) == len(objs) - idx - 1
+        assert len(kv_store_partition.data) == len(kv_store_partition.permissions)
 
     assert len(kv_store_partition.all(root_verify_key).ok()) == 0