Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
MSC4019: Encrypted event relationships #4019
base: main
Are you sure you want to change the base?
MSC4019: Encrypted event relationships #4019
Changes from 3 commits
6500f5a
07ffbce
6816a05
d22f344
75dc05d
a04abdd
dc70b77
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Obviously, server-side aggregations and the
/relationships
API will not work any more with this change.These exist for a reason: clients are heavily reliant on them for things like finding the reactions and threads which relate to a particular event.
It seems to me that these are "Potential issues" which need discussing here.
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.
For my use case (1-to-1 chats and small groups of at most 10 people), aggregations from server side just do not help much. Most used relation is reply, second most editing, and tiny little bits of reactions. Reply and editing chains wouldn't be spanned across like 100s of events.
The sole purpose of this is to sacrifice the ability for the server to track relationships in order to achieve better privacy.
Made this explicit in the potential issues section.
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.
@richvdh A somewhat recent discussion in Client Dev Room revealed no matrix clients use aggregations for Reactions. If a client does nobody was able to present one in that conversation. (Date of Discussion is Feburary 21nd of 2023. According to Central European Summer Time.)
So the concern that Reactions would break is if that conversation was accurate incorrect for most users.
Due to that reactions wont break a MSC like this is safe to move forward for Reactions atleast if that conversation was accurate. As we are trying to prove a negative in this case its up to whoever wants to do the work to find a counterexample.
Edit: lets update this comment. So i dug up that MSC2677 and this specific comment is where the call is made that relations in the case of reactions are not aggregated and this PR to synapse removes the code from synapse in line with the MSC.
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 think you mean reactions.
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.
You're right, reactions are not aggregated, I'd forgotten. Nevertheless the concern remains around other relation types (especially edits).
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.
@clokep is correct about what i intended to communicate. And yes its a valid concern to be concerned about other relation types.
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.
The
key
field for reactions makes sense to encrypt in private conversations. However, I think that fields denoting the relationship itself should always be unencrypted. It doesn't help with just server aggregation. Bots and clients will have it way easier getting relevant events on demand rather than needing a full sync cached and searched through. It could be made the default and no extra toggle (m.room.relationship_encryption
) would be needed; if a room is encrypted, all other relationship fields are stored inside the encrypted content, whereas theevent_id
andrel_type
remain outside, unencrypted.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 am talking about the possibility for clients to not rely on servers to keep record of event relationships. And it's optional. Clients that do want to rely on servers can simply not support it.