Repository: matrix-org/synapse

Issue:
logout returns M_UNKNOWN_TOKEN instead of M_MISSING_TOKEN
Using `/_matrix/client/r0/logout` without an access token returns the error

`M_UNKNOWN_TOKEN` `Access Token unknown or expired`

while every other unauthorized request, including `/_matrix/client/r0/logout/all` returns

`M_MISSING_TOKEN` `Missing access token.`

Discussion:

Patch:
diff --git a/changelog.d/5250.misc b/changelog.d/5250.misc
deleted file mode 100644
index 575a299a8..000000000
--- a/changelog.d/5250.misc
+++ /dev/null
@@ -1,1 +0,0 @@
-Simplification to Keyring.wait_for_previous_lookups.
diff --git a/changelog.d/5256.bugfix b/changelog.d/5256.bugfix
new file mode 100644
index 000000000..86316ab5d
--- /dev/null
+++ b/changelog.d/5256.bugfix
@@ -0,0 +1,1 @@
+Show the correct error when logging out and access token is missing.
diff --git a/changelog.d/5257.bugfix b/changelog.d/5257.bugfix
deleted file mode 100644
index 8334af9b9..000000000
--- a/changelog.d/5257.bugfix
+++ /dev/null
@@ -1,1 +0,0 @@
-Fix error code when there is an invalid parameter on /_matrix/client/r0/publicRooms
diff --git a/synapse/crypto/keyring.py b/synapse/crypto/keyring.py
index d6ad7f177..eaf41b983 100644
--- a/synapse/crypto/keyring.py
+++ b/synapse/crypto/keyring.py
@@ -180,7 +180,9 @@ class Keyring(object):
 
             # We want to wait for any previous lookups to complete before
             # proceeding.
-            yield self.wait_for_previous_lookups(server_to_deferred)
+            yield self.wait_for_previous_lookups(
+                [rq.server_name for rq in verify_requests], server_to_deferred
+            )
 
             # Actually start fetching keys.
             self._get_server_verify_keys(verify_requests)
@@ -213,11 +215,12 @@ class Keyring(object):
             logger.exception("Error starting key lookups")
 
     @defer.inlineCallbacks
-    def wait_for_previous_lookups(self, server_to_deferred):
+    def wait_for_previous_lookups(self, server_names, server_to_deferred):
         """Waits for any previous key lookups for the given servers to finish.
 
         Args:
-            server_to_deferred (dict[str, Deferred]): server_name to deferred which gets
+            server_names (list): list of server_names we want to lookup
+            server_to_deferred (dict): server_name to deferred which gets
                 resolved once we've finished looking up keys for that server.
                 The Deferreds should be regular twisted ones which call their
                 callbacks with no logcontext.
@@ -230,7 +233,7 @@ class Keyring(object):
         while True:
             wait_on = [
                 (server_name, self.key_downloads[server_name])
-                for server_name in server_to_deferred.keys()
+                for server_name in server_names
                 if server_name in self.key_downloads
             ]
             if not wait_on:
diff --git a/synapse/http/servlet.py b/synapse/http/servlet.py
index 197c65285..528125e73 100644
--- a/synapse/http/servlet.py
+++ b/synapse/http/servlet.py
@@ -55,7 +55,7 @@ def parse_integer_from_args(args, name, default=None, required=False):
             return int(args[name][0])
         except Exception:
             message = "Query parameter %r must be an integer" % (name,)
-            raise SynapseError(400, message, errcode=Codes.INVALID_PARAM)
+            raise SynapseError(400, message)
     else:
         if required:
             message = "Missing integer query parameter %r" % (name,)
diff --git a/synapse/rest/client/v1/logout.py b/synapse/rest/client/v1/logout.py
index 430c69233..ba20e7503 100644
--- a/synapse/rest/client/v1/logout.py
+++ b/synapse/rest/client/v1/logout.py
@@ -17,8 +17,6 @@ import logging
 
 from twisted.internet import defer
 
-from synapse.api.errors import AuthError
-
 from .base import ClientV1RestServlet, client_path_patterns
 
 logger = logging.getLogger(__name__)
@@ -38,23 +36,16 @@ class LogoutRestServlet(ClientV1RestServlet):
 
     @defer.inlineCallbacks
     def on_POST(self, request):
-        try:
-            requester = yield self.auth.get_user_by_req(request)
-        except AuthError:
-            # this implies the access token has already been deleted.
-            defer.returnValue((401, {
-                "errcode": "M_UNKNOWN_TOKEN",
-                "error": "Access Token unknown or expired"
-            }))
+        requester = yield self.auth.get_user_by_req(request)
+
+        if requester.device_id is None:
+            # the acccess token wasn't associated with a device.
+            # Just delete the access token
+            access_token = self._auth.get_access_token_from_request(request)
+            yield self._auth_handler.delete_access_token(access_token)
         else:
-            if requester.device_id is None:
-                # the acccess token wasn't associated with a device.
-                # Just delete the access token
-                access_token = self._auth.get_access_token_from_request(request)
-                yield self._auth_handler.delete_access_token(access_token)
-            else:
-                yield self._device_handler.delete_device(
-                    requester.user.to_string(), requester.device_id)
+            yield self._device_handler.delete_device(
+                requester.user.to_string(), requester.device_id)
 
         defer.returnValue((200, {}))
 


Test patch:
diff --git a/tests/crypto/test_keyring.py b/tests/crypto/test_keyring.py
index 4fba462d4..de61bad15 100644
--- a/tests/crypto/test_keyring.py
+++ b/tests/crypto/test_keyring.py
@@ -85,7 +85,7 @@ class KeyringTestCase(unittest.HomeserverTestCase):
         # we run the lookup in a logcontext so that the patched inlineCallbacks can check
         # it is doing the right thing with logcontexts.
         wait_1_deferred = run_in_context(
-            kr.wait_for_previous_lookups, {"server1": lookup_1_deferred}
+            kr.wait_for_previous_lookups, ["server1"], {"server1": lookup_1_deferred}
         )
 
         # there were no previous lookups, so the deferred should be ready
@@ -94,7 +94,7 @@ class KeyringTestCase(unittest.HomeserverTestCase):
         # set off another wait. It should block because the first lookup
         # hasn't yet completed.
         wait_2_deferred = run_in_context(
-            kr.wait_for_previous_lookups, {"server1": lookup_2_deferred}
+            kr.wait_for_previous_lookups, ["server1"], {"server1": lookup_2_deferred}
         )
 
         self.assertFalse(wait_2_deferred.called)