From 8c75b621bfe03725cc8da071516ebc66d3872760 Mon Sep 17 00:00:00 2001
From: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
Date: Wed, 26 Feb 2020 12:22:55 +0000
Subject: [PATCH] Ensure 'deactivated' parameter is a boolean on user admin
 API, Fix error handling of call to deactivate user (#6990)

---
 changelog.d/6990.bugfix         |  1 +
 synapse/rest/admin/users.py     | 11 +++---
 synapse/rest/client/v1/login.py |  1 +
 tests/rest/admin/test_user.py   | 59 +++++++++++++++++++++++++++++++++
 4 files changed, 68 insertions(+), 4 deletions(-)
 create mode 100644 changelog.d/6990.bugfix

diff --git a/changelog.d/6990.bugfix b/changelog.d/6990.bugfix
new file mode 100644
index 0000000000..8c1c48f4d4
--- /dev/null
+++ b/changelog.d/6990.bugfix
@@ -0,0 +1 @@
+Prevent user from setting 'deactivated' to anything other than a bool on the v2 PUT /users Admin API.
\ No newline at end of file
diff --git a/synapse/rest/admin/users.py b/synapse/rest/admin/users.py
index 2107b5dc56..c5b461a236 100644
--- a/synapse/rest/admin/users.py
+++ b/synapse/rest/admin/users.py
@@ -228,13 +228,16 @@ class UserRestServletV2(RestServlet):
                     )
 
             if "deactivated" in body:
-                deactivate = bool(body["deactivated"])
+                deactivate = body["deactivated"]
+                if not isinstance(deactivate, bool):
+                    raise SynapseError(
+                        400, "'deactivated' parameter is not of type boolean"
+                    )
+
                 if deactivate and not user["deactivated"]:
-                    result = await self.deactivate_account_handler.deactivate_account(
+                    await self.deactivate_account_handler.deactivate_account(
                         target_user.to_string(), False
                     )
-                    if not result:
-                        raise SynapseError(500, "Could not deactivate user")
 
             user = await self.admin_handler.get_user(target_user)
             return 200, user
diff --git a/synapse/rest/client/v1/login.py b/synapse/rest/client/v1/login.py
index 1294e080dc..2c99536678 100644
--- a/synapse/rest/client/v1/login.py
+++ b/synapse/rest/client/v1/login.py
@@ -599,6 +599,7 @@ class SSOAuthHandler(object):
         redirect_url = self._add_login_token_to_redirect_url(
             client_redirect_url, login_token
         )
+        # Load page
         request.redirect(redirect_url)
         finish_request(request)
 
diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py
index 490ce8f55d..cbe4a6a51f 100644
--- a/tests/rest/admin/test_user.py
+++ b/tests/rest/admin/test_user.py
@@ -507,3 +507,62 @@ class UserRestTestCase(unittest.HomeserverTestCase):
         self.assertEqual(1, channel.json_body["admin"])
         self.assertEqual(0, channel.json_body["is_guest"])
         self.assertEqual(1, channel.json_body["deactivated"])
+
+    def test_accidental_deactivation_prevention(self):
+        """
+        Ensure an account can't accidentally be deactivated by using a str value
+        for the deactivated body parameter
+        """
+        self.hs.config.registration_shared_secret = None
+
+        # Create user
+        body = json.dumps({"password": "abc123"})
+
+        request, channel = self.make_request(
+            "PUT",
+            self.url,
+            access_token=self.admin_user_tok,
+            content=body.encode(encoding="utf_8"),
+        )
+        self.render(request)
+
+        self.assertEqual(201, int(channel.result["code"]), msg=channel.result["body"])
+        self.assertEqual("@bob:test", channel.json_body["name"])
+        self.assertEqual("bob", channel.json_body["displayname"])
+
+        # Get user
+        request, channel = self.make_request(
+            "GET", self.url, access_token=self.admin_user_tok,
+        )
+        self.render(request)
+
+        self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"])
+        self.assertEqual("@bob:test", channel.json_body["name"])
+        self.assertEqual("bob", channel.json_body["displayname"])
+        self.assertEqual(0, channel.json_body["deactivated"])
+
+        # Change password (and use a str for deactivate instead of a bool)
+        body = json.dumps({"password": "abc123", "deactivated": "false"})  # oops!
+
+        request, channel = self.make_request(
+            "PUT",
+            self.url,
+            access_token=self.admin_user_tok,
+            content=body.encode(encoding="utf_8"),
+        )
+        self.render(request)
+
+        self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"])
+
+        # Check user is not deactivated
+        request, channel = self.make_request(
+            "GET", self.url, access_token=self.admin_user_tok,
+        )
+        self.render(request)
+
+        self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"])
+        self.assertEqual("@bob:test", channel.json_body["name"])
+        self.assertEqual("bob", channel.json_body["displayname"])
+
+        # Ensure they're still alive
+        self.assertEqual(0, channel.json_body["deactivated"])