Merge branch 'erikj/attestation_local_fix' of github.com:matrix-org/synapse into develop

3 files changed, 36 insertions, 9 deletions

diff --git a/synapse/groups/attestations.py b/synapse/groups/attestations.py
index c52e020989..28bbff9bbb 100644
--- a/synapse/groups/attestations.py
+++ b/synapse/groups/attestations.py
@@ -35,6 +35,8 @@ An attestsation is a signed blob of json that looks like:
     }
 """
 
+import logging
+
 from twisted.internet import defer
 
 from synapse.api.errors import SynapseError
@@ -44,6 +46,9 @@ from synapse.util.logcontext import preserve_fn
 from signedjson.sign import sign_json
 
 
+logger = logging.getLogger(__name__)
+
+
 # Default validity duration for new attestations we create
 DEFAULT_ATTESTATION_LENGTH_MS = 3 * 24 * 60 * 60 * 1000
 
@@ -150,12 +155,19 @@ class GroupAttestionRenewer(object):
 
         @defer.inlineCallbacks
         def _renew_attestation(group_id, user_id):
-            attestation = self.attestations.create_attestation(group_id, user_id)
-
-            if self.is_mine_id(group_id):
+            if not self.is_mine_id(group_id):
+                destination = get_domain_from_id(group_id)
+            elif not self.is_mine_id(user_id):
                 destination = get_domain_from_id(user_id)
             else:
-                destination = get_domain_from_id(group_id)
+                logger.warn(
+                    "Incorrectly trying to do attestations for user: %r in %r",
+                    user_id, group_id,
+                )
+                yield self.store.remove_attestation_renewal(group_id, user_id)
+                return
+
+            attestation = self.attestations.create_attestation(group_id, user_id)
 
             yield self.transport_client.renew_group_attestation(
                 destination, group_id, user_id,
diff --git a/synapse/groups/groups_server.py b/synapse/groups/groups_server.py
index 4f9e459136..7406f67d07 100644
--- a/synapse/groups/groups_server.py
+++ b/synapse/groups/groups_server.py
@@ -646,6 +646,7 @@ class GroupsServerHandler(object):
             raise SynapseError(403, "User not invited to group")
 
         if not self.hs.is_mine_id(requester_user_id):
+            local_attestation = self.attestations.create_attestation(group_id, user_id)
             remote_attestation = content["attestation"]
 
             yield self.attestations.verify_attestation(
@@ -654,13 +655,9 @@ class GroupsServerHandler(object):
                 group_id=group_id,
             )
         else:
+            local_attestation = None
             remote_attestation = None
 
-        local_attestation = self.attestations.create_attestation(
-            group_id,
-            requester_user_id,
-        )
-
         is_public = _parse_visibility_from_contents(content)
 
         yield self.store.add_user_to_group(
diff --git a/synapse/storage/group_server.py b/synapse/storage/group_server.py
index 095a3dd382..8c4ad0a9a9 100644
--- a/synapse/storage/group_server.py
+++ b/synapse/storage/group_server.py
@@ -1089,6 +1089,24 @@ class GroupServerStore(SQLBaseStore):
             desc="update_remote_attestion",
         )
 
+    def remove_attestation_renewal(self, group_id, user_id):
+        """Remove an attestation that we thought we should renew, but actually
+        shouldn't. Ideally this would never get called as we would never
+        incorrectly try and do attestations for local users on local groups.
+
+        Args:
+            group_id (str)
+            user_id (str)
+        """
+        return self._simple_delete(
+            table="group_attestations_renewals",
+            keyvalues={
+                "group_id": group_id,
+                "user_id": user_id,
+            },
+            desc="remove_attestation_renewal",
+        )
+
     @defer.inlineCallbacks
     def get_remote_attestation(self, group_id, user_id):
         """Get the attestation that proves the remote agrees that the user is