Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for MSC3823 - Account Suspension Part 2 #17255

Merged
merged 9 commits into from
Jun 24, 2024
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/17255.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add support for [MSC823](https://github.com/matrix-org/matrix-spec-proposals/pull/3823) - Account suspension.
4 changes: 4 additions & 0 deletions synapse/config/experimental.py
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,10 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:
("experimental", "msc4108_delegation_endpoint"),
)

self.msc3823_account_suspension = experimental.get(
"msc3823_account_suspension", False
)

self.msc3916_authenticated_media_enabled = experimental.get(
"msc3916_authenticated_media_enabled", False
)
Expand Down
11 changes: 11 additions & 0 deletions synapse/handlers/message.py
Original file line number Diff line number Diff line change
Expand Up @@ -642,6 +642,17 @@ async def create_event(
"""
await self.auth_blocking.check_auth_blocking(requester=requester)

if event_dict["type"] == EventTypes.Message:
reivilibre marked this conversation as resolved.
Show resolved Hide resolved
requester_suspended = await self.store.get_user_suspended_status(
requester.user.to_string()
)
if requester_suspended:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it make sense to only apply suspensions if the user isn't being puppeted? (I think there are things on requester that would help to check for that. Maybe a helper would be useful to ensure we're consistent about it everywhere)

Just thinking that server admins may still wish to e.g. change the displayname of a suspended user

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know much about puppeted accounts - Can a user access an account that is being puppeted?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'Puppeted' here only means that a server admin logged in to the user using the Admin API. https://element-hq.github.io/synapse/latest/admin_api/user_admin_api.html?highlight=login%20as%20user#login-as-a-user

Whilst it's the puppeted user that is being made to send the events and stuff, it's a trusted admin who is actually pulling the strings. So it seems logical that there'd be no need to apply any suspensions there?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Requester.authenticated_entity is what you want to read to see if a user is being puppeted or not. There should be quite a bit of example code around if it's not clear from the documentation on that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right I checked in with the team during our sync today and posed the question about puppeting - the consensus is we'd like to merge it as is, and possibly follow up with that change in the future depending on the feedback from the original work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that can probably wait until later, it shouldn't break anyone since you have to choose to use the experimental feature first.

raise SynapseError(
403,
"Sending messages while account is suspended is not allowed.",
Codes.USER_ACCOUNT_SUSPENDED,
)

if event_dict["type"] == EventTypes.Create and event_dict["state_key"] == "":
room_version_id = event_dict["content"]["room_version"]
maybe_room_version_obj = KNOWN_ROOM_VERSIONS.get(room_version_id)
Expand Down
3 changes: 3 additions & 0 deletions synapse/rest/admin/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@
ResetPasswordRestServlet,
SearchUsersRestServlet,
ShadowBanRestServlet,
SuspendAccountRestServlet,
UserAdminServlet,
UserByExternalId,
UserByThreePid,
Expand Down Expand Up @@ -327,6 +328,8 @@ def register_servlets(hs: "HomeServer", http_server: HttpServer) -> None:
BackgroundUpdateRestServlet(hs).register(http_server)
BackgroundUpdateStartJobRestServlet(hs).register(http_server)
ExperimentalFeaturesRestServlet(hs).register(http_server)
if hs.config.experimental.msc3823_account_suspension:
SuspendAccountRestServlet(hs).register(http_server)


def register_servlets_for_client_rest_resource(
Expand Down
39 changes: 39 additions & 0 deletions synapse/rest/admin/users.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,13 @@

import attr

from synapse._pydantic_compat import HAS_PYDANTIC_V2
from synapse.api.constants import Direction, UserTypes
from synapse.api.errors import Codes, NotFoundError, SynapseError
from synapse.http.servlet import (
RestServlet,
assert_params_in_dict,
parse_and_validate_json_object_from_request,
parse_boolean,
parse_enum,
parse_integer,
Expand All @@ -49,10 +51,17 @@
from synapse.storage.databases.main.registration import ExternalIDReuseException
from synapse.storage.databases.main.stats import UserSortOrder
from synapse.types import JsonDict, JsonMapping, UserID
from synapse.types.rest import RequestBodyModel

if TYPE_CHECKING:
from synapse.server import HomeServer

if TYPE_CHECKING or HAS_PYDANTIC_V2:
from pydantic.v1 import StrictBool
else:
from pydantic import StrictBool


logger = logging.getLogger(__name__)


Expand Down Expand Up @@ -732,6 +741,36 @@ async def on_POST(
return HTTPStatus.OK, {"id_server_unbind_result": id_server_unbind_result}


class SuspendAccountRestServlet(RestServlet):
PATTERNS = admin_patterns("/suspend/(?P<target_user_id>[^/]*)$")
H-Shay marked this conversation as resolved.
Show resolved Hide resolved

def __init__(self, hs: "HomeServer"):
self.auth = hs.get_auth()
self.is_mine = hs.is_mine
self.store = hs.get_datastores().main

class PutBody(RequestBodyModel):
suspend: StrictBool

async def on_PUT(
self, request: SynapseRequest, target_user_id: str
) -> Tuple[int, JsonDict]:
requester = await self.auth.get_user_by_req(request)
await assert_user_is_admin(self.auth, requester)

if not self.is_mine(UserID.from_string(target_user_id)):
raise SynapseError(HTTPStatus.BAD_REQUEST, "Can only suspend local users")

if not await self.store.get_user_by_id(target_user_id):
raise NotFoundError("User not found")

body = parse_and_validate_json_object_from_request(request, self.PutBody)
suspend = body.suspend
await self.store.set_user_suspended_status(target_user_id, suspend)

return HTTPStatus.OK, {f"user_{target_user_id}_suspended": suspend}


class AccountValidityRenewServlet(RestServlet):
PATTERNS = admin_patterns("/account_validity/validity$")

Expand Down
26 changes: 26 additions & 0 deletions synapse/rest/client/profile.py
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,19 @@ async def on_PUT(

propagate = _read_propagate(self.hs, request)

requester_suspended = (
await self.hs.get_datastores().main.get_user_suspended_status(
requester.user.to_string()
)
)

if requester_suspended:
raise SynapseError(
403,
"Updating displayname while account is suspended is not allowed.",
Codes.USER_ACCOUNT_SUSPENDED,
)
reivilibre marked this conversation as resolved.
Show resolved Hide resolved

await self.profile_handler.set_displayname(
user, requester, new_name, is_admin, propagate=propagate
)
Expand Down Expand Up @@ -167,6 +180,19 @@ async def on_PUT(

propagate = _read_propagate(self.hs, request)

requester_suspended = (
await self.hs.get_datastores().main.get_user_suspended_status(
requester.user.to_string()
)
)

if requester_suspended:
raise SynapseError(
403,
"Updating avatar URL while account is suspended is not allowed.",
Codes.USER_ACCOUNT_SUSPENDED,
)

await self.profile_handler.set_avatar_url(
user, requester, new_avatar_url, is_admin, propagate=propagate
)
Expand Down
14 changes: 14 additions & 0 deletions synapse/rest/client/room.py
Original file line number Diff line number Diff line change
Expand Up @@ -1120,6 +1120,20 @@ async def _do(
) -> Tuple[int, JsonDict]:
content = parse_json_object_from_request(request)

requester_suspended = await self._store.get_user_suspended_status(
requester.user.to_string()
)

if requester_suspended:
event = await self._store.get_event(event_id, allow_none=True)
if event:
if event.sender != requester.user.to_string():
raise SynapseError(
403,
"You can only redact your own events while account is suspended.",
Codes.USER_ACCOUNT_SUSPENDED,
)

# Ensure the redacts property in the content matches the one provided in
# the URL.
room_version = await self._store.get_room_version(room_id)
Expand Down
84 changes: 84 additions & 0 deletions tests/rest/admin/test_user.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
from synapse.api.errors import Codes, HttpResponseException, ResourceLimitError
from synapse.api.room_versions import RoomVersions
from synapse.media.filepath import MediaFilePaths
from synapse.rest import admin
from synapse.rest.client import (
devices,
login,
Expand Down Expand Up @@ -5005,3 +5006,86 @@ def test_success(self) -> None:
)
assert timestamp is not None
self.assertGreater(timestamp, self.clock.time_msec())


class UserSuspensionTestCase(unittest.HomeserverTestCase):
servlets = [
synapse.rest.admin.register_servlets,
login.register_servlets,
admin.register_servlets,
]

def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None:
self.admin = self.register_user("thomas", "hackme", True)
self.admin_tok = self.login("thomas", "hackme")

self.bad_user = self.register_user("teresa", "hackme")
self.bad_user_tok = self.login("teresa", "hackme")

self.store = hs.get_datastores().main

@override_config({"experimental_features": {"msc3823_account_suspension": True}})
def test_suspend_user(self) -> None:
# test that suspending user works
channel = self.make_request(
"PUT",
f"/_synapse/admin/v1/suspend/{self.bad_user}",
{"suspend": True},
access_token=self.admin_tok,
)
self.assertEqual(channel.code, 200)
self.assertEqual(channel.json_body, {f"user_{self.bad_user}_suspended": True})

res = self.get_success(self.store.get_user_suspended_status(self.bad_user))
self.assertEqual(True, res)

# test that un-suspending user works
channel2 = self.make_request(
"PUT",
f"/_synapse/admin/v1/suspend/{self.bad_user}",
{"suspend": False},
access_token=self.admin_tok,
)
self.assertEqual(channel2.code, 200)
self.assertEqual(channel2.json_body, {f"user_{self.bad_user}_suspended": False})

res2 = self.get_success(self.store.get_user_suspended_status(self.bad_user))
self.assertEqual(False, res2)

# test that trying to un-suspend user who isn't suspended doesn't cause problems
channel3 = self.make_request(
"PUT",
f"/_synapse/admin/v1/suspend/{self.bad_user}",
{"suspend": False},
access_token=self.admin_tok,
)
self.assertEqual(channel3.code, 200)
self.assertEqual(channel3.json_body, {f"user_{self.bad_user}_suspended": False})

res3 = self.get_success(self.store.get_user_suspended_status(self.bad_user))
self.assertEqual(False, res3)

# test that trying to suspend user who is already suspended doesn't cause problems
channel4 = self.make_request(
"PUT",
f"/_synapse/admin/v1/suspend/{self.bad_user}",
{"suspend": True},
access_token=self.admin_tok,
)
self.assertEqual(channel4.code, 200)
self.assertEqual(channel4.json_body, {f"user_{self.bad_user}_suspended": True})

res4 = self.get_success(self.store.get_user_suspended_status(self.bad_user))
self.assertEqual(True, res4)

channel5 = self.make_request(
"PUT",
f"/_synapse/admin/v1/suspend/{self.bad_user}",
{"suspend": True},
access_token=self.admin_tok,
)
self.assertEqual(channel5.code, 200)
self.assertEqual(channel5.json_body, {f"user_{self.bad_user}_suspended": True})

res5 = self.get_success(self.store.get_user_suspended_status(self.bad_user))
self.assertEqual(True, res5)
105 changes: 105 additions & 0 deletions tests/rest/client/test_rooms.py
Original file line number Diff line number Diff line change
Expand Up @@ -3819,3 +3819,108 @@ def test_no_outliers(self) -> None:

# Make sure the outlier event is not returned
self.assertNotEqual(channel.json_body["event_id"], outlier_event.event_id)


class UserSuspensionTests(unittest.HomeserverTestCase):
servlets = [
admin.register_servlets,
login.register_servlets,
room.register_servlets,
profile.register_servlets,
]

def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None:
self.user1 = self.register_user("thomas", "hackme")
self.tok1 = self.login("thomas", "hackme")

self.user2 = self.register_user("teresa", "hackme")
self.tok2 = self.login("teresa", "hackme")

self.room1 = self.helper.create_room_as(room_creator=self.user1, tok=self.tok1)
self.store = hs.get_datastores().main

def test_suspended_user_cannot_send_message_to_room(self) -> None:
# set the user as suspended
self.get_success(self.store.set_user_suspended_status(self.user1, True))

channel = self.make_request(
"PUT",
f"/rooms/{self.room1}/send/m.room.message/1",
access_token=self.tok1,
content={"body": "hello", "msgtype": "m.text"},
)
self.assertEqual(
channel.json_body["errcode"], "ORG.MATRIX.MSC3823.USER_ACCOUNT_SUSPENDED"
)

def test_suspended_user_cannot_change_profile_data(self) -> None:
# set the user as suspended
self.get_success(self.store.set_user_suspended_status(self.user1, True))

channel = self.make_request(
"PUT",
f"/_matrix/client/v3/profile/{self.user1}/avatar_url",
access_token=self.tok1,
content={"avatar_url": "mxc://matrix.org/wefh34uihSDRGhw34"},
shorthand=False,
)
self.assertEqual(
channel.json_body["errcode"], "ORG.MATRIX.MSC3823.USER_ACCOUNT_SUSPENDED"
)

channel2 = self.make_request(
"PUT",
f"/_matrix/client/v3/profile/{self.user1}/displayname",
access_token=self.tok1,
content={"displayname": "something offensive"},
shorthand=False,
)
self.assertEqual(
channel2.json_body["errcode"], "ORG.MATRIX.MSC3823.USER_ACCOUNT_SUSPENDED"
)

def test_suspended_user_cannot_redact_messages_other_than_their_own(self) -> None:
# first user sends message
self.make_request("POST", f"/rooms/{self.room1}/join", access_token=self.tok2)
res = self.helper.send_event(
self.room1,
"m.room.message",
{"body": "hello", "msgtype": "m.text"},
tok=self.tok2,
)
event_id = res["event_id"]

# second user sends message
self.make_request("POST", f"/rooms/{self.room1}/join", access_token=self.tok1)
res2 = self.helper.send_event(
self.room1,
"m.room.message",
{"body": "bad_message", "msgtype": "m.text"},
tok=self.tok1,
)
event_id2 = res2["event_id"]

# set the second user as suspended
self.get_success(self.store.set_user_suspended_status(self.user1, True))

# second user can't redact first user's message
channel = self.make_request(
"PUT",
f"/_matrix/client/v3/rooms/{self.room1}/redact/{event_id}/1",
access_token=self.tok1,
content={"reason": "bogus"},
shorthand=False,
)
self.assertEqual(
channel.json_body["errcode"], "ORG.MATRIX.MSC3823.USER_ACCOUNT_SUSPENDED"
)

# but can redact their own
channel = self.make_request(
"PUT",
f"/_matrix/client/v3/rooms/{self.room1}/redact/{event_id2}/1",
access_token=self.tok1,
content={"reason": "bogus"},
shorthand=False,
)
self.assertEqual(channel.code, 200)
Loading