Make AccessRules use the public rooms directory instead of checking a room's join rules on rule change #63

anoadragon453 · 2020-09-11T13:05:43Z

Requires matrix-org/synapse#8292 over on mainline, which we plan to cherry-pick for now.

This PR makes use of the new public room directory features added to ThirdPartyEventRules by the above PR, by:

Preventing publishing a room to the public rooms list if it has "unrestricted" access rule, during or after room creation.
Preventing a room from changing its access rule to "unrestricted" if it is currently published in the public rooms directory.

When reviewing this PR, it's recommend to skip the first commit, as it's simply the port of the above PR.

anoadragon453 · 2020-09-11T13:54:29Z

synapse/events/third_party_rules.py

@@ -76,8 +76,7 @@ def __init__(self, hs):
                public_room_manager=PublicRoomsManager(hs),
            )

-    @defer.inlineCallbacks
-    def check_event_allowed(self, event, context):
+    async def check_event_allowed(self, event, context):


We needed to async this method to eventually call the check on whether a room is public. Luckily this method is already awaited on everywhere!

synapse/handlers/room.py

synapse/third_party_rules/access_rules.py

synapse/events/third_party_rules.py

synapse/third_party_rules/access_rules.py

tests/rest/client/test_room_access_rules.py

synapse/third_party_rules/access_rules.py

synapse/events/third_party_rules.py

changelog.d/63.misc

changelog.d/8292.feature

synapse/third_party_rules/access_rules.py

anoadragon453 · 2020-09-11T17:25:52Z

Thanks @babolivier. I've made the requested changes - the rest need to be made on the mainline PR as it deals with code coming from there.

Otherwise we just need to make a decision with @giomfo about the logic change discussion here: #63 (comment)

Even if we decide to change it back, doing so is trivial :)

anoadragon453 · 2020-09-11T17:44:57Z

It was brought up that reverting the join_rules change in on_create_room is no different that simply creating a restricted room, and then setting its join rules to "public". This is intended behaviour, so we're going to continue removing that change in favour of using the public rooms list to manage visibility.

Suggested in matrix-org/synapse-dinsic#63

… room's join rules on rule change

anoadragon453 · 2020-09-14T18:32:21Z

I've rebased this PR on top of the latest mainline changes.

old

babolivier · 2020-09-15T09:49:18Z

It was brought up that reverting the join_rules change in on_create_room is no different that simply creating a restricted room, and then setting its join rules to "public". This is intended behaviour, so we're going to continue removing that change in favour of using the public rooms list to manage visibility.

Currently it isn't possible to change the join rules of a room to public if the access rule isn't restricted, see here, here and here. This is valid regardless of whether these rules are set on room creation or afterwards, so the case you're describing doesn't exist (outside of this PR). Doing so also deviates from the spec of im.vector.room.access_rules.

