From 886e5acc762b879b606773b511ff92345aef14c6 Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Wed, 16 Jan 2019 15:13:07 +0000
Subject: [PATCH 1/7] Store rejected remote invite events as outliers

Currently they're stored as non-outliers even though the server isn't in
the room, which can be problematic in places where the code assumes it
has the state for all non outlier events.

In particular, there is an edge case where persisting the leave event
triggers a state resolution, which requires looking up the room version
from state. Since the server doesn't have the state, this causes an
exception to be thrown.
---
 synapse/federation/federation_client.py | 10 ++++--
 synapse/handlers/federation.py          | 44 +++++++------------------
 synapse/storage/roommember.py           |  5 +--
 3 files changed, 21 insertions(+), 38 deletions(-)

diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py
index d05ed91d64b7..8fa726759e76 100644
--- a/synapse/federation/federation_client.py
+++ b/synapse/federation/federation_client.py
@@ -32,7 +32,6 @@
     HttpResponseException,
     SynapseError,
 )
-from synapse.events import builder
 from synapse.federation.federation_base import FederationBase, event_from_pdu_json
 from synapse.util import logcontext, unwrapFirstError
 from synapse.util.caches.expiringcache import ExpiringCache
@@ -66,6 +65,8 @@ def __init__(self, hs):
         self.state = hs.get_state_handler()
         self.transport_layer = hs.get_federation_transport_client()
 
