Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MSC2244: Mass redactions #2244

Merged
merged 10 commits into from
Nov 12, 2019
Merged

MSC2244: Mass redactions #2244

merged 10 commits into from
Nov 12, 2019

Conversation

tulir
Copy link
Member

@tulir tulir commented Aug 23, 2019

@ara4n
Copy link
Member

ara4n commented Aug 23, 2019

I really like this ftr; unsure why we've not thought of it before. The auth rule stuff definitely needs a sanity check from @richvdh & @erikjohnston & co though. It feels like the redaction should apply to all the targets events that it can rather than be soft-failed if any of the targets fail, but am not sure what the implications of that are for DAG consistency.

@turt2live turt2live added proposal A matrix spec change proposal unassigned-room-version Remove this label when things get versioned. labels Aug 23, 2019
@tulir
Copy link
Member Author

tulir commented Aug 23, 2019

Yeah, this is mostly a draft so the auth rule stuff can be figured out before reviewing

@auscompgeek
Copy link
Contributor

This could be useful for GDPR erasure as well perhaps?

@turt2live
Copy link
Member

GDPR erasure is a concern for MSC1228 because redactions do not remove metadata.

@ara4n
Copy link
Member

ara4n commented Aug 24, 2019

I think @auscompgeek’s point is that this could help GDPR data erasure (not metadata erasure). however, our position is that when someone sends you a message on matrix you own a copy of that message (much like email) and they do not have the right to mutilate your copy of that conversation under GDPR. if we did support this, it would need to be a different feature (that we’ve dubbed megaredact in the past) which would be a redaction that says “please redact everything you ever saw from this user”, rather than a list of event IDs as is proposed here. So: i don’t think we should consider this MSC in terms of GDPR.

Copy link
Member

@KitsuneRal KitsuneRal left a comment

Choose a reason for hiding this comment

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

From the client perspective I very much like the idea. I have left some comments on the current text.

proposals/2244-mass-redactions.md Show resolved Hide resolved
proposals/2244-mass-redactions.md Outdated Show resolved Hide resolved
proposals/2244-mass-redactions.md Outdated Show resolved Hide resolved
proposals/2244-mass-redactions.md Outdated Show resolved Hide resolved
proposals/2244-mass-redactions.md Outdated Show resolved Hide resolved
@turt2live turt2live self-requested a review August 26, 2019 02:50
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

Looks like this is heading in a sensible direction - thank you!

proposals/2244-mass-redactions.md Outdated Show resolved Hide resolved
proposals/2244-mass-redactions.md Show resolved Hide resolved
proposals/2244-mass-redactions.md Outdated Show resolved Hide resolved
proposals/2244-mass-redactions.md Outdated Show resolved Hide resolved
@ara4n ara4n mentioned this pull request Aug 27, 2019
tulir and others added 4 commits August 28, 2019 22:58
@tulir
Copy link
Member Author

tulir commented Aug 28, 2019

Force pushed to fix commit co-authors and signoffs

