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

WIP: Dmr/unblock catchup #15228

Closed
wants to merge 8 commits into from
Closed

WIP: Dmr/unblock catchup #15228

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6b70d44
Test
Mar 8, 2023
bebd7d2
Tweak docstring and type hint
Mar 8, 2023
c813f89
Flip logic and provide better name
Mar 8, 2023
0aa0201
Unconditionally omit remote events in partial state rooms
Mar 8, 2023
7f97783
Separate decision from action
Mar 8, 2023
1cb55e9
Track a set of strings, not EventBases
Mar 8, 2023
f139247
Avoid state lookups for events we'll ignore
Mar 8, 2023
31f2b15
Changelog
Mar 8, 2023
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
4 changes: 2 additions & 2 deletions synapse/handlers/federation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,15 +392,15 @@ async def _maybe_backfill_inner(
get_prev_content=False,
)

# We set `check_history_visibility_only` as we might otherwise get false
# We unset `filter_out_erased_senders` as we might otherwise get false
# positives from users having been erased.
filtered_extremities = await filter_events_for_server(
self._storage_controllers,
self.server_name,
self.server_name,
events_to_check,
redact=False,
check_history_visibility_only=True,
filter_out_erased_senders=False,
)
if filtered_extremities:
extremities_to_request.append(bp.event_id)
Expand Down
9 changes: 4 additions & 5 deletions synapse/visibility.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -567,7 +567,7 @@ async def filter_events_for_server(
local_server_name: str,
events: Sequence[EventBase],
redact: bool = True,
check_history_visibility_only: bool = False,
filter_out_erased_senders: bool = True,
) -> List[EventBase]:
"""Filter a list of events based on whether the target server is allowed to
see them.
Expand All @@ -590,8 +590,7 @@ async def filter_events_for_server(
events
redact: Controls what to do with events which have been filtered out.
If True, include their redacted forms; if False, omit them entirely.
check_history_visibility_only: Whether to only check the
history visibility, rather than things like if the sender has been
filter_out_erased_senders: If true, also filter out events whose sender has been
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved
erased. This is used e.g. during pagination to decide whether to
backfill or not.

Expand Down Expand Up @@ -628,7 +627,7 @@ def check_event_is_visible(
# server has no users in the room: redact
return False

if not check_history_visibility_only:
if filter_out_erased_senders:
erased_senders = await storage.main.are_users_erased(e.sender for e in events)
else:
# We don't want to check whether users are erased, which is equivalent
Expand All @@ -644,7 +643,7 @@ def check_event_is_visible(
# this check but would base the filtering on an outdated view of the membership events.

partial_state_invisible_events = set()
if not check_history_visibility_only:
Copy link
Contributor

@MatMaul MatMaul Mar 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

According to some failing tests we need to keep a way to shortcut this code for the backfill use case.
In this case this function is called with backward extremities we want to use as a base for backfilling to check each extremity in turn and ignore those which users on our server wouldn't be able to see.
So I think there is no way here that we will leak events we shouldn't, so skipping this should be fine.
Why does it make the tests fail ? I haven't looked deep enough yet to understand.

if filter_out_erased_senders:
for e in events:
sender_domain = get_domain_from_id(e.sender)
if (
Expand Down