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

add etag and count to key backup endpoints #5858

Merged
merged 22 commits into from
Nov 27, 2019
Merged

add etag and count to key backup endpoints #5858

merged 22 commits into from
Nov 27, 2019

Conversation

uhoreg
Copy link
Member

@uhoreg uhoreg commented Aug 15, 2019 •
edited
Loading

These two fields were proposed by the Riot mobile team to help clients guess if the backup has new keys since the client last uploaded keys. They are described in the MSC. There are some minor clarifications that need to be made to the MSC, but other than that, I think this is ready.

(Personally, I'm not too fond of calling the one field hash, since it isn't actually a hash, but I'm not bothered enough by it to try to change it at this point.)

@uhoreg
Copy link
Member Author

uhoreg commented Aug 16, 2019

(The sytest failures are due to #5857 not being merged yet)

uhoreg
uhoreg commented Aug 16, 2019
View reviewed changes
synapse/handlers/e2e_room_keys.py
for room_id, room in iteritems(room_keys["rooms"]):
for session_id, session in iteritems(room["sessions"]):
Copy link
Member Author

@uhoreg uhoreg Aug 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

FWIW, the diff gets hard to follow in this section, and it's much easier to just look at the file for this section.

@uhoreg uhoreg requested a review from a team August 16, 2019 04:00
richvdh
richvdh suggested changes Aug 16, 2019
View reviewed changes
changelog.d/5858.feature Outdated Show resolved Hide resolved
synapse/handlers/e2e_room_keys.py Outdated Show resolved Hide resolved
synapse/handlers/e2e_room_keys.py Show resolved Hide resolved
synapse/storage/e2e_room_keys.py Outdated Show resolved Hide resolved
synapse/storage/e2e_room_keys.py Outdated
)
result["auth_data"] = json.loads(result["auth_data"])
result["version"] = str(result["version"])
if not result["hash"]:
result["hash"] = 0
Copy link
Member

Choose a reason for hiding this comment

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

why 0? seems an odd choice when it will normally be a str

Copy link
Member Author

Choose a reason for hiding this comment

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

It's stored as a string because that's what the CS API expects (it's an opaque string), but it's always an int.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed the DB to store an int, and have the endpoints convert to string.

synapse/storage/e2e_room_keys.py Outdated Show resolved Hide resolved
synapse/storage/e2e_room_keys.py Outdated Show resolved Hide resolved
synapse/storage/e2e_room_keys.py Outdated Show resolved Hide resolved
synapse/storage/e2e_room_keys.py Outdated Show resolved Hide resolved
synapse/handlers/e2e_room_keys.py Outdated
pass
else:
raise
version_hash = int(version_info["hash"])
Copy link
Member

Choose a reason for hiding this comment

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

(Personally, I'm not too fond of calling the one field hash, since it isn't actually a hash, but I'm not bothered enough by it to try to change it at this point.)

oh, ugh. This is awful. I stared at the diff for ages trying to see where the hashing was happening.

What makes it so hard to change its name?

Copy link
Member Author

Choose a reason for hiding this comment

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

What makes it so hard to change its name?

Just that the Riot mobile apps have already implemented using that name, for quite a while. But given that it hasn't been implemented server-side, I guess changing it wouldn't break it any further. And also that if I don't know what a better name would be. :P

@manuroe would you mind if we changed the name of the "hash" field to something different?

Copy link
Member Author

Choose a reason for hiding this comment

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

changed to "etag", as that's what the MSC says now

uhoreg
uhoreg commented Aug 31, 2019
View reviewed changes
synapse/storage/e2e_room_keys.py Outdated Show resolved Hide resolved
synapse/storage/schema/delta/56/room_key_hash.sql Outdated Show resolved Hide resolved
tests/handlers/test_e2e_room_keys.py Outdated Show resolved Hide resolved
@uhoreg
Copy link
Member Author

uhoreg commented Aug 31, 2019

The mypy buildkite failure looks weird, and I'm guessing it's either not-my-fault, or something that will be fixed when I merge in develop (after fixing the merge conflicts).

@uhoreg uhoreg mentioned this pull request Oct 29, 2019
Closed
@uhoreg uhoreg changed the title add hash and count to key backup endpoints add etag and count to key backup endpoints Nov 6, 2019
@uhoreg
update the changelog too
00fa0c9
@uhoreg
fix unit test to use new API
8721067 
and improve docstring
@uhoreg
PostgreSQL doesn't like INTEGER UNSIGNED
a025746
@uhoreg
drop unsigned
3991e78 
because it looks like PostgreSQL wants the data type to be one word
@uhoreg uhoreg requested a review from richvdh November 7, 2019 03:27
richvdh
richvdh approved these changes Nov 25, 2019
View reviewed changes
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

I think this is largely plausible. I've got a few nits, mostly about docstrings; suggest you fix them and then (squash-)merge.

synapse/storage/data_stores/main/e2e_room_keys.py Outdated
specific key.

Args:
user_id(str): the user whose backup we're querying
Copy link
Member

Choose a reason for hiding this comment

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

largely for future reference: by convention, there is a space between the param name and the type, though annoyingly the rest of this file doesn't seem to have got the memo :/

synapse/storage/data_stores/main/e2e_room_keys.py Outdated
Args:
user_id(str): the user whose backup we're querying
version(str): the version ID of the backup we're querying about
room_keys(dict[dict[iterable[str]]]): a map of room IDs to dict which
Copy link
Member

Choose a reason for hiding this comment

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

the type syntax includes the key type: dict[str, dict[str, iterable[str]]]

We can maybe simplify the description:

            room_keys (dict[str, dict[str, iterable[str]]]): 
                map from roomID -> {"session": [session ids]}

synapse/storage/data_stores/main/e2e_room_keys.py Outdated
want to query

Returns:
Deferred[dict[dict[dict]]]: a map of room IDs to session IDs to room key
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Deferred[dict[dict[dict]]]: a map of room IDs to session IDs to room key
Deferred[dict[str, dict[str, dict]]]: a map of room IDs to session IDs to room key

synapse/storage/data_stores/main/e2e_room_keys.py Outdated
)
result["auth_data"] = json.loads(result["auth_data"])
result["version"] = str(result["version"])
if not result["etag"]:
Copy link
Member

Choose a reason for hiding this comment

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

an explict if result["etag"] is None would make the intention here clearer, though in practice it makes little difference.

synapse/storage/data_stores/main/e2e_room_keys.py Outdated
"""Update a given backup version

Args:
user_id(str): the user whose backup version we're updating
version(str): the version ID of the backup version we're updating
info(dict): the new backup version info to store
version_etag(int): tag of the keys in the backup
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
version_etag(int): tag of the keys in the backup
version_etag (Optional[int]): tag of the keys in the backup

Copy link
Member

Choose a reason for hiding this comment

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

what does it mean for this to be None?

synapse/storage/data_stores/main/e2e_room_keys.py
@@ -288,21 +370,31 @@ def _create_e2e_room_keys_version_txn(txn):
)

@trace
def update_e2e_room_keys_version(self, user_id, version, info):
def update_e2e_room_keys_version(
self, user_id, version, info=None, version_etag=None
Copy link
Member

Choose a reason for hiding this comment

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

if info is now allowed to be None, please update the type in docstring accordingly. what happens if it's not set?

synapse/storage/data_stores/main/e2e_room_keys.py Outdated
)
if info and "auth_data" in info:
updatevalues["auth_data"] = json.dumps(info["auth_data"])
if version_etag:
Copy link
Member

Choose a reason for hiding this comment

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

surely it's not correct to handle zero differently to other ints?

Suggested change
if version_etag:
if version_etag is not None:

synapse/storage/data_stores/main/e2e_room_keys.py
user_id(str): the user whose backup we're adding to
version(str): the version ID of the backup for the set of keys we're adding to
room_keys(iterable[(str, str, dict)]): the keys to add, in the form
(roomID, sessionID, keyData)
Copy link
Member

Choose a reason for hiding this comment

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

I don't love the fact that we have a storage-layer method making undocumented assumptions about the shape of the key data dict, but 🤷‍♂️

@uhoreg uhoreg merged commit 0d27aba into develop Nov 27, 2019
neilisfragile added a commit that referenced this pull request Dec 9, 2019
@neilisfragile
Merge tag 'v1.7.0rc1' into develop
0f3614f 
Synapse 1.7.0rc1 (2019-12-09)
=============================

Features
--------

- Implement per-room message retention policies. ([\#5815](#5815), [\#6436](#6436))
- Add etag and count fields to key backup endpoints to help clients guess if there are new keys. ([\#5858](#5858))
- Add `/admin/v2/users` endpoint with pagination. Contributed by Awesome Technologies Innovationslabor GmbH. ([\#5925](#5925))
- Require User-Interactive Authentication for `/account/3pid/add`, meaning the user's password will be required to add a third-party ID to their account. ([\#6119](#6119))
- Implement the `/_matrix/federation/unstable/net.atleastfornow/state/<context>` API as drafted in MSC2314. ([\#6176](#6176))
- Configure privacy-preserving settings by default for the room directory. ([\#6354](#6354))
- Add ephemeral messages support by partially implementing [MSC2228](matrix-org/matrix-spec-proposals#2228). ([\#6409](#6409))
- Add support for [MSC 2367](matrix-org/matrix-spec-proposals#2367), which allows specifying a reason on all membership events. ([\#6434](#6434))

Bugfixes
--------

- Transfer non-standard power levels on room upgrade. ([\#6237](#6237))
- Fix error from the Pillow library when uploading RGBA images. ([\#6241](#6241))
- Correctly apply the event filter to the `state`, `events_before` and `events_after` fields in the response to `/context` requests. ([\#6329](#6329))
- Fix caching devices for remote users when using workers, so that we don't attempt to refetch (and potentially fail) each time a user requests devices. ([\#6332](#6332))
- Prevent account data syncs getting lost across TCP replication. ([\#6333](#6333))
- Fix bug: TypeError in `register_user()` while using LDAP auth module. ([\#6406](#6406))
- Fix an intermittent exception when handling read-receipts. ([\#6408](#6408))
- Fix broken guest registration when there are existing blocks of numeric user IDs. ([\#6420](#6420))
- Fix startup error when http proxy is defined. ([\#6421](#6421))
- Fix error when using synapse_port_db on a vanilla synapse db. ([\#6449](#6449))
- Fix uploading multiple cross signing signatures for the same user. ([\#6451](#6451))
- Fix bug which lead to exceptions being thrown in a loop when a cross-signed device is deleted. ([\#6462](#6462))
- Fix `synapse_port_db` not exiting with a 0 code if something went wrong during the port process. ([\#6470](#6470))
- Improve sanity-checking when receiving events over federation. ([\#6472](#6472))
- Fix inaccurate per-block Prometheus metrics. ([\#6491](#6491))
- Fix small performance regression for sending invites. ([\#6493](#6493))
- Back out cross-signing code added in Synapse 1.5.0, which caused a performance regression. ([\#6494](#6494))

Improved Documentation
----------------------

- Update documentation and variables in user contributed systemd reference file. ([\#6369](#6369), [\#6490](#6490))
- Fix link in the user directory documentation. ([\#6388](#6388))
- Add build instructions to the docker readme. ([\#6390](#6390))
- Switch Ubuntu package install recommendation to use python3 packages in INSTALL.md. ([\#6443](#6443))
- Write some docs for the quarantine_media api. ([\#6458](#6458))
- Convert CONTRIBUTING.rst to markdown (among other small fixes). ([\#6461](#6461))

Deprecations and Removals
-------------------------

- Remove admin/v1/users_paginate endpoint. Contributed by Awesome Technologies Innovationslabor GmbH. ([\#5925](#5925))
- Remove fallback for federation with old servers which lack the /federation/v1/state_ids API. ([\#6488](#6488))

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

- Add benchmarks for structured logging and improve output performance. ([\#6266](#6266))
- Improve the performance of outputting structured logging. ([\#6322](#6322))
- Refactor some code in the event authentication path for clarity. ([\#6343](#6343), [\#6468](#6468), [\#6480](#6480))
- Clean up some unnecessary quotation marks around the codebase. ([\#6362](#6362))
- Complain on startup instead of 500'ing during runtime when `public_baseurl` isn't set when necessary. ([\#6379](#6379))
- Add a test scenario to make sure room history purges don't break `/messages` in the future. ([\#6392](#6392))
- Clarifications for the email configuration settings. ([\#6423](#6423))
- Add more tests to the blacklist when running in worker mode. ([\#6429](#6429))
- Refactor data store layer to support multiple databases in the future. ([\#6454](#6454), [\#6464](#6464), [\#6469](#6469), [\#6487](#6487))
- Port synapse.rest.client.v1 to async/await. ([\#6482](#6482))
- Port synapse.rest.client.v2_alpha to async/await. ([\#6483](#6483))
- Port SyncHandler to async/await. ([\#6484](#6484))
erikjohnston added a commit that referenced this pull request Dec 13, 2019
@erikjohnston
Merge tag 'v1.7.0'
bee1982 
Synapse 1.7.0 (2019-12-13)
==========================

This release changes the default settings so that only local authenticated users can query the server's room directory. See the [upgrade notes](UPGRADE.rst#upgrading-to-v170) for details.

Support for SQLite versions before 3.11 is now deprecated. A future release will refuse to start if used with an SQLite version before 3.11.

Administrators are reminded that SQLite should not be used for production instances. Instructions for migrating to Postgres are available [here](docs/postgres.md). A future release of synapse will, by default, disable federation for servers using SQLite.

No significant changes since 1.7.0rc2.

Synapse 1.7.0rc2 (2019-12-11)
=============================

Bugfixes
--------

- Fix incorrect error message for invalid requests when setting user's avatar URL. ([\#6497](#6497))
- Fix support for SQLite 3.7. ([\#6499](#6499))
- Fix regression where sending email push would not work when using a pusher worker. ([\#6507](#6507), [\#6509](#6509))

Synapse 1.7.0rc1 (2019-12-09)
=============================

Features
--------

- Implement per-room message retention policies. ([\#5815](#5815), [\#6436](#6436))
- Add etag and count fields to key backup endpoints to help clients guess if there are new keys. ([\#5858](#5858))
- Add `/admin/v2/users` endpoint with pagination. Contributed by Awesome Technologies Innovationslabor GmbH. ([\#5925](#5925))
- Require User-Interactive Authentication for `/account/3pid/add`, meaning the user's password will be required to add a third-party ID to their account. ([\#6119](#6119))
- Implement the `/_matrix/federation/unstable/net.atleastfornow/state/<context>` API as drafted in MSC2314. ([\#6176](#6176))
- Configure privacy-preserving settings by default for the room directory. ([\#6355](#6355))
- Add ephemeral messages support by partially implementing [MSC2228](matrix-org/matrix-spec-proposals#2228). ([\#6409](#6409))
- Add support for [MSC 2367](matrix-org/matrix-spec-proposals#2367), which allows specifying a reason on all membership events. ([\#6434](#6434))

Bugfixes
--------

- Transfer non-standard power levels on room upgrade. ([\#6237](#6237))
- Fix error from the Pillow library when uploading RGBA images. ([\#6241](#6241))
- Correctly apply the event filter to the `state`, `events_before` and `events_after` fields in the response to `/context` requests. ([\#6329](#6329))
- Fix caching devices for remote users when using workers, so that we don't attempt to refetch (and potentially fail) each time a user requests devices. ([\#6332](#6332))
- Prevent account data syncs getting lost across TCP replication. ([\#6333](#6333))
- Fix bug: TypeError in `register_user()` while using LDAP auth module. ([\#6406](#6406))
- Fix an intermittent exception when handling read-receipts. ([\#6408](#6408))
- Fix broken guest registration when there are existing blocks of numeric user IDs. ([\#6420](#6420))
- Fix startup error when http proxy is defined. ([\#6421](#6421))
- Fix error when using synapse_port_db on a vanilla synapse db. ([\#6449](#6449))
- Fix uploading multiple cross signing signatures for the same user. ([\#6451](#6451))
- Fix bug which lead to exceptions being thrown in a loop when a cross-signed device is deleted. ([\#6462](#6462))
- Fix `synapse_port_db` not exiting with a 0 code if something went wrong during the port process. ([\#6470](#6470))
- Improve sanity-checking when receiving events over federation. ([\#6472](#6472))
- Fix inaccurate per-block Prometheus metrics. ([\#6491](#6491))
- Fix small performance regression for sending invites. ([\#6493](#6493))
- Back out cross-signing code added in Synapse 1.5.0, which caused a performance regression. ([\#6494](#6494))

Improved Documentation
----------------------

- Update documentation and variables in user contributed systemd reference file. ([\#6369](#6369), [\#6490](#6490))
- Fix link in the user directory documentation. ([\#6388](#6388))
- Add build instructions to the docker readme. ([\#6390](#6390))
- Switch Ubuntu package install recommendation to use python3 packages in INSTALL.md. ([\#6443](#6443))
- Write some docs for the quarantine_media api. ([\#6458](#6458))
- Convert CONTRIBUTING.rst to markdown (among other small fixes). ([\#6461](#6461))

Deprecations and Removals
-------------------------

- Remove admin/v1/users_paginate endpoint. Contributed by Awesome Technologies Innovationslabor GmbH. ([\#5925](#5925))
- Remove fallback for federation with old servers which lack the /federation/v1/state_ids API. ([\#6488](#6488))

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

- Add benchmarks for structured logging and improve output performance. ([\#6266](#6266))
- Improve the performance of outputting structured logging. ([\#6322](#6322))
- Refactor some code in the event authentication path for clarity. ([\#6343](#6343), [\#6468](#6468), [\#6480](#6480))
- Clean up some unnecessary quotation marks around the codebase. ([\#6362](#6362))
- Complain on startup instead of 500'ing during runtime when `public_baseurl` isn't set when necessary. ([\#6379](#6379))
- Add a test scenario to make sure room history purges don't break `/messages` in the future. ([\#6392](#6392))
- Clarifications for the email configuration settings. ([\#6423](#6423))
- Add more tests to the blacklist when running in worker mode. ([\#6429](#6429))
- Refactor data store layer to support multiple databases in the future. ([\#6454](#6454), [\#6464](#6464), [\#6469](#6469), [\#6487](#6487))
- Port synapse.rest.client.v1 to async/await. ([\#6482](#6482))
- Port synapse.rest.client.v2_alpha to async/await. ([\#6483](#6483))
- Port SyncHandler to async/await. ([\#6484](#6484))
babolivier pushed a commit that referenced this pull request Sep 1, 2021
@anoadragon453
add etag and count to key backup endpoints (#5858)
8c493de 
* commit '0d27aba90':
  add etag and count to key backup endpoints (#5858)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@richvdh richvdh richvdh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@uhoreg @richvdh