## Proposal
This proposal builds upon [MSC2174](https://github.com/matrix-org/matrix-doc/pull/2174)
and suggests making the `redacts` field in the content of `m.room.redaction`
events an array of event ID strings instead of a single event ID string.
Copy link
Member

Choose a reason for hiding this comment

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

Having thought about this some more, I think one of the most common use cases for mass redactions is to handle brigading scenarios where a user joins a room solely in order to post abuse. I think we should bite the bullet and consider putting user_ids as well as event_ids in the redaction target to make it easier for moderators to handle this scenario.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, although that should be a separate proposal. Finding and redacting everything from a user probably needs some new auth rules that aren't related to just redacting many events at once.

to this is omitting the list of redacted event IDs from the data in the
`redacted_because` field.

## Security considerations
Copy link
Member

Choose a reason for hiding this comment

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

The biggest concern here is presumably that all the servers in the room could have to do quite a lot of work, churning through all the listed event IDs trying to redact them, with the associated cache impact etc depending on the implementation specifics. I suggest we at least mention that the implementation should process the redactions in the background in such a way to avoid large malicious redaction events from being an easy way to DoS a server.

Copy link
Member

Choose a reason for hiding this comment

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

Rapid fire of individual redaction events is not really better in this regard, is it? Events can be rate-limited, and megaredactions can be rate-limited more aggressively.

Copy link
Member

Choose a reason for hiding this comment

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

to flesh this out further: i discussed this with @erikjohnston back in Sept whilst we were looking at bulk redaction in riot as a quick fix. he had concerns that this makes it too easy to overload a homeserver with single requests which could require a lot of processing to service - which is why we ended up implementing “bulk redact by looping over /redact” in riot-web at the time instead.

however, I am not convinced - as kitsune says, the server can ratelimit processing a bulk redaction as it would individual ones. it also shifts the onus to the server rather than the client to trickle through the redactions (simplifying the client UX massively, given it doesn’t have to handle queuing and echo of the redactions). Also, redacting individually means that tonnes of redaction msgs end up in the DAG and have to be relayed through to all the listening clients via /sync, which puts much more load on the CS API (and bandwidth and processing on the clients) than syncing a single bulk-redact event.

So, on aggregate, I still think this MSC is a big improvement. @erikjohnston do you still have concerns?

@tulir tulir marked this pull request as ready for review August 31, 2019 18:12
@turt2live
Copy link
Member

in the interest of not delaying this more than it needs to be, and beating @ara4n to the punch:

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Aug 31, 2019

Team member @turt2live has proposed to merge this. The next step is review by the rest of the tagged people:

No concerns currently listed.

Once at least 75% of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@mscbot mscbot added finished-final-comment-period and removed final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. labels Nov 12, 2019
@mscbot
Copy link
Collaborator

mscbot commented Nov 12, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

@turt2live turt2live merged commit 037894d into matrix-org:master Nov 12, 2019
KitsuneRal added a commit to quotient-im/libQuotient that referenced this pull request Nov 22, 2019
Cross-ref: matrix-org/matrix-spec-proposals#2244.
The implementation does not validate the redaction event contents
against a particular room version, accepting both singular event ids
and lists of ids in any room. (In fact, Quotient cannot create objects of
different classes for the same event type depending on the room version -
see #362.) Also, this commit doesn't change Room::redactEvent - it still
only gets a single event id.
@35609902357
Copy link

I think @auscompgeek’s point is that this could help GDPR data erasure (not metadata erasure). however, our position is that when someone sends you a message on matrix you own a copy of that message (much like email) and they do not have the right to mutilate your copy of that conversation under GDPR. if we did support this, it would need to be a different feature (that we’ve dubbed megaredact in the past) which would be a redaction that says “please redact everything you ever saw from this user”, rather than a list of event IDs as is proposed here. So: i don’t think we should consider this MSC in terms of GDPR.

@ara4n This has everything to do with GDPR as GDPR defines the right to erasure (aka right to vanish, aka right to oblivion), so if a user wants to redact his own messages from a chat (and a server) he should have the means to do it, no matter if the recipient would approve or not. This also includes all answers where the user is quoted should redact his username from the answer and all metadata involved.

Bridges should probably be handled differently because a user deciding to delete all his traces from a group should not also delete all his contribution to a github project for example.

But as far as individual and group chats are concerned the user should have the means to exercise his right to oblivion and the process should not be cumbersome (as in one message at a time).

@tulir
Copy link
Member Author

tulir commented Apr 4, 2020

if a user wants to redact his own messages from a chat (and a server) he should have the means to do it

Matthew meant that this is not the correct means to do it, because a "megaredact" feature that redacts everything from a user instead of specific event IDs would be more correct.

This also includes all answers where the user is quoted should redact his username from the answer and all metadata involved.

Redacting metadata is also a different thing. This proposal simply extended redactions to support multiple events, so absolutely nothing to do with metadata. Better metadata-redacting ability might come with #1228 or a future version of that.


Other than that, I don't agree with what you said at all, but this is not the place for a GDPR conversation.

@turt2live turt2live added the kind:core MSC which is critical to the protocol's success label Apr 20, 2020
@auscompgeek
Copy link
Contributor

Are there any implementations of this MSC yet?

KitsuneRal added a commit to quotient-im/libQuotient that referenced this pull request Jan 16, 2021
Cross-ref: matrix-org/matrix-spec-proposals#2244.
The implementation does not validate the redaction event contents
against a particular room version, accepting both singular event ids
and lists of ids in any room. (In fact, Quotient cannot create objects of
different classes for the same event type depending on the room version -
see #362.) Also, this commit doesn't change Room::redactEvent - it still
only gets a single event id.
@turt2live turt2live added the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Jun 8, 2021
@turt2live
Copy link
Member

This is blocked on internal context before being implemented.

If someone is planning on implementing this, please talk to us first.

@turt2live turt2live added the blocked Something needs to be done before action can be taken on this PR/issue. label Mar 1, 2022
@@ -0,0 +1,78 @@
# Mass redactions
Copy link
Contributor

@ShadowJonathan ShadowJonathan May 4, 2022

Choose a reason for hiding this comment

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

As someone who wishes to implement this in Ruma, I'm missing a few key details from this proposal;

  1. This alters the format of redactions, currently matrix has a dedicated endpoint for redactions themselves, which takes the redacted event ID in the path.
    For mass redactions, shouldn't there be a new endpoint that'd allow the new format? To allow giving the event IDs to redact as a POST body? (Or the likes)
    Currently, there is no way (other than sending a custom event) to take advantage of this new redaction format, and even then it is not fully clear if sending it as a custom event is the intended way to mass-redact.
  2. There is not an example of the new redaction event, while this is minor, it is still crucial to make clear/explicit what the new format will be, especially given historical context surrounding redacts keys.
  3. It's not clear if the old redaction format (with a string as value) will persist alongside the new version, or if there will be a clear split on room versions.
    I'd like to note that, if both variants will persist, this'll be (probably one of) the only places in the matrix spec where a key can have multiple value types, this can possibly create complications and exceptions in (de)serialization libraries such as Ruma, in which I don't see a very clear path for serde to take this on. (Update, serde seems to handle this nicely through untagged) I'd like some clarity on that as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. Sending a custom event is the intended way, a custom endpoint wouldn't have any advantages.

  2. Federation:

    {
      "type": "m.room.redaction",
      "content": {
        "redacts": ["$1234", "$5678", "$9abc"]
      }
    }

    Client API (with backwards compatibility):

    {
      "type": "m.room.redaction",
      "content": {
        "redacts": ["$1234", "$5678", "$9abc"]
      },
      "redacts": "$1234"
    }
  3. The MSC2174 format will hopefully never be used anywhere, but either way only one format will be used in new room versions. In the C-S API, servers should add the top-level redacts key for backwards compatibility, but that's just locally and not in the actual federation event (and that field is always a string, never an array).

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you.

Re 3: So this means that the /redact/ API will spit out a one-item-array redaction event in new room versions, as described above? I'd leave it for a spec PR to clear up if the MSC2174 format will be used in new room versions.

Copy link
Member

Choose a reason for hiding this comment

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

@ShadowJonathan for extreme clarity: we kindly ask that this MSC is not implemented at this time, please. The SCT is aware of this MSC's state and wants to fix it, but there's external considerations which need to be made before implementation can begin.

@turt2live
Copy link
Member

This is blocked on internal context before being implemented.

If someone is planning on implementing this, please talk to us first.

Apologies for the unexpectedly long delay in clearing this. The concerns have been opened as #4084 for review. We strongly suggest that implementations be created with the concerns in mind, if not using MSC4084 itself.

@turt2live turt2live removed the blocked Something needs to be done before action can be taken on this PR/issue. label Dec 18, 2023
KitsuneRal added a commit to quotient-im/libQuotient that referenced this pull request Mar 9, 2024
Cross-ref: matrix-org/matrix-spec-proposals#2244.
The implementation does not validate the redaction event contents
against a particular room version, accepting both singular event ids
and lists of ids in any room. (In fact, Quotient cannot create objects of
different classes for the same event type depending on the room version -
see #362.) Also, this commit doesn't change Room::redactEvent - it still
only gets a single event id.
KitsuneRal added a commit to quotient-im/libQuotient that referenced this pull request Mar 9, 2024
Cross-ref: matrix-org/matrix-spec-proposals#2244.
The implementation does not validate the redaction event contents
against a particular room version, accepting both singular event ids
and lists of ids in any room. (In fact, Quotient cannot create objects
of different classes for the same event type depending on the room
version - see #362.) Also, this commit doesn't change
Room::redactEvent() - it still only gets a single event id, pending
MSC4084 (and, respectively, implementation of UIA in the library).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge finished-final-comment-period kind:core MSC which is critical to the protocol's success needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal unassigned-room-version Remove this label when things get versioned.
Projects
None yet
Development

Successfully merging this pull request may close these issues.