FWIW I'm strongly opposed to this change as it would be a security regression. The point of the public room directory is to advertise rooms, and it isn't in any shape of form an authorisation feature (which is precisely what the join rules are). If we dropped the checks on join rules in favour of checking the visibility of the room in the public room directory, nothing could prevent e.g. someone making up their own directory, outside of Matrix, which would list rooms that any blacklisted server can join without an invite (which isn't possible currently but would be made possible by this PR).

giomfo · 2020-09-15T12:56:24Z

@babolivier I understood your point of view, I will check again with DINUM if they are aware of this point that you named "security regression".
I don't think this is a "security regression", the actual room access will be still controlled by the im.vector.room.access_rules value.

I consider the sanity check on join_rule was introduced to limit the risk for an admin to give access to an exposed (=published) room to the users of the blacklisted servers without realising it. The DINUM request was already only for the rooms published in the room directory (which were the only rooms accessible by link).

Let's have a talk about this ;)

Note: I agree if the PR is merged, the following spec must be updated:

giomfo · 2020-09-15T14:50:08Z

I got an interesting suggestion from DINUM:
"A user from a blacklisted server must have an invite to join a room even if the room is accessible by link"

This means an update about the "unrestricted" preset:
"Doesn't apply any restriction on who can join the room, but a user from a blacklisted server must have an invite to join the room (even if the join_rule is public)

Correct method paths in tests.module_api.test_api

synapse/third_party_rules/access_rules.py

…ricted w invite Also do some public rooms directory -> public room list wording fixes.

giomfo

SGTM (I checked mainly the new logic about the room access)
I let @babolivier review too

synapse/third_party_rules/access_rules.py

tests/rest/client/test_room_access_rules.py

babolivier

Bit of docs that can use a bit more polishing, otherwise lgtm 🎉

synapse/third_party_rules/access_rules.py

tests/rest/client/test_room_access_rules.py

synapse/third_party_rules/access_rules.py

…oa/freeze_rooms * 'dinsic' of github.com:matrix-org/synapse-dinsic: Only assert valid next_link params when provided (#65) Don't push if an user account has expired (#58) Swap method calls in RoomAccessTestCase.test_change_rules (#64) Make all rooms noisy by default (#60) Make AccessRules use the public rooms directory instead of checking a room's join rules on rule change (#63) Override the power levels defaults, enforce mod requirement for invites, admin requirements for unknown state events (#61) RoomAccessRules cleanup (#62) Add a config option for validating 'next_link' parameters against a domain whitelist (#8275)

anoadragon453 force-pushed the anoa/access_rules_public_rooms branch 2 times, most recently from 5107fd4 to 47b604e Compare September 11, 2020 13:48

anoadragon453 commented Sep 11, 2020

View reviewed changes

giomfo reviewed Sep 11, 2020

View reviewed changes

synapse/handlers/room.py Outdated Show resolved Hide resolved

giomfo reviewed Sep 11, 2020

View reviewed changes

synapse/third_party_rules/access_rules.py Outdated Show resolved Hide resolved

anoadragon453 force-pushed the anoa/access_rules_public_rooms branch 2 times, most recently from 8aca853 to 07d1dae Compare September 11, 2020 16:46

babolivier previously requested changes Sep 11, 2020

View reviewed changes

anoadragon453 commented Sep 11, 2020

View reviewed changes

synapse/third_party_rules/access_rules.py Outdated Show resolved Hide resolved

anoadragon453 added a commit to matrix-org/synapse that referenced this pull request Sep 14, 2020

Some minor fixes

6ed082d

Suggested in matrix-org/synapse-dinsic#63

anoadragon453 added 2 commits September 14, 2020 19:19

Allow ThirdPartyEventRules modules to manipulate public room state

81d0fd6

Make AccessRules use the public rooms directory instead of checking a…

3a1adf3

… room's join rules on rule change

anoadragon453 force-pushed the anoa/access_rules_public_rooms branch from 93f1b02 to c949c30 Compare September 14, 2020 18:31

anoadragon453 force-pushed the anoa/access_rules_public_rooms branch from c949c30 to 507d3a2 Compare September 14, 2020 18:33

Changelog

19eb148

anoadragon453 force-pushed the anoa/access_rules_public_rooms branch from 507d3a2 to 19eb148 Compare September 15, 2020 09:26

anoadragon453 requested a review from babolivier September 15, 2020 09:35

anoadragon453 added 6 commits September 15, 2020 18:38

Prevent external users from joining rooms if they are not in the PRL

064745f

Fix module_api.public_room_list_manager call

4a0fcbd

async get_event_allowed

c868363

async on_create_room

f5bf163

Modify and add tests for public room list vs public join rules

0eb372b

Correct method paths in tests.module_api.test_api

Fix double-negative

e952eab

anoadragon453 force-pushed the anoa/access_rules_public_rooms branch from afbe238 to e952eab Compare September 16, 2020 10:23

anoadragon453 added 2 commits September 16, 2020 17:00

Update to deny a room join from a forbidden user with no invite

7a697a6

Allow only unrestricted and restricted rooms to have public join_rules

ec11ee0

anoadragon453 mentioned this pull request Sep 16, 2020

Swap method calls in RoomAccessTestCase.test_change_rules #64

Merged

giomfo reviewed Sep 16, 2020

View reviewed changes

synapse/third_party_rules/access_rules.py Outdated Show resolved Hide resolved

synapse/third_party_rules/access_rules.py Outdated Show resolved Hide resolved

synapse/third_party_rules/access_rules.py Outdated Show resolved Hide resolved

anoadragon453 added 2 commits September 17, 2020 10:50

Make use of get_domain_from_id

a66641d

Test forbidden users cannot join restricted room, but can join unrest…

951b956

…ricted w invite Also do some public rooms directory -> public room list wording fixes.

giomfo approved these changes Sep 17, 2020

View reviewed changes

Remove public_Chat restriction on unrestricted rooms

d3c5880

babolivier suggested changes Sep 17, 2020

View reviewed changes

anoadragon453 added 2 commits September 17, 2020 17:42

wording + fix check for whether a user has been invited to a room

f977611

Testing fixes from review comments

07dd721

babolivier approved these changes Sep 17, 2020

View reviewed changes

synapse/third_party_rules/access_rules.py Show resolved Hide resolved

tests/rest/client/test_room_access_rules.py Outdated Show resolved Hide resolved

synapse/third_party_rules/access_rules.py Outdated Show resolved Hide resolved

Wording fixes

c34b5bb

anoadragon453 merged commit 3fe1c84 into dinsic Sep 18, 2020

anoadragon453 deleted the anoa/access_rules_public_rooms branch September 18, 2020 10:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make AccessRules use the public rooms directory instead of checking a room's join rules on rule change #63

Make AccessRules use the public rooms directory instead of checking a room's join rules on rule change #63

anoadragon453 commented Sep 11, 2020 •

edited

Loading

anoadragon453 Sep 11, 2020

anoadragon453 commented Sep 11, 2020

anoadragon453 commented Sep 11, 2020 •

edited

Loading

anoadragon453 commented Sep 14, 2020

babolivier commented Sep 15, 2020 •

edited

Loading

giomfo commented Sep 15, 2020

giomfo commented Sep 15, 2020 •

edited

Loading

giomfo left a comment

babolivier left a comment

Make AccessRules use the public rooms directory instead of checking a room's join rules on rule change #63

Make AccessRules use the public rooms directory instead of checking a room's join rules on rule change #63

Conversation

anoadragon453 commented Sep 11, 2020 • edited Loading

anoadragon453 Sep 11, 2020

Choose a reason for hiding this comment

anoadragon453 commented Sep 11, 2020

anoadragon453 commented Sep 11, 2020 • edited Loading

anoadragon453 commented Sep 14, 2020

babolivier commented Sep 15, 2020 • edited Loading

giomfo commented Sep 15, 2020

giomfo commented Sep 15, 2020 • edited Loading

giomfo left a comment

Choose a reason for hiding this comment

babolivier left a comment

Choose a reason for hiding this comment

anoadragon453 commented Sep 11, 2020 •

edited

Loading

anoadragon453 commented Sep 11, 2020 •

edited

Loading

babolivier commented Sep 15, 2020 •

edited

Loading

giomfo commented Sep 15, 2020 •

edited

Loading