Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Remove the ability to query relations when the original event was redacted. #5629

Merged
merged 23 commits into from
Jul 18, 2019
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
3e9e100
Redact m.replace relations when the original event was redacted.
anoadragon453 Jul 5, 2019
d6c89b5
Add changelog
anoadragon453 Jul 5, 2019
f351013
lint
anoadragon453 Jul 5, 2019
0350589
actually unique txnid
anoadragon453 Jul 5, 2019
5bc5d60
prevent leakage on /aggregations and fix tests
anoadragon453 Jul 8, 2019
6e015df
Merge branch 'develop' into anoa/edit_redacting
anoadragon453 Jul 8, 2019
7eb0608
lint
anoadragon453 Jul 8, 2019
ae919fc
lint?
anoadragon453 Jul 8, 2019
498a3bb
Don't propagate redaction content to relation redactions
anoadragon453 Jul 9, 2019
2d4cbc0
go deeper into the dictionary luke
anoadragon453 Jul 9, 2019
c0649be
Properly fix
anoadragon453 Jul 9, 2019
06267e0
merge conflicts
anoadragon453 Jul 9, 2019
fe23482
Don't send redactions / relation. Use internal_metadata
anoadragon453 Jul 10, 2019
f8e8e21
Set internal_metadata on the correct object
anoadragon453 Jul 10, 2019
b91bc62
Merge branch 'develop' into anoa/edit_redacting
anoadragon453 Jul 10, 2019
f119a9d
Update changelog
anoadragon453 Jul 10, 2019
edb36b5
Improve spacing
anoadragon453 Jul 15, 2019
432c041
Prevent aggregation on any redacted event and small fixes
anoadragon453 Jul 18, 2019
509a612
Use PaginationChunk instead of hand-crafting it
anoadragon453 Jul 18, 2019
036ab09
Fix /aggregations logic
anoadragon453 Jul 18, 2019
4367853
Missed a word
anoadragon453 Jul 18, 2019
72a9675
Use correct tokens
anoadragon453 Jul 18, 2019
b335613
Simplify redaction comments and logic
anoadragon453 Jul 18, 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
1 change: 1 addition & 0 deletions changelog.d/5629.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Redact message edits when the original message has been redacted.
25 changes: 25 additions & 0 deletions synapse/rest/client/v1/room.py
Original file line number Diff line number Diff line change
Expand Up @@ -718,6 +718,7 @@ def __init__(self, hs):
self.handlers = hs.get_handlers()
self.event_creation_handler = hs.get_event_creation_handler()
self.auth = hs.get_auth()
self.store = hs.get_datastore()
anoadragon453 marked this conversation as resolved.
Show resolved Hide resolved

def register(self, http_server):
PATTERNS = "/rooms/(?P<room_id>[^/]*)/redact/(?P<event_id>[^/]*)"
Expand All @@ -740,6 +741,30 @@ def on_POST(self, request, room_id, event_id, txn_id=None):
txn_id=txn_id,
)

# Redact any m.replace relations of this event
relation_chunk = yield self.store.get_relations_for_event(
Copy link
Member

Choose a reason for hiding this comment

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

this looks like it is only handling the 5 most recent edits of an event. is that deliberate?

event_id, relation_type="m.replace", event_type="m.room.message"
)
relation_chunk_dict = relation_chunk.to_dict()
Copy link
Member

Choose a reason for hiding this comment

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

why do we do this?


# Extract each event_id out of the list of dictionaries
# Ex. {'chunk': [{'event_id': '$someid'}]}
relation_ids = [x["event_id"] for x in relation_chunk_dict.get("chunk", [])]

for relation_id in relation_ids:
yield self.event_creation_handler.create_and_send_nonmember_event(
Copy link
Member

Choose a reason for hiding this comment

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

This is going to hit the ratelimiter, and then the whole request will fail.

requester,
{
"type": EventTypes.Redaction,
"content": {},
"room_id": room_id,
"sender": requester.user.to_string(),
"redacts": relation_id,
},
txn_id=txn_id,
)

