From 9b2feef9eb9502bf07d51378c75fc6b690a15676 Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Thu, 26 Oct 2017 16:51:32 +0100 Subject: [PATCH] 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;