From f1da64461b08be2164f841514052b8526c5745dc Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 14 Dec 2021 17:06:44 +0000 Subject: [PATCH 01/13] Add a /users/v3/USER_ID endpoint which removes the 'password_hash' parameter --- synapse/rest/admin/__init__.py | 2 ++ synapse/rest/admin/users.py | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/synapse/rest/admin/__init__.py b/synapse/rest/admin/__init__.py index 701c609c1208..c3f73a1bd822 100644 --- a/synapse/rest/admin/__init__.py +++ b/synapse/rest/admin/__init__.py @@ -80,6 +80,7 @@ UserMembershipRestServlet, UserRegisterServlet, UserRestServletV2, + UserRestServletV3, UsersRestServletV2, UserTokenRestServlet, WhoisRestServlet, @@ -247,6 +248,7 @@ def register_servlets(hs: "HomeServer", http_server: HttpServer) -> None: UserAdminServlet(hs).register(http_server) UserMembershipRestServlet(hs).register(http_server) UserTokenRestServlet(hs).register(http_server) + UserRestServletV3(hs).register(http_server) UserRestServletV2(hs).register(http_server) UsersRestServletV2(hs).register(http_server) DeviceRestServlet(hs).register(http_server) diff --git a/synapse/rest/admin/users.py b/synapse/rest/admin/users.py index db678da4cf14..8de6f7fbb090 100644 --- a/synapse/rest/admin/users.py +++ b/synapse/rest/admin/users.py @@ -135,6 +135,8 @@ class UserRestServletV2(RestServlet): returns: 200 OK with user details if success otherwise an error. + THIS ENDPOINT IS DEPRECATED IN FAVOUR OF ITS V3 VARIANT. + Put request to allow an administrator to add or modify a user. This needs user to have administrator access in Synapse. We use PUT instead of POST since we already know the id of the user @@ -405,6 +407,38 @@ async def on_PUT( return 201, user +class UserRestServletV3(UserRestServletV2): + PATTERNS = admin_patterns("/users/(?P[^/]*)$", "v3") + + """Get request to list or update user details. + + This currently only differs from UserRestServletV2 in that it does not return + the 'password_hash' field for users. + """ + + async def on_GET( + self, request: SynapseRequest, user_id: str + ) -> Tuple[int, JsonDict]: + status_code, response_body = await super().on_GET(request, user_id) + + # Remove "password_hash" from the response in v3 of this API + if "password_hash" in response_body: + del response_body["password_hash"] + + return status_code, response_body + + async def on_PUT( + self, request: SynapseRequest, user_id: str + ) -> Tuple[int, JsonDict]: + status_code, response_body = await super().on_PUT(request, user_id) + + # Remove "password_hash" from the response in v3 of this API + if "password_hash" in response_body: + del response_body["password_hash"] + + return status_code, response_body + + class UserRegisterServlet(RestServlet): """ Attributes: From 7d17976544ba7cd5c82784d1d62314ebb7af3826 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 14 Dec 2021 17:07:32 +0000 Subject: [PATCH 02/13] Include the new v3 endpoint in /users/ tests As well as ensure that the v3 endpoint is removing the 'password_hash' field from responses. --- tests/rest/admin/test_user.py | 69 ++++++++++++++++++++++++++--------- 1 file changed, 51 insertions(+), 18 deletions(-) diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index eea675991cbc..e928b626a6e9 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -1154,6 +1154,13 @@ def _is_erased(self, user_id: str, expect: bool) -> None: self.assertFalse(self.get_success(d)) +# Test both v2 and v3 versions of this API +# TODO: Remove v2 once it is deprecated. Note that this may need to be +# changed if v3 of the endpoint diverges from v2 significantly +@parameterized_class( + ("endpoint_version",), + [("v2",), ("v3",)], +) class UserRestTestCase(unittest.HomeserverTestCase): servlets = [ @@ -1181,14 +1188,16 @@ def prepare(self, reactor, clock, hs): self.other_user, device_id=None, valid_until_ms=None ) ) - self.url_prefix = "/_synapse/admin/v2/users/%s" + + # self.endpoint_version is set by the @parameterized_class decorator above + self.url_prefix = f"/_synapse/admin/{self.endpoint_version}/users/%s" self.url_other_user = self.url_prefix % self.other_user def test_requester_is_no_admin(self): """ If the user is not a server admin, an error is returned. """ - url = "/_synapse/admin/v2/users/@bob:test" + url = self.url_prefix % "@bob:test" channel = self.make_request( "GET", @@ -1216,7 +1225,7 @@ def test_user_does_not_exist(self): channel = self.make_request( "GET", - "/_synapse/admin/v2/users/@unknown_person:test", + self.url_prefix % "@unknown_person:test", access_token=self.admin_user_tok, ) @@ -1337,7 +1346,7 @@ def test_create_server_admin(self): """ Check that a new admin user is created successfully. """ - url = "/_synapse/admin/v2/users/@bob:test" + url = self.url_prefix % "@bob:test" # Create user (server admin) body = { @@ -1386,7 +1395,7 @@ def test_create_user(self): """ Check that a new regular user is created successfully. """ - url = "/_synapse/admin/v2/users/@bob:test" + url = self.url_prefix % "@bob:test" # Create user body = { @@ -1478,7 +1487,7 @@ def test_create_user_mau_limit_reached_active_admin(self): ) # Register new user with admin API - url = "/_synapse/admin/v2/users/@bob:test" + url = self.url_prefix % "@bob:test" # Create user channel = self.make_request( @@ -1515,7 +1524,7 @@ def test_create_user_mau_limit_reached_passive_admin(self): ) # Register new user with admin API - url = "/_synapse/admin/v2/users/@bob:test" + url = self.url_prefix % "@bob:test" # Create user channel = self.make_request( @@ -1545,7 +1554,7 @@ def test_create_user_email_notif_for_new_users(self): Check that a new regular user is created successfully and got an email pusher. """ - url = "/_synapse/admin/v2/users/@bob:test" + url = self.url_prefix % "@bob:test" # Create user body = { @@ -1588,7 +1597,7 @@ def test_create_user_email_no_notif_for_new_users(self): Check that a new regular user is created successfully and got not an email pusher. """ - url = "/_synapse/admin/v2/users/@bob:test" + url = self.url_prefix % "@bob:test" # Create user body = { @@ -2085,10 +2094,14 @@ def test_deactivate_user(self): self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) self.assertEqual("@user:test", channel.json_body["name"]) self.assertTrue(channel.json_body["deactivated"]) - self.assertIsNone(channel.json_body["password_hash"]) self.assertEqual(0, len(channel.json_body["threepids"])) self.assertEqual("mxc://servername/mediaid", channel.json_body["avatar_url"]) self.assertEqual("User", channel.json_body["displayname"]) + + # This key is removed from v3 of the API + if self.endpoint_version == "v2": + self.assertIsNone(channel.json_body["password_hash"]) + # the user is deactivated, the threepid will be deleted # Get user @@ -2101,11 +2114,14 @@ def test_deactivate_user(self): self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) self.assertEqual("@user:test", channel.json_body["name"]) self.assertTrue(channel.json_body["deactivated"]) - self.assertIsNone(channel.json_body["password_hash"]) self.assertEqual(0, len(channel.json_body["threepids"])) self.assertEqual("mxc://servername/mediaid", channel.json_body["avatar_url"]) self.assertEqual("User", channel.json_body["displayname"]) + # Field 'password_hash' has been removed in v3 of this endpoint + if self.endpoint_version == "v2": + self.assertIsNone(channel.json_body["password_hash"]) + @override_config({"user_directory": {"enabled": True, "search_all_users": True}}) def test_change_name_deactivate_user_user_directory(self): """ @@ -2177,9 +2193,12 @@ def test_reactivate_user(self): self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) self.assertEqual("@user:test", channel.json_body["name"]) self.assertFalse(channel.json_body["deactivated"]) - self.assertIsNotNone(channel.json_body["password_hash"]) self._is_erased("@user:test", False) + # This key is removed from v3 of the API + if self.endpoint_version == "v2": + self.assertIsNotNone(channel.json_body["password_hash"]) + @override_config({"password_config": {"localdb_enabled": False}}) def test_reactivate_user_localdb_disabled(self): """ @@ -2209,9 +2228,12 @@ def test_reactivate_user_localdb_disabled(self): self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) self.assertEqual("@user:test", channel.json_body["name"]) self.assertFalse(channel.json_body["deactivated"]) - self.assertIsNone(channel.json_body["password_hash"]) self._is_erased("@user:test", False) + # This key is removed from v3 of the API + if self.endpoint_version == "v2": + self.assertIsNone(channel.json_body["password_hash"]) + @override_config({"password_config": {"enabled": False}}) def test_reactivate_user_password_disabled(self): """ @@ -2241,9 +2263,12 @@ def test_reactivate_user_password_disabled(self): self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) self.assertEqual("@user:test", channel.json_body["name"]) self.assertFalse(channel.json_body["deactivated"]) - self.assertIsNone(channel.json_body["password_hash"]) self._is_erased("@user:test", False) + # This key is removed from v3 of the API + if self.endpoint_version == "v2": + self.assertIsNone(channel.json_body["password_hash"]) + def test_set_user_as_admin(self): """ Test setting the admin flag on a user. @@ -2328,7 +2353,7 @@ def test_accidental_deactivation_prevention(self): Ensure an account can't accidentally be deactivated by using a str value for the deactivated body parameter """ - url = "/_synapse/admin/v2/users/@bob:test" + url = self.url_prefix % "@bob:test" # Create user channel = self.make_request( @@ -2392,18 +2417,21 @@ def _deactivate_user(self, user_id: str) -> None: # Deactivate the user. channel = self.make_request( "PUT", - "/_synapse/admin/v2/users/%s" % urllib.parse.quote(user_id), + self.url_prefix % urllib.parse.quote(user_id), access_token=self.admin_user_tok, content={"deactivated": True}, ) self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) self.assertTrue(channel.json_body["deactivated"]) - self.assertIsNone(channel.json_body["password_hash"]) self._is_erased(user_id, False) d = self.store.mark_user_erased(user_id) self.assertIsNone(self.get_success(d)) self._is_erased(user_id, True) + # This key is removed from v3 of the API + if self.endpoint_version == "v2": + self.assertIsNone(channel.json_body["password_hash"]) + def _check_fields(self, content: JsonDict): """Checks that the expected user attributes are present in content @@ -2416,13 +2444,18 @@ def _check_fields(self, content: JsonDict): self.assertIn("admin", content) self.assertIn("deactivated", content) self.assertIn("shadow_banned", content) - self.assertIn("password_hash", content) self.assertIn("creation_ts", content) self.assertIn("appservice_id", content) self.assertIn("consent_server_notice_sent", content) self.assertIn("consent_version", content) self.assertIn("external_ids", content) + # This key is removed from v3 of the API + if self.endpoint_version == "v2": + self.assertIn("password_hash", content) + else: + self.assertNotIn("password_hash", content) + class UserMembershipRestTestCase(unittest.HomeserverTestCase): From ebe5c79a17f85af4c6cd39eb458f5f309ea4e90a Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 14 Dec 2021 17:31:40 +0000 Subject: [PATCH 03/13] Changelog --- changelog.d/11576.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/11576.feature diff --git a/changelog.d/11576.feature b/changelog.d/11576.feature new file mode 100644 index 000000000000..e57a60bb9c5f --- /dev/null +++ b/changelog.d/11576.feature @@ -0,0 +1 @@ +Add a v3 of the Users Admin API which removes `"password_hash"` from response dictionaries. Deprecates the v2 version of the API. \ No newline at end of file From 87c4fc2c89654d7bd047b0841f3543a4fe6b7243 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 14 Jan 2022 12:17:32 +0000 Subject: [PATCH 04/13] Implement fix via a whitelist, rather than a blacklist This should prevent future internal keys leaking out externally without explicit intention. --- synapse/handlers/admin.py | 52 ++++++++++++++++++++++++++----------- synapse/rest/admin/users.py | 17 +++++------- 2 files changed, 43 insertions(+), 26 deletions(-) diff --git a/synapse/handlers/admin.py b/synapse/handlers/admin.py index 85157a138b71..24d3f6dd0738 100644 --- a/synapse/handlers/admin.py +++ b/synapse/handlers/admin.py @@ -55,21 +55,43 @@ async def get_whois(self, user: UserID) -> JsonDict: async def get_user(self, user: UserID) -> Optional[JsonDict]: """Function to get user details""" - ret = await self.store.get_user_by_id(user.to_string()) - if ret: - profile = await self.store.get_profileinfo(user.localpart) - threepids = await self.store.user_get_threepids(user.to_string()) - external_ids = [ - ({"auth_provider": auth_provider, "external_id": external_id}) - for auth_provider, external_id in await self.store.get_external_ids_by_user( - user.to_string() - ) - ] - ret["displayname"] = profile.display_name - ret["avatar_url"] = profile.avatar_url - ret["threepids"] = threepids - ret["external_ids"] = external_ids - return ret + user_info_dict = await self.store.get_user_by_id(user.to_string()) + if user_info_dict is None: + return None + + # Restrict returned information to a known set of fields. This prevents additional + # fields added to get_user_by_id from modifying Synapse's external API surface. + user_info_to_return = [ + "admin", + "deactivated", + "shadow_banned", + "creation_ts", + "appservice_id", + "consent_server_notice_sent", + "consent_version", + "user_type", + ] + + # Restrict returned keys to a known set. + user_info_dict = { + key: value for key, value in user_info_dict if key in user_info_to_return + } + + # Add additional user metadata + profile = await self.store.get_profileinfo(user.localpart) + threepids = await self.store.user_get_threepids(user.to_string()) + external_ids = [ + ({"auth_provider": auth_provider, "external_id": external_id}) + for auth_provider, external_id in await self.store.get_external_ids_by_user( + user.to_string() + ) + ] + user_info_dict["displayname"] = profile.display_name + user_info_dict["avatar_url"] = profile.avatar_url + user_info_dict["threepids"] = threepids + user_info_dict["external_ids"] = external_ids + + return user_info_dict async def export_user_data(self, user_id: str, writer: "ExfiltrationWriter") -> Any: """Write all data we have on the user to the given writer. diff --git a/synapse/rest/admin/users.py b/synapse/rest/admin/users.py index 8de6f7fbb090..7920ff505710 100644 --- a/synapse/rest/admin/users.py +++ b/synapse/rest/admin/users.py @@ -175,12 +175,11 @@ async def on_GET( if not self.hs.is_mine(target_user): raise SynapseError(HTTPStatus.BAD_REQUEST, "Can only look up local users") - ret = await self.admin_handler.get_user(target_user) - - if not ret: + user_info_dict = await self.admin_handler.get_user(target_user) + if not user_info_dict: raise NotFoundError("User not found") - return HTTPStatus.OK, ret + return HTTPStatus.OK, user_info_dict async def on_PUT( self, request: SynapseRequest, user_id: str @@ -401,10 +400,10 @@ async def on_PUT( target_user, requester, body["avatar_url"], True ) - user = await self.admin_handler.get_user(target_user) - assert user is not None + user_info_dict = await self.admin_handler.get_user(target_user) + assert user_info_dict is not None - return 201, user + return 201, user_info_dict class UserRestServletV3(UserRestServletV2): @@ -421,10 +420,6 @@ async def on_GET( ) -> Tuple[int, JsonDict]: status_code, response_body = await super().on_GET(request, user_id) - # Remove "password_hash" from the response in v3 of this API - if "password_hash" in response_body: - del response_body["password_hash"] - return status_code, response_body async def on_PUT( From 59a686e2368b552d25e0f7fdd941d761c6c09e6f Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 14 Jan 2022 12:18:51 +0000 Subject: [PATCH 05/13] Remove UserRestServletV3. --- synapse/rest/admin/__init__.py | 2 -- synapse/rest/admin/users.py | 30 ------------------------------ 2 files changed, 32 deletions(-) diff --git a/synapse/rest/admin/__init__.py b/synapse/rest/admin/__init__.py index c3f73a1bd822..701c609c1208 100644 --- a/synapse/rest/admin/__init__.py +++ b/synapse/rest/admin/__init__.py @@ -80,7 +80,6 @@ UserMembershipRestServlet, UserRegisterServlet, UserRestServletV2, - UserRestServletV3, UsersRestServletV2, UserTokenRestServlet, WhoisRestServlet, @@ -248,7 +247,6 @@ def register_servlets(hs: "HomeServer", http_server: HttpServer) -> None: UserAdminServlet(hs).register(http_server) UserMembershipRestServlet(hs).register(http_server) UserTokenRestServlet(hs).register(http_server) - UserRestServletV3(hs).register(http_server) UserRestServletV2(hs).register(http_server) UsersRestServletV2(hs).register(http_server) DeviceRestServlet(hs).register(http_server) diff --git a/synapse/rest/admin/users.py b/synapse/rest/admin/users.py index 7920ff505710..aea5f9c51470 100644 --- a/synapse/rest/admin/users.py +++ b/synapse/rest/admin/users.py @@ -135,8 +135,6 @@ class UserRestServletV2(RestServlet): returns: 200 OK with user details if success otherwise an error. - THIS ENDPOINT IS DEPRECATED IN FAVOUR OF ITS V3 VARIANT. - Put request to allow an administrator to add or modify a user. This needs user to have administrator access in Synapse. We use PUT instead of POST since we already know the id of the user @@ -406,34 +404,6 @@ async def on_PUT( return 201, user_info_dict -class UserRestServletV3(UserRestServletV2): - PATTERNS = admin_patterns("/users/(?P[^/]*)$", "v3") - - """Get request to list or update user details. - - This currently only differs from UserRestServletV2 in that it does not return - the 'password_hash' field for users. - """ - - async def on_GET( - self, request: SynapseRequest, user_id: str - ) -> Tuple[int, JsonDict]: - status_code, response_body = await super().on_GET(request, user_id) - - return status_code, response_body - - async def on_PUT( - self, request: SynapseRequest, user_id: str - ) -> Tuple[int, JsonDict]: - status_code, response_body = await super().on_PUT(request, user_id) - - # Remove "password_hash" from the response in v3 of this API - if "password_hash" in response_body: - del response_body["password_hash"] - - return status_code, response_body - - class UserRegisterServlet(RestServlet): """ Attributes: From c1b646d1251d55d24b0be7bbc12e73b91b8ee428 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 14 Jan 2022 12:22:00 +0000 Subject: [PATCH 06/13] Remove v3 API from tests. Remove mentions of password_hash While this could potentially let 'password_hash' slip through again, it --- docs/admin_api/user_admin_api.md | 1 - tests/rest/admin/test_user.py | 40 +------------------------------- 2 files changed, 1 insertion(+), 40 deletions(-) diff --git a/docs/admin_api/user_admin_api.md b/docs/admin_api/user_admin_api.md index ba574d795fcc..e7c215a1a4ff 100644 --- a/docs/admin_api/user_admin_api.md +++ b/docs/admin_api/user_admin_api.md @@ -36,7 +36,6 @@ It returns a JSON body like the following: "admin": 0, "deactivated": 0, "shadow_banned": 0, - "password_hash": "$2b$12$p9B4GkqYdRTPGD", "creation_ts": 1560432506, "appservice_id": null, "consent_server_notice_sent": null, diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index e928b626a6e9..a27c40442eb0 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -1154,13 +1154,6 @@ def _is_erased(self, user_id: str, expect: bool) -> None: self.assertFalse(self.get_success(d)) -# Test both v2 and v3 versions of this API -# TODO: Remove v2 once it is deprecated. Note that this may need to be -# changed if v3 of the endpoint diverges from v2 significantly -@parameterized_class( - ("endpoint_version",), - [("v2",), ("v3",)], -) class UserRestTestCase(unittest.HomeserverTestCase): servlets = [ @@ -1189,8 +1182,7 @@ def prepare(self, reactor, clock, hs): ) ) - # self.endpoint_version is set by the @parameterized_class decorator above - self.url_prefix = f"/_synapse/admin/{self.endpoint_version}/users/%s" + self.url_prefix = "/_synapse/admin/v2/users/%s" self.url_other_user = self.url_prefix % self.other_user def test_requester_is_no_admin(self): @@ -2098,10 +2090,6 @@ def test_deactivate_user(self): self.assertEqual("mxc://servername/mediaid", channel.json_body["avatar_url"]) self.assertEqual("User", channel.json_body["displayname"]) - # This key is removed from v3 of the API - if self.endpoint_version == "v2": - self.assertIsNone(channel.json_body["password_hash"]) - # the user is deactivated, the threepid will be deleted # Get user @@ -2118,10 +2106,6 @@ def test_deactivate_user(self): self.assertEqual("mxc://servername/mediaid", channel.json_body["avatar_url"]) self.assertEqual("User", channel.json_body["displayname"]) - # Field 'password_hash' has been removed in v3 of this endpoint - if self.endpoint_version == "v2": - self.assertIsNone(channel.json_body["password_hash"]) - @override_config({"user_directory": {"enabled": True, "search_all_users": True}}) def test_change_name_deactivate_user_user_directory(self): """ @@ -2195,10 +2179,6 @@ def test_reactivate_user(self): self.assertFalse(channel.json_body["deactivated"]) self._is_erased("@user:test", False) - # This key is removed from v3 of the API - if self.endpoint_version == "v2": - self.assertIsNotNone(channel.json_body["password_hash"]) - @override_config({"password_config": {"localdb_enabled": False}}) def test_reactivate_user_localdb_disabled(self): """ @@ -2230,10 +2210,6 @@ def test_reactivate_user_localdb_disabled(self): self.assertFalse(channel.json_body["deactivated"]) self._is_erased("@user:test", False) - # This key is removed from v3 of the API - if self.endpoint_version == "v2": - self.assertIsNone(channel.json_body["password_hash"]) - @override_config({"password_config": {"enabled": False}}) def test_reactivate_user_password_disabled(self): """ @@ -2265,10 +2241,6 @@ def test_reactivate_user_password_disabled(self): self.assertFalse(channel.json_body["deactivated"]) self._is_erased("@user:test", False) - # This key is removed from v3 of the API - if self.endpoint_version == "v2": - self.assertIsNone(channel.json_body["password_hash"]) - def test_set_user_as_admin(self): """ Test setting the admin flag on a user. @@ -2428,10 +2400,6 @@ def _deactivate_user(self, user_id: str) -> None: self.assertIsNone(self.get_success(d)) self._is_erased(user_id, True) - # This key is removed from v3 of the API - if self.endpoint_version == "v2": - self.assertIsNone(channel.json_body["password_hash"]) - def _check_fields(self, content: JsonDict): """Checks that the expected user attributes are present in content @@ -2450,12 +2418,6 @@ def _check_fields(self, content: JsonDict): self.assertIn("consent_version", content) self.assertIn("external_ids", content) - # This key is removed from v3 of the API - if self.endpoint_version == "v2": - self.assertIn("password_hash", content) - else: - self.assertNotIn("password_hash", content) - class UserMembershipRestTestCase(unittest.HomeserverTestCase): From e6e12a97645423b58cfce73d86aebfa6cb032d78 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 14 Jan 2022 12:31:27 +0000 Subject: [PATCH 07/13] Modify tests to only test against V2 again; check for no password_hash I considered simply dropping all mention of 'password_hash' from the tests, but I think it's better to keep it - at least for now - to ensure that extra keys don't accidentally sneak in again. --- tests/rest/admin/test_user.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index a27c40442eb0..34f87e4d962c 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -2090,6 +2090,9 @@ def test_deactivate_user(self): self.assertEqual("mxc://servername/mediaid", channel.json_body["avatar_url"]) self.assertEqual("User", channel.json_body["displayname"]) + # This key was removed intentionally. Ensure it is not accidentally re-included. + self.assertNotIn("password_hash", channel.json_body) + # the user is deactivated, the threepid will be deleted # Get user @@ -2106,6 +2109,9 @@ def test_deactivate_user(self): self.assertEqual("mxc://servername/mediaid", channel.json_body["avatar_url"]) self.assertEqual("User", channel.json_body["displayname"]) + # This key was removed intentionally. Ensure it is not accidentally re-included. + self.assertNotIn("password_hash", channel.json_body) + @override_config({"user_directory": {"enabled": True, "search_all_users": True}}) def test_change_name_deactivate_user_user_directory(self): """ @@ -2179,6 +2185,9 @@ def test_reactivate_user(self): self.assertFalse(channel.json_body["deactivated"]) self._is_erased("@user:test", False) + # This key was removed intentionally. Ensure it is not accidentally re-included. + self.assertNotIn("password_hash", channel.json_body) + @override_config({"password_config": {"localdb_enabled": False}}) def test_reactivate_user_localdb_disabled(self): """ @@ -2210,6 +2219,9 @@ def test_reactivate_user_localdb_disabled(self): self.assertFalse(channel.json_body["deactivated"]) self._is_erased("@user:test", False) + # This key was removed intentionally. Ensure it is not accidentally re-included. + self.assertNotIn("password_hash", channel.json_body) + @override_config({"password_config": {"enabled": False}}) def test_reactivate_user_password_disabled(self): """ @@ -2241,6 +2253,9 @@ def test_reactivate_user_password_disabled(self): self.assertFalse(channel.json_body["deactivated"]) self._is_erased("@user:test", False) + # This key was removed intentionally. Ensure it is not accidentally re-included. + self.assertNotIn("password_hash", channel.json_body) + def test_set_user_as_admin(self): """ Test setting the admin flag on a user. @@ -2400,6 +2415,9 @@ def _deactivate_user(self, user_id: str) -> None: self.assertIsNone(self.get_success(d)) self._is_erased(user_id, True) + # This key was removed intentionally. Ensure it is not accidentally re-included. + self.assertNotIn("password_hash", channel.json_body) + def _check_fields(self, content: JsonDict): """Checks that the expected user attributes are present in content @@ -2418,6 +2436,9 @@ def _check_fields(self, content: JsonDict): self.assertIn("consent_version", content) self.assertIn("external_ids", content) + # This key was removed intentionally. Ensure it is not accidentally re-included. + self.assertNotIn("password_hash", content) + class UserMembershipRestTestCase(unittest.HomeserverTestCase): From 83cf45322a12dd72145d539d3d39dae77029cd30 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 14 Jan 2022 13:40:57 +0000 Subject: [PATCH 08/13] Add some extra keys that were previously returned, but missing from the docs --- synapse/handlers/admin.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/admin.py b/synapse/handlers/admin.py index 24d3f6dd0738..8811ddef7cf3 100644 --- a/synapse/handlers/admin.py +++ b/synapse/handlers/admin.py @@ -62,6 +62,7 @@ async def get_user(self, user: UserID) -> Optional[JsonDict]: # Restrict returned information to a known set of fields. This prevents additional # fields added to get_user_by_id from modifying Synapse's external API surface. user_info_to_return = [ + "name", "admin", "deactivated", "shadow_banned", @@ -70,11 +71,14 @@ async def get_user(self, user: UserID) -> Optional[JsonDict]: "consent_server_notice_sent", "consent_version", "user_type", + "is_guest", ] # Restrict returned keys to a known set. user_info_dict = { - key: value for key, value in user_info_dict if key in user_info_to_return + key: value + for key, value in user_info_dict.items() + if key in user_info_to_return } # Add additional user metadata From 1f0b87d40640bfd8be4504214e2ae2fb80080288 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 14 Jan 2022 13:44:57 +0000 Subject: [PATCH 09/13] Add missing, previously-returned fields to /users Admin API docs --- docs/admin_api/user_admin_api.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/admin_api/user_admin_api.md b/docs/admin_api/user_admin_api.md index e7c215a1a4ff..ac2c2d6fb5b4 100644 --- a/docs/admin_api/user_admin_api.md +++ b/docs/admin_api/user_admin_api.md @@ -17,6 +17,7 @@ It returns a JSON body like the following: ```json { + "name": "@user:example.com", "displayname": "User", "threepids": [ { @@ -33,6 +34,7 @@ It returns a JSON body like the following: } ], "avatar_url": "", + "is_guest": 0, "admin": 0, "deactivated": 0, "shadow_banned": 0, From 06bef28d9b2fb86c25b270f461d8464ba12f2c26 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 14 Jan 2022 13:49:11 +0000 Subject: [PATCH 10/13] Update changelog --- changelog.d/11576.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/11576.feature b/changelog.d/11576.feature index e57a60bb9c5f..5be836ae022d 100644 --- a/changelog.d/11576.feature +++ b/changelog.d/11576.feature @@ -1 +1 @@ -Add a v3 of the Users Admin API which removes `"password_hash"` from response dictionaries. Deprecates the v2 version of the API. \ No newline at end of file +Remove the `"password_hash"` field from the response dictionaries of the [Users Admin API](https://matrix-org.github.io/synapse/latest/admin_api/user_admin_api.html). \ No newline at end of file From dab4b5ddd04d6fca6994cb4fa3e08bf84a51ebd8 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 14 Jan 2022 14:20:56 +0000 Subject: [PATCH 11/13] Use a set for user_info_to_return --- synapse/handlers/admin.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/admin.py b/synapse/handlers/admin.py index 8811ddef7cf3..00ab5e79bf2e 100644 --- a/synapse/handlers/admin.py +++ b/synapse/handlers/admin.py @@ -61,7 +61,7 @@ async def get_user(self, user: UserID) -> Optional[JsonDict]: # Restrict returned information to a known set of fields. This prevents additional # fields added to get_user_by_id from modifying Synapse's external API surface. - user_info_to_return = [ + user_info_to_return = { "name", "admin", "deactivated", @@ -72,7 +72,7 @@ async def get_user(self, user: UserID) -> Optional[JsonDict]: "consent_version", "user_type", "is_guest", - ] + } # Restrict returned keys to a known set. user_info_dict = { From 4f71ccc5f3520aaac81f5a2e124a10f3e6c39865 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 14 Jan 2022 14:22:29 +0000 Subject: [PATCH 12/13] 201 -> HTTPStatus.CREATED --- synapse/rest/admin/users.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/rest/admin/users.py b/synapse/rest/admin/users.py index aea5f9c51470..7da3c23320a4 100644 --- a/synapse/rest/admin/users.py +++ b/synapse/rest/admin/users.py @@ -401,7 +401,7 @@ async def on_PUT( user_info_dict = await self.admin_handler.get_user(target_user) assert user_info_dict is not None - return 201, user_info_dict + return HTTPStatus.CREATED, user_info_dict class UserRegisterServlet(RestServlet): From dfbec5740325e65d4637e633dc7a19c47a508fb1 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 14 Jan 2022 14:26:40 +0000 Subject: [PATCH 13/13] Note that displayname and avatar_url fields can be null --- docs/admin_api/user_admin_api.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/admin_api/user_admin_api.md b/docs/admin_api/user_admin_api.md index ac2c2d6fb5b4..08eeef2c5b35 100644 --- a/docs/admin_api/user_admin_api.md +++ b/docs/admin_api/user_admin_api.md @@ -15,10 +15,10 @@ server admin: [Admin API](../usage/administration/admin_api) It returns a JSON body like the following: -```json +```jsonc { "name": "@user:example.com", - "displayname": "User", + "displayname": "User", // can be null if not set "threepids": [ { "medium": "email", @@ -33,7 +33,7 @@ It returns a JSON body like the following: "validated_at": 1586458409743 } ], - "avatar_url": "", + "avatar_url": "", // can be null if not set "is_guest": 0, "admin": 0, "deactivated": 0,