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

Implement rechecking of redactions for room versions v3 #4499

Merged
merged 15 commits into from Jan 29, 2019

Conversation

Projects
None yet
3 participants
@erikjohnston
Copy link
Member

erikjohnston commented Jan 28, 2019

This starts implementing support for V3 rooms, while keeping V3 disabled

@erikjohnston erikjohnston force-pushed the erikj/redactions_eiah branch from a909470 to 2fbd985 Jan 29, 2019

erikjohnston added some commits Jan 29, 2019

Add RoomVersions.V3 constant, without enabling it
We add the constant, but don't add it to the known room versions. This
lets us start adding V3 logic, but the servers will never join or create
V3 rooms

@erikjohnston erikjohnston force-pushed the erikj/redactions_eiah branch from 2fbd985 to 7709d2b Jan 29, 2019

@erikjohnston erikjohnston changed the title [WIP] Implement rechecking of redactions for room versions v3 Implement rechecking of redactions for room versions v3 Jan 29, 2019

@erikjohnston erikjohnston requested a review from matrix-org/synapse-core Jan 29, 2019

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 29, 2019

Codecov Report

Merging #4499 into develop will decrease coverage by 0.03%.
The diff coverage is 52.94%.

@@             Coverage Diff             @@
##           develop    #4499      +/-   ##
===========================================
- Coverage    74.71%   74.67%   -0.04%     
===========================================
  Files          336      336              
  Lines        34258    34280      +22     
  Branches      5578     5585       +7     
===========================================
+ Hits         25595    25599       +4     
- Misses        7081     7099      +18     
  Partials      1582     1582
Show resolved Hide resolved synapse/api/constants.py Outdated
@richvdh
Copy link
Member

richvdh left a comment

erm: do we not need to check that the sender of a redaction matches the original sender when we pull the original event out of the database?

redacter_domain = get_domain_from_id(event.event_id)
redactee_domain = get_domain_from_id(event.redacts)
if redacter_domain == redactee_domain:
if room_version in (RoomVersions.V1, RoomVersions.V2, RoomVersions.VDH_TEST):

This comment has been minimized.

@richvdh

richvdh Jan 29, 2019

Member

can't we just drop VDH_TEST ?

@@ -63,6 +63,9 @@ def get_send_on_behalf_of(self):
"""
return getattr(self, "send_on_behalf_of", None)

def need_to_check_redaction(self):

This comment has been minimized.

@richvdh

richvdh Jan 29, 2019

Member

can has docstring please - what does this mean?

Show resolved Hide resolved synapse/storage/events_worker.py Outdated
Update synapse/storage/events_worker.py
Co-Authored-By: erikjohnston <erikj@jki.re>
@erikjohnston

This comment has been minimized.

Copy link
Member Author

erikjohnston commented Jan 29, 2019

erm: do we not need to check that the sender of a redaction matches the original sender when we pull the original event out of the database?

Argh, bollocks

erikjohnston added some commits Jan 29, 2019

@erikjohnston erikjohnston requested a review from matrix-org/synapse-core Jan 29, 2019

@erikjohnston

This comment has been minimized.

Copy link
Member Author

erikjohnston commented Jan 29, 2019

sytest looks to be unhappy that i killed off VDH_TEST

@richvdh
Copy link
Member

richvdh left a comment

lgtm

Starting in room v3 redaction events are accepted up front, and later
checked to see if the redacter and redactee's domains match.
If the sender of the redaction event is allowed to redact due to auth

This comment has been minimized.

@richvdh

richvdh Jan 29, 2019

Member

"is allowed to redact any event"

erikjohnston added some commits Jan 29, 2019

@erikjohnston erikjohnston merged commit e12313b into develop Jan 29, 2019

4 of 5 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
ci/circleci: sytestpy2merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2postgresmerged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3postgresmerged Your tests passed on CircleCI!
Details

turt2live added a commit to matrix-org/matrix-doc that referenced this pull request Jan 31, 2019

Add specification for room version 3: Event IDs as hashes
Original proposal: #1659
Implementation proofs (some traversing of the PR tree may be required to get all of them):
* matrix-org/synapse#4483
* matrix-org/synapse#4499

This doesn't intentionally change anything from the proposal.

**Implementation details**:

The simple part of this is the introduction of a rooms/v3.html document. The somewhat unclear part is the stuff done to the s2s definitions. This pulls `unsigned_pdu` out to `unsigned_pdu_base` (all fields except `event_id`) where it can be reused in `pdu` and `pdu_v3` (for rooms v3). These definitions are further moved into the room version specifications where they can highlight the exact schemas in detail.

Version 1 has been updated to include the pre-existing event format, however the core principles of the room have not been changed. The same applies to room version 2. Room versions have immutable core principles once in the spec, otherwise these format changes would land in a pre-existing version.

The client-server API event formats will need updating, however that is being punted to a different commit to try and keep these changes reviewable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment