handle empty backups according to latest spec proposal (#4123)

fixes #4056

4 files changed, 71 insertions, 52 deletions

diff --git a/changelog.d/4123.bugfix b/changelog.d/4123.bugfix
new file mode 100644
index 0000000000..b82bc2aad3
--- /dev/null
+++ b/changelog.d/4123.bugfix
@@ -0,0 +1 @@
+fix return code of empty key backups
diff --git a/synapse/handlers/e2e_room_keys.py b/synapse/handlers/e2e_room_keys.py
index 5edb3cfe04..42b040375f 100644
--- a/synapse/handlers/e2e_room_keys.py
+++ b/synapse/handlers/e2e_room_keys.py
@@ -19,7 +19,7 @@ from six import iteritems
 
 from twisted.internet import defer
 
-from synapse.api.errors import RoomKeysVersionError, StoreError, SynapseError
+from synapse.api.errors import NotFoundError, RoomKeysVersionError, StoreError
 from synapse.util.async_helpers import Linearizer
 
 logger = logging.getLogger(__name__)
@@ -55,6 +55,8 @@ class E2eRoomKeysHandler(object):
             room_id(string): room ID to get keys for, for None to get keys for all rooms
             session_id(string): session ID to get keys for, for None to get keys for all
                 sessions
+        Raises:
+            NotFoundError: if the backup version does not exist
         Returns:
             A deferred list of dicts giving the session_data and message metadata for
             these room keys.
@@ -63,13 +65,19 @@ class E2eRoomKeysHandler(object):
         # 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
+            try:
+                yield self.store.get_e2e_room_keys_version_info(user_id, version)
+            except StoreError as e:
+                if e.code == 404:
+                    raise NotFoundError("Unknown backup version")
+                else:
+                    raise
+
             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
@@ -120,7 +128,7 @@ class E2eRoomKeysHandler(object):
         }
 
         Raises:
-            SynapseError: with code 404 if there are no versions defined
+            NotFoundError: if there are no versions defined
             RoomKeysVersionError: if the uploaded version is not the current version
         """
 
@@ -134,7 +142,7 @@ class E2eRoomKeysHandler(object):
                 version_info = yield self.store.get_e2e_room_keys_version_info(user_id)
             except StoreError as e:
                 if e.code == 404:
-                    raise SynapseError(404, "Version '%s' not found" % (version,))
+                    raise NotFoundError("Version '%s' not found" % (version,))
                 else:
                     raise
 
@@ -148,7 +156,7 @@ class E2eRoomKeysHandler(object):
                     raise RoomKeysVersionError(current_version=version_info['version'])
                 except StoreError as e:
                     if e.code == 404:
-                        raise SynapseError(404, "Version '%s' not found" % (version,))
+                        raise NotFoundError("Version '%s' not found" % (version,))
                     else:
                         raise
 
diff --git a/synapse/rest/client/v2_alpha/room_keys.py b/synapse/rest/client/v2_alpha/room_keys.py
index 45b5817d8b..ab3f1bd21a 100644
--- a/synapse/rest/client/v2_alpha/room_keys.py
+++ b/synapse/rest/client/v2_alpha/room_keys.py
@@ -17,7 +17,7 @@ import logging
 
 from twisted.internet import defer
 
-from synapse.api.errors import Codes, SynapseError
+from synapse.api.errors import Codes, NotFoundError, SynapseError
 from synapse.http.servlet import (
     RestServlet,
     parse_json_object_from_request,
@@ -208,10 +208,25 @@ class RoomKeysServlet(RestServlet):
             user_id, version, room_id, session_id
         )
 
+        # Convert room_keys to the right format to return.
         if session_id:
-            room_keys = room_keys['rooms'][room_id]['sessions'][session_id]
+            # If the client requests a specific session, but that session was
+            # not backed up, then return an M_NOT_FOUND.
+            if room_keys['rooms'] == {}:
+                raise NotFoundError("No room_keys found")
+            else:
+                room_keys = room_keys['rooms'][room_id]['sessions'][session_id]
         elif room_id:
-            room_keys = room_keys['rooms'][room_id]
+            # If the client requests all sessions from a room, but no sessions
+            # are found, then return an empty result rather than an error, so
+            # that clients don't have to handle an error condition, and an
+            # empty result is valid.  (Similarly if the client requests all
+            # sessions from the backup, but in that case, room_keys is already
+            # in the right format, so we don't need to do anything about it.)
+            if room_keys['rooms'] == {}:
+                room_keys = {'sessions': {}}
+            else:
+                room_keys = room_keys['rooms'][room_id]
 
         defer.returnValue((200, room_keys))
 
diff --git a/tests/handlers/test_e2e_room_keys.py b/tests/handlers/test_e2e_room_keys.py
index 9e08eac0a5..c8994f416e 100644
--- a/tests/handlers/test_e2e_room_keys.py
+++ b/tests/handlers/test_e2e_room_keys.py
@@ -169,8 +169,8 @@ class E2eRoomKeysHandlerTestCase(unittest.TestCase):
         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:
@@ -179,19 +179,20 @@ class E2eRoomKeysHandlerTestCase(unittest.TestCase):
             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
@@ -345,17 +346,15 @@ class E2eRoomKeysHandlerTestCase(unittest.TestCase):
         # 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)
@@ -364,17 +363,15 @@ class E2eRoomKeysHandlerTestCase(unittest.TestCase):
             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)
@@ -384,14 +381,12 @@ class E2eRoomKeysHandlerTestCase(unittest.TestCase):
             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": {}
+        })