From 9b2feef9eb9502bf07d51378c75fc6b690a15676 Mon Sep 17 00:00:00 2001
From: Luke Barnard <lukeb@openmarket.com>
Date: Thu, 26 Oct 2017 16:51:32 +0100
Subject: [PATCH 01/25] Add is_public to groups table to allow for private
 groups

Prevent group API access to non-members for private groups

Also make all the group code paths consistent with `requester_user_id` always being the User ID of the requesting user.
---
 synapse/groups/groups_server.py               | 114 +++++++++---------
 synapse/rest/client/v2_alpha/groups.py        |  80 ++++++------
 .../storage/schema/delta/46/group_server.sql  |  17 +++
 3 files changed, 116 insertions(+), 95 deletions(-)
 create mode 100644 synapse/storage/schema/delta/46/group_server.sql

diff --git a/synapse/groups/groups_server.py b/synapse/groups/groups_server.py
index 23beb3187e..91c0b26107 100644
--- a/synapse/groups/groups_server.py
+++ b/synapse/groups/groups_server.py
@@ -49,7 +49,7 @@ class GroupsServerHandler(object):
         hs.get_groups_attestation_renewer()
 
     @defer.inlineCallbacks
-    def check_group_is_ours(self, group_id, and_exists=False, and_is_admin=None):
+    def check_group_is_ours(self, group_id, requester_user_id, and_exists=False, and_is_admin=None):
         """Check that the group is ours, and optionally if it exists.
 
         If group does exist then return group.
@@ -67,6 +67,10 @@ class GroupsServerHandler(object):
         if and_exists and not group:
             raise SynapseError(404, "Unknown group")
 
+        is_user_in_group = yield self.store.is_user_in_group(requester_user_id, group_id)
+        if is_user_in_group or not group.is_public:
+            raise SynapseError(404, "Unknown group")
+
         if and_is_admin:
             is_admin = yield self.store.is_user_admin_in_group(group_id, and_is_admin)
             if not is_admin:
@@ -84,7 +88,7 @@ class GroupsServerHandler(object):
 
         A user/room may appear in multiple roles/categories.
         """
-        yield self.check_group_is_ours(group_id, and_exists=True)
+        yield self.check_group_is_ours(group_id, requester_user_id, and_exists=True)
 
         is_user_in_group = yield self.store.is_user_in_group(requester_user_id, group_id)
 
@@ -153,10 +157,10 @@ class GroupsServerHandler(object):
         })
 
     @defer.inlineCallbacks
-    def update_group_summary_room(self, group_id, user_id, room_id, category_id, content):
+    def update_group_summary_room(self, group_id, requester_user_id, room_id, category_id, content):
         """Add/update a room to the group summary
         """
-        yield self.check_group_is_ours(group_id, and_exists=True, and_is_admin=user_id)
+        yield self.check_group_is_ours(group_id, requester_user_id, and_exists=True, and_is_admin=requester_user_id)
 
         RoomID.from_string(room_id)  # Ensure valid room id
 
@@ -175,10 +179,10 @@ class GroupsServerHandler(object):
         defer.returnValue({})
 
     @defer.inlineCallbacks
-    def delete_group_summary_room(self, group_id, user_id, room_id, category_id):
+    def delete_group_summary_room(self, group_id, requester_user_id, room_id, category_id):
         """Remove a room from the summary
         """
