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

MSC1849: Proposal for m.relates_to aggregations #1849

Closed
wants to merge 49 commits into from
Closed
Changes from 6 commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
0b5dab5
a very WIP draft of an aggregations MSC to replace #441
ara4n Feb 4, 2019
e03457d
incorporate notes from sync with erik
ara4n Apr 5, 2019
8567149
shower thoughts
ara4n Apr 5, 2019
ed2a3a0
more notes from today's sync with erik
ara4n Apr 5, 2019
07050a4
Update with new proposal
erikjohnston Apr 30, 2019
25b879b
fixup
erikjohnston Apr 30, 2019
f50de1f
Add some notes about extended annotations
erikjohnston Apr 30, 2019
ebb25e0
further updates from RLing with Erik
ara4n Apr 30, 2019
7db56e1
Merge branch 'master' of github.com:matrix-org/matrix-doc into matthe…
erikjohnston Apr 30, 2019
a9d2efa
Fix spacing
erikjohnston Apr 30, 2019
c47391e
Add proposed pagination API
erikjohnston May 1, 2019
1b24334
Flesh out the pagination API a bit
erikjohnston May 1, 2019
a12db5b
following RL review
ara4n May 1, 2019
3255ae0
Add edit example
erikjohnston May 2, 2019
e0f165e
Example has been added
erikjohnston May 2, 2019
d38d7a7
try to converge towards a proper reviewable draft
ara4n May 12, 2019
08de484
yet more notes on yet more edge cases
ara4n May 13, 2019
e78e7ad
Add a coherent overview; remove a bunch of FIXMEs
ara4n May 13, 2019
dacae1f
Add TOC
ara4n May 13, 2019
5375a23
markdown tweaks
ara4n May 13, 2019
0f7cf5e
typos
ara4n May 13, 2019
4bb8d6b
s/contents/content/g
ara4n May 13, 2019
60a3d61
fix wording
ara4n May 13, 2019
117ae97
Update proposals/1849-aggregations.md
ara4n May 13, 2019
8627fb7
Update proposals/1849-aggregations.md
ara4n May 13, 2019
2dde2c1
add m.new_content
ara4n May 14, 2019
b166d6c
Merge branch 'master' into matthew/msc1849
ara4n Jun 25, 2019
adb240d
WIP commit of rewrite to MSC1849
ara4n Jun 27, 2019
a6c8b65
WIP commit of rewrite to MSC1849
ara4n Jun 27, 2019
081ea3d
write up the new gappy sync stuff
ara4n Jul 5, 2019
9c90cc1
s/stale_relations/stale_events/
ara4n Jul 5, 2019
3f3d60e
rewrite the paginations section
ara4n Jul 16, 2019
d98d5b3
finish rewriting MSC1849, in theory, incorporating all review so far
ara4n Jul 29, 2019
c313489
Merge pull request #2154 from matrix-org/matthew/msc1849-rewrite
ara4n Jul 29, 2019
fa7c338
fix typos
ara4n Jul 29, 2019
1409119
Update proposals/1849-aggregations.md
ara4n Jul 30, 2019
e8fde9d
Update proposals/1849-aggregations.md
ara4n Jul 30, 2019
4d236c6
incorporate some of the PR review
ara4n Jul 30, 2019
f63b20a
remaining review
ara4n Jul 30, 2019
b228688
clarify `m.replace` bundle structure
ara4n Jul 30, 2019
fb4446f
wording tweaks
ara4n Jul 31, 2019
6225aae
Apply suggestions from code review
ara4n Jul 31, 2019
6751de6
PR review
ara4n Aug 1, 2019
eab778d
clarify ignored users
ara4n Aug 1, 2019
a63d27e
incorporate erik feedback
ara4n Aug 1, 2019
4e8d370
POST /event
ara4n Aug 1, 2019
e816233
clarify errors on /aggregations
ara4n Aug 1, 2019
4a96865
add bruno's local echo considerations
ara4n Aug 1, 2019
e57033a
arbitrarily restrict trailing slashes for now
ara4n Aug 1, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
315 changes: 315 additions & 0 deletions proposals/1849-aggregations.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,315 @@
# Proposal for aggregations via m.relates_to
Copy link
Contributor

