Add support for MSC4293 - Redact on Kick/Ban

[H-Shay]

Adds support for MSC4293

[tulir]

Does synapse already pass through the field from /ban requests to the member event?

[H-Shay]

Does synapse already pass through the field from /ban requests to the member event?

it does not, I have added support for that as well, missed it the first go around.

[anoadragon453]

Not a complete review, as I still need to look at tests and the DB updates. But a solid start!

Also be aware that I typically advise people to aim for writing Complement tests when implementing unstable MSCs, such that other homeserver implementations can test against it when it comes time for them to implement the feature.

You don't need to do it here as you've already written a lot of unit tests, but for future reference.

[H-Shay]

Build errors are odd, I wonder if they are related to #18559 , as the errors mention base64:

Building editable for matrix-synapse (pyproject.toml) did not run successfully.
    │ exit code: 1
    ╰─> [48 lines of output]
        A setup.py file already exists. Using it.
        running build_ext
        running build_rust
        error: failed to parse lock file at: /home/runner/work/synapse/synapse/Cargo.lock
        
        Caused by:
          package `base64` is specified twice in the lockfile
        error: `cargo metadata --manifest-path rust/Cargo.toml --format-version 1` failed with code 101

Otherwise I am not sure what I did to cause these

[erikjohnston]

Sorry, I broke develop :(

This PR should fix it #18561

[turt2live]

drive-by SCT review to meet implementation requirements on the MSC. Overall, looks great - thank you!

The major detail would be removal of the event type filters, but that looks trivial enough to consider the MSC implemented regardless.

[H-Shay]

@anoadragon453 just checking that this is still on your radar

[erikjohnston]

I'm wondering if this is the right architecture for dealing with such redactions. Pulling out all events sent by the user has a few potential issues: a) if there are a lot of events it might take a lot of time, and b) won't redact events that we haven't backfilled over federation yet. The MSC also seems to suggest that events should get unredacted if the ban is replaced with another state event without a redactions?

An alternate method may be to have a separate table that is room_redactions(room_id, sender), which we then check (as well as redactions table) when fetching the event? The new room_redactions would be updated when we add the ban/kick to the current_state_events table, and removed if it gets replaced?

[turt2live]

The MSC also seems to suggest that events should get unredacted if the ban is replaced with another state event without a redactions?

They shouldn't get unredacted, but the auto-redaction should cease for newly received events. I'll try to clarify this in the MSC itself.

An example series of events:

1. Alice sends a message
2. Bob bans Alice, with redact_events: true
3. [Servers and clients redact Alice's message in step 1]
4. Due to propagation delays, Alice's second message arrives after the ban. The server (and client, if it received it) redacts that message too.
5. Later, Bob unbans Alice, setting redact_events: false
6. Again due to propagation delays, or because Alice rejoined, Alice's third message arrives. It is not redacted.

The messages in steps 1 and 4 remain redacted even after step 5.

[H-Shay]

@erikjohnston Looking at it I think a room_redactions table might work (with an added end_ts column or something to indicate the end of a redaction application in the case that the ban is rescinded, etc), but would still need to pull all of the user's event ids (but not full events) out of the db because they are needed to invalidate the cache.

I don't have a sense of the time difference to pull event ids out of the database vs pulling the full events so it is unclear to me whether having to still pull the ids will negate the benefit of the new table - right now the PR does both so it is an obvious improvement but is it enough or does another approach need to be found?

[anoadragon453]

with an added end_ts column or something to indicate the end of a redaction application in the case that the ban is rescinded, etc

Wouldn't removing the row from the table be enough to stop redacting new events?

I don't have a sense of the time difference to pull event ids out of the database vs pulling the full events so it is unclear to me whether having to still pull the ids will negate the benefit of the new table

Pulling out only event IDs will certainly be much less data than full events, which should contribute directly to time taken executing the query. By how much depends on how large a user's events typically are. But you're right in that we'll still need to do the hard work of querying for every event a user has sent in a room.

---

The overview of this PR is:

The redactions table is updated to change the unique constraint from event_id to (event_id, redacts). This is so one event (a kick/ban membership) can redact multiple other events.

When a new event comes in over federation, add redacted_because to its unsigned and add a redaction to the local DB, then invalidate the event cache for that event.

/ban and /kick are updated with a org.matrix.msc4293.redact_events JSON body parameter. If provided, that field is added to the content of the ban/kick membership event.

When any event is persisted (local or over federation), all event IDs that a user has sent in a room are pulled out and entries in redactions are created for them. Each event has redacted_because added to it. The get_event cache is invalidated for each of these events.

---

What's more concerning to me is that the query to lookup all events a user has sent in a room happens is blocking during processing of a membership event. Perhaps that should be moved to a background task?

[erikjohnston]

but would still need to pull all of the user's event ids (but not full events) out of the db because they are needed to invalidate the cache.

I think you can invalidate those caches by room as well as by event ID?

Looking at it I think a room_redactions table might work (with an added end_ts column or something to indicate the end of a redaction application in the case that the ban is rescinded, etc)

We would probably want to do this by some sort of stream ordering or whatever. We might do this as a two step:

1. On receipt of ban, add room ID / target to table and record the range of stream orderings that should be redacted.
2. On receipt of new events (e.g. via backfill), check if they match the ban and if so record them as redacted.

or something

[anoadragon453]

Agree with Erik's comment above.

Otherwise some minor notes on the current code - which is a lot more readable, thank you!

[anoadragon453]

with an added end_ts column or something to indicate the end of a redaction application in the case that the ban is rescinded, etc

Wouldn't removing the row from the table be enough to stop redacting new events?

I don't have a sense of the time difference to pull event ids out of the database vs pulling the full events so it is unclear to me whether having to still pull the ids will negate the benefit of the new table

Pulling out only event IDs will certainly be much less data than full events, which should contribute directly to time taken executing the query. By how much depends on how large a user's events typically are. But you're right in that we'll still need to do the hard work of querying for every event a user has sent in a room.

---

The overview of this PR is:

The redactions table is updated to change the unique constraint from event_id to (event_id, redacts). This is so one event (a kick/ban membership) can redact multiple other events.

When a new event comes in over federation, add redacted_because to its unsigned and add a redaction to the local DB, then invalidate the event cache for that event.

/ban and /kick are updated with a org.matrix.msc4293.redact_events JSON body parameter. If provided, that field is added to the content of the ban/kick membership event.

When any event is persisted (local or over federation), all event IDs that a user has sent in a room are pulled out and entries in redactions are created for them. Each event has redacted_because added to it. The get_event cache is invalidated for each of these events.

---

What's more concerning to me is that the query to lookup all events a user has sent in a room happens is blocking during processing of a membership event. Perhaps that should be moved to a background task?

[H-Shay]

Right I have re-written the PR to take Erik's advice about creating a room_ban_redactions table. Room ban redactions are applied at the same place that regular redactions are, consulting the room_ban_redactions table to determine if there is an active redaction membership event for the room, user combination. I think most of the concerns raised have been answered, either directly or through no longer being relevant.

Re invalidating the cache, the cache I need to invalidate is the _get_event_cache - I do not see a function to invalidate it by room id (I am wondering how this would work because it seems to be a simple key, value pair of event_id, cache entry) but there might be something I am missing? Pulling the events and invalidating in the background might be a solution, but I am wondering how long that will generally take - how much slower is running a function in the background? My only concern is that it if it slows it down considerably then it might make more sense to just let it block, especially since at this point I believe the membership event itself has already been committed to the db.