-        yield self.check_group_is_ours(group_id, and_exists=True, and_is_admin=user_id)
+        yield self.check_group_is_ours(group_id, requester_user_id, and_exists=True, and_is_admin=requester_user_id)
 
         yield self.store.remove_room_from_summary(
             group_id=group_id,
@@ -189,10 +193,10 @@ class GroupsServerHandler(object):
         defer.returnValue({})
 
     @defer.inlineCallbacks
-    def get_group_categories(self, group_id, user_id):
+    def get_group_categories(self, group_id, requester_user_id):
         """Get all categories in a group (as seen by user)
         """
-        yield self.check_group_is_ours(group_id, and_exists=True)
+        yield self.check_group_is_ours(group_id, requester_user_id, and_exists=True)
 
         categories = yield self.store.get_group_categories(
             group_id=group_id,
@@ -200,10 +204,10 @@ class GroupsServerHandler(object):
         defer.returnValue({"categories": categories})
 
     @defer.inlineCallbacks
-    def get_group_category(self, group_id, user_id, category_id):
+    def get_group_category(self, group_id, requester_user_id, category_id):
         """Get a specific category in a group (as seen by user)
         """
-        yield self.check_group_is_ours(group_id, and_exists=True)
+        yield self.check_group_is_ours(group_id, requester_user_id, and_exists=True)
 
         res = yield self.store.get_group_category(
             group_id=group_id,
@@ -213,10 +217,10 @@ class GroupsServerHandler(object):
         defer.returnValue(res)
 
     @defer.inlineCallbacks
-    def update_group_category(self, group_id, user_id, category_id, content):
+    def update_group_category(self, group_id, requester_user_id, category_id, content):
         """Add/Update a group category
         """
-        yield self.check_group_is_ours(group_id, and_exists=True, and_is_admin=user_id)
+        yield self.check_group_is_ours(group_id, requester_user_id, and_exists=True, and_is_admin=requester_user_id)
 
         is_public = _parse_visibility_from_contents(content)
         profile = content.get("profile")
@@ -231,10 +235,10 @@ class GroupsServerHandler(object):
         defer.returnValue({})
 
     @defer.inlineCallbacks
-    def delete_group_category(self, group_id, user_id, category_id):
+    def delete_group_category(self, group_id, requester_user_id, category_id):
         """Delete a group category
         """
-        yield self.check_group_is_ours(group_id, and_exists=True, and_is_admin=user_id)
+        yield self.check_group_is_ours(group_id, requester_user_id, and_exists=True, and_is_admin=requester_user_id)
 
         yield self.store.remove_group_category(
             group_id=group_id,
@@ -244,10 +248,10 @@ class GroupsServerHandler(object):
         defer.returnValue({})
 
     @defer.inlineCallbacks
-    def get_group_roles(self, group_id, user_id):
+    def get_group_roles(self, group_id, requester_user_id):
         """Get all roles in a group (as seen by user)
         """
-        yield self.check_group_is_ours(group_id, and_exists=True)
+        yield self.check_group_is_ours(group_id, requester_user_id, and_exists=True)
 
         roles = yield self.store.get_group_roles(
             group_id=group_id,
@@ -255,10 +259,10 @@ class GroupsServerHandler(object):
         defer.returnValue({"roles": roles})
 
     @defer.inlineCallbacks
-    def get_group_role(self, group_id, user_id, role_id):
+    def get_group_role(self, group_id, requester_user_id, role_id):
         """Get a specific role in a group (as seen by user)
         """
-        yield self.check_group_is_ours(group_id, and_exists=True)
+        yield self.check_group_is_ours(group_id, requester_user_id, and_exists=True)
 
         res = yield self.store.get_group_role(
             group_id=group_id,
@@ -267,10 +271,10 @@ class GroupsServerHandler(object):
         defer.returnValue(res)
 
     @defer.inlineCallbacks
-    def update_group_role(self, group_id, user_id, role_id, content):
+    def update_group_role(self, group_id, requester_user_id, role_id, content):
         """Add/update a role in a group
         """
-        yield self.check_group_is_ours(group_id, and_exists=True, and_is_admin=user_id)
+        yield self.check_group_is_ours(group_id, requester_user_id, and_exists=True, and_is_admin=requester_user_id)
 
         is_public = _parse_visibility_from_contents(content)
 
@@ -286,10 +290,10 @@ class GroupsServerHandler(object):
         defer.returnValue({})
 
     @defer.inlineCallbacks
-    def delete_group_role(self, group_id, user_id, role_id):
+    def delete_group_role(self, group_id, requester_user_id, role_id):
         """Remove role from group
         """
-        yield self.check_group_is_ours(group_id, and_exists=True, and_is_admin=user_id)
+        yield self.check_group_is_ours(group_id, requester_user_id, and_exists=True, and_is_admin=requester_user_id)
 
         yield self.store.remove_group_role(
             group_id=group_id,
@@ -304,7 +308,7 @@ class GroupsServerHandler(object):
         """Add/update a users entry in the group summary
         """
         yield self.check_group_is_ours(
-            group_id, and_exists=True, and_is_admin=requester_user_id,
+            group_id, requester_user_id, and_exists=True, and_is_admin=requester_user_id,
         )
 
         order = content.get("order", None)
@@ -326,7 +330,7 @@ class GroupsServerHandler(object):
         """Remove a user from the group summary
         """
         yield self.check_group_is_ours(
-            group_id, and_exists=True, and_is_admin=requester_user_id,
+            group_id, requester_user_id, and_exists=True, and_is_admin=requester_user_id,
         )
 
         yield self.store.remove_user_from_summary(
@@ -342,7 +346,7 @@ class GroupsServerHandler(object):
         """Get the group profile as seen by requester_user_id
         """
 
-        yield self.check_group_is_ours(group_id)
+        yield self.check_group_is_ours(group_id, requester_user_id)
 
         group_description = yield self.store.get_group(group_id)
 
@@ -356,7 +360,7 @@ class GroupsServerHandler(object):
         """Update the group profile
         """
         yield self.check_group_is_ours(
-            group_id, and_exists=True, and_is_admin=requester_user_id,
+            group_id, requester_user_id, and_exists=True, and_is_admin=requester_user_id,
         )
 
         profile = {}
@@ -377,7 +381,7 @@ class GroupsServerHandler(object):
         The ordering is arbitrary at the moment
         """
 
-        yield self.check_group_is_ours(group_id, and_exists=True)
+        yield self.check_group_is_ours(group_id, requester_user_id, and_exists=True)
 
         is_user_in_group = yield self.store.is_user_in_group(requester_user_id, group_id)
 
@@ -425,7 +429,7 @@ class GroupsServerHandler(object):
         The ordering is arbitrary at the moment
         """
 
-        yield self.check_group_is_ours(group_id, and_exists=True)
+        yield self.check_group_is_ours(group_id, requester_user_id, and_exists=True)
 
         is_user_in_group = yield self.store.is_user_in_group(requester_user_id, group_id)
 
@@ -459,7 +463,7 @@ class GroupsServerHandler(object):
         This returns rooms in order of decreasing number of joined users
         """
 
-        yield self.check_group_is_ours(group_id, and_exists=True)
+        yield self.check_group_is_ours(group_id, requester_user_id, and_exists=True)
 
         is_user_in_group = yield self.store.is_user_in_group(requester_user_id, group_id)
 
@@ -500,7 +504,7 @@ class GroupsServerHandler(object):
         RoomID.from_string(room_id)  # Ensure valid room id
 
         yield self.check_group_is_ours(
-            group_id, and_exists=True, and_is_admin=requester_user_id
+            group_id, requester_user_id, and_exists=True, and_is_admin=requester_user_id
         )
 
         is_public = _parse_visibility_from_contents(content)
@@ -514,7 +518,7 @@ class GroupsServerHandler(object):
         """Remove room from group
         """
         yield self.check_group_is_ours(
-            group_id, and_exists=True, and_is_admin=requester_user_id
+            group_id, requester_user_id, and_exists=True, and_is_admin=requester_user_id
         )
 
         yield self.store.remove_room_from_group(group_id, room_id)
@@ -527,7 +531,7 @@ class GroupsServerHandler(object):
         """
 
         group = yield self.check_group_is_ours(
-            group_id, and_exists=True, and_is_admin=requester_user_id
+            group_id, requester_user_id, and_exists=True, and_is_admin=requester_user_id
         )
 
         # TODO: Check if user knocked
@@ -596,35 +600,35 @@ class GroupsServerHandler(object):
             raise SynapseError(502, "Unknown state returned by HS")
 
     @defer.inlineCallbacks
-    def accept_invite(self, group_id, user_id, content):
+    def accept_invite(self, group_id, requester_user_id, content):
         """User tries to accept an invite to the group.
 
         This is different from them asking to join, and so should error if no
         invite exists (and they're not a member of the group)
         """
 
-        yield self.check_group_is_ours(group_id, and_exists=True)
+        yield self.check_group_is_ours(group_id, requester_user_id, and_exists=True)
 
-        if not self.store.is_user_invited_to_local_group(group_id, user_id):
+        if not self.store.is_user_invited_to_local_group(group_id, requester_user_id):
             raise SynapseError(403, "User not invited to group")
 
-        if not self.hs.is_mine_id(user_id):
+        if not self.hs.is_mine_id(requester_user_id):
             remote_attestation = content["attestation"]
 
             yield self.attestations.verify_attestation(
                 remote_attestation,
-                user_id=user_id,
+                user_id=requester_user_id,
                 group_id=group_id,
             )
         else:
             remote_attestation = None
 
-        local_attestation = self.attestations.create_attestation(group_id, user_id)
+        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(
-            group_id, user_id,
+            group_id, requester_user_id,
             is_admin=False,
             is_public=is_public,
             local_attestation=local_attestation,
@@ -637,31 +641,31 @@ class GroupsServerHandler(object):
         })
 
     @defer.inlineCallbacks
-    def knock(self, group_id, user_id, content):
+    def knock(self, group_id, requester_user_id, content):
         """A user requests becoming a member of the group
         """
-        yield self.check_group_is_ours(group_id, and_exists=True)
+        yield self.check_group_is_ours(group_id, requester_user_id, and_exists=True)
 
         raise NotImplementedError()
 
     @defer.inlineCallbacks
-    def accept_knock(self, group_id, user_id, content):
+    def accept_knock(self, group_id, requester_user_id, content):
         """Accept a users knock to the room.
 
         Errors if the user hasn't knocked, rather than inviting them.
         """
 
-        yield self.check_group_is_ours(group_id, and_exists=True)
+        yield self.check_group_is_ours(group_id, requester_user_id, and_exists=True)
 
         raise NotImplementedError()
 
     @defer.inlineCallbacks
     def remove_user_from_group(self, group_id, user_id, requester_user_id, content):
-        """Remove a user from the group; either a user is leaving or and admin
-        kicked htem.
+        """Remove a user from the group; either a user is leaving or an admin
+        kicked them.
         """
 
-        yield self.check_group_is_ours(group_id, and_exists=True)
+        yield self.check_group_is_ours(group_id, requester_user_id, and_exists=True)
 
         is_kick = False
         if requester_user_id != user_id:
@@ -692,7 +696,7 @@ class GroupsServerHandler(object):
         defer.returnValue({})
 
     @defer.inlineCallbacks
-    def create_group(self, group_id, user_id, content):
+    def create_group(self, group_id, requester_user_id, content):
         group = yield self.check_group_is_ours(group_id)
 
         logger.info("Attempting to create group with ID: %r", group_id)
@@ -703,7 +707,7 @@ class GroupsServerHandler(object):
         if group:
             raise SynapseError(400, "Group already exists")
 
-        is_admin = yield self.auth.is_server_admin(UserID.from_string(user_id))
+        is_admin = yield self.auth.is_server_admin(UserID.from_string(requester_user_id))
         if not is_admin:
             if not self.hs.config.enable_group_creation:
                 raise SynapseError(
@@ -727,38 +731,38 @@ class GroupsServerHandler(object):
 
         yield self.store.create_group(
             group_id,
-            user_id,
+            requester_user_id,
             name=name,
             avatar_url=avatar_url,
             short_description=short_description,
             long_description=long_description,
         )
 
-        if not self.hs.is_mine_id(user_id):
+        if not self.hs.is_mine_id(requester_user_id):
             remote_attestation = content["attestation"]
 
             yield self.attestations.verify_attestation(
                 remote_attestation,
-                user_id=user_id,
+                user_id=requester_user_id,
                 group_id=group_id,
             )
 
-            local_attestation = self.attestations.create_attestation(group_id, user_id)
+            local_attestation = self.attestations.create_attestation(group_id, requester_user_id)
         else:
             local_attestation = None
             remote_attestation = None
 
         yield self.store.add_user_to_group(
-            group_id, user_id,
+            group_id, requester_user_id,
             is_admin=True,
             is_public=True,  # TODO
             local_attestation=local_attestation,
             remote_attestation=remote_attestation,
         )
 
-        if not self.hs.is_mine_id(user_id):
+        if not self.hs.is_mine_id(requester_user_id):
             yield self.store.add_remote_profile_cache(
-                user_id,
+                requester_user_id,
                 displayname=user_profile.get("displayname"),
                 avatar_url=user_profile.get("avatar_url"),
             )
diff --git a/synapse/rest/client/v2_alpha/groups.py b/synapse/rest/client/v2_alpha/groups.py
index 100f47ca9e..05a40d6941 100644
--- a/synapse/rest/client/v2_alpha/groups.py
+++ b/synapse/rest/client/v2_alpha/groups.py
@@ -39,20 +39,20 @@ class GroupServlet(RestServlet):
     @defer.inlineCallbacks
     def on_GET(self, request, group_id):
         requester = yield self.auth.get_user_by_req(request)
-        user_id = requester.user.to_string()
+        requester_user_id = requester.user.to_string()
 
-        group_description = yield self.groups_handler.get_group_profile(group_id, user_id)
+        group_description = yield self.groups_handler.get_group_profile(group_id, requester_user_id)
 
         defer.returnValue((200, group_description))
 
     @defer.inlineCallbacks
     def on_POST(self, request, group_id):
         requester = yield self.auth.get_user_by_req(request)
-        user_id = requester.user.to_string()
+        requester_user_id = requester.user.to_string()
 
         content = parse_json_object_from_request(request)
         yield self.groups_handler.update_group_profile(
-            group_id, user_id, content,
+            group_id, requester_user_id, content,
         )
 
         defer.returnValue((200, {}))
@@ -72,9 +72,9 @@ class GroupSummaryServlet(RestServlet):
     @defer.inlineCallbacks
     def on_GET(self, request, group_id):
         requester = yield self.auth.get_user_by_req(request)
-        user_id = requester.user.to_string()
+        requester_user_id = requester.user.to_string()
 
-        get_group_summary = yield self.groups_handler.get_group_summary(group_id, user_id)
+        get_group_summary = yield self.groups_handler.get_group_summary(group_id, requester_user_id)
 
         defer.returnValue((200, get_group_summary))
 
@@ -101,11 +101,11 @@ class GroupSummaryRoomsCatServlet(RestServlet):
     @defer.inlineCallbacks
     def on_PUT(self, request, group_id, category_id, room_id):
         requester = yield self.auth.get_user_by_req(request)
-        user_id = requester.user.to_string()
+        requester_user_id = requester.user.to_string()
 
         content = parse_json_object_from_request(request)
         resp = yield self.groups_handler.update_group_summary_room(
-            group_id, user_id,
+            group_id, requester_user_id,
             room_id=room_id,
             category_id=category_id,
             content=content,
@@ -116,10 +116,10 @@ class GroupSummaryRoomsCatServlet(RestServlet):
     @defer.inlineCallbacks
     def on_DELETE(self, request, group_id, category_id, room_id):
         requester = yield self.auth.get_user_by_req(request)
-        user_id = requester.user.to_string()
+        requester_user_id = requester.user.to_string()
 
         resp = yield self.groups_handler.delete_group_summary_room(
-            group_id, user_id,
+            group_id, requester_user_id,
             room_id=room_id,
             category_id=category_id,
         )
@@ -143,10 +143,10 @@ class GroupCategoryServlet(RestServlet):
     @defer.inlineCallbacks
     def on_GET(self, request, group_id, category_id):
         requester = yield self.auth.get_user_by_req(request)
-        user_id = requester.user.to_string()
+        requester_user_id = requester.user.to_string()
 
         category = yield self.groups_handler.get_group_category(
-            group_id, user_id,
+            group_id, requester_user_id,
             category_id=category_id,
         )
 
@@ -155,11 +155,11 @@ class GroupCategoryServlet(RestServlet):
     @defer.inlineCallbacks
     def on_PUT(self, request, group_id, category_id):
         requester = yield self.auth.get_user_by_req(request)
-        user_id = requester.user.to_string()
+        requester_user_id = requester.user.to_string()
 
         content = parse_json_object_from_request(request)
         resp = yield self.groups_handler.update_group_category(
-            group_id, user_id,
+            group_id, requester_user_id,
             category_id=category_id,
             content=content,
         )
@@ -169,10 +169,10 @@ class GroupCategoryServlet(RestServlet):
     @defer.inlineCallbacks
     def on_DELETE(self, request, group_id, category_id):
         requester = yield self.auth.get_user_by_req(request)
-        user_id = requester.user.to_string()
+        requester_user_id = requester.user.to_string()
 
         resp = yield self.groups_handler.delete_group_category(
-            group_id, user_id,
+            group_id, requester_user_id,
             category_id=category_id,
         )
 
@@ -195,10 +195,10 @@ class GroupCategoriesServlet(RestServlet):
     @defer.inlineCallbacks
     def on_GET(self, request, group_id):
         requester = yield self.auth.get_user_by_req(request)
-        user_id = requester.user.to_string()
+        requester_user_id = requester.user.to_string()
 
         category = yield self.groups_handler.get_group_categories(
-            group_id, user_id,
+            group_id, requester_user_id,
         )
 
         defer.returnValue((200, category))
@@ -220,10 +220,10 @@ class GroupRoleServlet(RestServlet):
     @defer.inlineCallbacks
     def on_GET(self, request, group_id, role_id):
         requester = yield self.auth.get_user_by_req(request)
-        user_id = requester.user.to_string()
+        requester_user_id = requester.user.to_string()
 
         category = yield self.groups_handler.get_group_role(
-            group_id, user_id,
+            group_id, requester_user_id,
             role_id=role_id,
         )
 
@@ -232,11 +232,11 @@ class GroupRoleServlet(RestServlet):
     @defer.inlineCallbacks
     def on_PUT(self, request, group_id, role_id):
         requester = yield self.auth.get_user_by_req(request)
-        user_id = requester.user.to_string()
+        requester_user_id = requester.user.to_string()
 
         content = parse_json_object_from_request(request)
         resp = yield self.groups_handler.update_group_role(
-            group_id, user_id,
+            group_id, requester_user_id,
             role_id=role_id,
             content=content,
         )
@@ -246,10 +246,10 @@ class GroupRoleServlet(RestServlet):
     @defer.inlineCallbacks
     def on_DELETE(self, request, group_id, role_id):
         requester = yield self.auth.get_user_by_req(request)
-        user_id = requester.user.to_string()
+        requester_user_id = requester.user.to_string()
 
         resp = yield self.groups_handler.delete_group_role(
-            group_id, user_id,
+            group_id, requester_user_id,
             role_id=role_id,
         )
 
@@ -272,10 +272,10 @@ class GroupRolesServlet(RestServlet):
     @defer.inlineCallbacks
     def on_GET(self, request, group_id):
         requester = yield self.auth.get_user_by_req(request)
-        user_id = requester.user.to_string()
+        requester_user_id = requester.user.to_string()
 
         category = yield self.groups_handler.get_group_roles(
-            group_id, user_id,
+            group_id, requester_user_id,
         )
 
         defer.returnValue((200, category))
@@ -343,9 +343,9 @@ class GroupRoomServlet(RestServlet):
     @defer.inlineCallbacks
     def on_GET(self, request, group_id):
         requester = yield self.auth.get_user_by_req(request)
-        user_id = requester.user.to_string()
+        requester_user_id = requester.user.to_string()
 
-        result = yield self.groups_handler.get_rooms_in_group(group_id, user_id)
+        result = yield self.groups_handler.get_rooms_in_group(group_id, requester_user_id)
 
         defer.returnValue((200, result))
 
@@ -364,9 +364,9 @@ class GroupUsersServlet(RestServlet):
     @defer.inlineCallbacks
     def on_GET(self, request, group_id):
         requester = yield self.auth.get_user_by_req(request)
-        user_id = requester.user.to_string()
+        requester_user_id = requester.user.to_string()
 
-        result = yield self.groups_handler.get_users_in_group(group_id, user_id)
+        result = yield self.groups_handler.get_users_in_group(group_id, requester_user_id)
 
         defer.returnValue((200, result))
 
@@ -385,9 +385,9 @@ class GroupInvitedUsersServlet(RestServlet):
     @defer.inlineCallbacks
     def on_GET(self, request, group_id):
         requester = yield self.auth.get_user_by_req(request)
-        user_id = requester.user.to_string()
+        requester_user_id = requester.user.to_string()
 
-        result = yield self.groups_handler.get_invited_users_in_group(group_id, user_id)
+        result = yield self.groups_handler.get_invited_users_in_group(group_id, requester_user_id)
 
         defer.returnValue((200, result))
 
@@ -407,14 +407,14 @@ class GroupCreateServlet(RestServlet):
     @defer.inlineCallbacks
     def on_POST(self, request):
         requester = yield self.auth.get_user_by_req(request)
-        user_id = requester.user.to_string()
+        requester_user_id = requester.user.to_string()
 
         # TODO: Create group on remote server
         content = parse_json_object_from_request(request)
         localpart = content.pop("localpart")
         group_id = GroupID(localpart, self.server_name).to_string()
 
-        result = yield self.groups_handler.create_group(group_id, user_id, content)
+        result = yield self.groups_handler.create_group(group_id, requester_user_id, content)
 
         defer.returnValue((200, result))
 
@@ -435,11 +435,11 @@ class GroupAdminRoomsServlet(RestServlet):
     @defer.inlineCallbacks
     def on_PUT(self, request, group_id, room_id):
         requester = yield self.auth.get_user_by_req(request)
-        user_id = requester.user.to_string()
+        requester_user_id = requester.user.to_string()
 
         content = parse_json_object_from_request(request)
         result = yield self.groups_handler.add_room_to_group(
-            group_id, user_id, room_id, content,
+            group_id, requester_user_id, room_id, content,
         )
 
         defer.returnValue((200, result))
@@ -447,10 +447,10 @@ class GroupAdminRoomsServlet(RestServlet):
     @defer.inlineCallbacks
     def on_DELETE(self, request, group_id, room_id):
         requester = yield self.auth.get_user_by_req(request)
-        user_id = requester.user.to_string()
+        requester_user_id = requester.user.to_string()
 
         result = yield self.groups_handler.remove_room_from_group(
-            group_id, user_id, room_id,
+            group_id, requester_user_id, room_id,
         )
 
         defer.returnValue((200, result))
@@ -685,9 +685,9 @@ class GroupsForUserServlet(RestServlet):
     @defer.inlineCallbacks
     def on_GET(self, request):
         requester = yield self.auth.get_user_by_req(request)
-        user_id = requester.user.to_string()
+        requester_user_id = requester.user.to_string()
 
-        result = yield self.groups_handler.get_joined_groups(user_id)
+        result = yield self.groups_handler.get_joined_groups(requester_user_id)
 
         defer.returnValue((200, result))
 
diff --git a/synapse/storage/schema/delta/46/group_server.sql b/synapse/storage/schema/delta/46/group_server.sql
new file mode 100644
index 0000000000..23ee1194d3
--- /dev/null
+++ b/synapse/storage/schema/delta/46/group_server.sql
@@ -0,0 +1,17 @@
+/* Copyright 2017 Vector Creations Ltd
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+-- whether non-members can access group APIs
+ALTER TABLE groups ADD COLUMN is_public BOOL DEFAULT 1 NOT NULL;

From 595fe67f01d73f3a1ccddaf9922ac8c7e7e367cb Mon Sep 17 00:00:00 2001
From: Luke Barnard <lukeb@openmarket.com>
Date: Thu, 26 Oct 2017 17:20:24 +0100
Subject: [PATCH 02/25] delint

---
 synapse/groups/groups_server.py        | 61 +++++++++++++++++++++-----
 synapse/rest/client/v2_alpha/groups.py | 21 +++++++--
 2 files changed, 67 insertions(+), 15 deletions(-)

diff --git a/synapse/groups/groups_server.py b/synapse/groups/groups_server.py
index 91c0b26107..75634febd0 100644
--- a/synapse/groups/groups_server.py
+++ b/synapse/groups/groups_server.py
@@ -49,7 +49,8 @@ class GroupsServerHandler(object):
         hs.get_groups_attestation_renewer()
 
     @defer.inlineCallbacks
-    def check_group_is_ours(self, group_id, requester_user_id, and_exists=False, and_is_admin=None):
+    def check_group_is_ours(self, group_id, requester_user_id,
+                            and_exists=False, and_is_admin=None):
         """Check that the group is ours, and optionally if it exists.
 
         If group does exist then return group.
@@ -157,10 +158,16 @@ class GroupsServerHandler(object):
         })
 
     @defer.inlineCallbacks
-    def update_group_summary_room(self, group_id, requester_user_id, room_id, category_id, content):
+    def update_group_summary_room(self, group_id, requester_user_id,
+                                  room_id, category_id, content):
         """Add/update a room to the group summary
         """
-        yield self.check_group_is_ours(group_id, requester_user_id, and_exists=True, and_is_admin=requester_user_id)
+        yield self.check_group_is_ours(
+            group_id,
+            requester_user_id,
+            and_exists=True,
+            and_is_admin=requester_user_id,
+        )
 
         RoomID.from_string(room_id)  # Ensure valid room id
 
@@ -179,10 +186,16 @@ class GroupsServerHandler(object):
         defer.returnValue({})
 
     @defer.inlineCallbacks
-    def delete_group_summary_room(self, group_id, requester_user_id, room_id, category_id):
+    def delete_group_summary_room(self, group_id, requester_user_id,
+                                  room_id, category_id):
         """Remove a room from the summary
         """
-        yield self.check_group_is_ours(group_id, requester_user_id, and_exists=True, and_is_admin=requester_user_id)
+        yield self.check_group_is_ours(
+            group_id,
+            requester_user_id,
+            and_exists=True,
+            and_is_admin=requester_user_id,
+        )
 
         yield self.store.remove_room_from_summary(
             group_id=group_id,
@@ -220,7 +233,12 @@ class GroupsServerHandler(object):
     def update_group_category(self, group_id, requester_user_id, category_id, content):
         """Add/Update a group category
         """
-        yield self.check_group_is_ours(group_id, requester_user_id, and_exists=True, and_is_admin=requester_user_id)
+        yield self.check_group_is_ours(
+            group_id,
+            requester_user_id,
+            and_exists=True,
+            and_is_admin=requester_user_id,
+        )
 
         is_public = _parse_visibility_from_contents(content)
         profile = content.get("profile")
@@ -238,7 +256,12 @@ class GroupsServerHandler(object):
     def delete_group_category(self, group_id, requester_user_id, category_id):
         """Delete a group category
         """
-        yield self.check_group_is_ours(group_id, requester_user_id, and_exists=True, and_is_admin=requester_user_id)
+        yield self.check_group_is_ours(
+            group_id,
+            requester_user_id,
+            and_exists=True,
+            and_is_admin=requester_user_id
+        )
 
         yield self.store.remove_group_category(
             group_id=group_id,
@@ -274,7 +297,12 @@ class GroupsServerHandler(object):
     def update_group_role(self, group_id, requester_user_id, role_id, content):
         """Add/update a role in a group
         """
-        yield self.check_group_is_ours(group_id, requester_user_id, and_exists=True, and_is_admin=requester_user_id)
+        yield self.check_group_is_ours(
+            group_id,
+            requester_user_id,
+            and_exists=True,
+            and_is_admin=requester_user_id,
+        )
 
         is_public = _parse_visibility_from_contents(content)
 
@@ -293,7 +321,12 @@ class GroupsServerHandler(object):
     def delete_group_role(self, group_id, requester_user_id, role_id):
         """Remove role from group
         """
-        yield self.check_group_is_ours(group_id, requester_user_id, and_exists=True, and_is_admin=requester_user_id)
+        yield self.check_group_is_ours(
+            group_id,
+            requester_user_id,
+            and_exists=True,
+            and_is_admin=requester_user_id,
+        )
 
         yield self.store.remove_group_role(
             group_id=group_id,
@@ -623,7 +656,10 @@ class GroupsServerHandler(object):
         else:
             remote_attestation = None
 
-        local_attestation = self.attestations.create_attestation(group_id, requester_user_id)
+        local_attestation = self.attestations.create_attestation(
+            group_id,
+            requester_user_id,
+        )
 
         is_public = _parse_visibility_from_contents(content)
 
@@ -747,7 +783,10 @@ class GroupsServerHandler(object):
                 group_id=group_id,
             )
 
-            local_attestation = self.attestations.create_attestation(group_id, requester_user_id)
+            local_attestation = self.attestations.create_attestation(
+                group_id,
+                requester_user_id,
+            )
         else:
             local_attestation = None
             remote_attestation = None
diff --git a/synapse/rest/client/v2_alpha/groups.py b/synapse/rest/client/v2_alpha/groups.py
index 05a40d6941..c97885cfc7 100644
--- a/synapse/rest/client/v2_alpha/groups.py
+++ b/synapse/rest/client/v2_alpha/groups.py
@@ -41,7 +41,10 @@ class GroupServlet(RestServlet):
         requester = yield self.auth.get_user_by_req(request)
         requester_user_id = requester.user.to_string()
 
-        group_description = yield self.groups_handler.get_group_profile(group_id, requester_user_id)
+        group_description = yield self.groups_handler.get_group_profile(
+            group_id,
+            requester_user_id,
+        )
 
         defer.returnValue((200, group_description))
 
@@ -74,7 +77,10 @@ class GroupSummaryServlet(RestServlet):
         requester = yield self.auth.get_user_by_req(request)
         requester_user_id = requester.user.to_string()
 
-        get_group_summary = yield self.groups_handler.get_group_summary(group_id, requester_user_id)
+        get_group_summary = yield self.groups_handler.get_group_summary(
+            group_id,
+            requester_user_id,
+        )
 
         defer.returnValue((200, get_group_summary))
 
@@ -387,7 +393,10 @@ class GroupInvitedUsersServlet(RestServlet):
         requester = yield self.auth.get_user_by_req(request)
         requester_user_id = requester.user.to_string()
 
-        result = yield self.groups_handler.get_invited_users_in_group(group_id, requester_user_id)
+        result = yield self.groups_handler.get_invited_users_in_group(
+            group_id,
+            requester_user_id,
+        )
 
         defer.returnValue((200, result))
 
@@ -414,7 +423,11 @@ class GroupCreateServlet(RestServlet):
         localpart = content.pop("localpart")
         group_id = GroupID(localpart, self.server_name).to_string()
 
-        result = yield self.groups_handler.create_group(group_id, requester_user_id, content)
+        result = yield self.groups_handler.create_group(
+            group_id,
+            requester_user_id,
+            content,
+        )
 
         defer.returnValue((200, result))
 

From cfa4e658e0cf0ba3286116a3f71635a2142496d8 Mon Sep 17 00:00:00 2001
From: Luke Barnard <lukeb@openmarket.com>
Date: Thu, 26 Oct 2017 17:23:49 +0100
Subject: [PATCH 03/25] Bump schema version to 46

---
 synapse/storage/prepare_database.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/synapse/storage/prepare_database.py b/synapse/storage/prepare_database.py
index 817c2185c8..a4e08e6757 100644
--- a/synapse/storage/prepare_database.py
+++ b/synapse/storage/prepare_database.py
@@ -25,7 +25,7 @@ logger = logging.getLogger(__name__)
 
 # Remember to update this number every time a change is made to database
 # schema files, so the users will be informed on server restarts.
-SCHEMA_VERSION = 45
+SCHEMA_VERSION = 46
 
 dir_path = os.path.abspath(os.path.dirname(__file__))
 

From e86cefcb6f594bf66bde577899e996b5c75fc63f Mon Sep 17 00:00:00 2001
From: Luke Barnard <lukeb@openmarket.com>
Date: Thu, 26 Oct 2017 17:24:54 +0100
Subject: [PATCH 04/25] Add groups table to BOOLEAN_COLUMNS in synapse_port_db

---
 scripts/synapse_port_db | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/synapse_port_db b/scripts/synapse_port_db
index a7a50e4d36..d6d8ee50cb 100755
--- a/scripts/synapse_port_db
+++ b/scripts/synapse_port_db
@@ -42,6 +42,7 @@ BOOLEAN_COLUMNS = {
     "public_room_list_stream": ["visibility"],
     "device_lists_outbound_pokes": ["sent"],
     "users_who_share_rooms": ["share_private"],
+    "groups": ["is_public"],
 }
 
 

From 713e60b9b6658d611b399d80a6ae429946713689 Mon Sep 17 00:00:00 2001
From: Luke Barnard <lukeb@openmarket.com>
Date: Thu, 26 Oct 2017 17:38:14 +0100
Subject: [PATCH 05/25] Awful hack to get default true

---
 synapse/storage/schema/delta/46/group_server.sql | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/synapse/storage/schema/delta/46/group_server.sql b/synapse/storage/schema/delta/46/group_server.sql
index 23ee1194d3..a892cff7e5 100644
--- a/synapse/storage/schema/delta/46/group_server.sql
+++ b/synapse/storage/schema/delta/46/group_server.sql
@@ -14,4 +14,5 @@
  */
 
 -- whether non-members can access group APIs
-ALTER TABLE groups ADD COLUMN is_public BOOL DEFAULT 1 NOT NULL;
+-- NB: awful hack to get the default to be true on postgres and 1 on sqlite
+ALTER TABLE groups ADD COLUMN is_public BOOL DEFAULT (1=1) NOT NULL;

From 007cd48af67576df23e988ea8a4abcbc64396c6a Mon Sep 17 00:00:00 2001
From: Luke Barnard <lukeb@openmarket.com>
Date: Thu, 26 Oct 2017 17:55:22 +0100
Subject: [PATCH 06/25] Recreate groups table instead of adding column

Adding a column with non-constant default not possible in sqlite3
---
 .../storage/schema/delta/46/group_server.sql   | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/synapse/storage/schema/delta/46/group_server.sql b/synapse/storage/schema/delta/46/group_server.sql
index a892cff7e5..e754b554f8 100644
--- a/synapse/storage/schema/delta/46/group_server.sql
+++ b/synapse/storage/schema/delta/46/group_server.sql
@@ -13,6 +13,20 @@
  * limitations under the License.
  */
 
--- whether non-members can access group APIs
+CREATE TABLE groups_new (
+    group_id TEXT NOT NULL,
+    name TEXT,  -- the display name of the room
+    avatar_url TEXT,
+    short_description TEXT,
+    long_description TEXT,
+    is_public BOOL NOT NULL -- whether non-members can access group APIs
+);
+
 -- NB: awful hack to get the default to be true on postgres and 1 on sqlite
-ALTER TABLE groups ADD COLUMN is_public BOOL DEFAULT (1=1) NOT NULL;
+INSERT INTO groups_new
+    SELECT group_id, name, avatar_url, short_description, long_description, (1=1) FROM groups;
+
+DROP TABLE groups;
+ALTER TABLE groups_new RENAME TO groups;
+
+CREATE UNIQUE INDEX groups_idx ON groups(group_id);

From 69e8a05f355f24bb1377b7d39812f98ea9f28bb4 Mon Sep 17 00:00:00 2001
From: Luke Barnard <lukeb@openmarket.com>
Date: Thu, 26 Oct 2017 17:55:58 +0100
Subject: [PATCH 07/25] Make it work

---
 synapse/groups/groups_server.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/synapse/groups/groups_server.py b/synapse/groups/groups_server.py
index 75634febd0..eac2f41768 100644
--- a/synapse/groups/groups_server.py
+++ b/synapse/groups/groups_server.py
@@ -69,7 +69,7 @@ class GroupsServerHandler(object):
             raise SynapseError(404, "Unknown group")
 
         is_user_in_group = yield self.store.is_user_in_group(requester_user_id, group_id)
-        if is_user_in_group or not group.is_public:
+        if not is_user_in_group or not group.is_public:
             raise SynapseError(404, "Unknown group")
 
         if and_is_admin:

From 785bd7fd75ffd944f6257185fe3129495e5fa6e7 Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <richard@matrix.org>
Date: Fri, 27 Oct 2017 00:01:00 +0100
Subject: [PATCH 08/25] Allow ASes to deactivate their own users

---
 synapse/handlers/auth.py                |  2 +-
 synapse/rest/client/v2_alpha/account.py | 48 ++++++++++++++++---------
 2 files changed, 33 insertions(+), 17 deletions(-)

diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py
index 9cef9d184b..acae4d9e0d 100644
--- a/synapse/handlers/auth.py
+++ b/synapse/handlers/auth.py
@@ -82,7 +82,7 @@ class AuthHandler(BaseHandler):
     def check_auth(self, flows, clientdict, clientip):
         """
         Takes a dictionary sent by the client in the login / registration
-        protocol and handles the login flow.
+        protocol and handles the User-Interactive Auth flow.
 
         As a side effect, this function fills in the 'creds' key on the user's
         session with a map, which maps each auth-type (str) to the relevant
diff --git a/synapse/rest/client/v2_alpha/account.py b/synapse/rest/client/v2_alpha/account.py
index 4990b22b9f..1a0d57a04a 100644
--- a/synapse/rest/client/v2_alpha/account.py
+++ b/synapse/rest/client/v2_alpha/account.py
@@ -13,22 +13,21 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
+import logging
 
 from twisted.internet import defer
 
+from synapse.api.auth import has_access_token
 from synapse.api.constants import LoginType
-from synapse.api.errors import LoginError, SynapseError, Codes
+from synapse.api.errors import Codes, LoginError, SynapseError
 from synapse.http.servlet import (
-    RestServlet, parse_json_object_from_request, assert_params_in_request
+    RestServlet, assert_params_in_request,
+    parse_json_object_from_request,
 )
 from synapse.util.async import run_on_reactor
 from synapse.util.msisdn import phone_number_to_msisdn
-
 from ._base import client_v2_patterns
 
-import logging
-
-
 logger = logging.getLogger(__name__)
 
 
@@ -172,6 +171,18 @@ class DeactivateAccountRestServlet(RestServlet):
     def on_POST(self, request):
         body = parse_json_object_from_request(request)
 
+        # if the caller provides an access token, it ought to be valid.
+        requester = None
+        if has_access_token(request):
+            requester = yield self.auth.get_user_by_req(
+                request,
+            )  # type: synapse.types.Requester
+
+        # allow ASes to dectivate their own users
+        if requester and requester.app_service:
+            yield self._deactivate_account(requester.user.to_string())
+            defer.returnValue((200, {}))
+
         authed, result, params, _ = yield self.auth_handler.check_auth([
             [LoginType.PASSWORD],
         ], body, self.hs.get_ip_from_request(request))
@@ -179,27 +190,32 @@ class DeactivateAccountRestServlet(RestServlet):
         if not authed:
             defer.returnValue((401, result))
 
-        user_id = None
-        requester = None
-
         if LoginType.PASSWORD in result:
+            user_id = result[LoginType.PASSWORD]
             # if using password, they should also be logged in
-            requester = yield self.auth.get_user_by_req(request)
-            user_id = requester.user.to_string()
-            if user_id != result[LoginType.PASSWORD]:
+            if requester is None:
+                raise SynapseError(
+                    400,
+                    "Deactivate account requires an access_token",
+                    errcode=Codes.MISSING_TOKEN
+                )
+            if requester.user.to_string() != user_id:
                 raise LoginError(400, "", Codes.UNKNOWN)
         else:
             logger.error("Auth succeeded but no known type!", result.keys())
             raise SynapseError(500, "", Codes.UNKNOWN)
 
-        # FIXME: Theoretically there is a race here wherein user resets password
-        # using threepid.
+        yield self._deactivate_account(user_id)
+        defer.returnValue((200, {}))
+
+    @defer.inlineCallbacks
+    def _deactivate_account(self, user_id):
+        # FIXME: Theoretically there is a race here wherein user resets
+        # password using threepid.
         yield self.store.user_delete_access_tokens(user_id)
         yield self.store.user_delete_threepids(user_id)
         yield self.store.user_set_password_hash(user_id, None)
 
-        defer.returnValue((200, {}))
-
 
 class EmailThreepidRequestTokenRestServlet(RestServlet):
     PATTERNS = client_v2_patterns("/account/3pid/email/requestToken$")

From 7a6546228b92723a891758d20c22c11beee0c9f9 Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <richard@matrix.org>
Date: Fri, 27 Oct 2017 00:04:31 +0100
Subject: [PATCH 09/25] Device deletion: check UI auth matches access token

(otherwise there's no point in the UI auth)
---
 synapse/rest/client/v2_alpha/devices.py | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/synapse/rest/client/v2_alpha/devices.py b/synapse/rest/client/v2_alpha/devices.py
index 2a2438b7dc..5321e5abbb 100644
--- a/synapse/rest/client/v2_alpha/devices.py
+++ b/synapse/rest/client/v2_alpha/devices.py
@@ -117,6 +117,8 @@ class DeviceRestServlet(servlet.RestServlet):
 
     @defer.inlineCallbacks
     def on_DELETE(self, request, device_id):
+        requester = yield self.auth.get_user_by_req(request)
+
         try:
             body = servlet.parse_json_object_from_request(request)
 
@@ -135,11 +137,12 @@ class DeviceRestServlet(servlet.RestServlet):
         if not authed:
             defer.returnValue((401, result))
 
-        requester = yield self.auth.get_user_by_req(request)
-        yield self.device_handler.delete_device(
-            requester.user.to_string(),
-            device_id,
-        )
+        # check that the UI auth matched the access token
+        user_id = result[constants.LoginType.PASSWORD]
+        if user_id != requester.user.to_string():
+            raise errors.AuthError(403, "Invalid auth")
+
+        yield self.device_handler.delete_device(user_id, device_id)
         defer.returnValue((200, {}))
 
     @defer.inlineCallbacks

From 585972b51a033d7082b3fba4013ad2ca544c846b Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Fri, 27 Oct 2017 09:44:34 +0100
Subject: [PATCH 10/25] Don't generate group attestations for local users

---
 synapse/groups/groups_server.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/synapse/groups/groups_server.py b/synapse/groups/groups_server.py
index 23beb3187e..96f112b580 100644
--- a/synapse/groups/groups_server.py
+++ b/synapse/groups/groups_server.py
@@ -609,6 +609,8 @@ class GroupsServerHandler(object):
             raise SynapseError(403, "User not invited to group")
 
         if not self.hs.is_mine_id(user_id):
+            local_attestation = self.attestations.create_attestation(group_id, user_id)
+
             remote_attestation = content["attestation"]
 
             yield self.attestations.verify_attestation(
@@ -617,10 +619,9 @@ class GroupsServerHandler(object):
                 group_id=group_id,
             )
         else:
+            local_attestation = None
             remote_attestation = None
 
-        local_attestation = self.attestations.create_attestation(group_id, user_id)
-
         is_public = _parse_visibility_from_contents(content)
 
         yield self.store.add_user_to_group(

From d8dde19f04799270186723f7f35dc32217dda33e Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Fri, 27 Oct 2017 09:55:01 +0100
Subject: [PATCH 11/25] Log if we try to do attestations for our own user and
 group

---
 synapse/groups/attestations.py | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/synapse/groups/attestations.py b/synapse/groups/attestations.py
index b751cf5e43..2e252b66a7 100644
--- a/synapse/groups/attestations.py
+++ b/synapse/groups/attestations.py
@@ -130,10 +130,16 @@ class GroupAttestionRenewer(object):
         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)
+            else 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,
+                )
+                return
 
             yield self.transport_client.renew_group_attestation(
                 destination, group_id, user_id,

From 195abfe7a5ec3b0d52812a3d7a04264f97376771 Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Fri, 27 Oct 2017 09:58:13 +0100
Subject: [PATCH 12/25] Remove incorrect attestations

---
 synapse/groups/attestations.py  |  1 +
 synapse/storage/group_server.py | 18 ++++++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/synapse/groups/attestations.py b/synapse/groups/attestations.py
index 2e252b66a7..0bd73b6a61 100644
--- a/synapse/groups/attestations.py
+++ b/synapse/groups/attestations.py
@@ -139,6 +139,7 @@ class GroupAttestionRenewer(object):
                     "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
 
             yield self.transport_client.renew_group_attestation(
diff --git a/synapse/storage/group_server.py b/synapse/storage/group_server.py
index 9e63db5c6c..ed2ee61ad2 100644
--- a/synapse/storage/group_server.py
+++ b/synapse/storage/group_server.py
@@ -1086,6 +1086,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_update_one(
+            table="_simple_delete",
+            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

From 82d8c1bacb085588b59021d21cd4df56b0d8411a Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Fri, 27 Oct 2017 10:30:21 +0100
Subject: [PATCH 13/25] Fixup

---
 synapse/groups/attestations.py  | 6 +++---
 synapse/storage/group_server.py | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/synapse/groups/attestations.py b/synapse/groups/attestations.py
index 0bd73b6a61..4656e854f0 100644
--- a/synapse/groups/attestations.py
+++ b/synapse/groups/attestations.py
@@ -128,11 +128,9 @@ class GroupAttestionRenewer(object):
 
         @defer.inlineCallbacks
         def _renew_attestation(group_id, user_id):
-            attestation = self.attestations.create_attestation(group_id, user_id)
-
             if not self.is_mine_id(group_id):
                 destination = get_domain_from_id(group_id)
-            else not self.is_mine_id(user_id):
+            elif not self.is_mine_id(user_id):
                 destination = get_domain_from_id(user_id)
             else:
                 logger.warn(
@@ -142,6 +140,8 @@ class GroupAttestionRenewer(object):
                 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,
                 content={"attestation": attestation},
diff --git a/synapse/storage/group_server.py b/synapse/storage/group_server.py
index ed2ee61ad2..ba3f5617fa 100644
--- a/synapse/storage/group_server.py
+++ b/synapse/storage/group_server.py
@@ -1095,8 +1095,8 @@ class GroupServerStore(SQLBaseStore):
             group_id (str)
             user_id (str)
         """
-        return self._simple_update_one(
-            table="_simple_delete",
+        return self._simple_delete(
+            table="group_attestations_renewals",
             keyvalues={
                 "group_id": group_id,
                 "user_id": user_id,

From 2ca46c7afcb0e0fe780e2ef2d8cefd34669fb1a9 Mon Sep 17 00:00:00 2001
From: Luke Barnard <lukeb@openmarket.com>
Date: Fri, 27 Oct 2017 10:48:01 +0100
Subject: [PATCH 14/25] Correct logic for checking private group membership

---
 synapse/groups/groups_server.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/synapse/groups/groups_server.py b/synapse/groups/groups_server.py
index eac2f41768..054d56abec 100644
--- a/synapse/groups/groups_server.py
+++ b/synapse/groups/groups_server.py
@@ -69,7 +69,7 @@ class GroupsServerHandler(object):
             raise SynapseError(404, "Unknown group")
 
         is_user_in_group = yield self.store.is_user_in_group(requester_user_id, group_id)
-        if not is_user_in_group or not group.is_public:
+        if group and not is_user_in_group and not group.is_public:
             raise SynapseError(404, "Unknown group")
 
         if and_is_admin:

From e27b76d11728ba0fa2cbbd99ac50d33dee95da63 Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Fri, 27 Oct 2017 10:54:02 +0100
Subject: [PATCH 15/25] Import logger

---
 synapse/groups/attestations.py | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/synapse/groups/attestations.py b/synapse/groups/attestations.py
index 4656e854f0..c060cff5dd 100644
--- a/synapse/groups/attestations.py
+++ b/synapse/groups/attestations.py
@@ -13,6 +13,8 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import logging
+
 from twisted.internet import defer
 
 from synapse.api.errors import SynapseError
@@ -22,6 +24,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
 

From c7d9f25d2242db2a5674a76f074858dbcf216d04 Mon Sep 17 00:00:00 2001
From: Luke Barnard <lukeb@openmarket.com>
Date: Fri, 27 Oct 2017 10:57:20 +0100
Subject: [PATCH 16/25] Fix create_group to pass requester_user_id

---
 synapse/groups/groups_server.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/synapse/groups/groups_server.py b/synapse/groups/groups_server.py
index 054d56abec..175ff433a1 100644
--- a/synapse/groups/groups_server.py
+++ b/synapse/groups/groups_server.py
@@ -733,7 +733,7 @@ class GroupsServerHandler(object):
 
     @defer.inlineCallbacks
     def create_group(self, group_id, requester_user_id, content):
-        group = yield self.check_group_is_ours(group_id)
+        group = yield self.check_group_is_ours(group_id, requester_user_id)
 
         logger.info("Attempting to create group with ID: %r", group_id)
 

From 6362298fa5cf6d0b80b199372bc6682d3a6b8101 Mon Sep 17 00:00:00 2001
From: Luke Barnard <lukeb@openmarket.com>
Date: Fri, 27 Oct 2017 11:04:20 +0100
Subject: [PATCH 17/25] Create groups with is_public = True

---
 synapse/storage/group_server.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/synapse/storage/group_server.py b/synapse/storage/group_server.py
index 9e63db5c6c..d2437ff9c5 100644
--- a/synapse/storage/group_server.py
+++ b/synapse/storage/group_server.py
@@ -1026,6 +1026,7 @@ class GroupServerStore(SQLBaseStore):
                 "avatar_url": avatar_url,
                 "short_description": short_description,
                 "long_description": long_description,
+                "is_public": True,
             },
             desc="create_group",
         )

From 124314672fdc984255277e504215889ebd1de0ed Mon Sep 17 00:00:00 2001
From: Luke Barnard <lukeb@openmarket.com>
Date: Fri, 27 Oct 2017 11:08:19 +0100
Subject: [PATCH 18/25] group is dict

---
 synapse/groups/groups_server.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/synapse/groups/groups_server.py b/synapse/groups/groups_server.py
index 175ff433a1..4f9e459136 100644
--- a/synapse/groups/groups_server.py
+++ b/synapse/groups/groups_server.py
@@ -69,7 +69,7 @@ class GroupsServerHandler(object):
             raise SynapseError(404, "Unknown group")
 
         is_user_in_group = yield self.store.is_user_in_group(requester_user_id, group_id)
-        if group and not is_user_in_group and not group.is_public:
+        if group and not is_user_in_group and not group["is_public"]:
             raise SynapseError(404, "Unknown group")
 
         if and_is_admin:

From 5451cc77926750c7da73202cf3251a72c5a6d497 Mon Sep 17 00:00:00 2001
From: Luke Barnard <lukeb@openmarket.com>
Date: Fri, 27 Oct 2017 11:27:43 +0100
Subject: [PATCH 19/25] Request is_public from database

---
 synapse/storage/group_server.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/synapse/storage/group_server.py b/synapse/storage/group_server.py
index d2437ff9c5..095a3dd382 100644
--- a/synapse/storage/group_server.py
+++ b/synapse/storage/group_server.py
@@ -35,7 +35,9 @@ class GroupServerStore(SQLBaseStore):
             keyvalues={
                 "group_id": group_id,
             },
-            retcols=("name", "short_description", "long_description", "avatar_url",),
+            retcols=(
+                "name", "short_description", "long_description", "avatar_url", "is_public"
+            ),
             allow_none=True,
             desc="is_user_in_group",
         )

From c067088747bea9b50afb1c1fad94e83bead754e3 Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Fri, 27 Oct 2017 11:28:12 +0100
Subject: [PATCH 20/25] Add comment about attestations

---
 synapse/groups/attestations.py | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/synapse/groups/attestations.py b/synapse/groups/attestations.py
index b751cf5e43..c52e020989 100644
--- a/synapse/groups/attestations.py
+++ b/synapse/groups/attestations.py
@@ -13,6 +13,28 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+"""Attestations ensure that users and groups can't lie about their memberships.
+
+When a user joins a group the HS and GS swap attestations, which allow them
+both to independently prove to third parties their membership.These
+attestations have a validity period so need to be periodically renewed.
+
+If a user leaves (or gets kicked out of) a group, either side can still use
+their attestation to "prove" their membership, until the attestation expires.
+Therefore attestations shouldn't be relied on to prove membership in important
+cases, but can for less important situtations, e.g. showing a users membership
+of groups on their profile, showing flairs, etc.abs
+
+An attestsation is a signed blob of json that looks like:
+
+    {
+        "user_id": "@foo:a.example.com",
+        "group_id": "+bar:b.example.com",
+        "valid_until_ms": 1507994728530,
+        "signatures":{"matrix.org":{"ed25519:auto":"..."}}
+    }
+"""
+
 from twisted.internet import defer
 
 from synapse.api.errors import SynapseError

From ca571b0ec3674cd477e9da5f8e9d20c4dfcaf58b Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Fri, 27 Oct 2017 11:57:27 +0100
Subject: [PATCH 21/25] Add jitter to validity period of attestations

This helps ensure that the renewals of attestations are spread out more
evenly.
---
 synapse/groups/attestations.py | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/synapse/groups/attestations.py b/synapse/groups/attestations.py
index b751cf5e43..fc5f92121e 100644
--- a/synapse/groups/attestations.py
+++ b/synapse/groups/attestations.py
@@ -13,6 +13,8 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import random
+
 from twisted.internet import defer
 
 from synapse.api.errors import SynapseError
@@ -25,6 +27,11 @@ from signedjson.sign import sign_json
 # Default validity duration for new attestations we create
 DEFAULT_ATTESTATION_LENGTH_MS = 3 * 24 * 60 * 60 * 1000
 
+# We add some jitter to the validity duration of attestations so that if we
+# add lots of users at once we don't need to renew them all at once.
+# The jitter is a multiplier picked randomly between the first and second number
+DEFAULT_ATTESTATION_JITTER = (0.9, 1.3)
+
 # Start trying to update our attestations when they come this close to expiring
 UPDATE_ATTESTATION_TIME_MS = 1 * 24 * 60 * 60 * 1000
 
@@ -73,10 +80,14 @@ class GroupAttestationSigning(object):
         """Create an attestation for the group_id and user_id with default
         validity length.
         """
+        validity_period = DEFAULT_ATTESTATION_LENGTH_MS
+        validity_period *= random.uniform(*DEFAULT_ATTESTATION_JITTER)
+        valid_until_ms = int(self.clock.time_msec() + validity_period)
+
         return sign_json({
             "group_id": group_id,
             "user_id": user_id,
-            "valid_until_ms": self.clock.time_msec() + DEFAULT_ATTESTATION_LENGTH_MS,
+            "valid_until_ms": valid_until_ms,
         }, self.server_name, self.signing_key)
 
 

From 977078f06d173771cae66836b23ee76ef1a58e26 Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Fri, 27 Oct 2017 15:10:50 +0100
Subject: [PATCH 22/25] Fix bad merge

---
 synapse/groups/groups_server.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/synapse/groups/groups_server.py b/synapse/groups/groups_server.py
index 7406f67d07..b021b7f77f 100644
--- a/synapse/groups/groups_server.py
+++ b/synapse/groups/groups_server.py
@@ -646,7 +646,9 @@ 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)
+            local_attestation = self.attestations.create_attestation(
+                group_id, requester_user_id,
+            )
             remote_attestation = content["attestation"]
 
             yield self.attestations.verify_attestation(

From d0abb4e8e6d6577bbe07465f8568b4eccef2c9f3 Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Fri, 27 Oct 2017 16:57:19 +0100
Subject: [PATCH 23/25] Fix typo when checking if user is invited to group

---
 synapse/groups/groups_server.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/synapse/groups/groups_server.py b/synapse/groups/groups_server.py
index b021b7f77f..cb2ff76a0d 100644
--- a/synapse/groups/groups_server.py
+++ b/synapse/groups/groups_server.py
@@ -642,7 +642,10 @@ class GroupsServerHandler(object):
 
         yield self.check_group_is_ours(group_id, requester_user_id, and_exists=True)
 
-        if not self.store.is_user_invited_to_local_group(group_id, requester_user_id):
+        is_invited = yield self.store.is_user_invited_to_local_group(
+            group_id, requester_user_id,
+        )
+        if not is_invited:
             raise SynapseError(403, "User not invited to group")
 
         if not self.hs.is_mine_id(requester_user_id):

From e51c2bcaef4b15a1e24a31b7edbfefbf93b7c425 Mon Sep 17 00:00:00 2001
From: Matthew Hodgson <matthew@matrix.org>
Date: Sun, 29 Oct 2017 20:47:06 +0000
Subject: [PATCH 24/25] move url_previews to MD as RST does my head in

---
 docs/{url_previews.rst => url_previews.md} | 2 ++
 1 file changed, 2 insertions(+)
 rename docs/{url_previews.rst => url_previews.md} (99%)

diff --git a/docs/url_previews.rst b/docs/url_previews.md
similarity index 99%
rename from docs/url_previews.rst
rename to docs/url_previews.md
index 634d9d907f..665554e165 100644
--- a/docs/url_previews.rst
+++ b/docs/url_previews.md
@@ -56,6 +56,7 @@ As a first cut, let's do #2 and have the receiver hit the API to calculate its o
 API
 ---
 
+```
 GET /_matrix/media/r0/preview_url?url=http://wherever.com
 200 OK
 {
@@ -66,6 +67,7 @@ GET /_matrix/media/r0/preview_url?url=http://wherever.com
     "og:description" : "“Synapse 0.12 is out! Lots of polishing, performance &amp;amp; bugfixes: /sync API, /r0 prefix, fulltext search, 3PID invites https://t.co/5alhXLLEGP”"
     "og:site_name"   : "Twitter"
 }
+```
 
 * Downloads the URL
   * If HTML, just stores it in RAM and parses it for OG meta tags

From 208a6647f13ed508309523aa0ed7b0250c97f886 Mon Sep 17 00:00:00 2001
From: Matthew Hodgson <matthew@matrix.org>
Date: Sun, 29 Oct 2017 20:54:20 +0000
Subject: [PATCH 25/25] fix typo

---
 synapse/config/cas.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/synapse/config/cas.py b/synapse/config/cas.py
index 938f6f25f8..8109e5f95e 100644
--- a/synapse/config/cas.py
+++ b/synapse/config/cas.py
@@ -41,7 +41,7 @@ class CasConfig(Config):
         #cas_config:
         #   enabled: true
         #   server_url: "https://cas-server.com"
-        #   service_url: "https://homesever.domain.com:8448"
+        #   service_url: "https://homeserver.domain.com:8448"
         #   #required_attributes:
         #   #    name: value
         """