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

Implement MSC3848: Introduce errcodes for specific event sending failures #13343

Merged
merged 19 commits into from
Jul 27, 2022
Merged
1 change: 1 addition & 0 deletions changelog.d/13343.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add new unstable error codes `ORG.MATRIX.MSC3848.ALREADY_JOINED`, `ORG.MATRIX.MSC3848.NOT_JOINED`, and `ORG.MATRIX.MSC3848.INSUFFICIENT_POWER` described in MSC3848.
11 changes: 8 additions & 3 deletions synapse/api/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
Codes,
InvalidClientTokenError,
MissingClientTokenError,
UnstableSpecAuthError,
)
from synapse.appservice import ApplicationService
from synapse.http import get_request_user_agent
Expand Down Expand Up @@ -106,8 +107,11 @@ async def check_user_in_room(
forgot = await self.store.did_forget(user_id, room_id)
if not forgot:
return membership, member_event_id

raise AuthError(403, "User %s not in room %s" % (user_id, room_id))
raise UnstableSpecAuthError(
403,
"User %s not in room %s" % (user_id, room_id),
errcode=Codes.NOT_JOINED,
)

async def get_user_by_req(
self,
Expand Down Expand Up @@ -600,8 +604,9 @@ async def check_user_in_room_or_world_readable(
== HistoryVisibility.WORLD_READABLE
):
return Membership.JOIN, None
raise AuthError(
raise UnstableSpecAuthError(
403,
"User %s not in room %s, and room previews are disabled"
% (user_id, room_id),
errcode=Codes.NOT_JOINED,
)
37 changes: 37 additions & 0 deletions synapse/api/errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,12 @@ class Codes(str, Enum):
INVALID_SIGNATURE = "M_INVALID_SIGNATURE"
USER_DEACTIVATED = "M_USER_DEACTIVATED"

# Part of MSC3848
# https://github.com/matrix-org/matrix-spec-proposals/pull/3848
ALREADY_JOINED = "ORG.MATRIX.MSC3848.ALREADY_JOINED"
NOT_JOINED = "ORG.MATRIX.MSC3848.NOT_JOINED"
INSUFFICIENT_POWER = "ORG.MATRIX.MSC3848.INSUFFICIENT_POWER"

# The account has been suspended on the server.
# By opposition to `USER_DEACTIVATED`, this is a reversible measure
# that can possibly be appealed and reverted.
Expand Down Expand Up @@ -307,6 +313,37 @@ def __init__(
super().__init__(code, msg, errcode, additional_fields)


class UnstableSpecAuthError(AuthError):
erikjohnston marked this conversation as resolved.
Show resolved Hide resolved
"""An error raised when a new error code is being proposed to replace a previous one.
This error will return a "org.matrix.unstable.errcode" property with the new error code,
with the previous error code still being defined in the "errcode" property.

This error will include `org.matrix.msc3848.unstable.errcode` in the C-S error body.
"""

def __init__(
self,
code: int,
msg: str,
errcode: str,
previous_errcode: str = Codes.FORBIDDEN,
additional_fields: Optional[dict] = None,
):
self.previous_errcode = previous_errcode
super().__init__(code, msg, errcode, additional_fields)

def error_dict(self, allow_unstable_fields: bool = False) -> "JsonDict":
fields = {}
if allow_unstable_fields:
fields["org.matrix.msc3848.unstable.errcode"] = self.errcode
return cs_error(
self.msg,
self.previous_errcode,
**fields,
**self._additional_fields,
)


class InvalidClientCredentialsError(SynapseError):
"""An error raised when there was a problem with the authorisation credentials
in a client request.
Expand Down
3 changes: 3 additions & 0 deletions synapse/config/experimental.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,3 +90,6 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:

# MSC3827: Filtering of /publicRooms by room type
self.msc3827_enabled: bool = experimental.get("msc3827_enabled", False)

# MSC3848: Introduce errcodes for specific event sending failures
self.msc3848_enabled: bool = experimental.get("msc3848_enabled", False)
62 changes: 51 additions & 11 deletions synapse/event_auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,13 @@
JoinRules,
Membership,
)
from synapse.api.errors import AuthError, EventSizeError, SynapseError
from synapse.api.errors import (
AuthError,
Codes,
EventSizeError,
SynapseError,
UnstableSpecAuthError,
)
from synapse.api.room_versions import (
KNOWN_ROOM_VERSIONS,
EventFormatVersions,
Expand Down Expand Up @@ -291,7 +297,11 @@ def check_state_dependent_auth_rules(
invite_level = get_named_level(auth_dict, "invite", 0)

if user_level < invite_level:
raise AuthError(403, "You don't have permission to invite users")
raise UnstableSpecAuthError(
403,
"You don't have permission to invite users",
errcode=Codes.INSUFFICIENT_POWER,
)
else:
logger.debug("Allowing! %s", event)
return
Expand Down Expand Up @@ -474,7 +484,11 @@ def _is_membership_change_allowed(
return

if not caller_in_room: # caller isn't joined
raise AuthError(403, "%s not in room %s." % (event.user_id, event.room_id))
raise UnstableSpecAuthError(
403,
"%s not in room %s." % (event.user_id, event.room_id),
errcode=Codes.NOT_JOINED,
)

if Membership.INVITE == membership:
# TODO (erikj): We should probably handle this more intelligently
Expand All @@ -484,10 +498,18 @@ def _is_membership_change_allowed(
if target_banned:
raise AuthError(403, "%s is banned from the room" % (target_user_id,))
elif target_in_room: # the target is already in the room.
raise AuthError(403, "%s is already in the room." % target_user_id)
raise UnstableSpecAuthError(
403,
"%s is already in the room." % target_user_id,
errcode=Codes.ALREADY_JOINED,
)
else:
if user_level < invite_level:
raise AuthError(403, "You don't have permission to invite users")
raise UnstableSpecAuthError(
403,
"You don't have permission to invite users",
errcode=Codes.INSUFFICIENT_POWER,
)
elif Membership.JOIN == membership:
# Joins are valid iff caller == target and:
# * They are not banned.
Expand Down Expand Up @@ -549,15 +571,27 @@ def _is_membership_change_allowed(
elif Membership.LEAVE == membership:
# TODO (erikj): Implement kicks.
if target_banned and user_level < ban_level:
raise AuthError(403, "You cannot unban user %s." % (target_user_id,))
raise UnstableSpecAuthError(
403,
"You cannot unban user %s." % (target_user_id,),
errcode=Codes.INSUFFICIENT_POWER,
)
elif target_user_id != event.user_id:
kick_level = get_named_level(auth_events, "kick", 50)

if user_level < kick_level or user_level <= target_level:
raise AuthError(403, "You cannot kick user %s." % target_user_id)
raise UnstableSpecAuthError(
403,
"You cannot kick user %s." % target_user_id,
errcode=Codes.INSUFFICIENT_POWER,
)
elif Membership.BAN == membership:
if user_level < ban_level or user_level <= target_level:
raise AuthError(403, "You don't have permission to ban")
raise UnstableSpecAuthError(
403,
"You don't have permission to ban",
errcode=Codes.INSUFFICIENT_POWER,
)
elif room_version.msc2403_knocking and Membership.KNOCK == membership:
if join_rule != JoinRules.KNOCK and (
not room_version.msc3787_knock_restricted_join_rule
Expand All @@ -567,7 +601,11 @@ def _is_membership_change_allowed(
elif target_user_id != event.user_id:
raise AuthError(403, "You cannot knock for other users")
elif target_in_room:
raise AuthError(403, "You cannot knock on a room you are already in")
raise UnstableSpecAuthError(
403,
"You cannot knock on a room you are already in",
errcode=Codes.ALREADY_JOINED,
)
elif caller_invited:
raise AuthError(403, "You are already invited to this room")
elif target_banned:
Expand Down Expand Up @@ -638,10 +676,11 @@ def _can_send_event(event: "EventBase", auth_events: StateMap["EventBase"]) -> b
user_level = get_user_power_level(event.user_id, auth_events)

if user_level < send_level:
raise AuthError(
raise UnstableSpecAuthError(
403,
"You don't have permission to post that to the room. "
+ "user_level (%d) < send_level (%d)" % (user_level, send_level),
errcode=Codes.INSUFFICIENT_POWER,
)

# Check state_key
Expand Down Expand Up @@ -716,9 +755,10 @@ def check_historical(
historical_level = get_named_level(auth_events, "historical", 100)

if user_level < historical_level:
raise AuthError(
raise UnstableSpecAuthError(
403,
'You don\'t have permission to send send historical related events ("insertion", "batch", and "marker")',
errcode=Codes.INSUFFICIENT_POWER,
)


Expand Down
8 changes: 7 additions & 1 deletion synapse/federation/federation_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
IncompatibleRoomVersionError,
NotFoundError,
SynapseError,
UnstableSpecAuthError,
UnsupportedRoomVersionError,
)
from synapse.api.room_versions import KNOWN_ROOM_VERSIONS, RoomVersion
Expand Down Expand Up @@ -469,7 +470,12 @@ async def process_pdus_for_room(room_id: str) -> None:
)
for pdu in pdus_by_room[room_id]:
event_id = pdu.event_id
pdu_results[event_id] = e.error_dict()
if isinstance(e, UnstableSpecAuthError):
Copy link
Member

Choose a reason for hiding this comment

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

Bleurgh, I'm not a fan of using isinstance TBH, its a bit of a smell. I'd rather we changed the signature of error_dict across all the errors to avoid this. At which point you probably do want to pass in either the config or HS object to make the error_dict API sane.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Bleurgh, I'm not a fan of using isinstance TBH, its a bit of a smell. I'd rather we changed the signature of error_dict across all the errors to avoid this.

Happy to 👍

At which point you probably do want to pass in either the config or HS object to make the error_dict API sane.

I'll try it with the config. I could see that coming up again. It might just be prior experience but I have a natural aversion to passing in chunky classes into smaller classes.

pdu_results[event_id] = e.error_dict(
self.hs.config.experimental.msc3848_enabled
)
else:
pdu_results[event_id] = e.error_dict()
return

for pdu in pdus_by_room[room_id]:
Expand Down
3 changes: 3 additions & 0 deletions synapse/handlers/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
LoginError,
StoreError,
SynapseError,
UnstableSpecAuthError,
UserDeactivatedError,
)
from synapse.api.ratelimiting import Ratelimiter
Expand Down Expand Up @@ -562,6 +563,8 @@ async def check_ui_auth(
await self.store.mark_ui_auth_stage_complete(
session.session_id, login_type, result
)
except UnstableSpecAuthError as e:
errordict = e.error_dict(self.hs.config.experimental.msc3848_enabled)
except LoginError as e:
# this step failed. Merge the error dict into the response
# so that the client can have another go.
Expand Down
13 changes: 11 additions & 2 deletions synapse/handlers/message.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
NotFoundError,
ShadowBanError,
SynapseError,
UnstableSpecAuthError,
UnsupportedRoomVersionError,
)
from synapse.api.room_versions import KNOWN_ROOM_VERSIONS
Expand Down Expand Up @@ -149,7 +150,11 @@ async def get_room_data(
"Attempted to retrieve data from a room for a user that has never been in it. "
"This should not have happened."
)
raise SynapseError(403, "User not in room", errcode=Codes.FORBIDDEN)
raise UnstableSpecAuthError(
403,
"User not in room",
errcode=Codes.NOT_JOINED,
)

return data

Expand Down Expand Up @@ -334,7 +339,11 @@ async def get_joined_members(self, requester: Requester, room_id: str) -> dict:
break
else:
# Loop fell through, AS has no interested users in room
raise AuthError(403, "Appservice not in room")
raise UnstableSpecAuthError(
403,
"Appservice not in room",
errcode=Codes.NOT_JOINED,
)

return {
user_id: {
Expand Down
5 changes: 3 additions & 2 deletions synapse/handlers/room_summary.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,11 @@
RoomTypes,
)
from synapse.api.errors import (
AuthError,
Codes,
NotFoundError,
StoreError,
SynapseError,
UnstableSpecAuthError,
UnsupportedRoomVersionError,
)
from synapse.api.ratelimiting import Ratelimiter
Expand Down Expand Up @@ -175,10 +175,11 @@ async def _get_room_hierarchy(

# First of all, check that the room is accessible.
if not await self._is_local_room_accessible(requested_room_id, requester):
raise AuthError(
raise UnstableSpecAuthError(
403,
"User %s not in room %s, and room previews are disabled"
% (requester, requested_room_id),
errcode=Codes.NOT_JOINED,
)

# If this is continuing a previous session, pull the persisted data.
Expand Down
22 changes: 19 additions & 3 deletions synapse/http/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
RedirectException,
SynapseError,
UnrecognizedRequestError,
UnstableSpecAuthError,
)
from synapse.http.site import SynapseRequest
from synapse.logging.context import defer_to_thread, preserve_fn, run_in_background
Expand Down Expand Up @@ -155,15 +156,22 @@ def is_method_cancellable(method: Callable[..., Any]) -> bool:
return getattr(method, "cancellable", False)


def return_json_error(f: failure.Failure, request: SynapseRequest) -> None:
def return_json_error(
f: failure.Failure, request: SynapseRequest, allow_unstable_fields: bool = False
) -> None:
"""Sends a JSON error response to clients."""

if f.check(SynapseError):
if f.check(UnstableSpecAuthError):
# mypy doesn't understand that f.check asserts the type.
exc: UnstableSpecAuthError = f.value # type: ignore
error_code = exc.code
error_dict = exc.error_dict(allow_unstable_fields)
logger.info("%s SynapseError: %s - %s", request, error_code, exc.msg)
elif f.check(SynapseError):
# mypy doesn't understand that f.check asserts the type.
exc: SynapseError = f.value # type: ignore
error_code = exc.code
error_dict = exc.error_dict()

logger.info("%s SynapseError: %s - %s", request, error_code, exc.msg)
elif f.check(CancelledError):
error_code = HTTP_STATUS_REQUEST_CANCELLED
Expand Down Expand Up @@ -575,6 +583,14 @@ async def _async_render(self, request: SynapseRequest) -> Tuple[int, Any]:

return callback_return

def _send_error_response(
self,
f: failure.Failure,
request: SynapseRequest,
) -> None:
"""Implements _AsyncResource._send_error_response"""
return_json_error(f, request, self.hs.config.experimental.msc3848_enabled)


class DirectServeHtmlResource(_AsyncResource):
"""A resource that will call `self._async_on_<METHOD>` on new requests,
Expand Down