Merge pull request #8090 from khoaguin/user-update-bug

Fix the bug "Guest user is able to change email to owner and essentially kills nodes"

Author: shubham3121

packages/syft/src/syft/service/user/user_service.py

@@ -229,13 +229,12 @@ def update(
         # Get user to be updated by its UID
         result = self.stash.get_by_uid(credentials=context.credentials, uid=uid)
 
-        # TODO: ADD Email Validation
-        # check if the email already exists
+        # check if the email already exists (with root's key)
         if user_update.email is not Empty:
-            user_with_email = self.stash.get_by_email(
-                credentials=context.credentials, email=user_update.email
+            user_with_email_exists: bool = self.stash.email_exists(
+                email=user_update.email
             )
-            if user_with_email.ok() is not None:
+            if user_with_email_exists:
                 return SyftError(
                     message=f"A user with the email {user_update.email} already exists."
                 )

packages/syft/src/syft/service/user/user_stash.py

@@ -76,6 +76,13 @@ def get_by_email(
         qks = QueryKeys(qks=[EmailPartitionKey.with_obj(email)])
         return self.query_one(credentials=credentials, qks=qks)
 
+    def email_exists(self, email: str) -> bool:
+        res = self.get_by_email(credentials=self.admin_verify_key().ok(), email=email)
+        if res.ok() is None:
+            return False
+        else:
+            return True
+
     def get_by_role(
         self, credentials: SyftVerifyKey, role: ServiceRole
     ) -> Result[Optional[User], str]:

packages/syft/tests/syft/users/user_test.py

@@ -8,6 +8,7 @@
 from syft import SyftSuccess
 from syft.client.api import SyftAPICall
 from syft.client.domain_client import DomainClient
+from syft.node.node import get_default_root_email
 from syft.node.worker import Worker
 from syft.service.context import AuthedServiceContext
 from syft.service.user.user import ServiceRole
@@ -32,7 +33,7 @@ def get_users(worker):
     )
 
 
-def get_mock_client(root_client, role):
+def get_mock_client(root_client, role) -> DomainClient:
     worker = root_client.api.connection.node
     client = worker.guest_client
     mail = Faker().email()
@@ -66,17 +67,17 @@ def manually_call_service(worker, client, service, args=None, kwargs=None):
 
 
 @pytest.fixture
-def guest_client(worker):
+def guest_client(worker) -> DomainClient:
     return get_mock_client(worker.root_client, ServiceRole.GUEST)
 
 
 @pytest.fixture
-def ds_client(worker):
+def ds_client(worker) -> DomainClient:
     return get_mock_client(worker.root_client, ServiceRole.DATA_SCIENTIST)
 
 
 @pytest.fixture
-def do_client(worker):
+def do_client(worker) -> DomainClient:
     return get_mock_client(worker.root_client, ServiceRole.DATA_OWNER)
 
 
@@ -233,6 +234,24 @@ def test_user_update(root_client):
         )
 
 
+def test_guest_user_update_to_root_email_failed(
+    root_client: DomainClient,
+    do_client: DomainClient,
+    guest_client: DomainClient,
+    ds_client: DomainClient,
+) -> None:
+    default_root_email: str = get_default_root_email()
+    user_update_to_root_email = UserUpdate(email=default_root_email)
+    for client in [root_client, do_client, guest_client, ds_client]:
+        res = client.api.services.user.update(
+            uid=client.me.id, user_update=user_update_to_root_email
+        )
+        assert isinstance(res, SyftError)
+        assert (
+            res.message == f"A user with the email {default_root_email} already exists."
+        )
+
+
 def test_user_view_set_password(worker: Worker, root_client: DomainClient) -> None:
     root_client.me.set_password("123", confirm=False)
     email = root_client.me.email
@@ -260,7 +279,6 @@ def test_user_view_set_invalid_email(
     [
         ("[email protected]", "[email protected]"),
         ("[email protected]", "[email protected]"),
-        ("[email protected]", "[email protected]"),
     ],
 )
 def test_user_view_set_email_success(
@@ -275,6 +293,22 @@ def test_user_view_set_email_success(
     assert isinstance(result2, SyftSuccess)
 
 
+def test_user_view_set_default_admin_email_failed(
+    ds_client: DomainClient, guest_client: DomainClient
+) -> None:
+    default_root_email = get_default_root_email()
+    result = ds_client.me.set_email(default_root_email)
+    assert isinstance(result, SyftError)
+    assert (
+        result.message == f"A user with the email {default_root_email} already exists."
+    )
+    result_2 = guest_client.me.set_email(default_root_email)
+    assert isinstance(result_2, SyftError)
+    assert (
+        result.message == f"A user with the email {default_root_email} already exists."
+    )
+
+
 def test_user_view_set_duplicated_email(
     root_client: DomainClient, ds_client: DomainClient, guest_client: DomainClient
 ) -> None: