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

MSC1763: Proposal for specifying configurable message retention periods #1763

Open
wants to merge 36 commits into
base: old_master
Choose a base branch
from

Conversation

@ara4n
Copy link
Member

@ara4n ara4n commented Dec 30, 2018

Rendered


FCP: #1763 (comment) ~Travis

@ara4n
Copy link
Member Author

@ara4n ara4n commented Dec 30, 2018

This is an attempt to unblock #440 and #447 and solve the problem of how to specify configurable per-room and per-user message retention. This is particularly driven by Disroot & Hackint disabling their Matrix bridges due to concerns over unlimited message retention, and meanwhile there are also corporate Matrix users on the horizon who require configurable history retention. Hence trying to slay the beast for once and for all...

proposals/1763-configurable-retention-periods.md Outdated Show resolved Hide resolved
proposals/1763-configurable-retention-periods.md Outdated Show resolved Hide resolved
proposals/1763-configurable-retention-periods.md Outdated Show resolved Hide resolved
proposals/1763-configurable-retention-periods.md Outdated Show resolved Hide resolved
proposals/1763-configurable-retention-periods.md Outdated Show resolved Hide resolved
proposals/1763-configurable-retention-periods.md Outdated Show resolved Hide resolved
proposals/1763-configurable-retention-periods.md Outdated Show resolved Hide resolved
proposals/1763-configurable-retention-periods.md Outdated Show resolved Hide resolved
proposals/1763-configurable-retention-periods.md Outdated Show resolved Hide resolved
proposals/1763-configurable-retention-periods.md Outdated Show resolved Hide resolved
proposals/1763-configurable-retention-periods.md Outdated Show resolved Hide resolved
proposals/1763-configurable-retention-periods.md Outdated Show resolved Hide resolved
proposals/1763-configurable-retention-periods.md Outdated Show resolved Hide resolved
proposals/1763-configurable-retention-periods.md Outdated Show resolved Hide resolved
proposals/1763-configurable-retention-periods.md Outdated Show resolved Hide resolved
proposals/1763-configurable-retention-periods.md Outdated Show resolved Hide resolved
proposals/1763-configurable-retention-periods.md Outdated Show resolved Hide resolved
proposals/1763-configurable-retention-periods.md Outdated Show resolved Hide resolved
proposals/1763-configurable-retention-periods.md Outdated Show resolved Hide resolved
@richvdh richvdh self-requested a review Jan 2, 2019
@ara4n
Copy link
Member Author

@ara4n ara4n commented Feb 18, 2019

One thought: i wonder if we should do something to expire megolm key backup for sessions whose events have all been deleted from the server...

@richvdh richvdh removed their request for review Feb 27, 2019
* otherwise, don't apply the algorithm.

The maximum lifetime of an event is calculated as:
* if specified, the `max_lifetime` field in the `m.room.retention` event (as of `E`) for the room.
Copy link
Member

@babolivier babolivier Nov 12, 2019

(as of E)

Isn't that going to create holes in the DAG of rooms and make pagination (and maybe also federation) potentially faffy? Also, in the event of the first retention policy of a room being set in the middle of the history of the room, won't that make it difficult/impossible to reach the messages that were sent before the policy was set?

