From cadd9441915850cbe636091adc2a537ca38f9f58 Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Thu, 11 Nov 2021 18:56:46 +0000 Subject: [PATCH 1/3] Avoid sharing room hierarchy responses between users Different users may be allowed to see different rooms within a space, so sharing responses between users is inadvisable. --- changelog.d/11355.bugfix | 1 + synapse/handlers/room_summary.py | 11 +++++++++-- 2 files changed, 10 insertions(+), 2 deletions(-) create mode 100644 changelog.d/11355.bugfix diff --git a/changelog.d/11355.bugfix b/changelog.d/11355.bugfix new file mode 100644 index 000000000000..91639f14b2ab --- /dev/null +++ b/changelog.d/11355.bugfix @@ -0,0 +1 @@ +Fix a bug introduced in 1.41.0 where space hierarchy responses would be incorrectly reused if multiple users were to make the same request at the same time. diff --git a/synapse/handlers/room_summary.py b/synapse/handlers/room_summary.py index fb26ee7ad7bc..8181cc0b5267 100644 --- a/synapse/handlers/room_summary.py +++ b/synapse/handlers/room_summary.py @@ -97,7 +97,7 @@ def __init__(self, hs: "HomeServer"): # If a user tries to fetch the same page multiple times in quick succession, # only process the first attempt and return its result to subsequent requests. self._pagination_response_cache: ResponseCache[ - Tuple[str, bool, Optional[int], Optional[int], Optional[str]] + Tuple[str, str, bool, Optional[int], Optional[int], Optional[str]] ] = ResponseCache( hs.get_clock(), "get_room_hierarchy", @@ -282,7 +282,14 @@ async def get_room_hierarchy( # This is due to the pagination process mutating internal state, attempting # to process multiple requests for the same page will result in errors. return await self._pagination_response_cache.wrap( - (requested_room_id, suggested_only, max_depth, limit, from_token), + ( + requester, + requested_room_id, + suggested_only, + max_depth, + limit, + from_token, + ), self._get_room_hierarchy, requester, requested_room_id, From 72725611f0d6d65a665378467fd379ae79c25d97 Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Tue, 16 Nov 2021 14:47:33 +0000 Subject: [PATCH 2/3] Add tests for `get_room_hierarchy` caching --- tests/handlers/test_room_summary.py | 55 +++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/tests/handlers/test_room_summary.py b/tests/handlers/test_room_summary.py index d3d0bf1ac57e..dbb4bc4658b5 100644 --- a/tests/handlers/test_room_summary.py +++ b/tests/handlers/test_room_summary.py @@ -14,6 +14,8 @@ from typing import Any, Iterable, List, Optional, Tuple from unittest import mock +from twisted.internet.defer import ensureDeferred + from synapse.api.constants import ( EventContentFields, EventTypes, @@ -316,6 +318,59 @@ def test_visibility(self): AuthError, ) + def test_room_hierarchy_cache(self) -> None: + """In-flight room hierarchy requests are deduplicated.""" + # Run two `get_room_hierarchy` calls up until they block. + deferred1 = ensureDeferred( + self.handler.get_room_hierarchy(self.user, self.space) + ) + deferred2 = ensureDeferred( + self.handler.get_room_hierarchy(self.user, self.space) + ) + + # Complete the two calls. + result1 = self.get_success(deferred1) + result2 = self.get_success(deferred2) + + # Both `get_room_hierarchy` calls should return the same result. + expected = [(self.space, [self.room]), (self.room, ())] + self._assert_hierarchy(result1, expected) + self._assert_hierarchy(result2, expected) + self.assertIs(result1, result2) + + # A subsequent `get_room_hierarchy` call should reuse the result. + result3 = self.get_success( + self.handler.get_room_hierarchy(self.user, self.space) + ) + self._assert_hierarchy(result3, expected) + self.assertIsNot(result1, result3) + + def test_room_hierarchy_cache_sharing(self) -> None: + """Room hierarchy responses for different users are not shared.""" + user2 = self.register_user("user2", "pass") + + # Make the room within the space invite-only. + self.helper.send_state( + self.room, + event_type=EventTypes.JoinRules, + body={"join_rule": JoinRules.INVITE}, + tok=self.token, + ) + + # Run two `get_room_hierarchy` calls for different users up until they block. + deferred1 = ensureDeferred( + self.handler.get_room_hierarchy(self.user, self.space) + ) + deferred2 = ensureDeferred(self.handler.get_room_hierarchy(user2, self.space)) + + # Complete the two calls. + result1 = self.get_success(deferred1) + result2 = self.get_success(deferred2) + + # The `get_room_hierarchy` calls should return different results. + self._assert_hierarchy(result1, [(self.space, [self.room]), (self.room, ())]) + self._assert_hierarchy(result2, [(self.space, [self.room])]) + def _create_room_with_join_rule( self, join_rule: str, room_version: Optional[str] = None, **extra_content ) -> str: From d8ce086ff8075264bf1d2ec93f8e5cb6928b5fdc Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Tue, 16 Nov 2021 14:55:38 +0000 Subject: [PATCH 3/3] fix comment --- tests/handlers/test_room_summary.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/handlers/test_room_summary.py b/tests/handlers/test_room_summary.py index dbb4bc4658b5..7b95844b55d3 100644 --- a/tests/handlers/test_room_summary.py +++ b/tests/handlers/test_room_summary.py @@ -338,7 +338,7 @@ def test_room_hierarchy_cache(self) -> None: self._assert_hierarchy(result2, expected) self.assertIs(result1, result2) - # A subsequent `get_room_hierarchy` call should reuse the result. + # A subsequent `get_room_hierarchy` call should not reuse the result. result3 = self.get_success( self.handler.get_room_hierarchy(self.user, self.space) )