From 8f98260552f4f39f003bc1fbf6da159d9138081d Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Fri, 27 Aug 2021 16:33:41 +0100 Subject: Fix incompatibility with Twisted < 21. (#10713) Turns out that the functionality added in #10546 to skip TLS was incompatible with older Twisted versions, so we need to be a bit more inventive. Also, add a test to (hopefully) not break this in future. Sadly, testing TLS is really hard. --- tests/handlers/test_send_email.py | 112 ++++++++++++++++++++++++++++++++++++++ tests/server.py | 15 ++++- 2 files changed, 124 insertions(+), 3 deletions(-) create mode 100644 tests/handlers/test_send_email.py (limited to 'tests') diff --git a/tests/handlers/test_send_email.py b/tests/handlers/test_send_email.py new file mode 100644 index 0000000000..6f77b1237c --- /dev/null +++ b/tests/handlers/test_send_email.py @@ -0,0 +1,112 @@ +# Copyright 2021 The Matrix.org Foundation C.I.C. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + + +from typing import List, Tuple + +from zope.interface import implementer + +from twisted.internet import defer +from twisted.internet.address import IPv4Address +from twisted.internet.defer import ensureDeferred +from twisted.mail import interfaces, smtp + +from tests.server import FakeTransport +from tests.unittest import HomeserverTestCase + + +@implementer(interfaces.IMessageDelivery) +class _DummyMessageDelivery: + def __init__(self): + # (recipient, message) tuples + self.messages: List[Tuple[smtp.Address, bytes]] = [] + + def receivedHeader(self, helo, origin, recipients): + return None + + def validateFrom(self, helo, origin): + return origin + + def record_message(self, recipient: smtp.Address, message: bytes): + self.messages.append((recipient, message)) + + def validateTo(self, user: smtp.User): + return lambda: _DummyMessage(self, user) + + +@implementer(interfaces.IMessageSMTP) +class _DummyMessage: + """IMessageSMTP implementation which saves the message delivered to it + to the _DummyMessageDelivery object. + """ + + def __init__(self, delivery: _DummyMessageDelivery, user: smtp.User): + self._delivery = delivery + self._user = user + self._buffer: List[bytes] = [] + + def lineReceived(self, line): + self._buffer.append(line) + + def eomReceived(self): + message = b"\n".join(self._buffer) + b"\n" + self._delivery.record_message(self._user.dest, message) + return defer.succeed(b"saved") + + def connectionLost(self): + pass + + +class SendEmailHandlerTestCase(HomeserverTestCase): + def test_send_email(self): + """Happy-path test that we can send email to a non-TLS server.""" + h = self.hs.get_send_email_handler() + d = ensureDeferred( + h.send_email( + "foo@bar.com", "test subject", "Tests", "HTML content", "Text content" + ) + ) + # there should be an attempt to connect to localhost:25 + self.assertEqual(len(self.reactor.tcpClients), 1) + (host, port, client_factory, _timeout, _bindAddress) = self.reactor.tcpClients[ + 0 + ] + self.assertEqual(host, "localhost") + self.assertEqual(port, 25) + + # wire it up to an SMTP server + message_delivery = _DummyMessageDelivery() + server_protocol = smtp.ESMTP() + server_protocol.delivery = message_delivery + # make sure that the server uses the test reactor to set timeouts + server_protocol.callLater = self.reactor.callLater # type: ignore[assignment] + + client_protocol = client_factory.buildProtocol(None) + client_protocol.makeConnection(FakeTransport(server_protocol, self.reactor)) + server_protocol.makeConnection( + FakeTransport( + client_protocol, + self.reactor, + peer_address=IPv4Address("TCP", "127.0.0.1", 1234), + ) + ) + + # the message should now get delivered + self.get_success(d, by=0.1) + + # check it arrived + self.assertEqual(len(message_delivery.messages), 1) + user, msg = message_delivery.messages.pop() + self.assertEqual(str(user), "foo@bar.com") + self.assertIn(b"Subject: test subject", msg) diff --git a/tests/server.py b/tests/server.py index 6fddd3b305..b861c7b866 100644 --- a/tests/server.py +++ b/tests/server.py @@ -10,9 +10,10 @@ from zope.interface import implementer from twisted.internet import address, threads, udp from twisted.internet._resolver import SimpleResolverComplexifier -from twisted.internet.defer import Deferred, fail, succeed +from twisted.internet.defer import Deferred, fail, maybeDeferred, succeed from twisted.internet.error import DNSLookupError from twisted.internet.interfaces import ( + IAddress, IHostnameResolver, IProtocol, IPullProducer, @@ -511,6 +512,9 @@ class FakeTransport: will get called back for connectionLost() notifications etc. """ + _peer_address: Optional[IAddress] = attr.ib(default=None) + """The value to be returend by getPeer""" + disconnecting = False disconnected = False connected = True @@ -519,7 +523,7 @@ class FakeTransport: autoflush = attr.ib(default=True) def getPeer(self): - return None + return self._peer_address def getHost(self): return None @@ -572,7 +576,12 @@ class FakeTransport: self.producerStreaming = streaming def _produce(): - d = self.producer.resumeProducing() + if not self.producer: + # we've been unregistered + return + # some implementations of IProducer (for example, FileSender) + # don't return a deferred. + d = maybeDeferred(self.producer.resumeProducing) d.addCallback(lambda x: self._reactor.callLater(0.1, _produce)) if not streaming: -- cgit 1.5.1 From 52c7a51cfc568ed61d800df99dcca25dc3b5fe3e Mon Sep 17 00:00:00 2001 From: reivilibre <38398653+reivilibre@users.noreply.github.com> Date: Tue, 31 Aug 2021 10:09:58 +0100 Subject: Merge pull request from GHSA-3x4c-pq33-4w3q * Add some tests to characterise the problem Some failing. Current states: RoomsMemberListTestCase test_get_member_list ... [OK] test_get_member_list_mixed_memberships ... [OK] test_get_member_list_no_permission ... [OK] test_get_member_list_no_permission_former_member ... [OK] test_get_member_list_no_permission_former_member_with_at_token ... [FAIL] test_get_member_list_no_room ... [OK] test_get_member_list_no_permission_with_at_token ... [FAIL] * Correct the tests * Check user is/was member before divulging room membership * Pull out only the 1 membership event we want. * Update tests/rest/client/v1/test_rooms.py Co-authored-by: Erik Johnston * Fixup tests (following apply review suggestion) Co-authored-by: Erik Johnston --- synapse/handlers/message.py | 23 +++++++++-- tests/rest/client/v1/test_rooms.py | 84 +++++++++++++++++++++++++++++++++++++- 2 files changed, 103 insertions(+), 4 deletions(-) (limited to 'tests') diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 8a0024ce84..101a29c6d3 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -183,20 +183,37 @@ class MessageHandler: if not last_events: raise NotFoundError("Can't find event for token %s" % (at_token,)) + last_event = last_events[0] + + # check whether the user is in the room at that time to determine + # whether they should be treated as peeking. + state_map = await self.state_store.get_state_for_event( + last_event.event_id, + StateFilter.from_types([(EventTypes.Member, user_id)]), + ) + + joined = False + membership_event = state_map.get((EventTypes.Member, user_id)) + if membership_event: + joined = membership_event.membership == Membership.JOIN + + is_peeking = not joined visible_events = await filter_events_for_client( self.storage, user_id, last_events, filter_send_to_client=False, + is_peeking=is_peeking, ) - event = last_events[0] if visible_events: room_state_events = await self.state_store.get_state_for_events( - [event.event_id], state_filter=state_filter + [last_event.event_id], state_filter=state_filter ) - room_state: Mapping[Any, EventBase] = room_state_events[event.event_id] + room_state: Mapping[Any, EventBase] = room_state_events[ + last_event.event_id + ] else: raise AuthError( 403, diff --git a/tests/rest/client/v1/test_rooms.py b/tests/rest/client/v1/test_rooms.py index 0c9cbb9aff..50100a5ae4 100644 --- a/tests/rest/client/v1/test_rooms.py +++ b/tests/rest/client/v1/test_rooms.py @@ -29,7 +29,7 @@ from synapse.api.constants import EventContentFields, EventTypes, Membership from synapse.api.errors import HttpResponseException from synapse.handlers.pagination import PurgeStatus from synapse.rest import admin -from synapse.rest.client import account, directory, login, profile, room +from synapse.rest.client import account, directory, login, profile, room, sync from synapse.types import JsonDict, RoomAlias, UserID, create_requester from synapse.util.stringutils import random_string @@ -381,6 +381,8 @@ class RoomPermissionsTestCase(RoomBase): class RoomsMemberListTestCase(RoomBase): """Tests /rooms/$room_id/members/list REST events.""" + servlets = RoomBase.servlets + [sync.register_servlets] + user_id = "@sid1:red" def test_get_member_list(self): @@ -397,6 +399,86 @@ class RoomsMemberListTestCase(RoomBase): channel = self.make_request("GET", "/rooms/%s/members" % room_id) self.assertEquals(403, channel.code, msg=channel.result["body"]) + def test_get_member_list_no_permission_with_at_token(self): + """ + Tests that a stranger to the room cannot get the member list + (in the case that they use an at token). + """ + room_id = self.helper.create_room_as("@someone.else:red") + + # first sync to get an at token + channel = self.make_request("GET", "/sync") + self.assertEquals(200, channel.code) + sync_token = channel.json_body["next_batch"] + + # check that permission is denied for @sid1:red to get the + # memberships of @someone.else:red's room. + channel = self.make_request( + "GET", + f"/rooms/{room_id}/members?at={sync_token}", + ) + self.assertEquals(403, channel.code, msg=channel.result["body"]) + + def test_get_member_list_no_permission_former_member(self): + """ + Tests that a former member of the room can not get the member list. + """ + # create a room, invite the user and the user joins + room_id = self.helper.create_room_as("@alice:red") + self.helper.invite(room_id, "@alice:red", self.user_id) + self.helper.join(room_id, self.user_id) + + # check that the user can see the member list to start with + channel = self.make_request("GET", "/rooms/%s/members" % room_id) + self.assertEquals(200, channel.code, msg=channel.result["body"]) + + # ban the user + self.helper.change_membership(room_id, "@alice:red", self.user_id, "ban") + + # check the user can no longer see the member list + channel = self.make_request("GET", "/rooms/%s/members" % room_id) + self.assertEquals(403, channel.code, msg=channel.result["body"]) + + def test_get_member_list_no_permission_former_member_with_at_token(self): + """ + Tests that a former member of the room can not get the member list + (in the case that they use an at token). + """ + # create a room, invite the user and the user joins + room_id = self.helper.create_room_as("@alice:red") + self.helper.invite(room_id, "@alice:red", self.user_id) + self.helper.join(room_id, self.user_id) + + # sync to get an at token + channel = self.make_request("GET", "/sync") + self.assertEquals(200, channel.code) + sync_token = channel.json_body["next_batch"] + + # check that the user can see the member list to start with + channel = self.make_request( + "GET", "/rooms/%s/members?at=%s" % (room_id, sync_token) + ) + self.assertEquals(200, channel.code, msg=channel.result["body"]) + + # ban the user (Note: the user is actually allowed to see this event and + # state so that they know they're banned!) + self.helper.change_membership(room_id, "@alice:red", self.user_id, "ban") + + # invite a third user and let them join + self.helper.invite(room_id, "@alice:red", "@bob:red") + self.helper.join(room_id, "@bob:red") + + # now, with the original user, sync again to get a new at token + channel = self.make_request("GET", "/sync") + self.assertEquals(200, channel.code) + sync_token = channel.json_body["next_batch"] + + # check the user can no longer see the updated member list + channel = self.make_request( + "GET", "/rooms/%s/members?at=%s" % (room_id, sync_token) + ) + self.assertEquals(403, channel.code, msg=channel.result["body"]) + def test_get_member_list_mixed_memberships(self): room_creator = "@some_other_guy:red" room_id = self.helper.create_room_as(room_creator) -- cgit 1.5.1 From cb35df940a828bc40b96daed997b5ad4c7842fd3 Mon Sep 17 00:00:00 2001 From: reivilibre <38398653+reivilibre@users.noreply.github.com> Date: Tue, 31 Aug 2021 11:24:09 +0100 Subject: Merge pull request from GHSA-jj53-8fmw-f2w2 --- synapse/groups/groups_server.py | 18 +++++++++++-- tests/rest/client/v2_alpha/test_groups.py | 43 +++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 2 deletions(-) create mode 100644 tests/rest/client/v2_alpha/test_groups.py (limited to 'tests') diff --git a/synapse/groups/groups_server.py b/synapse/groups/groups_server.py index 3dc55ab861..d6b75ac27f 100644 --- a/synapse/groups/groups_server.py +++ b/synapse/groups/groups_server.py @@ -332,6 +332,13 @@ class GroupsServerWorkerHandler: requester_user_id, group_id ) + # Note! room_results["is_public"] is about whether the room is considered + # public from the group's point of view. (i.e. whether non-group members + # should be able to see the room is in the group). + # This is not the same as whether the room itself is public (in the sense + # of being visible in the room directory). + # As such, room_results["is_public"] itself is not sufficient to determine + # whether any given user is permitted to see the room's metadata. room_results = await self.store.get_rooms_in_group( group_id, include_private=is_user_in_group ) @@ -341,8 +348,15 @@ class GroupsServerWorkerHandler: room_id = room_result["room_id"] joined_users = await self.store.get_users_in_room(room_id) + + # check the user is actually allowed to see the room before showing it to them + allow_private = requester_user_id in joined_users + entry = await self.room_list_handler.generate_room_entry( - room_id, len(joined_users), with_alias=False, allow_private=True + room_id, + len(joined_users), + with_alias=False, + allow_private=allow_private, ) if not entry: @@ -354,7 +368,7 @@ class GroupsServerWorkerHandler: chunk.sort(key=lambda e: -e["num_joined_members"]) - return {"chunk": chunk, "total_room_count_estimate": len(room_results)} + return {"chunk": chunk, "total_room_count_estimate": len(chunk)} class GroupsServerHandler(GroupsServerWorkerHandler): diff --git a/tests/rest/client/v2_alpha/test_groups.py b/tests/rest/client/v2_alpha/test_groups.py new file mode 100644 index 0000000000..bfa9336baa --- /dev/null +++ b/tests/rest/client/v2_alpha/test_groups.py @@ -0,0 +1,43 @@ +from synapse.rest.client.v1 import room +from synapse.rest.client.v2_alpha import groups + +from tests import unittest +from tests.unittest import override_config + + +class GroupsTestCase(unittest.HomeserverTestCase): + user_id = "@alice:test" + room_creator_user_id = "@bob:test" + + servlets = [room.register_servlets, groups.register_servlets] + + @override_config({"enable_group_creation": True}) + def test_rooms_limited_by_visibility(self): + group_id = "+spqr:test" + + # Alice creates a group + channel = self.make_request("POST", "/create_group", {"localpart": "spqr"}) + self.assertEquals(channel.code, 200, msg=channel.text_body) + self.assertEquals(channel.json_body, {"group_id": group_id}) + + # Bob creates a private room + room_id = self.helper.create_room_as(self.room_creator_user_id, is_public=False) + self.helper.auth_user_id = self.room_creator_user_id + self.helper.send_state( + room_id, "m.room.name", {"name": "bob's secret room"}, tok=None + ) + self.helper.auth_user_id = self.user_id + + # Alice adds the room to her group. + channel = self.make_request( + "PUT", f"/groups/{group_id}/admin/rooms/{room_id}", {} + ) + self.assertEquals(channel.code, 200, msg=channel.text_body) + self.assertEquals(channel.json_body, {}) + + # Alice now tries to retrieve the room list of the space. + channel = self.make_request("GET", f"/groups/{group_id}/rooms") + self.assertEquals(channel.code, 200, msg=channel.text_body) + self.assertEquals( + channel.json_body, {"chunk": [], "total_room_count_estimate": 0} + ) -- cgit 1.5.1