Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Fix that user cannot /forget rooms after the last member has left #13546

Merged
merged 6 commits into from
Aug 30, 2022
Merged

Fix that user cannot /forget rooms after the last member has left #13546

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
06be1aa
Fix that user cannot `/forget` rooms after the last member has left
dklimpel Aug 17, 2022
efd0e19
newsfile
dklimpel Aug 17, 2022
0a19083
Merge remote-tracking branch 'synapse/develop' into fix_forget
dklimpel Aug 17, 2022
aa302cf
update tests
dklimpel Aug 17, 2022
847d8ba
Remove test `test_rejoin_forgotten_by_server`
dklimpel Aug 26, 2022
a2528f2
Remove import `event_injection`
dklimpel Aug 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/13546.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix bug that user cannot `/forget` rooms after the last member has left the room.
7 changes: 5 additions & 2 deletions synapse/handlers/room_member.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]:
Expand Down
126 changes: 123 additions & 3 deletions tests/handlers/test_room_member.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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):
Expand Down Expand Up @@ -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))
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is failing

Error: 
Traceback (most recent call last):
  File "/home/runner/work/synapse/synapse/tests/handlers/test_room_member.py", line 376, in test_rejoin_forgotten_by_server
    self.helper.join(self.room_id, user=self.alice, tok=self.alice_token)
  File "/home/runner/work/synapse/synapse/tests/rest/client/utils.py", line 178, in join
    self.change_membership(
  File "/home/runner/work/synapse/synapse/tests/rest/client/utils.py", line 313, in change_membership
    assert channel.code == expect_code, "Expected: %d, got: %d, resp: %r" % (
builtins.AssertionError: Expected: 200, got: 404, resp: b'{"errcode":"M_UNKNOWN","error":"Can\'t join remote room because no servers that are in the room have been provided."}'

tests.handlers.test_room_member.RoomMemberMasterHandlerTestCase.test_rejoin_forgotten_by_server
-------------------------------------------------------------------------------

I am not sure if this is a bug in Synapse or a wrong test.
The error occures there:

synapse/synapse/handlers/room_member.py

Lines 1668 to 1689 in 0a4efbc

async def _remote_join(
self,
requester: Requester,
remote_room_hosts: List[str],
room_id: str,
user: UserID,
content: dict,
) -> Tuple[str, int]:
"""Implements RoomMemberHandler._remote_join"""
# filter ourselves out of remote_room_hosts: do_invite_join ignores it
# and if it is the only entry we'd like to return a 404 rather than a
# 500.
remote_room_hosts = [
host for host in remote_room_hosts if host != self.hs.hostname
]
if len(remote_room_hosts) == 0:
raise SynapseError(
404,
"Can't join remote room because no servers "
"that are in the room have been provided.",
)

Actually, this is not a fault of forgetting rooms, rather of leaving and joining rooms.
Perhaps related to:

How to deal with it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Synapse doesn't know how to rejoin the user to self.room_id because

  • the server has no record of self.room_id after it's been forgotten
  • the server doesn't know any other servers in that room.

This one might be better handled with a complement test, so that you genuinely do have a remote homeserver; otherwise I think you'd have to do a fair chunk of mocking to do it as a Synapse-only test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • the server doesn't know any other servers in that room.

Interesting. The room id is actually one of his own, since his user originally created the room.
A little bit off topic: The server only finds the room again when a local user is invited or with a foreign alias.

The conclusion: The test should be removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. The room id is actually one of his own, since his user originally created the room.

The server name in any room ID is not meant to be parsed; it's only there to ensure that two servers don't create a room with the same room ID.
That's why the room ID having been created by this server is actually irrelevant here, I think.

(This surprises people: they assume that !blahblah:example.com should be enough to join a room via example.com and are confused when Synapse will refuse to if you don't tell it any server names. There are also proposals to remove the server name from room IDs, if I remember, instead using the hash of the create event or something equivalent.)

A little bit off topic: The server only finds the room again when a local user is invited or with a foreign alias.

This is more or less what I'd expect: in both those cases, the server gets a list of other servers that are in the room.
The server then knows which server it can use to do the /make_join, /send_join handshake with — see https://spec.matrix.org/v1.3/server-server-api/#joining-rooms if you haven't already to get a picture of how this works.

Joining a remote room requires you to know at least one server that's in the room already, otherwise you have nowhere to join it via.

I agree with @DMRobertson here that a Complement test for this seems like it would be ideal, since it would let you prove this works externally with 2 real servers and you don't have to do a lot of mocking.


# 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))
)