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

E2E backups #4019

Merged
merged 47 commits into from Oct 15, 2018

Conversation

6 participants
@dbkr
Member

dbkr commented Oct 8, 2018

Supersedes #3757 and #2731

Note that most of this has already been reviewed when it was in 2731 (unsure if it would be easier or more confusing to merge this branch over to that one and re-open that PR).

This is MSC1687: https://github.com/uhoreg/matrix-doc/blob/e2e_backup/proposals/1219-storing-megolm-keys-serverside.md

Sytests: matrix-org/sytest#503

dbkr added some commits Oct 9, 2018

@dbkr dbkr requested a review from matrix-org/synapse-core Oct 9, 2018

@dbkr dbkr referenced this pull request Oct 9, 2018

Closed

E2e backups #3757

@uhoreg

This comment has been minimized.

Member

uhoreg commented Oct 10, 2018

I took a quick look at @dbkr's changes, and they look fine to me. It would be great to get a full review from a Synapse dev.

@manuroe manuroe referenced this pull request Oct 11, 2018

Open

Implement E2E key backup #2070

3 of 5 tasks complete

@erikjohnston erikjohnston self-assigned this Oct 11, 2018

@erikjohnston

Looks good! Just needs a little bit of sprucing up.

FTR: I've only really been reviewing the code, rather than trying to grok how E2E backups are meant to work as a whole.

For future reference, it'd also be good to avoid using @defer.inlineCallbacks in tests and instead use the test reactor infrastructure (which is also fairly nice). There are a number of advantages to this (Though personally the main one is that if you CTRL^C trial it sometimes completely wedges if using a real reactor)

return exception.error_dict()
else:
logger.error("Unknown exception type: %s", type(exception))
return {}

This comment has been minimized.

@erikjohnston

erikjohnston Oct 12, 2018

Member

This appears unused?

This comment has been minimized.

@dbkr

dbkr Oct 12, 2018

Member

mm, so it does

@@ -0,0 +1,283 @@
# -*- coding: utf-8 -*-
# Copyright 2017 New Vector Ltd

This comment has been minimized.

@erikjohnston
room, or a given session.
See EndToEndRoomKeyStore.get_e2e_room_keys for full details.
Returns:

This comment has been minimized.

@erikjohnston

erikjohnston Oct 12, 2018

Member

Can you also add Args section, giving types (and description if not obvious)

This comment has been minimized.

@erikjohnston

erikjohnston Oct 12, 2018

Member

(And the same for the other functions.)

This comment has been minimized.

@dbkr

dbkr Oct 12, 2018

Member

done

if e.code == 404:
raise SynapseError(404, "Version '%s' not found" % (version,))
else:
raise e

This comment has been minimized.

@erikjohnston

erikjohnston Oct 12, 2018

Member

Please just use raise rather than raise e as you end up losing a bunch of stack traces

This comment has been minimized.

@dbkr

dbkr Oct 12, 2018

Member

ah, good to know

if E2eRoomKeysHandler._should_replace_room_key(current_room_key, room_key):
yield self.store.set_e2e_room_key(
user_id, version, room_id, session_id, room_key
)

This comment has been minimized.

@erikjohnston

erikjohnston Oct 12, 2018

Member

Does this not want to be in the lock?

This comment has been minimized.

@dbkr

dbkr Oct 12, 2018

Member

where by lock you mean try block? I don't think so as the try is just catching 404s from the get.