We could think of solutions like retrieving the most recent expired event and purging everything before (though we'd need to take min_lifetime into account and figure out what to do if the retention policy is lacking, which seems to be left as an implementation detail), or redacting events upon expiry and only purging them if there's no event before, or also calculating the expiration date of an event using the current retention policy in the room rather than as it was when E was sent (i.e. making it an implementation detail whether the policy used is the current one in the room or the one as of E).

Copy link
Member

@babolivier babolivier Feb 26, 2020

fwiw from my own experience since the release of the support for this feature in Synapse people seem to expect retention policies to apply retroactively, so perhaps we should just use the latest m.room.retention state event in the room (even though I can see how it creates a different behaviour than most state events).

@4nd3r

This comment has been hidden.

@richvdh

This comment has been hidden.

@richvdh
Copy link
Member

@richvdh richvdh commented Nov 28, 2019

This has been partially implemented in synapse by matrix-org/synapse#6358.

According to @babolivier, differences from this MSC are:

  • support for the min_lifetime field
  • the fact that the MSC mandates that the policy used to purge an event is the one in effect when the event is sent whereas the implem uses the current one in the room (ie, #1763 (comment))

resource constrained), but this isn't recommended given it may result in
history being irrevocably lost against the senders' wishes.

## Pruning algorithm
Copy link
Member Author

@ara4n ara4n Dec 1, 2019

Do need something here to encourage clients to delete/discard the megolm keys for pruned e2e convos?

@MurzNN
Copy link
Contributor

@MurzNN MurzNN commented Feb 26, 2020

Does this proposal provide any ways to inform Matrix clients about message deleting events on server side? At now it works on server side (using Synapse) well, but deleted messages are stay visible in room timeline on client side in cache, until user manually clean caches.

And even after manual cache cleaning, clients have problems with read markers, that points to non-exist event. So room stay always in "bold" status on Riot, workaround is post new message to room, and use RiotX-Andorid "dismiss read marker" feature for reset "bad" read marker.

@babolivier
Copy link
Member

@babolivier babolivier commented Feb 27, 2020

Clients can see the m.room.retention event as well as server, and they can therefore handle removing "expired" events from their caches. The main issue wrt clients is rather that none has gotten around implementing it so far (at least as far as I know).

By the way, for further comments, please start a thread (go to the PR's diff and click the + on a line that looks relevant, or the first line of the file if none does, to start one) to avoid too much noise on the main stream (which we'd prefer to keep for reviews and messages from the bot about the MSC's status).

degree of privacy by requesting all data is purged from all clients after a
given time.

Retention is only considered for non-state events.
Copy link

@pv pv May 4, 2020

Stupid question as general audience: what does this imply for the room topic, membership, etc. state data? Is the full history of e.g. who was a member of a room and when retained or purged? If retained, should the summary on the top mention this limitation?

Copy link
Member

@babolivier babolivier May 5, 2020

Those are state events and according to the current MSC they must be retained. The issue here is that some state events are used to authorise new events, e.g. so you can't send messages into a room that you haven't joined (i.e. in the state of which there's no join event from you), so purging state events could potentially break the room. We could theoretically avoid that by carefully selecting which state events should not be purged and which ones can (and I'm not even sure about that) but then it becomes a ticking time bomb because one day we're bound to forget about that and make some changes in state events without updating the retention policies spec and break everything.

Copy link
Member

@babolivier babolivier May 5, 2020

If retained, should the summary on the top mention this limitation?

I think this limitation is mentioned at the right place here, but ymmv.

Copy link

@pv pv May 5, 2020

The summary contains a statement "... set of rules which allow users, room admins and server admins to determine how long data should be stored for a room, from the perspective of respecting the privacy requirements of that room" which seems to incorrectly imply that the retention rules apply to all data. This was my initial understanding also when reading the configuration file in the current synapse implementation. Just a suggestion from user perspective, but I think it would be important to be clear what it does and doesn't do, so that people can make an informed decision.

Copy link
Member

@babolivier babolivier May 5, 2020

Oh right, that makes sense, fair point.

@richvdh
Copy link
Member

@richvdh richvdh commented Jul 9, 2020

@mscbot fcp concern the implementation does not match the MSC

(It seems like this might be trivially solved by making the MSC match the implementation?)

@anoadragon453
Copy link
Member

@anoadragon453 anoadragon453 commented Jul 10, 2020

@mscbot fcp concern the implementation does not match the MSC

The bot does not recognise edits I'm afraid.

Edit: lol

@mscbot
Copy link
Collaborator

@mscbot mscbot commented Jul 10, 2020

Unknown disposition 'concern'.

@anoadragon453
Copy link
Member

@anoadragon453 anoadragon453 commented Jul 10, 2020

@mscbot concern the implementation does not match the MSC

If the user's retention settings conflicts with those in the room, then the
user's clients are expected to warn the user when participating in the room.
A conflict exists if the user has configured their client to create rooms with
retention settings which differing from the values on the `m.room.retention`
Copy link
Member

@babolivier babolivier Jan 12, 2021

Suggested change
retention settings which differing from the values on the `m.room.retention`
retention settings which differ from the values on the `m.room.retention`

```

The above example means that servers receiving messages in this room should
store the event for only 86400 seconds (1 day), as measured from that
Copy link
Member

@babolivier babolivier Jan 12, 2021

max_lifetime is in milliseconds.

@richvdh
Copy link
Member

@richvdh richvdh commented Mar 16, 2021

since this doesn't have a complete implementation, I don't think it's ready for FCP.

@mscbot fcp cancel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment