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.

3 files changed, 116 insertions, 95 deletions

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;