Skip to content

Commit 8f98260

Browse files
authored
Fix incompatibility with Twisted < 21. (matrix-org#10713)
Turns out that the functionality added in matrix-org#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.
1 parent f03cafb commit 8f98260

File tree

5 files changed

+173
-21
lines changed

5 files changed

+173
-21
lines changed

changelog.d/10713.bugfix

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Fix a regression introduced in Synapse 1.41 which broke email transmission on Systems using older versions of the Twisted library.

mypy.ini

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,7 @@ files =
8787
tests/test_utils,
8888
tests/handlers/test_password_providers.py,
8989
tests/handlers/test_room_summary.py,
90+
tests/handlers/test_send_email.py,
9091
tests/rest/client/v1/test_login.py,
9192
tests/rest/client/v2_alpha/test_auth.py,
9293
tests/util/test_itertools.py,

synapse/handlers/send_email.py

Lines changed: 47 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,12 @@
1919
from io import BytesIO
2020
from typing import TYPE_CHECKING, Optional
2121

22+
from pkg_resources import parse_version
23+
24+
import twisted
2225
from twisted.internet.defer import Deferred
23-
from twisted.internet.interfaces import IReactorTCP
24-
from twisted.mail.smtp import ESMTPSenderFactory
26+
from twisted.internet.interfaces import IOpenSSLContextFactory, IReactorTCP
27+
from twisted.mail.smtp import ESMTPSender, ESMTPSenderFactory
2528

2629
from synapse.logging.context import make_deferred_yieldable
2730

@@ -30,6 +33,19 @@
3033

3134
logger = logging.getLogger(__name__)
3235

36+
_is_old_twisted = parse_version(twisted.__version__) < parse_version("21")
37+
38+
39+
class _NoTLSESMTPSender(ESMTPSender):
40+
"""Extend ESMTPSender to disable TLS
41+
42+
Unfortunately, before Twisted 21.2, ESMTPSender doesn't give an easy way to disable
43+
TLS, so we override its internal method which it uses to generate a context factory.
44+
"""
45+
46+
def _getContextFactory(self) -> Optional[IOpenSSLContextFactory]:
47+
return None
48+
3349

3450
async def _sendmail(
3551
reactor: IReactorTCP,
@@ -42,7 +58,7 @@ async def _sendmail(
4258
password: Optional[bytes] = None,
4359
require_auth: bool = False,
4460
require_tls: bool = False,
45-
tls_hostname: Optional[str] = None,
61+
enable_tls: bool = True,
4662
) -> None:
4763
"""A simple wrapper around ESMTPSenderFactory, to allow substitution in tests
4864
@@ -57,24 +73,37 @@ async def _sendmail(
5773
password: password to give when authenticating
5874
require_auth: if auth is not offered, fail the request
5975
require_tls: if TLS is not offered, fail the reqest
60-
tls_hostname: TLS hostname to check for. None to disable TLS.
76+
enable_tls: True to enable TLS. If this is False and require_tls is True,
77+
the request will fail.
6178
"""
6279
msg = BytesIO(msg_bytes)
63-
6480
d: "Deferred[object]" = Deferred()
6581

66-
factory = ESMTPSenderFactory(
67-
username,
68-
password,
69-
from_addr,
70-
to_addr,
71-
msg,
72-
d,
73-
heloFallback=True,
74-
requireAuthentication=require_auth,
75-
requireTransportSecurity=require_tls,
76-
hostname=tls_hostname,
77-
)
82+
def build_sender_factory(**kwargs) -> ESMTPSenderFactory:
83+
return ESMTPSenderFactory(
84+
username,
85+
password,
86+
from_addr,
87+
to_addr,
88+
msg,
89+
d,
90+
heloFallback=True,
91+
requireAuthentication=require_auth,
92+
requireTransportSecurity=require_tls,
93+
**kwargs,
94+
)
95+
96+
if _is_old_twisted:
97+
# before twisted 21.2, we have to override the ESMTPSender protocol to disable
98+
# TLS
99+
factory = build_sender_factory()
100+
101+
if not enable_tls:
102+
factory.protocol = _NoTLSESMTPSender
103+
else:
104+
# for twisted 21.2 and later, there is a 'hostname' parameter which we should
105+
# set to enable TLS.
106+
factory = build_sender_factory(hostname=smtphost if enable_tls else None)
78107

79108
# the IReactorTCP interface claims host has to be a bytes, which seems to be wrong
80109
reactor.connectTCP(smtphost, smtpport, factory, timeout=30, bindAddress=None) # type: ignore[arg-type]
@@ -154,5 +183,5 @@ async def send_email(
154183
password=self._smtp_pass,
155184
require_auth=self._smtp_user is not None,
156185
require_tls=self._require_transport_security,
157-
tls_hostname=self._smtp_host if self._enable_tls else None,
186+
enable_tls=self._enable_tls,
158187
)

tests/handlers/test_send_email.py

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
# Copyright 2021 The Matrix.org Foundation C.I.C.
2+
#
3+
# Licensed under the Apache License, Version 2.0 (the "License");
4+
# you may not use this file except in compliance with the License.
5+
# You may obtain a copy of the License at
6+
#
7+
# http://www.apache.org/licenses/LICENSE-2.0
8+
#
9+
# Unless required by applicable law or agreed to in writing, software
10+
# distributed under the License is distributed on an "AS IS" BASIS,
11+
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
# See the License for the specific language governing permissions and
13+
# limitations under the License.
14+
15+
16+
from typing import List, Tuple
17+
18+
from zope.interface import implementer
19+
20+
from twisted.internet import defer
21+
from twisted.internet.address import IPv4Address
22+
from twisted.internet.defer import ensureDeferred
23+
from twisted.mail import interfaces, smtp
24+
25+
from tests.server import FakeTransport
26+
from tests.unittest import HomeserverTestCase
27+
28+
29+
@implementer(interfaces.IMessageDelivery)
30+
class _DummyMessageDelivery:
31+
def __init__(self):
32+
# (recipient, message) tuples
33+
self.messages: List[Tuple[smtp.Address, bytes]] = []
34+
35+
def receivedHeader(self, helo, origin, recipients):
36+
return None
37+
38+
def validateFrom(self, helo, origin):
39+
return origin
40+
41+
def record_message(self, recipient: smtp.Address, message: bytes):
42+
self.messages.append((recipient, message))
43+
44+
def validateTo(self, user: smtp.User):
45+
return lambda: _DummyMessage(self, user)
46+
47+
48+
@implementer(interfaces.IMessageSMTP)
49+
class _DummyMessage:
50+
"""IMessageSMTP implementation which saves the message delivered to it
51+
to the _DummyMessageDelivery object.
52+
"""
53+
54+
def __init__(self, delivery: _DummyMessageDelivery, user: smtp.User):
55+
self._delivery = delivery
56+
self._user = user
57+
self._buffer: List[bytes] = []
58+
59+
def lineReceived(self, line):
60+
self._buffer.append(line)
61+
62+
def eomReceived(self):
63+
message = b"\n".join(self._buffer) + b"\n"
64+
self._delivery.record_message(self._user.dest, message)
65+
return defer.succeed(b"saved")
66+
67+
def connectionLost(self):
68+
pass
69+
70+
71+
class SendEmailHandlerTestCase(HomeserverTestCase):
72+
def test_send_email(self):
73+
"""Happy-path test that we can send email to a non-TLS server."""
74+
h = self.hs.get_send_email_handler()
75+
d = ensureDeferred(
76+
h.send_email(
77+
"[email protected]", "test subject", "Tests", "HTML content", "Text content"
78+
)
79+
)
80+
# there should be an attempt to connect to localhost:25
81+
self.assertEqual(len(self.reactor.tcpClients), 1)
82+
(host, port, client_factory, _timeout, _bindAddress) = self.reactor.tcpClients[
83+
0
84+
]
85+
self.assertEqual(host, "localhost")
86+
self.assertEqual(port, 25)
87+
88+
# wire it up to an SMTP server
89+
message_delivery = _DummyMessageDelivery()
90+
server_protocol = smtp.ESMTP()
91+
server_protocol.delivery = message_delivery
92+
# make sure that the server uses the test reactor to set timeouts
93+
server_protocol.callLater = self.reactor.callLater # type: ignore[assignment]
94+
95+
client_protocol = client_factory.buildProtocol(None)
96+
client_protocol.makeConnection(FakeTransport(server_protocol, self.reactor))
97+
server_protocol.makeConnection(
98+
FakeTransport(
99+
client_protocol,
100+
self.reactor,
101+
peer_address=IPv4Address("TCP", "127.0.0.1", 1234),
102+
)
103+
)
104+
105+
# the message should now get delivered
106+
self.get_success(d, by=0.1)
107+
108+
# check it arrived
109+
self.assertEqual(len(message_delivery.messages), 1)
110+
user, msg = message_delivery.messages.pop()
111+
self.assertEqual(str(user), "[email protected]")
112+
self.assertIn(b"Subject: test subject", msg)

tests/server.py

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,10 @@
1010

1111
from twisted.internet import address, threads, udp
1212
from twisted.internet._resolver import SimpleResolverComplexifier
13-
from twisted.internet.defer import Deferred, fail, succeed
13+
from twisted.internet.defer import Deferred, fail, maybeDeferred, succeed
1414
from twisted.internet.error import DNSLookupError
1515
from twisted.internet.interfaces import (
16+
IAddress,
1617
IHostnameResolver,
1718
IProtocol,
1819
IPullProducer,
@@ -511,6 +512,9 @@ class FakeTransport:
511512
will get called back for connectionLost() notifications etc.
512513
"""
513514

515+
_peer_address: Optional[IAddress] = attr.ib(default=None)
516+
"""The value to be returend by getPeer"""
517+
514518
disconnecting = False
515519
disconnected = False
516520
connected = True
@@ -519,7 +523,7 @@ class FakeTransport:
519523
autoflush = attr.ib(default=True)
520524

521525
def getPeer(self):
522-
return None
526+
return self._peer_address
523527

524528
def getHost(self):
525529
return None
@@ -572,7 +576,12 @@ def registerProducer(self, producer, streaming):
572576
self.producerStreaming = streaming
573577

574578
def _produce():
575-
d = self.producer.resumeProducing()
579+
if not self.producer:
580+
# we've been unregistered
581+
return
582+
# some implementations of IProducer (for example, FileSender)
583+
# don't return a deferred.
584+
d = maybeDeferred(self.producer.resumeProducing)
576585
d.addCallback(lambda x: self._reactor.callLater(0.1, _produce))
577586

578587
if not streaming:

0 commit comments

Comments
 (0)