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

handle empty backups according to latest spec proposal #4123

Merged
merged 8 commits into from
Nov 5, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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/4123.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
fix return code of empty key backups
6 changes: 3 additions & 3 deletions synapse/handlers/e2e_room_keys.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,13 +63,13 @@ def get_room_keys(self, user_id, version, room_id=None, session_id=None):
# we deliberately take the lock to get keys so that changing the version
# works atomically
with (yield self._upload_linearizer.queue(user_id)):
# make sure the backup version exists
yield self.store.get_e2e_room_keys_version_info(user_id, version)
Copy link
Member

Choose a reason for hiding this comment

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

does this return a 404 or a 400?


results = yield self.store.get_e2e_room_keys(
user_id, version, room_id, session_id
)

if results['rooms'] == {}:
raise SynapseError(404, "No room_keys found")

defer.returnValue(results)

@defer.inlineCallbacks
Expand Down
10 changes: 8 additions & 2 deletions synapse/rest/client/v2_alpha/room_keys.py
Original file line number Diff line number Diff line change
Expand Up @@ -209,9 +209,15 @@ def on_GET(self, request, room_id, session_id):
)

if session_id:
room_keys = room_keys['rooms'][room_id]['sessions'][session_id]
if room_keys['rooms'] == {}:
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 finding this a bit opaque, and I think it could do with some comments to explain what's going on.

It feels odd to me that, if the room is missing from the backup, we 404 if the session_id is passed, but not if it isn't.

raise SynapseError(404, "No room_keys found", Codes.NOT_FOUND)
Copy link
Member

Choose a reason for hiding this comment

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

NotFoundError is a thing, fwiw

else:
room_keys = room_keys['rooms'][room_id]['sessions'][session_id]
elif room_id:
room_keys = room_keys['rooms'][room_id]
if room_keys['rooms'] == {}:
room_keys = {'sessions': {}}
else:
room_keys = room_keys['rooms'][room_id]

defer.returnValue((200, room_keys))

Expand Down
79 changes: 37 additions & 42 deletions tests/handlers/test_e2e_room_keys.py
Original file line number Diff line number Diff line change
Expand Up @@ -169,8 +169,8 @@ def test_delete_version(self):
self.assertEqual(res, 404)

@defer.inlineCallbacks
def test_get_missing_room_keys(self):
"""Check that we get a 404 on querying missing room_keys
def test_get_missing_backup(self):
"""Check that we get a 404 on querying missing backup
"""
res = None
try:
Expand All @@ -179,19 +179,20 @@ def test_get_missing_room_keys(self):
res = e.code
self.assertEqual(res, 404)

# check we also get a 404 even if the version is valid
@defer.inlineCallbacks
def test_get_missing_room_keys(self):
"""Check we get an empty response from an empty backup
"""
version = yield self.handler.create_version(self.local_user, {
"algorithm": "m.megolm_backup.v1",
"auth_data": "first_version_auth_data",
})
self.assertEqual(version, "1")

res = None
try:
yield self.handler.get_room_keys(self.local_user, version)
except errors.SynapseError as e:
res = e.code
self.assertEqual(res, 404)
res = yield self.handler.get_room_keys(self.local_user, version)
self.assertDictEqual(res, {
"rooms": {}
})

# TODO: test the locking semantics when uploading room_keys,
# although this is probably best done in sytest
Expand Down Expand Up @@ -345,17 +346,15 @@ def test_delete_room_keys(self):
# check for bulk-delete
yield self.handler.upload_room_keys(self.local_user, version, room_keys)
yield self.handler.delete_room_keys(self.local_user, version)
res = None
try:
yield self.handler.get_room_keys(
self.local_user,
version,
room_id="!abc:matrix.org",
session_id="c0ff33",
)
except errors.SynapseError as e:
res = e.code
self.assertEqual(res, 404)
res = yield self.handler.get_room_keys(
self.local_user,
version,
room_id="!abc:matrix.org",
session_id="c0ff33",
)
self.assertDictEqual(res, {
"rooms": {}
})

# check for bulk-delete per room
yield self.handler.upload_room_keys(self.local_user, version, room_keys)
Expand All @@ -364,17 +363,15 @@ def test_delete_room_keys(self):
version,
room_id="!abc:matrix.org",
)
res = None
try:
yield self.handler.get_room_keys(
self.local_user,
version,
room_id="!abc:matrix.org",
session_id="c0ff33",
)
except errors.SynapseError as e:
res = e.code
self.assertEqual(res, 404)
res = yield self.handler.get_room_keys(
self.local_user,
version,
room_id="!abc:matrix.org",
session_id="c0ff33",
)
self.assertDictEqual(res, {
"rooms": {}
})

# check for bulk-delete per session
yield self.handler.upload_room_keys(self.local_user, version, room_keys)
Expand All @@ -384,14 +381,12 @@ def test_delete_room_keys(self):
room_id="!abc:matrix.org",
session_id="c0ff33",
)
res = None
try:
yield self.handler.get_room_keys(
self.local_user,
version,
room_id="!abc:matrix.org",
session_id="c0ff33",
)
except errors.SynapseError as e:
res = e.code
self.assertEqual(res, 404)
res = yield self.handler.get_room_keys(
self.local_user,
version,
room_id="!abc:matrix.org",
session_id="c0ff33",
)
self.assertDictEqual(res, {
"rooms": {}
})