# Return the event_id of the original event's redaction
anoadragon453 marked this conversation as resolved.
Show resolved Hide resolved
defer.returnValue((200, {"event_id": event.event_id}))

def on_PUT(self, request, room_id, event_id, txn_id):
Expand Down
14 changes: 13 additions & 1 deletion synapse/rest/client/v2_alpha/relations.py
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,12 @@ def on_GET(self, request, room_id, parent_id, relation_type=None, event_type=Non
from_token = parse_string(request, "from")
to_token = parse_string(request, "to")

# Check if the event is redacted, and if so return an empty chunk
# list and zero tokens
if "redacted_because" in event.unsigned:
Copy link
Member

Choose a reason for hiding this comment

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

is this pattern used anywhere else? I'm not sure it is the best way to check if an event is redacted

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't sure either, but it is used elsewhere.

self.assertFalse("redacted_because" in event.unsigned)

Might be best to create a method is_event_redacted, though not sure how it would check that otherwise.

Copy link
Member Author

Choose a reason for hiding this comment

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

This has now been replaced with internal_metadata.

res = {"chunk": []}
Copy link
Member

Choose a reason for hiding this comment

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

I'm struggling to understand the existing behaviour here (get_relations_for_event returns a pagination structure, which we then overwrite with different contents?), but it seems to return an original_event, which I can't find mentioned in the MSC. It's therefore hard to know if it is safe to not return it in this case.

I'm generally rather uneasy about unspecced behaviour which clients will come to rely on.

Copy link
Member Author

Choose a reason for hiding this comment

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

The original_event came from #5626 which lead to a comment on the rewrite MSC here.

Copy link
Member Author

Choose a reason for hiding this comment

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

After replacing things with PaginationChunk, original_event is now included in the response whether the event is redacted or not.

Not including it was an oversight on my part.

defer.returnValue((200, res))

if from_token:
from_token = RelationPaginationToken.from_string(from_token)

Expand Down Expand Up @@ -224,7 +230,7 @@ def on_GET(self, request, room_id, parent_id, relation_type=None, event_type=Non

# This checks that a) the event exists and b) the user is allowed to
# view it.
yield self.event_handler.get_event(requester.user, room_id, parent_id)
event = yield self.event_handler.get_event(requester.user, room_id, parent_id)

if relation_type not in (RelationTypes.ANNOTATION, None):
raise SynapseError(400, "Relation type must be 'annotation'")
Expand All @@ -233,6 +239,12 @@ def on_GET(self, request, room_id, parent_id, relation_type=None, event_type=Non
from_token = parse_string(request, "from")
to_token = parse_string(request, "to")

# Check if the event is redacted, and if so return an empty chunk
# list and zero tokens
if "redacted_because" in event.unsigned:
res = {"chunk": []}
anoadragon453 marked this conversation as resolved.
Show resolved Hide resolved
defer.returnValue((200, res))

if from_token:
from_token = AggregationPaginationToken.from_string(from_token)

Expand Down
116 changes: 113 additions & 3 deletions tests/rest/client/v2_alpha/test_relations.py
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ def test_deny_membership(self):
def test_deny_double_react(self):
"""Test that we deny relations on membership events
"""
channel = self._send_relation(RelationTypes.ANNOTATION, "m.reaction", "a")
channel = self._send_relation(RelationTypes.ANNOTATION, "m.reaction", key="a")
self.assertEquals(200, channel.code, channel.json_body)

channel = self._send_relation(RelationTypes.ANNOTATION, "m.reaction", "a")
Expand Down Expand Up @@ -540,14 +540,122 @@ def test_multi_edit(self):
{"event_id": edit_event_id, "sender": self.user_id}, m_replace_dict
)

def test_relations_redaction_redacts_edits(self):
"""Test that edits of an event are redacted when the original event
is redacted.
"""
# Send a new event
res = self.helper.send(self.room, body="Heyo!", tok=self.user_token)
original_event_id = res["event_id"]

# Add a relation
channel = self._send_relation(
RelationTypes.REPLACE,
"m.room.message",
parent_id=original_event_id,
content={
"msgtype": "m.text",
"body": "Wibble",
"m.new_content": {"msgtype": "m.text", "body": "First edit"},
},
)
self.assertEquals(200, channel.code, channel.json_body)

# Check the relation is returned
request, channel = self.make_request(
"GET",
"/_matrix/client/unstable/rooms/%s/relations/%s/m.replace/m.room.message"
% (self.room, original_event_id),
access_token=self.user_token,
)
self.render(request)
self.assertEquals(200, channel.code, channel.json_body)

self.assertIn("chunk", channel.json_body)
self.assertEquals(len(channel.json_body["chunk"]), 1)

# Redact the original event
request, channel = self.make_request(
"PUT",
"/rooms/%s/redact/%s/%s"
% (self.room, original_event_id, "test_relations_redaction_redacts_edits"),
access_token=self.user_token,
content="{}",
)
self.render(request)
self.assertEquals(200, channel.code, channel.json_body)

# Try to check for remaining m.replace relations
request, channel = self.make_request(
"GET",
"/_matrix/client/unstable/rooms/%s/relations/%s/m.replace/m.room.message"
% (self.room, original_event_id),
access_token=self.user_token,
)
self.render(request)
self.assertEquals(200, channel.code, channel.json_body)

# Check that no relations are returned
self.assertIn("chunk", channel.json_body)
self.assertEquals(channel.json_body["chunk"], [])

def test_aggregations_redaction_prevents_access_to_aggregations(self):
"""Test that annotations of an event are redacted when the original event
is redacted.
"""
# Send a new event
res = self.helper.send(self.room, body="Hello!", tok=self.user_token)
original_event_id = res["event_id"]

# Add a relation
channel = self._send_relation(
RelationTypes.ANNOTATION, "m.reaction", key="👍", parent_id=original_event_id
)
self.assertEquals(200, channel.code, channel.json_body)

# Redact the original
request, channel = self.make_request(
"PUT",
"/rooms/%s/redact/%s/%s"
% (
self.room,
original_event_id,
"test_aggregations_redaction_prevents_access_to_aggregations",
),
access_token=self.user_token,
content="{}",
)
self.render(request)
self.assertEquals(200, channel.code, channel.json_body)

# Check that aggregations returns zero
request, channel = self.make_request(
"GET",
"/_matrix/client/unstable/rooms/%s/aggregations/%s/m.annotation/m.reaction"
% (self.room, original_event_id),
access_token=self.user_token,
)
self.render(request)
self.assertEquals(200, channel.code, channel.json_body)

self.assertIn("chunk", channel.json_body)
self.assertEquals(channel.json_body["chunk"], [])

def _send_relation(
self, relation_type, event_type, key=None, content={}, access_token=None
self,
relation_type,
event_type,
key=None,
content={},
access_token=None,
parent_id=None,
):
"""Helper function to send a relation pointing at `self.parent_id`

Args:
relation_type (str): One of `RelationTypes`
event_type (str): The type of the event to create
parent_id (str): The event_id this relation relates to. If None, then self.parent_id
key (str|None): The aggregation key used for m.annotation relation
type.
content(dict|None): The content of the created event.
Expand All @@ -564,10 +672,12 @@ def _send_relation(
if key:
query = "?key=" + six.moves.urllib.parse.quote_plus(key.encode("utf-8"))

original_id = parent_id if parent_id else self.parent_id

request, channel = self.make_request(
"POST",
"/_matrix/client/unstable/rooms/%s/send_relation/%s/%s/%s%s"
% (self.room, self.parent_id, relation_type, event_type, query),
% (self.room, original_id, relation_type, event_type, query),
json.dumps(content).encode("utf-8"),
access_token=access_token,
)
Expand Down