"""
if version:
raise SynapseError(405, "Cannot POST to a specific version")

This comment has been minimized.

@erikjohnston

erikjohnston Oct 12, 2018

Member

Can we please split this servlet into two: 1) which doesn't accept a version and only works for PUT and 2) which requires a version and only works with GET/DELETE.

This has the advantages of:

  1. Its easier to grasp the API shapes without having to read the code
  2. Don't have to re-implement all the validations/error handling

It does somewhat look like we've forgotten to check that we're actually given a version in GET/DELETE handlers for example

This comment has been minimized.

@dbkr

dbkr Oct 12, 2018

Member

done

if e.code == 404:
e.errcode = Codes.NOT_FOUND
e.msg = "No backup found"
raise e

This comment has been minimized.

@erikjohnston

erikjohnston Oct 12, 2018

Member

Can you make a new error rather than rewriting the existing one please? Otherwise we may end up with bizarre hybrid exceptions :)

This comment has been minimized.

@erikjohnston

erikjohnston Oct 12, 2018

Member

TBH, I'm not even sure we want to bother rewriting anything here. If we do, we should probably do the same for the DELETE handler.

In fact, for consistency sake we should probably make the exceptions returned by the handler on unknown version be consistent, rather than the exception being specific to each REST endpoint.

This comment has been minimized.

@dbkr

dbkr Oct 12, 2018

Member

ah yes :)

# See the License for the specific language governing permissions and
# limitations under the License.
import simplejson as json

This comment has been minimized.

@erikjohnston

erikjohnston Oct 12, 2018

Member

We use just import json nowadays

session_id(str): the session whose room_key we're setting
room_key(dict): the room_key being set
Raises:
StoreError if stuff goes wrong, probably

This comment has been minimized.

@erikjohnston

erikjohnston Oct 12, 2018

Member

I don't think its worth having this comment :)

@@ -0,0 +1,361 @@
# -*- coding: utf-8 -*-
# Copyright 2017 New Vector Ltd

This comment has been minimized.

@erikjohnston

@erikjohnston erikjohnston assigned dbkr and unassigned erikjohnston Oct 12, 2018

@dbkr

Hopefully all addressed now - ptal

return exception.error_dict()
else:
logger.error("Unknown exception type: %s", type(exception))
return {}

This comment has been minimized.

@dbkr

dbkr Oct 12, 2018

Member

mm, so it does

room, or a given session.
See EndToEndRoomKeyStore.get_e2e_room_keys for full details.
Returns:

This comment has been minimized.

@dbkr

dbkr Oct 12, 2018

Member

done

if e.code == 404:
raise SynapseError(404, "Version '%s' not found" % (version,))
else:
raise e

This comment has been minimized.

@dbkr

dbkr Oct 12, 2018

Member

ah, good to know

if E2eRoomKeysHandler._should_replace_room_key(current_room_key, room_key):
yield self.store.set_e2e_room_key(
user_id, version, room_id, session_id, room_key
)

This comment has been minimized.

@dbkr

dbkr Oct 12, 2018

Member

where by lock you mean try block? I don't think so as the try is just catching 404s from the get.

# purely for legibility.
if room_key['is_verified'] and not current_room_key['is_verified']:
pass

This comment has been minimized.

@dbkr

dbkr Oct 12, 2018

Member

mm, agreed

"""
results = yield self.store.get_e2e_room_keys_version_info(user_id, version)
defer.returnValue(results)

This comment has been minimized.

@dbkr

dbkr Oct 12, 2018

Member

mm, I see what you mean

"""
if version:
raise SynapseError(405, "Cannot POST to a specific version")

This comment has been minimized.

@dbkr

dbkr Oct 12, 2018

Member

done

if e.code == 404:
e.errcode = Codes.NOT_FOUND
e.msg = "No backup found"
raise e

This comment has been minimized.

@dbkr

dbkr Oct 12, 2018

Member

ah yes :)

dbkr added some commits Oct 12, 2018

@dbkr dbkr assigned erikjohnston and unassigned dbkr Oct 12, 2018

@neilisfragile neilisfragile moved this from To Do to In Progress: Operational/bug fixes in Backend Core Team Oct 12, 2018

@erikjohnston

Before this goes live we should probably move this functionality into a worker, as it sounds like this API is going to get hit a lot

@erikjohnston erikjohnston assigned dbkr and unassigned erikjohnston Oct 15, 2018

@dbkr dbkr merged commit 03c1103 into develop Oct 15, 2018

6 checks passed

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
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

Backend Core Team automation moved this from In Progress: Operational/bug fixes to Done - Operations Oct 15, 2018

@richvdh

This comment has been minimized.

Member

richvdh commented Oct 17, 2018

@dbkr please please please can we squash-merge things like this in future?

michaelkaye added a commit that referenced this pull request Oct 22, 2018

Merge tag 'v0.33.7' into dinsic
**Warning**: This release removes the example email notification templates from
`res/templates` (they are now internal to the python package). This should only
affect you if you (a) deploy your Synapse instance from a git checkout or a
github snapshot URL, and (b) have email notifications enabled.

If you have email notifications enabled, you should ensure that
`email.template_dir` is either configured to point at a directory where you
have installed customised templates, or leave it unset to use the default
templates.

The configuration parser will try to detect the situation where
`email.template_dir` is incorrectly set to `res/templates` and do the right
thing, but will warn about this.

Features
--------

- Ship the example email templates as part of the package ([\#4052](#4052))
- Add support for end-to-end key backup (MSC1687) ([\#4019](#4019))

Bugfixes
--------

- Fix bug which made get_missing_events return too few events ([\#4045](#4045))
- Fix bug in event persistence logic which caused 'NoneType is not iterable' ([\#3995](#3995))
- Fix exception in background metrics collection ([\#3996](#3996))
- Fix exception handling in fetching remote profiles ([\#3997](#3997))
- Fix handling of rejected threepid invites ([\#3999](#3999))
- Workers now start on Python 3. ([\#4027](#4027))
- Synapse now starts on Python 3.7. ([\#4033](#4033))

Internal Changes
----------------

- Log exceptions in looping calls ([\#4008](#4008))
- Optimisation for serving federation requests ([\#4017](#4017))
- Add metric to count number of non-empty sync responses ([\#4022](#4022))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment