From 099829d5a95b913c47634d13391d6c9f200f0bde Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <richard@matrix.org>
Date: Mon, 1 Apr 2019 12:28:40 +0100
Subject: [PATCH 1/2] use attr.s for VerifyKeyRequest

because namedtuple is awful
---
 changelog.d/5296.misc     |  1 +
 synapse/crypto/keyring.py | 38 +++++++++++++++++++++-----------------
 2 files changed, 22 insertions(+), 17 deletions(-)
 create mode 100644 changelog.d/5296.misc

diff --git a/changelog.d/5296.misc b/changelog.d/5296.misc
new file mode 100644
index 000000000000..a038a6f7f64b
--- /dev/null
+++ b/changelog.d/5296.misc
@@ -0,0 +1 @@
+Refactor keyring.VerifyKeyRequest to use attr.s.
diff --git a/synapse/crypto/keyring.py b/synapse/crypto/keyring.py
index c63f106cf366..e1e026214f01 100644
--- a/synapse/crypto/keyring.py
+++ b/synapse/crypto/keyring.py
@@ -15,12 +15,12 @@
 # limitations under the License.
 
 import logging
-from collections import namedtuple
 
 import six
 from six import raise_from
 from six.moves import urllib
 
+import attr
 from signedjson.key import (
     decode_verify_key_bytes,
     encode_verify_key_base64,
@@ -57,22 +57,26 @@
 logger = logging.getLogger(__name__)
 
 
-VerifyKeyRequest = namedtuple(
-    "VerifyRequest", ("server_name", "key_ids", "json_object", "deferred")
-)
-"""
-A request for a verify key to verify a JSON object.
-
-Attributes:
-    server_name(str): The name of the server to verify against.
-    key_ids(set(str)): The set of key_ids to that could be used to verify the
-        JSON object
-    json_object(dict): The JSON object to verify.
-    deferred(Deferred[str, str, nacl.signing.VerifyKey]):
-        A deferred (server_name, key_id, verify_key) tuple that resolves when
-        a verify key has been fetched. The deferreds' callbacks are run with no
-        logcontext.
-"""
+@attr.s(slots=True, cmp=False)
+class VerifyKeyRequest(object):
+    """
+    A request for a verify key to verify a JSON object.
+
+    Attributes:
+        server_name(str): The name of the server to verify against.
+        key_ids(set[str]): The set of key_ids to that could be used to verify the
+            JSON object
+        json_object(dict): The JSON object to verify.
+        deferred(Deferred[str, str, nacl.signing.VerifyKey]):
+            A deferred (server_name, key_id, verify_key) tuple that resolves when
+            a verify key has been fetched. The deferreds' callbacks are run with no
+            logcontext.
+    """
+
+    server_name = attr.ib()
+    key_ids = attr.ib()
+    json_object = attr.ib()
+    deferred = attr.ib()
 
 
 class KeyLookupError(ValueError):

From 8ea2f756a947d668afc9a6b22707c12a29af6be4 Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <richard@matrix.org>
Date: Wed, 29 May 2019 17:21:39 +0100
Subject: [PATCH 2/2] Remove some pointless exception handling

The verify_request deferred already returns a suitable SynapseError, so I don't
really know what we expect to achieve by doing more wrapping, other than log
spam.

Fixes #4278.
---
 changelog.d/5300.bugfix   |  1 +
 synapse/crypto/keyring.py | 33 ++++++++-------------------------
 2 files changed, 9 insertions(+), 25 deletions(-)
 create mode 100644 changelog.d/5300.bugfix

diff --git a/changelog.d/5300.bugfix b/changelog.d/5300.bugfix
new file mode 100644
index 000000000000..049e93cd5a41
--- /dev/null
+++ b/changelog.d/5300.bugfix
@@ -0,0 +1 @@
+Fix noisy 'no key for server' logs.
diff --git a/synapse/crypto/keyring.py b/synapse/crypto/keyring.py
index e1e026214f01..5756478ad7af 100644
--- a/synapse/crypto/keyring.py
+++ b/synapse/crypto/keyring.py
@@ -64,13 +64,19 @@ class VerifyKeyRequest(object):
 
     Attributes:
         server_name(str): The name of the server to verify against.
+
         key_ids(set[str]): The set of key_ids to that could be used to verify the
             JSON object
+
         json_object(dict): The JSON object to verify.
+
         deferred(Deferred[str, str, nacl.signing.VerifyKey]):
             A deferred (server_name, key_id, verify_key) tuple that resolves when
             a verify key has been fetched. The deferreds' callbacks are run with no
             logcontext.
+
+            If we are unable to find a key which satisfies the request, the deferred
+            errbacks with an M_UNAUTHORIZED SynapseError.
     """
 
     server_name = attr.ib()
@@ -771,31 +777,8 @@ def _handle_key_deferred(verify_request):
         SynapseError if there was a problem performing the verification
     """
     server_name = verify_request.server_name
-    try:
-        with PreserveLoggingContext():
-            _, key_id, verify_key = yield verify_request.deferred
-    except KeyLookupError as e:
-        logger.warn(
-            "Failed to download keys for %s: %s %s",
-            server_name,
-            type(e).__name__,
-            str(e),
-        )
-        raise SynapseError(
-            502, "Error downloading keys for %s" % (server_name,), Codes.UNAUTHORIZED
-        )
-    except Exception as e:
-        logger.exception(
-            "Got Exception when downloading keys for %s: %s %s",
-            server_name,
-            type(e).__name__,
-            str(e),
-        )
-        raise SynapseError(
-            401,
-            "No key for %s with id %s" % (server_name, verify_request.key_ids),
-            Codes.UNAUTHORIZED,
-        )
+    with PreserveLoggingContext():
+        _, key_id, verify_key = yield verify_request.deferred
 
     json_object = verify_request.json_object