Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Filter out rooms from the room directory being served to other homeservers when those rooms block that homeserver by their Access Control Lists. #16759

Merged
merged 12 commits into from Jan 8, 2024
Merged

Filter out rooms from the room directory being served to other homeservers when those rooms block that homeserver by their Access Control Lists. #16759

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
5413007
Pass `from_federation_origin` down into room list retrieval code
reivilibre Dec 19, 2023
7c0354b
Don't cache /publicRooms response for inbound federated requests
reivilibre Dec 19, 2023
efc6966
fixup! Don't cache /publicRooms response for inbound federated requests
reivilibre Dec 19, 2023
f7a2078
Cap the number of /publicRooms entries to 100
reivilibre Dec 19, 2023
a937f08
Simplify code now that you can't request unlimited rooms
reivilibre Dec 19, 2023
f969202
Filter out rooms from federated requests that don't have the correct ACL
reivilibre Dec 20, 2023
c04dff6
Request a handful more when filtering ACLs so that we can try to avoi…
reivilibre Dec 20, 2023
d2ad5b9
Newsfile
reivilibre Dec 21, 2023
b2cab24
Correctly emit `prev_batch` even if no results found in this batch
reivilibre Jan 2, 2024
db6a030
Add a test
reivilibre Jan 3, 2024
f260a7e
Use (0, empty room ID) as first-from-last pagination token, not the w…
reivilibre Jan 8, 2024
a51b959
Change log message (and move to DEBUG since it is fairly inappropriat…
reivilibre Jan 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 23 additions & 9 deletions synapse/handlers/room_list.py
View file
Open in desktop
Expand Up @@ -272,19 +272,33 @@ def build_room_entry(room: LargestRoomStats) -> JsonDict:

more_to_come_from_database = num_results == probing_limit

if forwards and has_batch_token:
# If there was a token given then we assume that there
# must be previous results, even if there were no results in this batch.
if first_considered_room is not None:
response["prev_batch"] = RoomListNextBatch(
last_joined_members=first_considered_room.joined_members,
last_room_id=first_considered_room.room_id,
direction_is_forward=False,
).to_token()
else:
# If we didn't find any results this time,
# we don't have an actual room ID to put in the token.
# So instead we re-use the room ID from last time but make the
# bound inclusive, by tacking on a 0x01 byte at the end
# (s+"\x00" is the first string following s, but we can't use "\x00"
# in Postgres, so use "\x01" anyway.)
Copy link
Member

Choose a reason for hiding this comment

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

Is this an existing bug or is this somehow caused by the new filtering? I'm somewhat failing to see why we need to add this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is an existing bug. I hit it during testing and thought it was suspicious enough it was worth fixing

Copy link
Member

Choose a reason for hiding this comment

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

Ah, right. If first_considered_room is None then doesn't that mean that we have reached the end of the list and there are no more rooms? And if so then can we not reuse since_token for the bound?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the previous chunk had rooms !A, !B, !C then (from memory) you get a next_chunk token saying !C. The query that follows for the next request has a bound of > !C.

To go back to the prior chunk again, you need the query to have a bound of < !D. But if we have an empty chunk, we don't have any known room ID to stand in for !D, hence the idea of cheekily constructing a fake one "!C\x01"...

If we give the since_token of !C back, then going back to the prior chunk will only return !A, !B as rooms.

Copy link
Member

Choose a reason for hiding this comment

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

Oh right, yes I see. Has to be said that I'm not a huge fan of just chucking a byte on the end, but I suppose it works.

Can we use the RoomListNextBatch(0, "") since we know we're at the end? Or maybe we should always use the since_token for the prev batch and change the equality bounds on the DB query to avoid all this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

those are a bit cleverer, yeah. RoomListNextBatch(0, "") sounds like a reasonable idea to me. I think flipping the bounds would also have been fine but I know there is scope to mess that up fairly easily...

Copy link
Member

Choose a reason for hiding this comment

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

Yup fair. I think your solution would have worked fine, but might just cause confusion having invalid room IDs in it, I dunno 🤷 Thanks for changing

assert batch_token is not None
response["prev_batch"] = RoomListNextBatch(
last_joined_members=batch_token.last_joined_members,
last_room_id=batch_token.last_room_id + "\x01",
direction_is_forward=False,
).to_token()

if num_results > 0:
assert first_considered_room is not None
assert last_considered_room is not None
if forwards:
if has_batch_token:
# If there was a token given then we assume that there
# must be previous results.
response["prev_batch"] = RoomListNextBatch(
last_joined_members=first_considered_room.joined_members,
last_room_id=first_considered_room.room_id,
direction_is_forward=False,
).to_token()

if more_to_come_from_database or cut_off_due_to_limit:
response["next_batch"] = RoomListNextBatch(
last_joined_members=last_considered_room.joined_members,
Expand Down