Prevent showing non-fed rooms in fed /publicRooms by erikjohnston · Pull Request #4736 · matrix-org/synapse

erikjohnston · 2019-02-25T15:11:41Z

Replaces #4714

…lic_rooms_federate

synapse/handlers/room_list.py

codecov-io · 2019-02-25T15:27:28Z

Codecov Report

Merging #4736 into develop will decrease coverage by 0.05%.
The diff coverage is 50%.

@@             Coverage Diff             @@
##           develop    #4736      +/-   ##
===========================================
- Coverage    75.14%   75.09%   -0.06%     
===========================================
  Files          340      340              
  Lines        34826    34861      +35     
  Branches      5704     5711       +7     
===========================================
+ Hits         26170    26178       +8     
- Misses        7046     7071      +25     
- Partials      1610     1612       +2

richvdh

generally looks pretty sane, though I am led to the question of why you would ever want to set this option to True?

richvdh · 2019-02-25T18:25:13Z

synapse/handlers/room_list.py

                                   search_filter=None,
-                                   network_tuple=EMPTY_THIRD_PARTY_ID,):
+                                   network_tuple=EMPTY_THIRD_PARTY_ID,
+                                   from_federation=False):


docstring pls

richvdh · 2019-02-26T10:46:43Z

synapse/handlers/room_list.py

+
+        Args:
+            room_id (str): The room's ID.
+            disabled should be shown.


richvdh · 2019-02-26T10:48:30Z

synapse/handlers/room_list.py

+            if result["m.federate"] == False:
+                # This is a non-federating room and the config has chosen not
+                # to show these rooms to other servers
+                chunk.append(None)


why do we not just pass ?

The original code was returning None and then appending it to the chunk, so I kept doing so. Not sure if it's necessary or we can just leave it out.

Looking at the code, I don't think Nones in there are particularly useful, so just not inserting anything (by returning instead) seems like the right way to go.

richvdh · 2019-02-26T10:49:25Z

synapse/handlers/room_list.py


-        if result and _matches_room_entry(result, search_filter):
+        if from_federation and not self.config.allow_non_federated_in_public_rooms:
+            if result["m.federate"] == False:


result may be None. Suggest adding

if not result: return

richvdh · 2019-02-26T10:50:44Z

synapse/handlers/room_list.py

                              search_filter=None,
                              network_tuple=EMPTY_THIRD_PARTY_ID,
-                              timeout=None,):
+                              from_federation=False, timeout=None,):


Suggested change

from_federation=False, timeout=None,):

from_federation=False,

timeout=None,

):

(or whatever the right alignment for the closing paren is with this style)

richvdh · 2019-02-26T10:52:07Z

synapse/handlers/room_list.py

    @defer.inlineCallbacks
    def _append_room_entry_to_chunk(self, room_id, num_joined_users, chunk, limit,
-                                    search_filter):
+                                    search_filter, from_federation=False):


docstring for from_federation wouldn't hurt.

richvdh · 2019-02-26T11:51:53Z

synapse/handlers/room_list.py