Choose a reason for hiding this comment

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

Please bear in mind that I'm posting this comment with my community member hat on rather than my NV employee one

While I haven't seen it stated explicitly in the proposal (I might have missed it though), from what I heard and read in various exchanges, including element-hq/element-web#9793, the way edits are going to be implemented specifically allows admins and mods in rooms to edit anyone's messages in rooms, which I see as a massive red flag and a huge ethical concern in a protocol that's currently mainly used for IM.

I've already discussed my PoV on this in the linked issue, but I thought I should also make my point clear in this timeline in order to keep the discussion around aggregations in one place.

I see this specific aspect of edits as a huge risk, as imho it's easy to abuse, and abuse would cause confusion in the best case scenario, impersonation with potentially harmful consequences for the victim in the worst case scenario. As a user, I feel like the risk it makes me take, in terms of potential disinformation and how I might act upon it without realising the fake, to even read messages as edited (rather than see the edits as individual messages) exceeds by a lot what the positive aspect of the feature. And I find it very intrusive to even allow other users to edit my own messages without my consent.

I do see the point that it might be a useful tool for moderation, but it really sounds to me like it brings little to the table, nor can I see the absolute need for it given we already have a redaction feature. I'm waaaay more comfortable with mods deleting my messages rather than putting something I don't think or agree with under my name.

On a UX/UI point of view, I'd tend to think that this aspect adds a hard requirement for a perfectly explicit UX that allows users to figure out from a glance, and without additional actions, who authored which part of a message as easily and as straightforwardly as figuring out who authored a message, and this on every client that has at least one user. This doesn't sound a realistic goal to me.
A small and brief text at the end of the message like the "(edited)" we have now is not enough, imho, as I myself often misses it when reading messages from others, and I'm almost sure that non-technical users would either miss it as well most of the time, or not know what it means and decide not to care (my dad would definitely be in either of those 100% of the time). Plus, it turns an informational metadata that you don't have to process if you don't care into an information that'd be presented as non-important but is in fact as important as the message and its context if you want to understand both.

