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

Hoist cache_joined_hosts_for_event to caller #10986

Merged
merged 1 commit into from
Oct 5, 2021
Merged

Hoist cache_joined_hosts_for_event to caller #10986

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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/10986.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Clean up some of the federation event authentication code for clarity.
18 changes: 8 additions & 10 deletions synapse/handlers/federation_event.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,11 @@ async def on_send_membership_event(
)

# all looks good, we can persist the event.

# First, precalculate the joined hosts so that the federation sender doesn't
# need to.
await self._event_creation_handler.cache_joined_hosts_for_event(event, context)
Copy link
Member

Choose a reason for hiding this comment

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

Previously we would do this even if context.rejected was truth-y. I suspect that wasn't helpful since the event is rejected, but wanted to point this out as a change in behavior.

Also, I find the comment a bit odd "fIrst...", but there's no follow-up (no "then" / "next" / "second"). 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Previously we would do this even if context.rejected was truth-y. I suspect that wasn't helpful since the event is rejected, but wanted to point this out as a change in behavior.

yeah, it's deliberate, but I think the only difference is that we won't be populating a cache entry that will never be used.

Also, I find the comment a bit odd "fIrst...", but there's no follow-up (no "then" / "next" / "second")

mmhmm. it's in reference to the line above: "we can persist the event ... But first, precalculate the joined hosts".

Copy link
Member

Choose a reason for hiding this comment

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

Fair! Not a huge deal, just felt like I was left with a cliffhanger!


await self._run_push_actions_and_persist_event(event, context)
return event, context

Expand Down Expand Up @@ -1291,17 +1296,10 @@ async def _check_event_auth(
except AuthError as e:
logger.warning("Failed auth resolution for %r because %s", event, e)
context.rejected = RejectedReason.AUTH_ERROR
return context
Copy link
Member Author

Choose a reason for hiding this comment

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

nowhere else in this method sets context.rejected (and it can't be set before entry), so this is equivalent to the removed if not context.rejected line.


if not context.rejected:
await self._check_for_soft_fail(event, state, backfilled, origin=origin)
await self._maybe_kick_guest_users(event)

# If we are going to send this event over federation we precaclculate
# the joined hosts.
if event.internal_metadata.get_send_on_behalf_of():
await self._event_creation_handler.cache_joined_hosts_for_event(
event, context
)
await self._check_for_soft_fail(event, state, backfilled, origin=origin)
await self._maybe_kick_guest_users(event)

return context

Expand Down