+        """Generate a public room list.
+
+        Args:
+            limit (int): Maximum amount of rooms to return.


richvdh · 2019-02-26T11:52:02Z

synapse/handlers/room_list.py

+
+        Args:
+            limit (int): Maximum amount of rooms to return.
+            since_token (str)


richvdh · 2019-02-26T11:52:09Z

synapse/handlers/room_list.py

+        Args:
+            limit (int): Maximum amount of rooms to return.
+            since_token (str)
+            search_filter (dict): Dictionary to filter rooms by.


dict|None

richvdh · 2019-02-26T11:52:22Z

synapse/handlers/room_list.py

+                Setting to None returns all public rooms across all lists.
+            from_federation (bool): Whether this request originated from a
+                federating server or a client. Used for room filtering.
+            timeout (int): Amount of seconds to wait for a response before


richvdh · 2019-02-26T11:53:40Z

synapse/handlers/room_list.py

        to the `chunk` if it matches the search filter
+
+        Args:
+            room_id (str): The ID of the room.


Again, if you're going to add these, please get the types right

synapse/synapse/storage/state.py

Line 486 in 40c2271

room_id (str)

Are they supposed to be RoomIDs?

Ah, you meant adding |None. Have done so for the other types yes.

richvdh · 2019-02-26T11:56:01Z

synapse/handlers/room_list.py


        if from_federation and not self.config.allow_non_federated_in_public_rooms:
-            if result["m.federate"] == False:
+            if not result or result["m.federate"] is False:


not result["m.federate"] is clearer

The problem was that if "m.federate" wasn't in result, it would imply a federating room.

I've realized this logic doesn't cover that and could lead to a crash, and thus have replaced with:

if from_federation: if "m.federate" in result and not result["m.federate"]: # This is a room that other servers cannot join. Do not show them # this room. return

well, m.federate will always be present in this case afaict.

However, for the record, what you want to handle the case you mention is if not result.get("m.federate", True).

Also, consider merging it with the other if: if from_federation and result...

Yep, split it up as two ands was getting unclean. This is cleaner though, thank you.

richvdh · 2019-02-26T11:59:04Z

synapse/handlers/room_list.py

-            num_joined_users)
+        result = yield self.generate_room_entry(room_id, num_joined_users)

        if from_federation and not self.config.allow_non_federated_in_public_rooms:


unless I'm completely misreading this, it will never do anything if from_federation is true.

I still suggest adding:

if not result: return

before looking at from_federation or whatever.

Addressed changes.

richvdh

lgtm \o/

anoadragon453 · 2019-02-26T12:30:18Z

\ooooooooooooo/

anoadragon453 and others added 4 commits February 25, 2019 15:04

Config option to prevent showing non-fed rooms in fed /publicRooms

e4b078a

Don't restrict non-fed rooms over client APIs

bd398b8

Docs and arg name clarification

0749360

Merge branch 'develop' of github.com:matrix-org/synapse into anoa/pub…

4b9e507

…lic_rooms_federate

erikjohnston requested a review from a team February 25, 2019 15:12

erikjohnston commented Feb 25, 2019

View reviewed changes

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

anoadragon453 added 2 commits February 25, 2019 15:23

Simplify call to generate_room_entry

84c0a20

syntax derp

8aaf7ff

anoadragon453 added 2 commits February 25, 2019 17:28

elif not else if

1330aa4

Fix group's call to generate_room_entry

96c4082

richvdh reviewed Feb 26, 2019

View reviewed changes

anoadragon453 added 2 commits February 26, 2019 11:25

Address rich comments

71669a0

Result may be None

6946c20

anoadragon453 requested a review from richvdh February 26, 2019 11:36

Make not showing non-federated rooms the default

6728bf3

richvdh previously requested changes Feb 26, 2019

View reviewed changes

anoadragon453 added 3 commits February 26, 2019 12:04

Clean up room chunk logic

40c2271

Correct docstring types and chunk logic

16565e6

Correct indent

a712aa3

anoadragon453 requested a review from richvdh February 26, 2019 12:18

Cleaner chunk logic

c441476

richvdh approved these changes Feb 26, 2019

View reviewed changes

anoadragon453 merged commit a1a6473 into develop Feb 26, 2019

anoadragon453 deleted the anoa/public_rooms_federate branch February 26, 2019 13:07

anoadragon453 changed the title ~~Config option to prevent showing non-fed rooms in fed /publicRooms~~ Prevent showing non-fed rooms in fed /publicRooms Feb 26, 2019

anoadragon453 mentioned this pull request Feb 26, 2019

Revert "Prevent showing non-fed rooms in fed /publicRooms" #4745

Merged

anoadragon453 restored the anoa/public_rooms_federate branch February 26, 2019 13:25

This was referenced Feb 26, 2019

Prevent showing non-fed rooms in fed /publicRooms v2 #4746

Merged

[DINSIC] Prevent showing non-fed rooms in fed /publicRooms #4747

Merged

anoadragon453 mentioned this pull request Apr 29, 2020

Apply federation check for /publicRooms with filter list #7367

Merged

Uh oh!

Conversation

erikjohnston commented Feb 25, 2019

Uh oh!

Uh oh!

codecov-io commented Feb 25, 2019 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

anoadragon453 commented Feb 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Feb 25, 2019 •

edited by codecov bot

Loading