As to the argument "some platforms such as some forums and github do it", I'd answer that it doesn't make it a good thing to have (and some chose not to have it, such as Reddit afaik). I already find it pretty intrusive and badly implemented there, where the use of such a feature is more justified than IM (e.g. GitHub's target audience is used to anything being editable by a bunch of people and processing changes histories).

Also, as a user, I really feel like that, with this rolled out everywhere, Matrix, something that got my interest because it gives me control over my communications, takes a big chunk of this control back without me being able to do anything about it, and it makes me sad. And will probably prevent me from making announcements, or even speaking, in rooms in which I don't know each mod and admin to some extent.

I'm currently chatting with (h)activists and journalists (and some other people with interest in tech and/or privacy) to try to get them interested in Matrix, but this element being added to Matrix would probably greatly hamper these efforts, because I can't genuinely promote a platform which allows disinformation and impersonation by design.

In summary, to me this aspect of message editing is both risky and unnecessary, and will likely keep me away from message editing as long as it's a thing, and that makes me sad (especially since I was waiting on such a feature for a long time). Someone abusing it would be able to make me say "I like nazis" in a way that would look very convincing and legitimate, all without my consent (and possibly without even me noticing), and most non-technical people would just think I like nazis and possibly hate me for it. I like a message editing feature in which I'm the only one that can edit my messages and noone else, and if a mod has an issue with it, they can talk to me or just delete my message.

Copy link
Member

Choose a reason for hiding this comment

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

We are currently not allowing edits from other users right now.

That being said, it sounds like all you're asking for is that if this does get implemented then we ensure that the UX is very clear about the fact it has been edited by a third party? That seems entirely reasonable and easy enough to do, e.g. changing the avatar to an amalgamation of the sender and editor as well as the display name, etc.

Its also worth noting that you can always see edit history, which feels like a net win over redactions (which IMO are a horrible moderation tool) in terms of maintaining control over the conversation.

In general we have a balancing act between moderation tools and "control", after all moderation tools by definition are about taking power from individuals and giving them to moderators. I think its clear that we do need more moderation tools in Matrix, otherwise it will not be usable by most communities. However, so long as we are clear in our UX I think we can support a happy compromise that allows Matrix to be useful across the board.

Copy link
Contributor

Choose a reason for hiding this comment

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

We are currently not allowing edits from other users right now.

Even mods/admins? If so, can we please keep it like this?

Its also worth noting that you can always see edit history

My point is that I doubt most non-tech users will use it, or even notice there is such a thing.

e.g. changing the avatar to an amalgamation of the sender and editor as well as the display name, etc.

That would be a good start tbh, though I don't think relying on Riot to do the right thing UX-wise to prevent abuses on all of Matrix is the right way to go, nor is fair to other clients.

Copy link
Contributor

@babolivier babolivier Jun 28, 2019

Choose a reason for hiding this comment

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

That would be a good start tbh, though I don't think relying on Riot to do the right thing UX-wise to prevent abuses on all of Matrix is the right way to go, nor is fair to other clients.

To get more in depth on what I mean, I think if we are to rely on client UX to prevent abuses to go horribly wrong, it needs to:

  • show which part of the message was authored by each editor, without any more difficulty to get or additional action than seeing which message was authored by each user, edit histories are nice for power users but I think its usefulness stops here.
  • be very good at this on every client that exist and will ever exist (and support edits).

Which is why I don't consider this as a reasonable expectation.

I would rather have Matrix prevent abuses that could go horribly wrong instead.

Copy link
Member

Choose a reason for hiding this comment

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

My point is that I doubt most non-tech users will use it, or even notice there is such a thing.

Really? Again, this feels like a solvable UX issue. If there is a big fat button that screams "This messages has been EDITED click here to see the history" users will quickly realise that its a thing.

That would be a good start tbh, though I don't think relying on Riot to do the right thing UX-wise to prevent abuses on all of Matrix is the right way to go, nor is fair to other clients.

I'm sympathetic to this view point, but again it feels like an argument that can be made against many features. What we see in Matrix (and across tech) is that UIs are greatly informed by existing implementations. If we implement an appropriate UX in Riot I would be comfortable assuming that the majority of clients will similarly highlight third party edits, and certainly if they didn't then users of those clients would quickly start complaining about it.

Can other clients implement confusing UX? Yes, they can, but that's true of many things today.

(I also fail to see how its unfair to other clients? Its not like we're special casing Riot in anyway?)

  • be very good at this on every client that exist and will ever exist (and support edits).

I think this is setting an impossibly high bar. Why does this not apply to end to end encryption? We heavily rely on clients implementing the correct behaviour, and the consequences of them getting it wrong are at least as damaging, if not more so, than third party edit UX mess ups.

We cannot create a protocol that is 100% fool proof against attackers and client implementation problems. What we can do is to try and highlight such areas of high risk and give clear and unambiguous guidelines on how to deal with those issues, setting appropriate examples in the flagship apps that are built.


Broadly, yes, one of the reasons we aren't implementing third party edits today is over concerns that it is more effort than its worth, and that implementing the necessary UX is non-trivial to get right.

Copy link
Contributor

Choose a reason for hiding this comment

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

Really? Again, this feels like a solvable UX issue. If there is a big fat button that screams "This messages has been EDITED click here to see the history" users will quickly realise that its a thing.

If there's a big fat button easy to spot, maybe. With the current UX in Riot ("(edited)" after the message), probably not. I'm convinced most non-tech people would just see it as "here's something I don't fully understand but doesn't prevent me from reading the timeline so let's just not care". And again, I myself misses it quite often, and am convinced that most non-tech people I know would either miss it or be in the "not understanding, thus not caring" scenario.

users of those clients would quickly start complaining about it

Only if the client provides the tools to spot it I think, which there's no guarantee of.

Why does this not apply to end to end encryption?

It does apply, but I wouldn't say it's comparable because there's nothing a server implem can do about a client implementing E2EE in a confusing way, which is not true with edits. My point is that I believe that if we can avoid clients from causing damage to Matrix users because they don't provide the right UX, we probably should imho.

(I also fail to see how its unfair to other clients? Its not like we're special casing Riot in anyway?)

Not sure if that's what you meant, but I understood that part of your message as "we'll have a nice UX in Riot so it's not a problem".

We cannot create a protocol that is 100% fool proof against attackers and client implementation problems.

Which imho doesn't mean it's not worth trying.

setting appropriate examples in the flagship apps that are built.

Then this may need to be more explicit, because I never saw Riot as an example to follow UX-wise when building a client, nor have I seen any client dev talk about it that way.
And I also don't like the idea of seeing Riot as a reference implementation, because that's what a "flagship app that should be the example to follow" sounds like to me. Riot is developed by NV, not the Matrix.org Foundation, and should imho be as valid as an example as any other clients out there.

concerns that it is more effort than its worth, and that implementing the necessary UX is non-trivial to get right.

As a user, I feel these concerns are enough to make me start looking for a non-Matrix-based IM platform to move to if it gets implemented or allowed in the spec.

Copy link
Member

Choose a reason for hiding this comment

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

Then this may need to be more explicit, because I never saw Riot as an example to follow

Let me bit a bit more explicit about this: Riot is not special cased in any way at all. That doesn't mean that Riot, as currently the client with the most user share, won't have a direct impact on how other clients implement features. I'm not saying that other clients must, or event should, follow the choices Riot makes, and indeed it'd be pointless if clients did. But what will happen is that client implementers will take cues from how other clients have implemented features, and try and insure that they implement similar safe guards.

As a spec we can't ignore how the existing ecosystem will implement things.

We would also, of course, spell all this out in the spec. We could also ensure that third party edits behave differently down the /sync api to ensure that clients do specifically handle them differently.

If there's a big fat button easy to spot, maybe. With the current UX in Riot ("(edited)" after the message), probably not.

I'm saying if we implement third party events we should change that?

I'm convinced most non-tech people would just see it as "here's something I don't fully understand but doesn't prevent me from reading the timeline so let's just not care". And again, I myself misses it quite often, and am convinced that most non-tech people I know would either miss it or be in the "not understanding, thus not caring" scenario.

Hence my suggestion that we change the timeline to not keep the avatar and display name of the original sender. That way, if the user doesn't understand what's happened they won't believe it came from the original sender either.

Only if the client provides the tools to spot it I think, which there's no guarantee of.

There's no guarantee that clients won't display the wrong avatars next to messages. Yes, that's a bug that we've seen.

We cannot create a protocol that is 100% fool proof against attackers and client implementation problems.

Which imho doesn't mean it's not worth trying.

Of course we should try, my point throughout all of this is that perfect is an enemy of good. What matters is what happens in practice, if an attack is not actually possible in practice then it doesn't really matter if its theoretically possible in other scenarios. We can create the most theoretically secure app in the world, and no one would use it if it doesn't have the features that people actually need.

It does apply, but I wouldn't say it's comparable because there's nothing a server implem can do about a client implementing E2EE in a confusing way, which is not true with edits.

From an end user security perspective the distinction doesn't really matter? Yes, from a practical perspective it means that we need to be careful to help clients ensure that they do the right thing to be secure, just like we do for E2E.


> WIP WIP WIP WIP WIP WIP WIP WIP WIP WIP

A very rough WIP set of notes on how relations could work in Matrix.

Today, replies looks like:

```json
"type": "m.room.message",
"contents": {
ara4n marked this conversation as resolved.
Show resolved Hide resolved
"m.relates_to": {
"m.in_reply_to": {
"event_id": "$another:event.com"
}
}
}
```

`m.relates_to` is the signal to the server that the fields within describe
aggregation operations.

We would like to add support for other types of relations, including message
editing and reactions.


## Types of relations

There are three broad types of relations: annotations, replacements and
references.

Annotations are things like reactions, which should be displayed alongside the
original event. These should support be aggregated so that e.g. if twenty people
ara4n marked this conversation as resolved.
Show resolved Hide resolved
"likes" an event we can bundle the twenty events together when sending the
original event to clients. Another usage of an annotation is e.g. for bots, who
could use annotations to report the success/failure or progress of a command.
turt2live marked this conversation as resolved.
Show resolved Hide resolved

Replacements are essentially edits, and indicate that instead of giving clients
the original event they should be handed the replacement event instead. Clients
should be able to request all replacements of an event, i.e. the "edit history".
ara4n marked this conversation as resolved.
Show resolved Hide resolved

References things like replies, where a later event refers to an earlier event
in some way. The server should include references when sending an event to the
client so they can display the number of replies, and navigate easily to them.

These types effect how the server bundles the related events with the original,
and so the type must be known to servers when handling relations. However, the
exact semantics of a particular relation only needs to be known by clients. This
means that if we include the relation type in the related event we can use the
event type to easily add new types of e.g annotations without requiring server
side support.


## Aggregating and paginating relations

In large rooms an event may end up having a large number of related events, and
so we do not want to have to include all relations when sending the event to the
client. How we limit depends on the relation type.

Annotations are grouped by their event type and an "aggregation key", and the
top N groups with the highest number is included in the event. For example,
reactions would be implemented as a `m.reaction` with aggration key of e.g.
`👍`.

TODO: Should we include anything other than event type, aggregation key and
ara4n marked this conversation as resolved.
Show resolved Hide resolved
count?


Replacements replace the original event, and so no aggregation is required.
Though care must be taken by the server to ensure that if there are multiple
replacement events it consistently chooses the same one as all other servers.
The replacement event should also include a reference to the original event ID
so that clients can tell that the message has been edited.

For references the original event should include the list of `type` and
`event_id` of the earliest N references.

TODO: Do we need the type? Do we want to differentiate between replies and
other types of references? This assumes the type of the related event gives
some hint to clients.

In each case where we limit what is included there should be a corresponding API
to paginate the full sets of events. Annotations would need APIs for both
fetching more groups and fetching events in a group.


## Event format

All the information about the relation is put under `m.relates_to` key.

A reply would look something like:
Copy link
Member

Choose a reason for hiding this comment

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

Note: This is backwards incompatible and doesn't have fallback semantics for clients which don't care about replies. This could lead to confusing conversations where someone replies X hours later and others don't have context on what they're talking about.

I'd much rather fix https://github.com/matrix-org/matrix-doc/issues/1541 with this iteration (possibly https://github.com/matrix-org/matrix-doc/issues/1661 too), accepting https://github.com/matrix-org/matrix-doc/issues/1654 as a risk. This could be trivially represented as:

{
    "type": "m.room.message",
    "content": {
        "body": "> <@alice:example.org> Who loves shelties?\n\ni <3 shelties",
        "... formatted body stuff ...": "as required by existing spec",
        "m.relates_to": {
            "rel_type": "m.reference",
            "event_id": "$some_event_id",
            "reply": "i <3 shelties"
        }
    }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

switching replies to using relations has been descoped from this; there's already too much in flight to get edits & reactions finally totally locked down.


```json
{
"type": "m.room.message",
"contents": {
ara4n marked this conversation as resolved.
Show resolved Hide resolved
"m.relates_to": {
"type": "m.references",
"event_id": "$some_event_id"
}
}
}
```

And a reaction might look like the following, where we define for `m.reaction`
that the aggregation key is the unicode reaction itself.

```json
{
"type": "m.reaction",
"contents": {
ara4n marked this conversation as resolved.
Show resolved Hide resolved
"m.relates_to": {
"type": "m.annotation",
"event_id": "$some_event_id",
"aggregation_key": "👍"
}
}
}
```

TODO: This limits an event to only having one relation, on the assumption
ara4n marked this conversation as resolved.
Show resolved Hide resolved
that there are no use cases and that it will make life simpler.


An event that has relations might look something like:

```json
{
...,
"unsigned": {
"m.relations": {
"m.annotation": [
{
"type": "m.reaction",
"aggregation_key": "👍",
"count": 3
}
],
"m.reference": {
"chunk": [
{
"type": "m.room.message",
"event_id": "$some_event"
}
],
"limited": false,
"count": 1
}
}
}
}
```


## End to end encryption

Since the server bundles related events the relation information must not be
encrypted.

For aggregations of annotations there are two options:
ara4n marked this conversation as resolved.
Show resolved Hide resolved

1. Don't group together annotations and have the aggregation_key encrypted, so
as to not leak how someone reacted (though server would still see that they
did).
2. In some way encrypt the `aggregation_key`, with the properties that different
users and clients reacting in the same way to the same event produce the same
`aggregation_key`, but isn't something the server can calculate and is
different between different events (to stop statistical analysis). Clients
also need to be able to go from encrypted `aggregation_key` to the actual
reaction.
ara4n marked this conversation as resolved.
Show resolved Hide resolved

One suggestion here was to use the decryption key of the event as a base for
a shared secret.


## CS API

Sending a related event uses an equivalent of the normal send API (with an
equivalent `PUT` API):

```
POST /_matrix/client/r0/rooms/{roomId}/send_relation/{parent_id}/{relation_type}/{event_type}
ara4n marked this conversation as resolved.
Show resolved Hide resolved
ara4n marked this conversation as resolved.
Show resolved Hide resolved
{
// event contents
}
```
Copy link
Member

Choose a reason for hiding this comment

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

We need to figure out how to send the aggregation key

Copy link
Member

Choose a reason for hiding this comment

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

We add a key query string param with the (url encoded) aggregation key

Copy link
Member Author

Choose a reason for hiding this comment

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

@erikjohnston why? can't we just fish it out of the event contents?


Whenever an event that has relations is sent to the client, e.g. pagination,
event search etc, the server bundles the relations into the event as per above.
ara4n marked this conversation as resolved.
Show resolved Hide resolved

The `parent_id` is:

* For annotations the event being displayed (which may be an edit)
* For replaces/edits the original event (not previous edits)
* For references should be the original event (?)

The same happens in the sync API, howevr the client will need to handle new
relations themselves when they come down incremental sync.


## Edge cases
ara4n marked this conversation as resolved.
Show resolved Hide resolved
ara4n marked this conversation as resolved.
Show resolved Hide resolved
ara4n marked this conversation as resolved.
Show resolved Hide resolved
ara4n marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

@turt2live turt2live May 14, 2019

Choose a reason for hiding this comment

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

how does all of this interact when you've ignored the user who reacted, edited, etc someone else's message? For example: User A (not ignored) says something that User B (ignored) edits - does the server send the edit down or does it let the conversation diverge?

Copy link
Member

Choose a reason for hiding this comment

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

  1. If you ignore a user, their activity should not come through to you - otherwise edits become an effective side-channel for the ignored user to reach those who ignored them.
  2. I'm pretty reserved against cross-user edits without an editor having an elevated power level for that: think of "edit wars" that Wikipedia is so well-known for. Alternatively, a room admin should have ability to "seal" the message (along with marking the specific edit as final) to protect it from further edits.
  3. For the same reason I believe that edits (unlike annotations and replies) should form a DAG, with the strictly ordered special "seal" event to stop further edits from being accepted to the DAG. Servers should be able to linearise it in a similar (same?) way we linearise room state transitions now.

Copy link
Member

Choose a reason for hiding this comment

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

It seems like given the aggregations happening in the proposal that the client can still receive the aggregation for ignored users (which is fine, I think) in terms of a count. The new APIs should probably filter them out though, and mention that they do this.

Copy link
Member Author

Choose a reason for hiding this comment

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

In an ideal world, i'd prefer that we filtered out all relations from ignored users - including aggregated reaction counts. It's awful if you've ignored someone who's harassing you, and then see anonymous +1 🍆 reactions or whatever all over your msgs.

@erikjohnston how do we handle this atm in synapse?

Copy link
Member

Choose a reason for hiding this comment

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

I believe we filter out reactions from ignored user, except when doing aggregate counts. I'm a bit wary of committing to having the counts take account of ignored users as that explodes the complexity of the calculation, which happens anytime we send events down to the client.

Copy link
Member Author

Choose a reason for hiding this comment

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

agreed that it explodes the complexity... but it also does introduce quite a miserable harassment vector. :/

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, its not ideal but I'm just not sure matrix.org would handle the extra load terribly well.

Copy link
Member

Choose a reason for hiding this comment

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

What about a partial solution like replacing the count with zero if all reactions are from ignored users?

ara4n marked this conversation as resolved.
Show resolved Hide resolved

What happens when you react to an edit?
Copy link
Contributor

Choose a reason for hiding this comment

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

There is some discussion on element-hq/element-web#10136 (comment) about this, with the TL;DR being the intention for:

  • sending reactions to target the edit, not the message
  • clients' checking edits for reactions to display
  • servers have to look for reactions on edits when bundling

If clients don't do this in a consistent way edits might be shown in one client but not in another, so we might want to spec this, or even enforce it server-side.

Copy link
Member Author

@ara4n ara4n Jul 31, 2019

Choose a reason for hiding this comment

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

this section is trying to spec it consistently: reactions are strictly per edit, and you don't bundle them together. it sounds like we're not aligned though; for me, edits should effectively delete/replace reactions.

Copy link
Contributor

Choose a reason for hiding this comment

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

On other messaging services that have both reactions and edits that I've seen, editing a message doesn't remove the reactions, so such behaviour may be surprising to those moving away from other platforms. It would be good to have consistency though.

Copy link
Contributor

@bwindels bwindels Jul 31, 2019

Choose a reason for hiding this comment

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

this section is trying to spec it consistently: reactions are strictly per edit, and you don't bundle them together. it sounds like we're not aligned though; for me, edits should effectively delete/replace reactions.

Right, I think there are 3 questions that could be further clarified below:

  • Is reacting to an original event that got edited allowed (either by spec or server)? The language below doesn't say but seems to imply so. If it is, this can give inconsistent results between clients w.r.t. which reactions are displayed for edited events.
  • Should clients ignore reactions on the original edited event?
  • How is deleting/replacing reactions achieved? By clients ignoring reactions on anything but the last edit?

The linked riot ticket is proposing, for edited events, to have a client indeed always react to the latest edit the client is aware of when reacting, and also only take reactions on the last edit into account for displaying. Hence, reactions to previous edits will still exist (but indeed not be bundled by the server anymore), but be ignored by clients once they receive the new edit.

If this is the way we want to go, we might want to spec it in greater detail to help clients be consistent.

One could also contemplate returning an error when trying to react to an original edited event or any of its edits but the last one.

Riot doesn't do any of this yet btw, just always reacts to the original event for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay. i agree with @auscompgeek's point that other services don't reset reactions when you edit - i guess people assume that the scope for abuse (you say "matrix rocks"; get 400 likes; then replace it with "matrix sucks") is mitigated by the fact that readers can check the message history to see what reactions happened when.

In which case, I think the simpler approach might be to just go and apply the reactions to the original message and ignore those on edits, i.e. the behaviour we have today? Rather than aggregating the reactions over all the edits.

@erikjohnston wdyt?

* You should be able to, but the reaction should be attributed to the edit (or
its contents) rather than the message as a whole.
* So how do you aggregate?
* Suggestion: edits gather their own reactions, and the clients should display
the reactions on the most recent edit.
* This provides a social pressure to get your edits in quickly before there
are many reactions, otherwise the reactions will get lost.
* And it avoids us randomly aggregating reactions to potentially very
different contents of messages.

How do you handle racing edits?
* The edits could form a DAG of relations for robustness.
* Tie-break between forward DAG extremities based on origin_ts
* this should be different from the target event_id in the relations, to
make it easier to know what is being replaced.
* hard to see who is responsible for linearising the DAG when receiving.
Nasty for the client to do it, but the server would have to buffer,
meaning relations could get stuck if an event in the DAG is unavailable.
* ...or do we just always order by on origin_ts, and rely on a social problem
for it not to be abused?
* problem is that other relation types might well need a more robust way of
ordering. XXX: can we think of any?
* could add the DAG in later if it's really needed?

Redactions
* Redacting an edited event in the UI should redact the original; the client
will need to redact the original event to make this happen.
* Is this not problematic when trying to remove illegal content from servers?
* Clients could also try to expand the relations and redact those too if they
wanted to, but in practice the server shouldn't send down relations to
redacted messages, so it's overkill.
* You can also redact specific relations if needed (e.g. to remove a reaction
from ever happening)
* If you redact an relation, we keep the relation DAG (and solve that metadata
leak alongside our others)

What does it mean to call /context on a relation?
* We should probably just return the root event for now, and then refine it in
future for threading?

## Federation considerations

In general, no special considerations are needed for federation; relational
events are just sent as needed over federation same as any other event type -
aggregated onto the original event if needed.

We have a problem with resynchronising relations after a gap in federation:
We have no way of knowing that an edit happened in the gap to one of the events
we already have. So, we'll show inconsistent data until we backfill the gap.
* We could write this off as a limitation.
* Or we could make *ALL* relations a DAG, so we can spot holes at the next
relation, and go walk the DAG to pull in the missing relations? Then, the
next relation for an event could pull in any of the missing relations.
Socially this probably doesn't work as reactions will likely drop-off over
time, so by the time your server comes back there won't be any more reactions
pulling the missing ones in.
* Could we also ask the server, after a gap, to provide all the relations which
happened during the gap to events whose root preceeded the gap.
* "Give me all relations which happened between this set of
forward-extremities when I lost sync, and the point i've rejoined the DAG,
for events which preceeded the gap"?
* Would be hard to auth all the relations which this api coughed up.
* We could auth them based only the auth events of the relation, except we
lose the context of the nearby DAG which we'd have if it was a normal
backfilled event.
* As a result it would be easier for a server to retrospectively lie about
events on behalf of its users.
* This probably isn't the end of the world, plus it's more likely to be
consistent than if we leave a gap.
* i.e. it's better to consistent with a small chance of being maliciously
wrong, than inconsistent with a guaranteed chance of being innocently
wrong.
* We'd need to worry about pagination.
* This is probably the best solution, but can also be added as a v2.


## Historical context

pik's MSC441 has:

Define the JSON schema for the aggregation event, so the server can work out
which fields should be aggregated.

```json
"type": "m.room._aggregation.emoticon",
"contents": {
"emoticon": "::smile::",
"msgtype": "?",
"target_id": "$another:event.com"
}
```

These would then aggregated, based on target_id, and returned as annotations on
the source event in an `aggregation_data` field:

```json
"contents": {
...
"aggregation_data": {
"m.room._aggregation.emoticon": {
"aggregation_data": [
{
"emoticon": "::smile::",
"event_id": "$14796538949JTYis:pik-test",
"sender": "@pik:pik-test"
}
],
"latest_event_id": "$14796538949JTYis:pik-test"
}
}
}
```