+        self.event_builder_factory = hs.get_event_builder_factory()
+
         self._get_pdu_cache = ExpiringCache(
             cache_name="get_pdu_cache",
             clock=self._clock,
@@ -571,7 +572,12 @@ def send_request(destination):
             if "prev_state" not in pdu_dict:
                 pdu_dict["prev_state"] = []
 
-            ev = builder.EventBuilder(pdu_dict)
+            # Strip off the fields that we want to clobber.
+            pdu_dict.pop("origin", None)
+            pdu_dict.pop("origin_server_ts", None)
+            pdu_dict.pop("unsigned", None)
+
+            ev = self.event_builder_factory.new(pdu_dict)
 
             defer.returnValue(
                 (destination, ev)
diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py
index a3bb864bb223..70be87cd3dea 100644
--- a/synapse/handlers/federation.py
+++ b/synapse/handlers/federation.py
@@ -43,10 +43,7 @@
     StoreError,
     SynapseError,
 )
-from synapse.crypto.event_signing import (
-    add_hashes_and_signatures,
-    compute_event_signature,
-)
+from synapse.crypto.event_signing import compute_event_signature
 from synapse.events.validator import EventValidator
 from synapse.replication.http.federation import (
     ReplicationCleanRoomRestServlet,
@@ -58,7 +55,6 @@
 from synapse.util import logcontext, unwrapFirstError
 from synapse.util.async_helpers import Linearizer
 from synapse.util.distributor import user_joined_room
-from synapse.util.frozenutils import unfreeze
 from synapse.util.logutils import log_function
 from synapse.util.retryutils import NotRetryingDestination
 from synapse.visibility import filter_events_for_server
@@ -1083,7 +1079,9 @@ def do_invite_join(self, target_hosts, room_id, joinee, content):
         handled_events = set()
 
         try:
-            event = self._sign_event(event)
+            self._sign_event(event)
+            event.internal_metadata.outlier = False
+
             # Try the host we successfully got a response to /make_join/
             # request first.
             try:
@@ -1289,13 +1287,7 @@ def on_invite_request(self, origin, pdu):
         event.internal_metadata.outlier = True
         event.internal_metadata.invite_from_remote = True
 
-        event.signatures.update(
-            compute_event_signature(
-                event,
-                self.hs.hostname,
-                self.hs.config.signing_key[0]
-            )
-        )
+        self._sign_event(event)
 
         context = yield self.state_handler.compute_event_context(event)
         yield self.persist_events_and_notify([(event, context)])
@@ -1313,7 +1305,7 @@ def do_remotely_reject_invite(self, target_hosts, room_id, user_id):
         # Mark as outlier as we don't have any state for this event; we're not
         # even in the room.
         event.internal_metadata.outlier = True
-        event = self._sign_event(event)
+        self._sign_event(event)
 
         # Try the host that we succesfully called /make_leave/ on first for
         # the /send_leave/ request.
@@ -1358,26 +1350,14 @@ def _make_and_verify_event(self, target_hosts, room_id, user_id, membership,
         defer.returnValue((origin, event))
 
     def _sign_event(self, event):
-        event.internal_metadata.outlier = False
-
-        builder = self.event_builder_factory.new(
-            unfreeze(event.get_pdu_json())
-        )
-
-        builder.event_id = self.event_builder_factory.create_event_id()
-        builder.origin = self.hs.hostname
-
-        if not hasattr(event, "signatures"):
-            builder.signatures = {}
-
-        add_hashes_and_signatures(
-            builder,
-            self.hs.hostname,
-            self.hs.config.signing_key[0],
+        event.signatures.update(
+            compute_event_signature(
+                event,
+                self.hs.hostname,
+                self.hs.config.signing_key[0]
+            )
         )
 
-        return builder.build()
-
     @defer.inlineCallbacks
     @log_function
     def on_make_leave_request(self, room_id, user_id):
diff --git a/synapse/storage/roommember.py b/synapse/storage/roommember.py
index 0707f9a86aa0..c7488f425969 100644
--- a/synapse/storage/roommember.py
+++ b/synapse/storage/roommember.py
@@ -591,10 +591,7 @@ def _store_room_members_txn(self, txn, events, backfilled):
             # i.e., its something that has just happened.
             # The only current event that can also be an outlier is if its an
             # invite that has come in across federation.
-            is_new_state = not backfilled and (
-                not event.internal_metadata.is_outlier()
-                or event.internal_metadata.is_invite_from_remote()
-            )
+            is_new_state = not backfilled
             is_mine = self.hs.is_mine_id(event.state_key)
             if is_new_state and is_mine:
                 if event.membership == Membership.INVITE:

From 183738f469b13b62f6c28e8dfe015371a8e938ff Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Wed, 16 Jan 2019 15:25:34 +0000
Subject: [PATCH 2/7] Newsfile

---
 changelog.d/4405.bugfix | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 changelog.d/4405.bugfix

diff --git a/changelog.d/4405.bugfix b/changelog.d/4405.bugfix
new file mode 100644
index 000000000000..974d799b88ad
--- /dev/null
+++ b/changelog.d/4405.bugfix
@@ -0,0 +1 @@
+Fix bug when rejecting remote invites

From 07f62da55ac8903f7ea224255b8defd122724ec4 Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Wed, 23 Jan 2019 19:44:37 +0000
Subject: [PATCH 3/7] Remove unnecessary '_sign_event'

---
 synapse/federation/federation_client.py |  9 ++++++++-
 synapse/handlers/federation.py          | 14 --------------
 2 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py
index 8fa726759e76..f4adcb556d30 100644
--- a/synapse/federation/federation_client.py
+++ b/synapse/federation/federation_client.py
@@ -32,6 +32,7 @@
     HttpResponseException,
     SynapseError,
 )
+from synapse.crypto.event_signing import add_hashes_and_signatures
 from synapse.federation.federation_base import FederationBase, event_from_pdu_json
 from synapse.util import logcontext, unwrapFirstError
 from synapse.util.caches.expiringcache import ExpiringCache
@@ -577,7 +578,13 @@ def send_request(destination):
             pdu_dict.pop("origin_server_ts", None)
             pdu_dict.pop("unsigned", None)
 
-            ev = self.event_builder_factory.new(pdu_dict)
+            builder = self.event_builder_factory.new(pdu_dict)
+            add_hashes_and_signatures(
+                builder,
+                self.hs.hostname,
+                self.hs.config.signing_key[0]
+            )
+            ev = builder.build()
 
             defer.returnValue(
                 (destination, ev)
diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py
index 70be87cd3dea..9a14ba4517ba 100644
--- a/synapse/handlers/federation.py
+++ b/synapse/handlers/federation.py
@@ -43,7 +43,6 @@
     StoreError,
     SynapseError,
 )
-from synapse.crypto.event_signing import compute_event_signature
 from synapse.events.validator import EventValidator
 from synapse.replication.http.federation import (
     ReplicationCleanRoomRestServlet,
@@ -1079,7 +1078,6 @@ def do_invite_join(self, target_hosts, room_id, joinee, content):
         handled_events = set()
 
         try:
-            self._sign_event(event)
             event.internal_metadata.outlier = False
 
             # Try the host we successfully got a response to /make_join/
@@ -1287,8 +1285,6 @@ def on_invite_request(self, origin, pdu):
         event.internal_metadata.outlier = True
         event.internal_metadata.invite_from_remote = True
 
-        self._sign_event(event)
-
         context = yield self.state_handler.compute_event_context(event)
         yield self.persist_events_and_notify([(event, context)])
 
@@ -1305,7 +1301,6 @@ def do_remotely_reject_invite(self, target_hosts, room_id, user_id):
         # Mark as outlier as we don't have any state for this event; we're not
         # even in the room.
         event.internal_metadata.outlier = True
-        self._sign_event(event)
 
         # Try the host that we succesfully called /make_leave/ on first for
         # the /send_leave/ request.
@@ -1349,15 +1344,6 @@ def _make_and_verify_event(self, target_hosts, room_id, user_id, membership,
         assert(event.room_id == room_id)
         defer.returnValue((origin, event))
 
-    def _sign_event(self, event):
-        event.signatures.update(
-            compute_event_signature(
-                event,
-                self.hs.hostname,
-                self.hs.config.signing_key[0]
-            )
-        )
-
     @defer.inlineCallbacks
     @log_function
     def on_make_leave_request(self, room_id, user_id):

From 7c288c22500e2045d36a29c38d2671fad6484e30 Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Wed, 23 Jan 2019 20:05:44 +0000
Subject: [PATCH 4/7] Clarify the invite flows

---
 synapse/events/__init__.py     |  8 ++++++--
 synapse/handlers/federation.py | 12 +++++++++++-
 synapse/storage/roommember.py  | 11 +++++++----
 3 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/synapse/events/__init__.py b/synapse/events/__init__.py
index 84c75495d544..5030636c7e91 100644
--- a/synapse/events/__init__.py
+++ b/synapse/events/__init__.py
@@ -41,8 +41,12 @@ def get_dict(self):
     def is_outlier(self):
         return getattr(self, "outlier", False)
 
-    def is_invite_from_remote(self):
-        return getattr(self, "invite_from_remote", False)
+    def is_new_remote_event(self):
+        """Whether this is a new remote event, like an invite or an invite
+        rejection. This is needed as those events are marked as outliers, but
+        they still need to be processed.
+        """
+        return getattr(self, "new_remote_event", False)
 
     def get_send_on_behalf_of(self):
         """Whether this server should send the event on behalf of another server.
diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py
index 9a14ba4517ba..e017cab777e8 100644
--- a/synapse/handlers/federation.py
+++ b/synapse/handlers/federation.py
@@ -43,6 +43,7 @@
     StoreError,
     SynapseError,
 )
+from synapse.crypto.event_signing import compute_event_signature
 from synapse.events.validator import EventValidator
 from synapse.replication.http.federation import (
     ReplicationCleanRoomRestServlet,
@@ -1283,7 +1284,15 @@ def on_invite_request(self, origin, pdu):
             )
 
         event.internal_metadata.outlier = True
-        event.internal_metadata.invite_from_remote = True
+        event.internal_metadata.new_remote_event = True
+
+        event.signatures.update(
+            compute_event_signature(
+                event,
+                self.hs.hostname,
+                self.hs.config.signing_key[0]
+            )
+        )
 
         context = yield self.state_handler.compute_event_context(event)
         yield self.persist_events_and_notify([(event, context)])
@@ -1301,6 +1310,7 @@ def do_remotely_reject_invite(self, target_hosts, room_id, user_id):
         # Mark as outlier as we don't have any state for this event; we're not
         # even in the room.
         event.internal_metadata.outlier = True
+        event.internal_metadata.new_remote_event = True
 
         # Try the host that we succesfully called /make_leave/ on first for
         # the /send_leave/ request.
diff --git a/synapse/storage/roommember.py b/synapse/storage/roommember.py
index c7488f425969..40b13de80b44 100644
--- a/synapse/storage/roommember.py
+++ b/synapse/storage/roommember.py
@@ -588,10 +588,13 @@ def _store_room_members_txn(self, txn, events, backfilled):
             )
 
             # We update the local_invites table only if the event is "current",
-            # i.e., its something that has just happened.
-            # The only current event that can also be an outlier is if its an
-            # invite that has come in across federation.
-            is_new_state = not backfilled
+            # i.e., its something that has just happened. If the event is an
+            # outlier it is only current if its a "new remote event", like a
+            # remote invite or a rejection of a remote invite.
+            is_new_state = not backfilled and (
+                not event.internal_metadata.is_outlier()
+                or event.internal_metadata.is_new_remote_event()
+            )
             is_mine = self.hs.is_mine_id(event.state_key)
             if is_new_state and is_mine:
                 if event.membership == Membership.INVITE:

From b8082a54451bb4db30e3b2a4d19dc8cb23330eb7 Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Thu, 24 Jan 2019 17:33:19 +0000
Subject: [PATCH 5/7] Use term 'out of band membership' instead

---
 synapse/events/__init__.py     | 9 +++++----
 synapse/handlers/federation.py | 4 ++--
 synapse/storage/roommember.py  | 6 +++---
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/synapse/events/__init__.py b/synapse/events/__init__.py
index 5030636c7e91..48289cad0672 100644
--- a/synapse/events/__init__.py
+++ b/synapse/events/__init__.py
@@ -41,12 +41,13 @@ def get_dict(self):
     def is_outlier(self):
         return getattr(self, "outlier", False)
 
-    def is_new_remote_event(self):
-        """Whether this is a new remote event, like an invite or an invite
+    def is_out_of_band_membership(self):
+        """Whether this is an out of band membership, like an invite or an invite
         rejection. This is needed as those events are marked as outliers, but
-        they still need to be processed.
+        they still need to be processed as if they're new events (e.g. updating
+        invite state in the database, relaying to clients, etc).
         """
-        return getattr(self, "new_remote_event", False)
+        return getattr(self, "out_of_band_membership", False)
 
     def get_send_on_behalf_of(self):
         """Whether this server should send the event on behalf of another server.
diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py
index e017cab777e8..242719b7ce51 100644
--- a/synapse/handlers/federation.py
+++ b/synapse/handlers/federation.py
@@ -1284,7 +1284,7 @@ def on_invite_request(self, origin, pdu):
             )
 
         event.internal_metadata.outlier = True
-        event.internal_metadata.new_remote_event = True
+        event.internal_metadata.out_of_band_membership = True
 
         event.signatures.update(
             compute_event_signature(
@@ -1310,7 +1310,7 @@ def do_remotely_reject_invite(self, target_hosts, room_id, user_id):
         # Mark as outlier as we don't have any state for this event; we're not
         # even in the room.
         event.internal_metadata.outlier = True
-        event.internal_metadata.new_remote_event = True
+        event.internal_metadata.out_of_band_membership = True
 
         # Try the host that we succesfully called /make_leave/ on first for
         # the /send_leave/ request.
diff --git a/synapse/storage/roommember.py b/synapse/storage/roommember.py
index 40b13de80b44..592c1bcd33c8 100644
--- a/synapse/storage/roommember.py
+++ b/synapse/storage/roommember.py
@@ -589,11 +589,11 @@ def _store_room_members_txn(self, txn, events, backfilled):
 
             # We update the local_invites table only if the event is "current",
             # i.e., its something that has just happened. If the event is an
-            # outlier it is only current if its a "new remote event", like a
-            # remote invite or a rejection of a remote invite.
+            # outlier it is only current if its an "out of band membership",
+            # like a remote invite or a rejection of a remote invite.
             is_new_state = not backfilled and (
                 not event.internal_metadata.is_outlier()
-                or event.internal_metadata.is_new_remote_event()
+                or event.internal_metadata.is_out_of_band_membership()
             )
             is_mine = self.hs.is_mine_id(event.state_key)
             if is_new_state and is_mine:

From 9139b87be420fcfce22e70a7c35ba52b2ea32f3a Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Thu, 24 Jan 2019 18:04:02 +0000
Subject: [PATCH 6/7] Remove unecessary setting of outlier bit

---
 synapse/handlers/federation.py | 2 --
 1 file changed, 2 deletions(-)

diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py
index 242719b7ce51..d53b716ffbbe 100644
--- a/synapse/handlers/federation.py
+++ b/synapse/handlers/federation.py
@@ -1079,8 +1079,6 @@ def do_invite_join(self, target_hosts, room_id, joinee, content):
         handled_events = set()
 
         try:
-            event.internal_metadata.outlier = False
-
             # Try the host we successfully got a response to /make_join/
             # request first.
             try:

From 5ee1f997a8e7177077e2c5f0750e28725a452791 Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Thu, 24 Jan 2019 18:08:08 +0000
Subject: [PATCH 7/7] Update make_membership_event docs

---
 synapse/federation/federation_client.py | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py
index f4adcb556d30..df7d18700cee 100644
--- a/synapse/federation/federation_client.py
+++ b/synapse/federation/federation_client.py
@@ -524,6 +524,8 @@ def make_membership_event(self, destinations, room_id, user_id, membership,
         Does so by asking one of the already participating servers to create an
         event with proper context.
 
+        Returns a fully signed and hashed event.
+
         Note that this does not append any events to any graphs.
 
         Args:
@@ -538,8 +540,9 @@ def make_membership_event(self, destinations, room_id, user_id, membership,
             params (dict[str, str|Iterable[str]]): Query parameters to include in the
                 request.
         Return:
-            Deferred: resolves to a tuple of (origin (str), event (object))
-            where origin is the remote homeserver which generated the event.
+            Deferred[tuple[str, FrozenEvent]]: resolves to a tuple of `origin`
+            and event where origin is the remote homeserver which generated
+            the event.
 
             Fails with a ``SynapseError`` if the chosen remote server
             returns a 300/400 code.