Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add is_public to groups table to allow for private groups #2582

Merged
merged 12 commits into from
Oct 27, 2017

Conversation

lukebarnard1
Copy link
Contributor

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.

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.
Copy link
Member

@erikjohnston erikjohnston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PEP8 is sad

synapse/groups/groups_server.py:52:91: E501 line too long (100 > 90 characters)
synapse/groups/groups_server.py:160:91: E501 line too long (100 > 90 characters)
synapse/groups/groups_server.py:163:91: E501 line too long (116 > 90 characters)
synapse/groups/groups_server.py:182:91: E501 line too long (91 > 90 characters)
synapse/groups/groups_server.py:185:91: E501 line too long (116 > 90 characters)
synapse/groups/groups_server.py:223:91: E501 line too long (116 > 90 characters)
synapse/groups/groups_server.py:241:91: E501 line too long (116 > 90 characters)
synapse/groups/groups_server.py:277:91: E501 line too long (116 > 90 characters)
synapse/groups/groups_server.py:296:91: E501 line too long (116 > 90 characters)
synapse/groups/groups_server.py:626:91: E501 line too long (93 > 90 characters)
synapse/groups/groups_server.py:750:91: E501 line too long (97 > 90 characters)
synapse/rest/client/v2_alpha/groups.py:44:91: E501 line too long (100 > 90 characters)
synapse/rest/client/v2_alpha/groups.py:77:91: E501 line too long (100 > 90 characters)
synapse/rest/client/v2_alpha/groups.py:390:91: E501 line too long (98 > 90 characters)
synapse/rest/client/v2_alpha/groups.py:417:91: E501 line too long (93 > 90 characters)

@@ -67,6 +67,10 @@ def check_group_is_ours(self, group_id, and_exists=False, and_is_admin=None):
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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably only do this if the group exists, in particular this currently doesn't work well with create_group. (Speaking of which, you forgot to add requester_user_id to create_group, which is why its failing)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't that also be if not is_user_in_group...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes o\

*/

-- whether non-members can access group APIs
ALTER TABLE groups ADD COLUMN is_public BOOL DEFAULT 1 NOT NULL;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need to change the schema version in synapse.storage.prepare_database

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need to add this to the scripts/synapse_port_db list of tables with boolean columns

Luke Barnard added 2 commits October 26, 2017 17:55
Adding a column with non-constant default not possible in sqlite3
@erikjohnston
Copy link
Member

(Ideally we should also add a sytest for this)

@hawkowl hawkowl deleted the luke/group-is-public branch September 20, 2018 13:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants