Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Remove the 'password_hash' from the Users Admin API endpoint response dictionary #11576

Merged
merged 13 commits into from
Jan 14, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/11576.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
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).
9 changes: 5 additions & 4 deletions docs/admin_api/user_admin_api.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,10 @@ server admin: [Admin API](../usage/administration/admin_api)

It returns a JSON body like the following:

```json
```jsonc
{
"displayname": "User",
"name": "@user:example.com",
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved
"displayname": "User", // can be null if not set
"threepids": [
{
"medium": "email",
Expand All @@ -32,11 +33,11 @@ It returns a JSON body like the following:
"validated_at": 1586458409743
}
],
"avatar_url": "<avatar_url>",
"avatar_url": "<avatar_url>", // can be null if not set
"is_guest": 0,
"admin": 0,
"deactivated": 0,
"shadow_banned": 0,
"password_hash": "$2b$12$p9B4GkqYdRTPGD",
"creation_ts": 1560432506,
"appservice_id": null,
"consent_server_notice_sent": null,
Expand Down
56 changes: 41 additions & 15 deletions synapse/handlers/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,21 +55,47 @@ 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 = {
"name",
"admin",
"deactivated",
"shadow_banned",
"creation_ts",
"appservice_id",
"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.items()
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
anoadragon453 marked this conversation as resolved.
Show resolved Hide resolved
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.
Expand Down
13 changes: 6 additions & 7 deletions synapse/rest/admin/users.py
Original file line number Diff line number Diff line change
Expand Up @@ -173,12 +173,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
Expand Down Expand Up @@ -399,10 +398,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 HTTPStatus.CREATED, user_info_dict


class UserRegisterServlet(RestServlet):
Expand Down
50 changes: 33 additions & 17 deletions tests/rest/admin/test_user.py
Original file line number Diff line number Diff line change
Expand Up @@ -1181,14 +1181,15 @@ 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.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",
Expand Down Expand Up @@ -1216,7 +1217,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,
)

Expand Down Expand Up @@ -1337,7 +1338,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 = {
Expand Down Expand Up @@ -1386,7 +1387,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 = {
Expand Down Expand Up @@ -1478,7 +1479,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(
Expand Down Expand Up @@ -1515,7 +1516,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(
Expand Down Expand Up @@ -1545,7 +1546,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 = {
Expand Down Expand Up @@ -1588,7 +1589,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 = {
Expand Down Expand Up @@ -2085,10 +2086,13 @@ 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 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
Expand All @@ -2101,11 +2105,13 @@ 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 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):
"""
Expand Down Expand Up @@ -2177,9 +2183,11 @@ 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 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):
"""
Expand Down Expand Up @@ -2209,9 +2217,11 @@ 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 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):
"""
Expand Down Expand Up @@ -2241,9 +2251,11 @@ 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 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.
Expand Down Expand Up @@ -2328,7 +2340,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(
Expand Down Expand Up @@ -2392,18 +2404,20 @@ 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 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

Expand All @@ -2416,13 +2430,15 @@ 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 was removed intentionally. Ensure it is not accidentally re-included.
self.assertNotIn("password_hash", content)


class UserMembershipRestTestCase(unittest.HomeserverTestCase):

Expand Down