Skip to content
Open
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/18475.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Make ACLs apply to EDUs per [MSC4163](https://github.com/matrix-org/matrix-spec-proposals/pull/4163).
47 changes: 47 additions & 0 deletions synapse/federation/federation_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -551,6 +551,53 @@ async def _process_edu(edu_dict: JsonDict) -> None:
edu_type=edu_dict["edu_type"],
content=edu_dict["content"],
)

# Server ACL's apply to `EduTypes.TYPING` per MSC4163:
#
# > For typing notifications (m.typing), the room_id field inside
# > content should be checked, with the typing notification ignored if
# > the origin of the request is a server which is forbidden by the
# > room's ACL. Ignoring the typing notification means that the EDU
# > MUST be dropped upon receipt.
if edu.edu_type == EduTypes.TYPING:
origin_host, _ = parse_server_name(origin)
room_id = edu.content["room_id"]
try:
await self.check_server_matches_acl(origin_host, room_id)
except AuthError:
logger.warning(
"Ignoring typing EDU for room %s from banned server", room_id
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Ignoring typing EDU for room %s from banned server", room_id
"Ignoring typing EDU for room %s from banned server because of ACL's", room_id

)
return

# Server ACL's apply to `EduTypes.RECEIPT` per MSC4163:
#
# > For read receipts (m.receipt), all receipts inside a room_id
# > inside content should be ignored if the origin of the request is
# > forbidden by the room's ACL.
if edu.edu_type == EduTypes.RECEIPT:
origin_host, _ = parse_server_name(origin)
to_remove = set()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
to_remove = set()
to_remove_room_ids = set()

for room_id in edu.content.keys():
try:
await self.check_server_matches_acl(origin_host, room_id)
except AuthError:
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels like we should also have a general except Exception that logger.exception(...) like below.

Perhaps we should just nest everything under that single try

to_remove.add(room_id)

if to_remove:
logger.warning(
"Ignoring receipts in EDU for rooms %s from banned server %s",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Ignoring receipts in EDU for rooms %s from banned server %s",
"Ignoring receipts in EDU for rooms %s from banned server %s because of ACL's",

to_remove,
origin_host,
)

for room_id in to_remove:
edu.content.pop(room_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

Mutating the edu itself it fine because we're creating it in this scope. But mutating edu.content is sketchy because that comes directly from the edu_dict being passed in.

Maybe one day we can lint for this kind thing (python/mypy#11076)


if not edu.content:
# If we've removed all the rooms, we can just ignore the whole EDU
return

try:
await self.registry.on_edu(edu.edu_type, origin, edu.content)
except Exception:
Expand Down
Loading