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

Prevent historical state from being pushed to an application service via /transactions (MSC2716) #11265

Merged
merged 5 commits into from
Nov 18, 2021
Merged

Prevent historical state from being pushed to an application service via /transactions (MSC2716) #11265

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
95624a6
Mark historical state as historical internally
MadLittleMods Nov 5, 2021
c30a35b
Add useful debug logs while making a fake AS /transactions endpoint i…
MadLittleMods Nov 6, 2021
64df8cd
Add changelog
MadLittleMods Nov 6, 2021
e580d47
Add exc_info to debug stack trace
MadLittleMods Nov 9, 2021
15e3246
Add historical to commentdoc
MadLittleMods Nov 11, 2021
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
17 changes: 15 additions & 2 deletions synapse/appservice/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,13 +231,26 @@ async def push_bulk(
json_body=body,
args={"access_token": service.hs_token},
)
logger.debug(
"push_bulk to %s succeeded! events=%s",
uri,
[event.get("event_id") for event in events],
)
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved
sent_transactions_counter.labels(service.id).inc()
sent_events_counter.labels(service.id).inc(len(events))
return True
except CodeMessageException as e:
logger.warning("push_bulk to %s received %s", uri, e.code)
logger.warning(
"push_bulk to %s received code=%s msg=%s", uri, e.code, e.msg
)
except Exception as ex:
logger.warning("push_bulk to %s threw exception %s", uri, ex)
logger.warning(
"push_bulk to %s threw exception(%s) %s args=%s",
uri,
type(ex).__name__,
ex,
ex.args,
)
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved
failed_transactions_counter.labels(service.id).inc()
return False

Expand Down
2 changes: 2 additions & 0 deletions synapse/handlers/room_batch.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,7 @@ async def persist_state_events_at_start(
action=membership,
content=event_dict["content"],
outlier=True,
historical=True,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of these changes are just piping historical down to create_event

update_membership
update_membership_locked
_local_membership_update
create_event

prev_event_ids=[prev_event_id_for_state_chain],
# Make sure to use a copy of this list because we modify it
# later in the loop here. Otherwise it will be the same
Expand All @@ -240,6 +241,7 @@ async def persist_state_events_at_start(
),
event_dict,
outlier=True,
historical=True,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of these changes are just piping historical down to create_event

create_and_send_nonmember_event
create_event

Copy link
Member

Choose a reason for hiding this comment

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

whatever else happens, please please please make sure that any parameters are added to the docstrings at each level, with the intended purpose clearly described.

Relatedly: as a general rule, I would say it is preferable for method parameters to describe a change in behaviour ("inhibit_sync_to_clent") rather than describe some bit of state that the function is free to interpret as it wishes: it's much clearer for a later reader to understand how things tie together, particularly when things then get modified later on. Obviously this isn't a hard-and-fast rule, but it's worth considering.

[Currently we have a bit of a mess in some places (FederationEventHander.backfilled being a prime example) where you have to dig through 150 methods to find out what a flag actually does, and then somehow reverse-engineer what it is supposed to do. Don't do 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.

please please please make sure that any parameters are added to the docstrings at each level, with the intended purpose clearly described.

Will do 🙇

Relatedly: as a general rule, I would say it is preferable for method parameters to describe a change in behaviour ("inhibit_sync_to_clent") rather than describe some bit of state that the function is free to interpret as it wishes

Seems reasonable. I've created a separate issue to track this #11300

prev_event_ids=[prev_event_id_for_state_chain],
# Make sure to use a copy of this list because we modify it
# later in the loop here. Otherwise it will be the same
Expand Down
6 changes: 6 additions & 0 deletions synapse/handlers/room_member.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,7 @@ async def _local_membership_update(
content: Optional[dict] = None,
require_consent: bool = True,
outlier: bool = False,
historical: bool = False,
) -> Tuple[str, int]:
"""
Internal membership update function to get an existing event or create
Expand Down Expand Up @@ -337,6 +338,7 @@ async def _local_membership_update(
auth_event_ids=auth_event_ids,
require_consent=require_consent,
outlier=outlier,
historical=historical,
)

prev_state_ids = await context.get_prev_state_ids()
Expand Down Expand Up @@ -433,6 +435,7 @@ async def update_membership(
new_room: bool = False,
require_consent: bool = True,
outlier: bool = False,
historical: bool = False,
prev_event_ids: Optional[List[str]] = None,
auth_event_ids: Optional[List[str]] = None,
) -> Tuple[str, int]:
Expand Down Expand Up @@ -487,6 +490,7 @@ async def update_membership(
new_room=new_room,
require_consent=require_consent,
outlier=outlier,
historical=historical,
prev_event_ids=prev_event_ids,
auth_event_ids=auth_event_ids,
)
Expand All @@ -507,6 +511,7 @@ async def update_membership_locked(
new_room: bool = False,
require_consent: bool = True,
outlier: bool = False,
historical: bool = False,
prev_event_ids: Optional[List[str]] = None,
auth_event_ids: Optional[List[str]] = None,
) -> Tuple[str, int]:
Expand Down Expand Up @@ -657,6 +662,7 @@ async def update_membership_locked(
content=content,
require_consent=require_consent,
outlier=outlier,
historical=historical,
)

latest_event_ids = await self.store.get_prev_events_for_room(room_id)
Expand Down