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

anoadragon453 · 2019-07-05T16:58:04Z

Forbid viewing relations on an event once it has been redacted.

codecov · 2019-07-08T16:10:22Z

Codecov Report

Merging #5629 into develop will decrease coverage by 0.07%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    matrix-org/synapse#5629      +/-   ##
===========================================
- Coverage    63.25%   63.18%   -0.08%     
===========================================
  Files          328      328              
  Lines        35877    35951      +74     
  Branches      5915     5919       +4     
===========================================
+ Hits         22695    22715      +20     
- Misses       11555    11608      +53     
- Partials      1627     1628       +1

codecov · 2019-07-08T16:10:23Z

Codecov Report

Merging #5629 into develop will decrease coverage by 0.01%.
The diff coverage is 82.6%.

@@             Coverage Diff             @@
##           develop    matrix-org/synapse#5629      +/-   ##
===========================================
- Coverage    63.26%   63.24%   -0.02%     
===========================================
  Files          328      328              
  Lines        35850    35858       +8     
  Branches      5908     5910       +2     
===========================================
  Hits         22679    22679              
- Misses       11547    11555       +8     
  Partials      1624     1624

* develop: Update ModuleApi to avoid register(generate_token=True) (#5640) Factor out some redundant code in the login impl (#5639) Move get_or_create_user to test code (#5628) Add a few more common environment directory names to black exclusion (#5630) Add default push rule to ignore reactions (#5623) Add origin_server_ts and sender fields to m.replace (#5613) Remove support for invite_3pid_guest. (#5625) remove dead transaction persist code (#5622) Fixes to the federation rate limiter (#5621)

richvdh

this logic should probably go in the Handler layer rather than the Rest layer (the general idea is that one day we might replace the Rest layer with some other API).

More generally, I'm a bit worried about how this interface will work in practice:

What if there is an event with 100 edits - do we create 101 redaction events?
If we do want to create 101 redaction events, we'll have to do something about the rate-limiter, otherwise we'll send out 5 and then fail.

Is it possible for a server which receives a redaction for the original event to know that should apply to later edits as well?

Basically I think I'd like someone like erik or matthew who have been involved with the design of reactions to comment on whether this is the best approach.

richvdh · 2019-07-09T16:44:38Z

synapse/rest/client/v1/room.py

+        relation_chunk = yield self.store.get_relations_for_event(
+            event_id, relation_type="m.replace", event_type="m.room.message"
+        )
+        relation_chunk_dict = relation_chunk.to_dict()


why do we do this?

richvdh · 2019-07-09T16:46:47Z

synapse/rest/client/v1/room.py

@@ -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(


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

richvdh · 2019-07-09T16:50:24Z

synapse/rest/client/v1/room.py

+        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(


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

richvdh · 2019-07-09T16:53:19Z

synapse/rest/client/v2_alpha/relations.py

@@ -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:


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

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

synapse/tests/storage/test_redaction.py

Line 135 in 32e7c9e

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.

This has now been replaced with internal_metadata.

anoadragon453 · 2019-07-09T17:29:41Z

Basically I think I'd like someone like erik or matthew who have been involved with the design of reactions to comment on whether this is the best approach.

@erikjohnston @ara4n I mentioned this here. Where you want to hash this out is up to you. For reference I suggested in there adding an event that could redact multiple events.

anoadragon453 · 2019-07-10T14:30:58Z

After IRL'ing with @erikjohnston we've decided to just prevent /relations and /aggregations from leaking the relations back to the clients once events have been redacted.

But not actually send out a redaction event per relation.

So the scope of this PR is to leave relations unredacted for now.

* develop: Remove access-token support from RegistrationStore.register (#5642) Don't bundle aggregations when retrieving the original event (#5654) Add a linting script (#5627) Correct pep517 flag in readme (#5651) remove unused and unnecessary check for FederationDeniedError (#5645) Changelog Lint Use application/json when querying the IS's /store-invite endpoint

anoadragon453 · 2019-07-10T15:47:51Z

@erikjohnston Where was the db query you mentioned should also update the internal_metadata's view of whether an event has been redacted?

erikjohnston · 2019-07-12T16:06:20Z

synapse/synapse/events/utils.py

Lines 37 to 57 in 32e7c9e

    
           def prune_event(event): 
        
               """ Returns a pruned version of the given event, which removes all keys we 
        
               don't know about or think could potentially be dodgy. 
        
               This is used when we "redact" an event. We want to remove all fields that 
        
               the user has specified, but we do want to keep necessary information like 
        
               type, state_key etc. 
        
               Args: 
        
                   event (FrozenEvent) 
        
               Returns: 
        
                   FrozenEvent 
        
               """ 
        
               pruned_event_dict = prune_event_dict(event.get_dict()) 
        
               from . import event_type_from_format_version 
        
               return event_type_from_format_version(event.format_version)( 
        
                   pruned_event_dict, event.internal_metadata.get_dict() 
        
               )

I think is probably where you want to register that an event has been redacted.

anoadragon453 · 2019-07-15T10:20:19Z

@erikjohnston Oh, that's where I have it in the PR atm.

richvdh

generally looks sensible but I'm worried that some of this stuff isn't specced :/

synapse/rest/client/v1/room.py

synapse/rest/client/v2_alpha/relations.py

richvdh · 2019-07-17T10:45:06Z

synapse/rest/client/v2_alpha/relations.py

+        # Check if the event is redacted, and if so return an empty chunk
+        # list and zero tokens
+        if event.internal_metadata.is_redacted():
+            res = {"chunk": []}


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.

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

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.

richvdh · 2019-07-17T10:46:23Z

Sends a redaction for each m.replace relation on an event when the original event itself is redacted.

this doesn't seem to be true (any more)?

richvdh

this looks much nicer now.

LGTM modulo the slightly confusing comment. Can you clarify it, then merge?

synapse/rest/client/v2_alpha/relations.py

@skalarproduktraum

v1.2.0rc1 Features -------- - Add support for opentracing. ([\#5544](#5544), [\#5712](#5712)) - Add ability to pull all locally stored events out of synapse that a particular user can see. ([\#5589](#5589)) - Add a basic admin command app to allow server operators to run Synapse admin commands separately from the main production instance. ([\#5597](#5597)) - Add `sender` and `origin_server_ts` fields to `m.replace`. ([\#5613](#5613)) - Add default push rule to ignore reactions. ([\#5623](#5623)) - Include the original event when asking for its relations. ([\#5626](#5626)) - Implement `session_lifetime` configuration option, after which access tokens will expire. ([\#5660](#5660)) - Return "This account has been deactivated" when a deactivated user tries to login. ([\#5674](#5674)) - Enable aggregations support by default ([\#5714](#5714)) Bugfixes -------- - Fix 'utime went backwards' errors on daemonization. ([\#5609](#5609)) - Various minor fixes to the federation request rate limiter. ([\#5621](#5621)) - Forbid viewing relations on an event once it has been redacted. ([\#5629](#5629)) - Fix requests to the `/store_invite` endpoint of identity servers being sent in the wrong format. ([\#5638](#5638)) - Fix newly-registered users not being able to lookup their own profile without joining a room. ([\#5644](#5644)) - Fix bug in #5626 that prevented the original_event field from actually having the contents of the original event in a call to `/relations`. ([\#5654](#5654)) - Fix 3PID bind requests being sent to identity servers as `application/x-form-www-urlencoded` data, which is deprecated. ([\#5658](#5658)) - Fix some problems with authenticating redactions in recent room versions. ([\#5699](#5699), [\#5700](#5700), [\#5707](#5707)) - Ignore redactions of m.room.create events. ([\#5701](#5701)) Updates to the Docker image --------------------------- - Base Docker image on a newer Alpine Linux version (3.8 -> 3.10). ([\#5619](#5619)) - Add missing space in default logging file format generated by the Docker image. ([\#5620](#5620)) Improved Documentation ---------------------- - Add information about nginx normalisation to reverse_proxy.rst. Contributed by @skalarproduktraum - thanks! ([\#5397](#5397)) - --no-pep517 should be --no-use-pep517 in the documentation to setup the development environment. ([\#5651](#5651)) - Improvements to Postgres setup instructions. Contributed by @Lrizika - thanks! ([\#5661](#5661)) - Minor tweaks to postgres documentation. ([\#5675](#5675)) Deprecations and Removals ------------------------- - Remove support for the `invite_3pid_guest` configuration setting. ([\#5625](#5625)) Internal Changes ---------------- - Move logging code out of `synapse.util` and into `synapse.logging`. ([\#5606](#5606), [\#5617](#5617)) - Add a blacklist file to the repo to blacklist certain sytests from failing CI. ([\#5611](#5611)) - Make runtime errors surrounding password reset emails much clearer. ([\#5616](#5616)) - Remove dead code for persiting outgoing federation transactions. ([\#5622](#5622)) - Add `lint.sh` to the scripts-dev folder which will run all linting steps required by CI. ([\#5627](#5627)) - Move RegistrationHandler.get_or_create_user to test code. ([\#5628](#5628)) - Add some more common python virtual-environment paths to the black exclusion list. ([\#5630](#5630)) - Some counter metrics exposed over Prometheus have been renamed, with the old names preserved for backwards compatibility and deprecated. See `docs/metrics-howto.rst` for details. ([\#5636](#5636)) - Unblacklist some user_directory sytests. ([\#5637](#5637)) - Factor out some redundant code in the login implementation. ([\#5639](#5639)) - Update ModuleApi to avoid register(generate_token=True). ([\#5640](#5640)) - Remove access-token support from `RegistrationHandler.register`, and rename it. ([\#5641](#5641)) - Remove access-token support from `RegistrationStore.register`, and rename it. ([\#5642](#5642)) - Improve logging for auto-join when a new user is created. ([\#5643](#5643)) - Remove unused and unnecessary check for FederationDeniedError in _exception_to_failure. ([\#5645](#5645)) - Fix a small typo in a code comment. ([\#5655](#5655)) - Clean up exception handling around client access tokens. ([\#5656](#5656)) - Add a mechanism for per-test homeserver configuration in the unit tests. ([\#5657](#5657)) - Inline issue_access_token. ([\#5659](#5659)) - Update the sytest BuildKite configuration to checkout Synapse in `/src`. ([\#5664](#5664)) - Add a `docker` type to the towncrier configuration. ([\#5673](#5673)) - Convert `synapse.federation.transport.server` to `async`. Might improve some stack traces. ([\#5689](#5689)) - Documentation for opentracing. ([\#5703](#5703))

…acted. (#5629)

anoadragon453 added 5 commits July 5, 2019 17:56

Redact m.replace relations when the original event was redacted.

3e9e100

Add changelog

d6c89b5

lint

f351013

actually unique txnid

0350589

prevent leakage on /aggregations and fix tests

5bc5d60

anoadragon453 added 7 commits July 8, 2019 17:10

lint

7eb0608

lint?

ae919fc

Don't propagate redaction content to relation redactions

498a3bb

go deeper into the dictionary luke

2d4cbc0

Properly fix

c0649be

merge conflicts

06267e0

anoadragon453 requested a review from a team July 9, 2019 14:09

richvdh reviewed Jul 9, 2019

View reviewed changes

anoadragon453 added 4 commits July 10, 2019 16:14

Don't send redactions / relation. Use internal_metadata

fe23482

Set internal_metadata on the correct object

f8e8e21

Update changelog

f119a9d

anoadragon453 changed the title ~~Redact m.replace relations when the original event was redacted.~~ Remove the ability to query relations when the original event was redacted. Jul 10, 2019

anoadragon453 requested a review from a team July 11, 2019 10:11

richvdh requested review from erikjohnston and removed request for a team July 12, 2019 15:54

erikjohnston removed their request for review July 12, 2019 16:06

erikjohnston assigned anoadragon453 Jul 12, 2019

Improve spacing

edb36b5

anoadragon453 requested a review from a team July 15, 2019 10:21

anoadragon453 removed their assignment Jul 15, 2019

richvdh suggested changes Jul 17, 2019

View reviewed changes

anoadragon453 added 5 commits July 18, 2019 11:45

Prevent aggregation on any redacted event and small fixes

432c041

Use PaginationChunk instead of hand-crafting it

509a612

Fix /aggregations logic

036ab09

Missed a word

4367853

Use correct tokens

72a9675

anoadragon453 requested a review from a team July 18, 2019 11:40

richvdh approved these changes Jul 18, 2019

View reviewed changes

synapse/rest/client/v2_alpha/relations.py Outdated Show resolved Hide resolved

Simplify redaction comments and logic

b335613

anoadragon453 merged commit b2a382e into develop Jul 18, 2019

anoadragon453 deleted the anoa/edit_redacting branch July 18, 2019 13:41

bwindels mentioned this pull request Jul 30, 2019

MSC1849: Proposal for m.relates_to aggregations matrix-org/matrix-spec-proposals#1849

Closed

anoadragon453 added a commit that referenced this pull request Feb 19, 2020

Remove the ability to query relations when the original event was red…

005c265

…acted. (#5629)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

anoadragon453 commented Jul 5, 2019 •

edited

Loading

codecov bot commented Jul 8, 2019

codecov bot commented Jul 8, 2019 •

edited

Loading

richvdh left a comment

richvdh Jul 9, 2019

richvdh Jul 9, 2019

richvdh Jul 9, 2019

richvdh Jul 9, 2019

anoadragon453 Jul 9, 2019

anoadragon453 Jul 10, 2019

anoadragon453 commented Jul 9, 2019

anoadragon453 commented Jul 10, 2019

anoadragon453 commented Jul 10, 2019

erikjohnston commented Jul 12, 2019

anoadragon453 commented Jul 15, 2019

richvdh left a comment

richvdh Jul 17, 2019

anoadragon453 Jul 18, 2019

anoadragon453 Jul 18, 2019

richvdh commented Jul 17, 2019

richvdh left a comment

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

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

Conversation

anoadragon453 commented Jul 5, 2019 • edited Loading

codecov bot commented Jul 8, 2019

Codecov Report

codecov bot commented Jul 8, 2019 • edited Loading

Codecov Report

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anoadragon453 commented Jul 9, 2019

anoadragon453 commented Jul 10, 2019

anoadragon453 commented Jul 10, 2019

erikjohnston commented Jul 12, 2019

anoadragon453 commented Jul 15, 2019

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh commented Jul 17, 2019

richvdh left a comment

Choose a reason for hiding this comment

anoadragon453 commented Jul 5, 2019 •

edited

Loading

codecov bot commented Jul 8, 2019 •

edited

Loading