-
Notifications
You must be signed in to change notification settings - Fork 383
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
MSC2228: Self destructing events #2228
base: old_master
Are you sure you want to change the base?
Conversation
based on lessons learned from #1763
I think this is good for review; if people think it's on the right track, I'll propose FCP (although N.B. we have no urgent plans to implement this in synapse - just wanted to rescue the content from #1763 and make it saner, particularly if anyone else wanted to implement it in their server or contribute it) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
largely the concept seems straightforward to me. This also seems like a good candidate for proposing FCP early and letting checkmarks accumulate over time.
Co-Authored-By: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Co-Authored-By: Travis Ralston <travpc@gmail.com>
@mscbot fcp merge |
This FCP proposal has been cancelled by #2228 (comment). Team member @mscbot has proposed to merge this. The next step is review by the rest of the tagged people: Concerns:
Once at least 75% of reviewers approve (and there are no outstanding concerns), 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 information about what commands tagged team members can give me. |
(it feels premature to suggest fcp on this without having checked whether the impl works though - if we have to change it based on the impl, are we going to have to fcp it again?) |
Theory is we don't design a system that sucks to implement. Minor changes don't need approval (they're at the discretion of the person reviewing the spec PR), however major changes need a MSC against the MSC to be incorporated in the spec. |
This comment was marked as resolved.
This comment was marked as resolved.
|
||
We could let the user specify an expiry time for messages relative to when | ||
they were sent rather than when they were read. However, I can't think of a | ||
good enough use case to justify complicating the proposal with that feature. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH, having the expiry based on the send time rather than the receive time seems more natural to me, as it seems more predictable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it may be more predictable for the sender, but we should surely consider the UX from the receiver's perspective, where it will just be frustrating to rush to read messages relative to when the sender sent them rather than when the receiver read them. Imagine how annoying it would be to receive a msg with a 5s timeout and discover that 4s of synapse latency meant you only had 1s to open the push, launch the app, sync and read it before your client gleefully deletes it...
That said, there may be a fairly obscure use case here around time-limited promotions - where a user sends a message to a room saying "You have 30 mins from now to download my exclusive artwork!!!" and then posts a link which autodestructs 30 mins after sending.
So perhaps we do want to include this after all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ara4n Sounds to me like there is a use case that you may not be aware of: 🙂
The history of a chat should be deleted automatically after x weeks, because it is known that the messages in the chat will no longer be relevant and should rather not be kept forever on all devices, in case an account would be compromised after years. In this case, it is also desirable that the messages are deleted and do not remain stored for example on inactive accounts which haven't sent any read receipts.
In my surrounding a very common practice to set the chats in Signal to 4 weeks to more or less guarantee that not unnecessary much of the chat history is stored on all devices :)
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
matrix-org/synapse#12524 requests that support for this MSC be visible in the capabilities response. |
Furthermore, that request seems motivated by an attempt to implement this MSC in a client, see syphon-org/syphon#660 |
@jakewb-b, @sundbry: If you still feel your comments are relevant, please put them on the proposal document (https://github.com/matrix-org/matrix-spec-proposals/pull/2228/files) rather than the main PR, to make it possible to track which threads have been addressed. |
Once a given server has received a read receipt for this message from a member | ||
in the room (other than the sender), then the message's self-destruct timer | ||
should be started for that user. Once the timer is complete, the server | ||
should redact the event from that member's perspective, and send the user a | ||
synthetic `m.redaction` event in the room to the reader's clients on behalf of | ||
the sender. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused by the expected behaviour of
m.self_destruct
.In the section on server side behaviour, it seems like the server should redact the event on a per user basis based on when it receives a read receipt from that user. So the server ends up performing multiple synthetic redactions for a given event.
Once a given server has received a read receipt for this message from a member in the room (other than the sender), then the message's self-destruct timer should be started for that user. Once the timer is complete, the server should redact the event from that member's perspective, and send the user a synthetic m.redaction event in the room to the reader's clients on behalf of the sender.
but in the Proposal section it says
m.self_destruct: the duration in milliseconds after which the participating servers should redact this event on behalf of the sender, after seeing an explicit read receipt delivered for the message from all users in the room.
Which is something quite different, in that it if a given user never sees the message or their client does not send a read receipt the event will never be synthetically redacted.
The former seem preferable to me, thinking in terms of how this sort of feature works in other services. Separately it will be much hard to implement the latter in a robust manner.
@ara4n what was your intention?
...
After irl chat we agreed that expiry messages from the client's perspective is the right way forward. We could even consider not sending the synthetic redaction and trust the client to handle it, though my sense is that we should.
So from the previous comment we will implement this:-
Once a given server has received a read receipt for this message from a member in the room (other than the sender), then the message's self-destruct timer should be started for that user. Once the timer is complete, the server should redact the event from that member's perspective, and send the user a synthetic m.redaction event in the room to the reader's clients on behalf of the sender.
## Tradeoffs | ||
|
||
We could purge rather than redact destructed messages from the DB, but that | ||
would fragment the DAG so we don't do that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's worth noting that P2P Matrix is driving work to support fragmented DAGs, and it may not be so unreasonable to just delete the message outright once that work lands. However, I wouldn't create an artificial dependency on that at this point; instead we might just need another MSC in future to make self-destructing events simply delete rather than self-redact.
In terms of security and data retention it is a very important feature I think. It could make migration from Signal easier. |
If I understand this correctly, it implements an (important) special case of this. I am afraid that implementing the special case will lead away from eventually implementing the full solution. Then later another special case (e.g. "keep trying") is implemented, and the end may be mosaic mess. |
any update on why the merge is being blocked? would love an update! |
It's not being "blocked", it's just not ready to merge yet (see all the open comment threads), and nobody is actively working on it. |
It would also be good if this were implemented in a way facilitating the implementation of element-hq/element-meta#712 . |
Clients and servers MUST send explicit read receipts per-message for | ||
self-destructing messages (rather than for the most recently read message, | ||
as is the normal operation), so that messages can be destructed as requested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This stipulation is a bit puzzling to me, because we make the point of trying to paper over lack of support from clients by synthesising redactions.
However, if we require clients to acknowledge each individual self-destructing event, we have already lost backwards compatibility, so there'd be no point in synthesising redactions.
(on a tangent, I'm not sure why this mentions servers sending read receipts — when do servers send read receipts anyway?)
In my opinion we should remove this stipulation and aim for backwards compat (i.e. allow the read receipt to be for the most recently read message, as per usual).
Once a given server has received a read receipt for this message from a member | ||
in the room (other than the sender), then the message's self-destruct timer | ||
should be started for that user. Once the timer is complete, the server | ||
should redact the event from that member's perspective, and send the user a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sending this synthetic redaction will mean we have to generate an event ID for it. That part is easy, but what happens if the client then tries to use that fake event's ID for anything such as:
- replying to it
/_matrix/client/v3/rooms/{roomId}/context/{eventId}
/rooms/{roomId}/event/{eventId}
/_matrix/client/v3/rooms/{roomId}/redact/{eventId}/{txnId}
/rooms/{roomId}/relations/{eventId}
(and friends)POST /_matrix/client/v3/rooms/{roomId}/receipt/{receiptType}/{eventId}
- perhaps some others I missed, but these are all I can see for now
Most of these may be fairly easy to handle but it's still worth noting as extra implementation work.
The idea of synthetic events may still make good sense, but we should be aware of what we're signing up for — it's not quite as simple as just jamming something in /sync. On the Synapse side, after seeing this I would be tempted to implement a generic synthetic event mechanism in case we re-use this idea in the future — do we expect we may do?
A new proposal for self-destructing events via redactions, based on lessons learned from #1763.
Rendered