From 06be1aa1bd4d8e2317b4482bedf98d3b04742e5a Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Wed, 17 Aug 2022 09:31:41 +0200 Subject: [PATCH 1/5] Fix that user cannot `/forget` rooms after the last member has left --- synapse/handlers/room_member.py | 7 +- tests/handlers/test_room_member.py | 126 ++++++++++++++++++++++++++++- 2 files changed, 128 insertions(+), 5 deletions(-) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 70dc69c80931..9535e2446b2a 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -1923,8 +1923,11 @@ async def forget(self, user: UserID, room_id: str) -> None: ]: raise SynapseError(400, "User %s in room %s" % (user_id, room_id)) - if membership: - await self.store.forget(user_id, room_id) + # In normal case this call is only required if `membership` is not `None`. + # But: After the last member had left the room, the background update + # `_background_remove_left_rooms` is deleting rows related to this room from + # the table `current_state_events` and `get_current_state_events` is `None`. + await self.store.forget(user_id, room_id) def get_users_which_can_issue_invite(auth_events: StateMap[EventBase]) -> List[str]: diff --git a/tests/handlers/test_room_member.py b/tests/handlers/test_room_member.py index b4e1405aee17..667e2f625545 100644 --- a/tests/handlers/test_room_member.py +++ b/tests/handlers/test_room_member.py @@ -6,7 +6,7 @@ import synapse.rest.client.login import synapse.rest.client.room from synapse.api.constants import EventTypes, Membership -from synapse.api.errors import LimitExceededError +from synapse.api.errors import LimitExceededError, SynapseError from synapse.crypto.event_signing import add_hashes_and_signatures from synapse.events import FrozenEventV3 from synapse.federation.federation_client import SendJoinResult @@ -16,8 +16,12 @@ from tests.replication._base import RedisMultiWorkerStreamTestCase from tests.server import make_request -from tests.test_utils import make_awaitable -from tests.unittest import FederatingHomeserverTestCase, override_config +from tests.test_utils import event_injection, make_awaitable +from tests.unittest import ( + FederatingHomeserverTestCase, + HomeserverTestCase, + override_config, +) class TestJoinsLimitedByPerRoomRateLimiter(FederatingHomeserverTestCase): @@ -287,3 +291,119 @@ def test_local_users_joining_on_another_worker_contribute_to_rate_limit( ), LimitExceededError, ) + + +class RoomMemberMasterHandlerTestCase(HomeserverTestCase): + servlets = [ + synapse.rest.admin.register_servlets, + synapse.rest.client.login.register_servlets, + synapse.rest.client.room.register_servlets, + ] + + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: + self.handler = hs.get_room_member_handler() + self.store = hs.get_datastores().main + + # Create two users. + self.alice = self.register_user("alice", "pass") + self.alice_ID = UserID.from_string(self.alice) + self.alice_token = self.login("alice", "pass") + self.bob = self.register_user("bob", "pass") + self.bob_ID = UserID.from_string(self.bob) + self.bob_token = self.login("bob", "pass") + + # Create a room on this homeserver. + self.room_id = self.helper.create_room_as(self.alice, tok=self.alice_token) + + def test_leave_and_forget(self) -> None: + """Tests that forget a room is successfully. The test is performed with two users, + as forgetting by the last user respectively after all users had left the + is a special edge case.""" + self.helper.join(self.room_id, user=self.bob, tok=self.bob_token) + + # alice is not the last room member that leaves and forgets the room + self.helper.leave(self.room_id, user=self.alice, tok=self.alice_token) + self.get_success(self.handler.forget(self.alice_ID, self.room_id)) + self.assertTrue( + self.get_success(self.store.did_forget(self.alice, self.room_id)) + ) + + # TODO: server has not forgotten the room + # self.assertFalse( + # self.get_success(self.store.is_locally_forgotten_room(self.room_id)) + # ) + + def test_leave_and_forget_last_user(self) -> None: + """Tests that forget a room is successfully when the last user has left the room.""" + + # alice is the last room member that leaves and forgets the room + self.helper.leave(self.room_id, user=self.alice, tok=self.alice_token) + self.get_success(self.handler.forget(self.alice_ID, self.room_id)) + self.assertTrue( + self.get_success(self.store.did_forget(self.alice, self.room_id)) + ) + + # TODO: server has forgotten the room + # self.assertTrue( + # self.get_success(self.store.is_locally_forgotten_room(self.room_id)) + # ) + + def test_forget_when_not_left(self) -> None: + """Tests that a user cannot not forgets a room that has not left.""" + self.get_failure(self.handler.forget(self.alice_ID, self.room_id), SynapseError) + + def test_rejoin_forgotten_by_server(self) -> None: + """Test that a user that has forgotten a room can do a re-join. + The room was also forgotten from the local server and only + remote users are in the room.""" + self.get_success( + event_injection.inject_member_event( + self.hs, self.room_id, "@charlie:elsewhere", "join" + ) + ) + + self.helper.leave(self.room_id, user=self.alice, tok=self.alice_token) + self.get_success(self.handler.forget(self.alice_ID, self.room_id)) + self.assertTrue( + self.get_success(self.store.did_forget(self.alice, self.room_id)) + ) + + # TODO: server has forgotten the room + # self.assertTrue( + # self.get_success(self.store.is_locally_forgotten_room(self.room_id)) + # ) + + self.helper.join(self.room_id, user=self.alice, tok=self.alice_token) + self.assertFalse( + self.get_success(self.store.did_forget(self.alice, self.room_id)) + ) + + # TODO: server has not forgotten the room + # self.assertFalse( + # self.get_success(self.store.is_locally_forgotten_room(self.room_id)) + # ) + + def test_rejoin_forgotten_by_user(self) -> None: + """Test that a user that has forgotten a room can do a re-join. + The room was not forgotten from the local server. + One local user is still member of the room.""" + self.helper.join(self.room_id, user=self.bob, tok=self.bob_token) + + self.helper.leave(self.room_id, user=self.alice, tok=self.alice_token) + self.get_success(self.handler.forget(self.alice_ID, self.room_id)) + self.assertTrue( + self.get_success(self.store.did_forget(self.alice, self.room_id)) + ) + + # TODO: server has forgotten the room + # self.assertFalse( + # self.get_success(self.store.is_locally_forgotten_room(self.room_id)) + # ) + + self.helper.join(self.room_id, user=self.alice, tok=self.alice_token) + # TODO: A join to a room does not invalidate the forgotten cache + # see https://github.com/matrix-org/synapse/issues/13262 + self.store.did_forget.invalidate_all() + self.assertFalse( + self.get_success(self.store.did_forget(self.alice, self.room_id)) + ) From efd0e193a88b88ef9753ca657503a4389bce2395 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Wed, 17 Aug 2022 09:41:17 +0200 Subject: [PATCH 2/5] newsfile --- changelog.d/13546.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/13546.bugfix diff --git a/changelog.d/13546.bugfix b/changelog.d/13546.bugfix new file mode 100644 index 000000000000..83bc3a61d2c6 --- /dev/null +++ b/changelog.d/13546.bugfix @@ -0,0 +1 @@ +Fix bug that user cannot `/forget` rooms after the last member has left the room. \ No newline at end of file From aa302cff783718228f2a3f4e614f2b8fc3624e35 Mon Sep 17 00:00:00 2001 From: dklimpel <5740567+dklimpel@users.noreply.github.com> Date: Wed, 17 Aug 2022 12:28:21 +0200 Subject: [PATCH 3/5] update tests --- tests/handlers/test_room_member.py | 42 ++++++++++++++++-------------- tests/storage/test_roommember.py | 4 +-- 2 files changed, 24 insertions(+), 22 deletions(-) diff --git a/tests/handlers/test_room_member.py b/tests/handlers/test_room_member.py index 667e2f625545..264552078502 100644 --- a/tests/handlers/test_room_member.py +++ b/tests/handlers/test_room_member.py @@ -328,10 +328,10 @@ def test_leave_and_forget(self) -> None: self.get_success(self.store.did_forget(self.alice, self.room_id)) ) - # TODO: server has not forgotten the room - # self.assertFalse( - # self.get_success(self.store.is_locally_forgotten_room(self.room_id)) - # ) + # the server has not forgotten the room + self.assertFalse( + self.get_success(self.store.is_locally_forgotten_room(self.room_id)) + ) def test_leave_and_forget_last_user(self) -> None: """Tests that forget a room is successfully when the last user has left the room.""" @@ -343,10 +343,10 @@ def test_leave_and_forget_last_user(self) -> None: self.get_success(self.store.did_forget(self.alice, self.room_id)) ) - # TODO: server has forgotten the room - # self.assertTrue( - # self.get_success(self.store.is_locally_forgotten_room(self.room_id)) - # ) + # the server has forgotten the room + self.assertTrue( + self.get_success(self.store.is_locally_forgotten_room(self.room_id)) + ) def test_forget_when_not_left(self) -> None: """Tests that a user cannot not forgets a room that has not left.""" @@ -356,6 +356,8 @@ def test_rejoin_forgotten_by_server(self) -> None: """Test that a user that has forgotten a room can do a re-join. The room was also forgotten from the local server and only remote users are in the room.""" + + # add remote user self.get_success( event_injection.inject_member_event( self.hs, self.room_id, "@charlie:elsewhere", "join" @@ -368,20 +370,20 @@ def test_rejoin_forgotten_by_server(self) -> None: self.get_success(self.store.did_forget(self.alice, self.room_id)) ) - # TODO: server has forgotten the room - # self.assertTrue( - # self.get_success(self.store.is_locally_forgotten_room(self.room_id)) - # ) + # the server has forgotten the room + self.assertTrue( + self.get_success(self.store.is_locally_forgotten_room(self.room_id)) + ) self.helper.join(self.room_id, user=self.alice, tok=self.alice_token) self.assertFalse( self.get_success(self.store.did_forget(self.alice, self.room_id)) ) - # TODO: server has not forgotten the room - # self.assertFalse( - # self.get_success(self.store.is_locally_forgotten_room(self.room_id)) - # ) + # the server has not forgotten the room anymore + self.assertFalse( + self.get_success(self.store.is_locally_forgotten_room(self.room_id)) + ) def test_rejoin_forgotten_by_user(self) -> None: """Test that a user that has forgotten a room can do a re-join. @@ -395,10 +397,10 @@ def test_rejoin_forgotten_by_user(self) -> None: self.get_success(self.store.did_forget(self.alice, self.room_id)) ) - # TODO: server has forgotten the room - # self.assertFalse( - # self.get_success(self.store.is_locally_forgotten_room(self.room_id)) - # ) + # the server has not forgotten the room + self.assertFalse( + self.get_success(self.store.is_locally_forgotten_room(self.room_id)) + ) self.helper.join(self.room_id, user=self.alice, tok=self.alice_token) # TODO: A join to a room does not invalidate the forgotten cache diff --git a/tests/storage/test_roommember.py b/tests/storage/test_roommember.py index ceec69028513..8794401823e1 100644 --- a/tests/storage/test_roommember.py +++ b/tests/storage/test_roommember.py @@ -158,7 +158,7 @@ def test__null_byte_in_display_name_properly_handled(self) -> None: # Check that alice's display name is now None self.assertEqual(row[0]["display_name"], None) - def test_room_is_locally_forgotten(self): + def test_room_is_locally_forgotten(self) -> None: """Test that when the last local user has forgotten a room it is known as forgotten.""" # join two local and one remote user self.room = self.helper.create_room_as(self.u_alice, tok=self.t_alice) @@ -199,7 +199,7 @@ def test_room_is_locally_forgotten(self): self.get_success(self.store.is_locally_forgotten_room(self.room)) ) - def test_join_locally_forgotten_room(self): + def test_join_locally_forgotten_room(self) -> None: """Tests if a user joins a forgotten room the room is not forgotten anymore.""" self.room = self.helper.create_room_as(self.u_alice, tok=self.t_alice) self.assertFalse( From 847d8ba8b4206acbef00db912ac65ee76c54d805 Mon Sep 17 00:00:00 2001 From: Dirk Klimpel <5740567+dklimpel@users.noreply.github.com> Date: Fri, 26 Aug 2022 08:12:15 +0200 Subject: [PATCH 4/5] Remove test `test_rejoin_forgotten_by_server` --- tests/handlers/test_room_member.py | 33 ------------------------------ 1 file changed, 33 deletions(-) diff --git a/tests/handlers/test_room_member.py b/tests/handlers/test_room_member.py index 264552078502..886720214fe6 100644 --- a/tests/handlers/test_room_member.py +++ b/tests/handlers/test_room_member.py @@ -352,39 +352,6 @@ def test_forget_when_not_left(self) -> None: """Tests that a user cannot not forgets a room that has not left.""" self.get_failure(self.handler.forget(self.alice_ID, self.room_id), SynapseError) - def test_rejoin_forgotten_by_server(self) -> None: - """Test that a user that has forgotten a room can do a re-join. - The room was also forgotten from the local server and only - remote users are in the room.""" - - # add remote user - self.get_success( - event_injection.inject_member_event( - self.hs, self.room_id, "@charlie:elsewhere", "join" - ) - ) - - self.helper.leave(self.room_id, user=self.alice, tok=self.alice_token) - self.get_success(self.handler.forget(self.alice_ID, self.room_id)) - self.assertTrue( - self.get_success(self.store.did_forget(self.alice, self.room_id)) - ) - - # the server has forgotten the room - self.assertTrue( - self.get_success(self.store.is_locally_forgotten_room(self.room_id)) - ) - - self.helper.join(self.room_id, user=self.alice, tok=self.alice_token) - self.assertFalse( - self.get_success(self.store.did_forget(self.alice, self.room_id)) - ) - - # the server has not forgotten the room anymore - self.assertFalse( - self.get_success(self.store.is_locally_forgotten_room(self.room_id)) - ) - def test_rejoin_forgotten_by_user(self) -> None: """Test that a user that has forgotten a room can do a re-join. The room was not forgotten from the local server. From a2528f2def78975050ffd8efc6224df03113e076 Mon Sep 17 00:00:00 2001 From: Dirk Klimpel <5740567+dklimpel@users.noreply.github.com> Date: Fri, 26 Aug 2022 08:17:58 +0200 Subject: [PATCH 5/5] Remove import `event_injection` --- tests/handlers/test_room_member.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/handlers/test_room_member.py b/tests/handlers/test_room_member.py index 886720214fe6..f67234d2cb3c 100644 --- a/tests/handlers/test_room_member.py +++ b/tests/handlers/test_room_member.py @@ -16,7 +16,7 @@ from tests.replication._base import RedisMultiWorkerStreamTestCase from tests.server import make_request -from tests.test_utils import event_injection, make_awaitable +from tests.test_utils import make_awaitable from tests.unittest import ( FederatingHomeserverTestCase, HomeserverTestCase,