Merge pull request #9344 from OpenMined/root-admin-delete-test

add tests to prevent updating root users
OpenMined · Oct 9, 2024 · 72aa14c · 72aa14c
2 parents 99e6659 + eadb45e
commit 72aa14c
Show file tree

Hide file tree

Showing 2 changed files with 54 additions and 9 deletions.
diff --git a/notebooks/scenarios/bigquery/000-start-and-configure-server-and-admins.ipynb b/notebooks/scenarios/bigquery/000-start-and-configure-server-and-admins.ipynb
@@ -25,6 +25,7 @@
     "\n",
     "# syft absolute\n",
     "import syft as sy\n",
+    "from syft.server.credentials import SyftVerifyKey\n",
     "from syft.util.test_helpers.checkpoint import create_checkpoint\n",
     "from syft.util.test_helpers.email_helpers import get_email_server"
    ]
@@ -83,9 +84,7 @@
    "metadata": {},
    "outputs": [],
    "source": [
-    "root_client = sy.login(\n",
-    "    url=\"http://localhost:8080\", email=ROOT_EMAIL, password=ROOT_PASSWORD\n",
-    ")"
+    "root_client = server.login(email=ROOT_EMAIL, password=ROOT_PASSWORD)"
    ]
   },
   {
@@ -97,6 +96,44 @@
     "root_client.users"
    ]
   },
+  {
+   "cell_type": "markdown",
+   "metadata": {},
+   "source": [
+    "# Verify we cannot update the root role or verify key"
+   ]
+  },
+  {
+   "cell_type": "code",
+   "execution_count": null,
+   "metadata": {},
+   "outputs": [],
+   "source": [
+    "root_admin_id = root_client.users.search(email=ROOT_EMAIL)[0].id"
+   ]
+  },
+  {
+   "cell_type": "code",
+   "execution_count": null,
+   "metadata": {},
+   "outputs": [],
+   "source": [
+    "with sy.raises(sy.SyftException):\n",
+    "    root_client.users.update(uid=root_admin_id, role=\"guest\")"
+   ]
+  },
+  {
+   "cell_type": "code",
+   "execution_count": null,
+   "metadata": {},
+   "outputs": [],
+   "source": [
+    "with sy.raises(sy.SyftException):\n",
+    "    root_client.users.update(\n",
+    "        uid=root_admin_id, verify_key=SyftVerifyKey.from_string(\"0\" * 64)\n",
+    "    )"
+   ]
+  },
   {
    "cell_type": "markdown",
    "metadata": {},
@@ -234,11 +271,6 @@
   }
  ],
  "metadata": {
-  "kernelspec": {
-   "display_name": "Python 3 (ipykernel)",
-   "language": "python",
-   "name": "python3"
-  },
   "language_info": {
    "codemirror_mode": {
     "name": "ipython",
@@ -249,7 +281,7 @@
    "name": "python",
    "nbconvert_exporter": "python",
    "pygments_lexer": "ipython3",
-   "version": "3.11.10"
+   "version": "3.12.5"
   }
  },
  "nbformat": 4,

diff --git a/packages/syft/src/syft/service/user/user_service.py b/packages/syft/src/syft/service/user/user_service.py
@@ -461,6 +461,19 @@ def update(
                     public_message=f"You are not allowed to modify '{field_name}'."
                 )
 
+        # important to prevent root admins from shooting themselves in the foot
+        if (
+            user_update.role is not Empty  # type: ignore
+            and user.verify_key == context.server.verify_key
+        ):
+            raise SyftException(public_message="Cannot update root role")
+
+        if (
+            user_update.verify_key is not Empty
+            and user.verify_key == context.server.verify_key
+        ):
+            raise SyftException(public_message="Cannot update root verify key")
+
         if user_update.name is not Empty and user_update.name.strip() == "":  # type: ignore[comparison-overlap]
             raise SyftException(public_message="Name can't be an empty string.")