From eeebd15c42936bab9987d24977e5e0fbc58c6f80 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 16 Dec 2020 09:15:37 -0500 Subject: [PATCH 01/32] Take Mock out of ``allmydata.test.test_connections.TCP`` --- src/allmydata/node.py | 4 +-- src/allmydata/test/test_connections.py | 50 ++++++++++++++++++++++---- 2 files changed, 45 insertions(+), 9 deletions(-) diff --git a/src/allmydata/node.py b/src/allmydata/node.py index c5433c33c..e08c07508 100644 --- a/src/allmydata/node.py +++ b/src/allmydata/node.py @@ -669,8 +669,8 @@ def create_connection_handlers(config, i2p_provider, tor_provider): # create that handler, so hints which want it will be ignored. handlers = { "tcp": _make_tcp_handler(), - "tor": tor_provider.get_tor_handler(), - "i2p": i2p_provider.get_i2p_handler(), + "tor": tor_provider.get_client_endpoint(), + "i2p": i2p_provider.get_client_endpoint(), } log.msg( format="built Foolscap connection handlers for: %(known_handlers)s", diff --git a/src/allmydata/test/test_connections.py b/src/allmydata/test/test_connections.py index 30aac8446..a5746ced1 100644 --- a/src/allmydata/test/test_connections.py +++ b/src/allmydata/test/test_connections.py @@ -1,31 +1,67 @@ import os import mock + from twisted.trial import unittest from twisted.internet import reactor, endpoints, defer from twisted.internet.interfaces import IStreamClientEndpoint + from foolscap.connections import tcp + +from testtools.matchers import ( + MatchesDict, + IsInstance, + Equals, +) + from ..node import PrivacyError, config_from_string from ..node import create_connection_handlers from ..node import create_main_tub from ..util.i2p_provider import create as create_i2p_provider from ..util.tor_provider import create as create_tor_provider +from .common import ( + SyncTestCase, + ConstantAddresses, +) + BASECONFIG = "" -class TCP(unittest.TestCase): - - def test_default(self): +class CreateConnectionHandlersTests(SyncTestCase): + """ + Tests for the Foolscap connection handlers return by + ``create_connection_handlers``. + """ + def test_foolscap_handlers(self): + """ + ``create_connection_handlers`` returns a Foolscap connection handlers + dictionary mapping ``"tcp"`` to + ``foolscap.connections.tcp.DefaultTCP``, ``"tor"`` to the supplied Tor + provider's handler, and ``"i2p"`` to the supplied I2P provider's + handler. + """ config = config_from_string( "fake.port", "no-basedir", BASECONFIG, ) - _, foolscap_handlers = create_connection_handlers(config, mock.Mock(), mock.Mock()) - self.assertIsInstance( - foolscap_handlers['tcp'], - tcp.DefaultTCP, + tor_endpoint = object() + tor = ConstantAddresses(handler=tor_endpoint) + i2p_endpoint = object() + i2p = ConstantAddresses(handler=i2p_endpoint) + _, foolscap_handlers = create_connection_handlers( + config, + i2p, + tor, + ) + self.assertThat( + foolscap_handlers, + MatchesDict({ + "tcp": IsInstance(tcp.DefaultTCP), + "i2p": Equals(i2p_endpoint), + "tor": Equals(tor_endpoint), + }), ) From d3f8839f1bb4d2f4d1f7f6e12052bef126b28774 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 16 Dec 2020 10:34:07 -0500 Subject: [PATCH 02/32] Duplicate of allmydata.test.test_tor_provider.Provider.test_handler_disabled --- src/allmydata/test/test_connections.py | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/allmydata/test/test_connections.py b/src/allmydata/test/test_connections.py index a5746ced1..66319066c 100644 --- a/src/allmydata/test/test_connections.py +++ b/src/allmydata/test/test_connections.py @@ -67,16 +67,6 @@ class CreateConnectionHandlersTests(SyncTestCase): class Tor(unittest.TestCase): - def test_disabled(self): - config = config_from_string( - "fake.port", - "no-basedir", - BASECONFIG + "[tor]\nenabled = false\n", - ) - tor_provider = create_tor_provider(reactor, config) - h = tor_provider.get_tor_handler() - self.assertEqual(h, None) - def test_unimportable(self): with mock.patch("allmydata.util.tor_provider._import_tor", return_value=None): From 17d9988d4560a8e78a6d820eb087cef0fdf47691 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 16 Dec 2020 10:34:33 -0500 Subject: [PATCH 03/32] Duplicate of allmydata.test.test_tor_provider.Provider.test_handler_no_tor --- src/allmydata/test/test_connections.py | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/allmydata/test/test_connections.py b/src/allmydata/test/test_connections.py index 66319066c..6c5085593 100644 --- a/src/allmydata/test/test_connections.py +++ b/src/allmydata/test/test_connections.py @@ -67,14 +67,6 @@ class CreateConnectionHandlersTests(SyncTestCase): class Tor(unittest.TestCase): - def test_unimportable(self): - with mock.patch("allmydata.util.tor_provider._import_tor", - return_value=None): - config = config_from_string("fake.port", "no-basedir", BASECONFIG) - tor_provider = create_tor_provider(reactor, config) - h = tor_provider.get_tor_handler() - self.assertEqual(h, None) - def test_default(self): h1 = mock.Mock() with mock.patch("foolscap.connections.tor.default_socks", From b5d4a2579b172dfff9c147d09f5914cb1b5f86bb Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 16 Dec 2020 10:37:41 -0500 Subject: [PATCH 04/32] Duplicate of allmydata.test.test_i2p_provider.Provider.test_handler_disabled --- src/allmydata/test/test_connections.py | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/allmydata/test/test_connections.py b/src/allmydata/test/test_connections.py index 6c5085593..d59c415aa 100644 --- a/src/allmydata/test/test_connections.py +++ b/src/allmydata/test/test_connections.py @@ -212,16 +212,6 @@ class Tor(unittest.TestCase): class I2P(unittest.TestCase): - def test_disabled(self): - config = config_from_string( - "fake.port", - "no-basedir", - BASECONFIG + "[i2p]\nenabled = false\n", - ) - i2p_provider = create_i2p_provider(None, config) - h = i2p_provider.get_i2p_handler() - self.assertEqual(h, None) - def test_unimportable(self): config = config_from_string( "fake.port", From ec9851f6d89ff0ff434a2428e815c67ea0571c69 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 16 Dec 2020 10:38:13 -0500 Subject: [PATCH 05/32] Duplicate of allmydata.test.test_i2p_provider.Provider.test_handler_no_i2p --- src/allmydata/test/test_connections.py | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/src/allmydata/test/test_connections.py b/src/allmydata/test/test_connections.py index d59c415aa..2e936a2df 100644 --- a/src/allmydata/test/test_connections.py +++ b/src/allmydata/test/test_connections.py @@ -212,18 +212,6 @@ class Tor(unittest.TestCase): class I2P(unittest.TestCase): - def test_unimportable(self): - config = config_from_string( - "fake.port", - "no-basedir", - BASECONFIG, - ) - with mock.patch("allmydata.util.i2p_provider._import_i2p", - return_value=None): - i2p_provider = create_i2p_provider(reactor, config) - h = i2p_provider.get_i2p_handler() - self.assertEqual(h, None) - def test_default(self): config = config_from_string("fake.port", "no-basedir", BASECONFIG) h1 = mock.Mock() From 71ced4c228f799aa96985e8eee5b763c7a08d0cd Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 16 Dec 2020 11:03:37 -0500 Subject: [PATCH 06/32] Duplicate of allmydata.test.test_tor_provider.Provider.test_handler_socks_endpoint --- src/allmydata/test/test_connections.py | 11 ----------- src/allmydata/test/test_tor_provider.py | 4 ++++ 2 files changed, 4 insertions(+), 11 deletions(-) diff --git a/src/allmydata/test/test_connections.py b/src/allmydata/test/test_connections.py index 2e936a2df..6e4a84e5e 100644 --- a/src/allmydata/test/test_connections.py +++ b/src/allmydata/test/test_connections.py @@ -67,17 +67,6 @@ class CreateConnectionHandlersTests(SyncTestCase): class Tor(unittest.TestCase): - def test_default(self): - h1 = mock.Mock() - with mock.patch("foolscap.connections.tor.default_socks", - return_value=h1) as f: - - config = config_from_string("fake.port", "no-basedir", BASECONFIG) - tor_provider = create_tor_provider(reactor, config) - h = tor_provider.get_tor_handler() - self.assertEqual(f.mock_calls, [mock.call()]) - self.assertIdentical(h, h1) - def _do_test_launch(self, executable): # the handler is created right away config = BASECONFIG+"[tor]\nlaunch = true\n" diff --git a/src/allmydata/test/test_tor_provider.py b/src/allmydata/test/test_tor_provider.py index bfc962831..c6d950632 100644 --- a/src/allmydata/test/test_tor_provider.py +++ b/src/allmydata/test/test_tor_provider.py @@ -349,6 +349,10 @@ class Provider(unittest.TestCase): cfs2.assert_called_with(reactor, ep_desc) def test_handler_socks_endpoint(self): + """ + If not configured otherwise, the Tor provider returns a Socks-based + handler. + """ tor = mock.Mock() handler = object() tor.socks_endpoint = mock.Mock(return_value=handler) From 61778bc799d147bee8c01f36a7b90e53076b4efd Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 16 Dec 2020 11:05:51 -0500 Subject: [PATCH 07/32] Duplicate of allmydata.test.test_tor_provider.CreateOnion.test_launch --- src/allmydata/test/test_connections.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/allmydata/test/test_connections.py b/src/allmydata/test/test_connections.py index 6e4a84e5e..22ad4a4ca 100644 --- a/src/allmydata/test/test_connections.py +++ b/src/allmydata/test/test_connections.py @@ -103,9 +103,6 @@ class Tor(unittest.TestCase): cfs.assert_called_with(reactor, "ep_desc") self.assertIs(cep, tcep) - def test_launch(self): - self._do_test_launch(None) - def test_launch_executable(self): self._do_test_launch("/special/tor") From 01b31e0680d81da0b749e230bc7e7afe703a26fc Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 16 Dec 2020 11:06:57 -0500 Subject: [PATCH 08/32] Duplicate of allmydata.test.test_tor_provider.CreateOnion.test_launch_executable --- src/allmydata/test/test_connections.py | 39 -------------------------- 1 file changed, 39 deletions(-) diff --git a/src/allmydata/test/test_connections.py b/src/allmydata/test/test_connections.py index 22ad4a4ca..34bb3d9a8 100644 --- a/src/allmydata/test/test_connections.py +++ b/src/allmydata/test/test_connections.py @@ -67,45 +67,6 @@ class CreateConnectionHandlersTests(SyncTestCase): class Tor(unittest.TestCase): - def _do_test_launch(self, executable): - # the handler is created right away - config = BASECONFIG+"[tor]\nlaunch = true\n" - if executable: - config += "tor.executable = %s\n" % executable - h1 = mock.Mock() - with mock.patch("foolscap.connections.tor.control_endpoint_maker", - return_value=h1) as f: - - config = config_from_string("fake.port", ".", config) - tp = create_tor_provider("reactor", config) - h = tp.get_tor_handler() - - private_dir = config.get_config_path("private") - exp = mock.call(tp._make_control_endpoint, - takes_status=True) - self.assertEqual(f.mock_calls, [exp]) - self.assertIdentical(h, h1) - - # later, when Foolscap first connects, Tor should be launched - reactor = "reactor" - tcp = object() - tcep = object() - launch_tor = mock.Mock(return_value=defer.succeed(("ep_desc", tcp))) - cfs = mock.Mock(return_value=tcep) - with mock.patch("allmydata.util.tor_provider._launch_tor", launch_tor): - with mock.patch("allmydata.util.tor_provider.clientFromString", cfs): - d = tp._make_control_endpoint(reactor, - update_status=lambda status: None) - cep = self.successResultOf(d) - launch_tor.assert_called_with(reactor, executable, - os.path.abspath(private_dir), - tp._txtorcon) - cfs.assert_called_with(reactor, "ep_desc") - self.assertIs(cep, tcep) - - def test_launch_executable(self): - self._do_test_launch("/special/tor") - def test_socksport_unix_endpoint(self): h1 = mock.Mock() with mock.patch("foolscap.connections.tor.socks_endpoint", From f7c92bf4c94f2319c680d2d3267bae8c473a2569 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 16 Dec 2020 11:11:22 -0500 Subject: [PATCH 09/32] Duplicate of allmydata.test.test_i2p_provider.Provider.test_handler_default --- src/allmydata/test/test_connections.py | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/allmydata/test/test_connections.py b/src/allmydata/test/test_connections.py index 34bb3d9a8..8ec6da619 100644 --- a/src/allmydata/test/test_connections.py +++ b/src/allmydata/test/test_connections.py @@ -159,16 +159,6 @@ class Tor(unittest.TestCase): class I2P(unittest.TestCase): - def test_default(self): - config = config_from_string("fake.port", "no-basedir", BASECONFIG) - h1 = mock.Mock() - with mock.patch("foolscap.connections.i2p.default", - return_value=h1) as f: - i2p_provider = create_i2p_provider(reactor, config) - h = i2p_provider.get_i2p_handler() - self.assertEqual(f.mock_calls, [mock.call(reactor, keyfile=None)]) - self.assertIdentical(h, h1) - def test_samport(self): config = config_from_string( "fake.port", From ececae2ce95caf70702e5a0bc6908a3b399b467d Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 16 Dec 2020 11:12:36 -0500 Subject: [PATCH 10/32] Duplicate of allmydata.test.test_i2p_provider.Provider.test_handler_sam_endpoint --- src/allmydata/test/test_connections.py | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/src/allmydata/test/test_connections.py b/src/allmydata/test/test_connections.py index 8ec6da619..f6b1af4c2 100644 --- a/src/allmydata/test/test_connections.py +++ b/src/allmydata/test/test_connections.py @@ -159,23 +159,6 @@ class Tor(unittest.TestCase): class I2P(unittest.TestCase): - def test_samport(self): - config = config_from_string( - "fake.port", - "no-basedir", - BASECONFIG + "[i2p]\nsam.port = tcp:localhost:1234\n", - ) - h1 = mock.Mock() - with mock.patch("foolscap.connections.i2p.sam_endpoint", - return_value=h1) as f: - i2p_provider = create_i2p_provider(reactor, config) - h = i2p_provider.get_i2p_handler() - - self.assertEqual(len(f.mock_calls), 1) - ep = f.mock_calls[0][1][0] - self.assertIsInstance(ep, endpoints.TCP4ClientEndpoint) - self.assertIdentical(h, h1) - def test_samport_and_launch(self): config = config_from_string( "no-basedir", From e84860ef15c1ed0d7b652dec84c5369b6f4075b4 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 16 Dec 2020 11:13:52 -0500 Subject: [PATCH 11/32] Duplicate of allmydata.test.test_i2p_provider.Provider.test_handler_launch --- src/allmydata/test/test_connections.py | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/src/allmydata/test/test_connections.py b/src/allmydata/test/test_connections.py index f6b1af4c2..f03c0ea76 100644 --- a/src/allmydata/test/test_connections.py +++ b/src/allmydata/test/test_connections.py @@ -174,21 +174,6 @@ class I2P(unittest.TestCase): str(ctx.exception) ) - def test_launch(self): - config = config_from_string( - "fake.port", - "no-basedir", - BASECONFIG + "[i2p]\nlaunch = true\n", - ) - h1 = mock.Mock() - with mock.patch("foolscap.connections.i2p.launch", - return_value=h1) as f: - i2p_provider = create_i2p_provider(reactor, config) - h = i2p_provider.get_i2p_handler() - exp = mock.call(i2p_configdir=None, i2p_binary=None) - self.assertEqual(f.mock_calls, [exp]) - self.assertIdentical(h, h1) - def test_launch_executable(self): config = config_from_string( "fake.port", From 6d66be43b9279f0a7bee018b7cb3f23a4cbda306 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 16 Dec 2020 11:14:46 -0500 Subject: [PATCH 12/32] Duplicate of allmydata.test.test_i2p_provider.Provider.test_handler_launch_configdir --- src/allmydata/test/test_connections.py | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/src/allmydata/test/test_connections.py b/src/allmydata/test/test_connections.py index f03c0ea76..0496eb05c 100644 --- a/src/allmydata/test/test_connections.py +++ b/src/allmydata/test/test_connections.py @@ -189,21 +189,6 @@ class I2P(unittest.TestCase): self.assertEqual(f.mock_calls, [exp]) self.assertIdentical(h, h1) - def test_launch_configdir(self): - config = config_from_string( - "fake.port", - "no-basedir", - BASECONFIG + "[i2p]\nlaunch = true\n" + "i2p.configdir = cfg\n", - ) - h1 = mock.Mock() - with mock.patch("foolscap.connections.i2p.launch", - return_value=h1) as f: - i2p_provider = create_i2p_provider(reactor, config) - h = i2p_provider.get_i2p_handler() - exp = mock.call(i2p_configdir="cfg", i2p_binary=None) - self.assertEqual(f.mock_calls, [exp]) - self.assertIdentical(h, h1) - def test_launch_configdir_and_executable(self): config = config_from_string( "no-basedir", From 81b684b583c3304b995e8ff635b34aeee6405fe2 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 16 Dec 2020 11:15:21 -0500 Subject: [PATCH 13/32] Duplicate of allmydata.test.test_i2p_provider.Provider.test_handler_launch_configdir_executable --- src/allmydata/test/test_connections.py | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/src/allmydata/test/test_connections.py b/src/allmydata/test/test_connections.py index 0496eb05c..c276393b2 100644 --- a/src/allmydata/test/test_connections.py +++ b/src/allmydata/test/test_connections.py @@ -189,22 +189,6 @@ class I2P(unittest.TestCase): self.assertEqual(f.mock_calls, [exp]) self.assertIdentical(h, h1) - def test_launch_configdir_and_executable(self): - config = config_from_string( - "no-basedir", - "fake.port", - BASECONFIG + "[i2p]\nlaunch = true\n" + - "i2p.executable = i2p\n" + "i2p.configdir = cfg\n", - ) - h1 = mock.Mock() - with mock.patch("foolscap.connections.i2p.launch", - return_value=h1) as f: - i2p_provider = create_i2p_provider(reactor, config) - h = i2p_provider.get_i2p_handler() - exp = mock.call(i2p_configdir="cfg", i2p_binary="i2p") - self.assertEqual(f.mock_calls, [exp]) - self.assertIdentical(h, h1) - def test_configdir(self): config = config_from_string( "fake.port", From 8271dbf3e65360d1be875d6aaebd13ca92416f59 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 16 Dec 2020 11:15:51 -0500 Subject: [PATCH 14/32] Duplicate of allmydata.test.test_i2p_provider.Provider.test_handler_configdir --- src/allmydata/test/test_connections.py | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/src/allmydata/test/test_connections.py b/src/allmydata/test/test_connections.py index c276393b2..bfb84536c 100644 --- a/src/allmydata/test/test_connections.py +++ b/src/allmydata/test/test_connections.py @@ -189,20 +189,6 @@ class I2P(unittest.TestCase): self.assertEqual(f.mock_calls, [exp]) self.assertIdentical(h, h1) - def test_configdir(self): - config = config_from_string( - "fake.port", - "no-basedir", - BASECONFIG + "[i2p]\ni2p.configdir = cfg\n", - ) - h1 = mock.Mock() - with mock.patch("foolscap.connections.i2p.local_i2p", - return_value=h1) as f: - i2p_provider = create_i2p_provider(None, config) - h = i2p_provider.get_i2p_handler() - - self.assertEqual(f.mock_calls, [mock.call("cfg")]) - self.assertIdentical(h, h1) class Connections(unittest.TestCase): From 7eb9f2ce5467143b95a71a5804a0b131e2edfe22 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 16 Dec 2020 11:20:45 -0500 Subject: [PATCH 15/32] Moved into allmydata.test.test_i2p_provider This follows the local convention of using mock even though I'm trying to get rid of mock. This is because it keeps the test_i2p_provider suite consistent which means it won't make removing mock from test_i2p_provider later much harder and lets me avoid doing that work now. --- src/allmydata/test/test_connections.py | 16 ---------------- src/allmydata/test/test_i2p_provider.py | 14 ++++++++++++++ 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/src/allmydata/test/test_connections.py b/src/allmydata/test/test_connections.py index bfb84536c..b35de60e1 100644 --- a/src/allmydata/test/test_connections.py +++ b/src/allmydata/test/test_connections.py @@ -174,22 +174,6 @@ class I2P(unittest.TestCase): str(ctx.exception) ) - def test_launch_executable(self): - config = config_from_string( - "fake.port", - "no-basedir", - BASECONFIG + "[i2p]\nlaunch = true\n" + "i2p.executable = i2p\n", - ) - h1 = mock.Mock() - with mock.patch("foolscap.connections.i2p.launch", - return_value=h1) as f: - i2p_provider = create_i2p_provider(reactor, config) - h = i2p_provider.get_i2p_handler() - exp = mock.call(i2p_configdir=None, i2p_binary="i2p") - self.assertEqual(f.mock_calls, [exp]) - self.assertIdentical(h, h1) - - class Connections(unittest.TestCase): def setUp(self): diff --git a/src/allmydata/test/test_i2p_provider.py b/src/allmydata/test/test_i2p_provider.py index a724b300e..37f2333f5 100644 --- a/src/allmydata/test/test_i2p_provider.py +++ b/src/allmydata/test/test_i2p_provider.py @@ -277,6 +277,20 @@ class Provider(unittest.TestCase): i2p.local_i2p.assert_called_with("configdir") self.assertIs(h, handler) + def test_handler_launch_executable(self): + i2p = mock.Mock() + handler = object() + i2p.launch = mock.Mock(return_value=handler) + reactor = object() + + with mock_i2p(i2p): + p = i2p_provider.create(reactor, + FakeConfig(launch=True, + **{"i2p.executable": "myi2p"})) + h = p.get_i2p_handler() + self.assertIs(h, handler) + i2p.launch.assert_called_with(i2p_configdir=None, i2p_binary="myi2p") + def test_handler_default(self): i2p = mock.Mock() handler = object() From 468895c74dc3823cea58de999903610817aaf058 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 16 Dec 2020 11:28:36 -0500 Subject: [PATCH 16/32] Duplicate of allmydata.test.test_tor_provider.Provider.test_handler_control_endpoint --- src/allmydata/test/test_connections.py | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/src/allmydata/test/test_connections.py b/src/allmydata/test/test_connections.py index b35de60e1..2989c10eb 100644 --- a/src/allmydata/test/test_connections.py +++ b/src/allmydata/test/test_connections.py @@ -141,22 +141,6 @@ class Tor(unittest.TestCase): str(ctx.exception) ) - def test_controlport(self): - h1 = mock.Mock() - with mock.patch("foolscap.connections.tor.control_endpoint", - return_value=h1) as f: - config = config_from_string( - "fake.port", - "no-basedir", - BASECONFIG + "[tor]\ncontrol.port = tcp:localhost:1234\n", - ) - tor_provider = create_tor_provider(reactor, config) - h = tor_provider.get_tor_handler() - self.assertEqual(len(f.mock_calls), 1) - ep = f.mock_calls[0][1][0] - self.assertIsInstance(ep, endpoints.TCP4ClientEndpoint) - self.assertIdentical(h, h1) - class I2P(unittest.TestCase): def test_samport_and_launch(self): From 3d564f97d59619bac5c1ee976fec6a0ba76379f1 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 16 Dec 2020 11:48:33 -0500 Subject: [PATCH 17/32] Switch away from mock in a few more simple cases in test_connections.py --- src/allmydata/test/test_connections.py | 48 +++++++++++++++++++++----- 1 file changed, 39 insertions(+), 9 deletions(-) diff --git a/src/allmydata/test/test_connections.py b/src/allmydata/test/test_connections.py index 2989c10eb..59652730f 100644 --- a/src/allmydata/test/test_connections.py +++ b/src/allmydata/test/test_connections.py @@ -1,8 +1,7 @@ -import os import mock from twisted.trial import unittest -from twisted.internet import reactor, endpoints, defer +from twisted.internet import reactor from twisted.internet.interfaces import IStreamClientEndpoint from foolscap.connections import tcp @@ -165,7 +164,11 @@ class Connections(unittest.TestCase): self.config = config_from_string("fake.port", self.basedir, BASECONFIG) def test_default(self): - default_connection_handlers, _ = create_connection_handlers(self.config, mock.Mock(), mock.Mock()) + default_connection_handlers, _ = create_connection_handlers( + self.config, + ConstantAddresses(handler=object()), + ConstantAddresses(handler=object()), + ) self.assertEqual(default_connection_handlers["tcp"], "tcp") self.assertEqual(default_connection_handlers["tor"], "tor") self.assertEqual(default_connection_handlers["i2p"], "i2p") @@ -176,7 +179,11 @@ class Connections(unittest.TestCase): "no-basedir", BASECONFIG + "[connections]\ntcp = tor\n", ) - default_connection_handlers, _ = create_connection_handlers(config, mock.Mock(), mock.Mock()) + default_connection_handlers, _ = create_connection_handlers( + config, + ConstantAddresses(handler=object()), + ConstantAddresses(handler=object()), + ) self.assertEqual(default_connection_handlers["tcp"], "tor") self.assertEqual(default_connection_handlers["tor"], "tor") @@ -207,7 +214,11 @@ class Connections(unittest.TestCase): BASECONFIG + "[connections]\ntcp = unknown\n", ) with self.assertRaises(ValueError) as ctx: - create_connection_handlers(config, mock.Mock(), mock.Mock()) + create_connection_handlers( + config, + ConstantAddresses(handler=object()), + ConstantAddresses(handler=object()), + ) self.assertIn("'tahoe.cfg [connections] tcp='", str(ctx.exception)) self.assertIn("uses unknown handler type 'unknown'", str(ctx.exception)) @@ -217,7 +228,11 @@ class Connections(unittest.TestCase): "no-basedir", BASECONFIG + "[connections]\ntcp = disabled\n", ) - default_connection_handlers, _ = create_connection_handlers(config, mock.Mock(), mock.Mock()) + default_connection_handlers, _ = create_connection_handlers( + config, + ConstantAddresses(handler=object()), + ConstantAddresses(handler=object()), + ) self.assertEqual(default_connection_handlers["tcp"], None) self.assertEqual(default_connection_handlers["tor"], "tor") self.assertEqual(default_connection_handlers["i2p"], "i2p") @@ -232,7 +247,11 @@ class Privacy(unittest.TestCase): ) with self.assertRaises(PrivacyError) as ctx: - create_connection_handlers(config, mock.Mock(), mock.Mock()) + create_connection_handlers( + config, + ConstantAddresses(handler=object()), + ConstantAddresses(handler=object()), + ) self.assertEqual( str(ctx.exception), @@ -247,7 +266,11 @@ class Privacy(unittest.TestCase): BASECONFIG + "[connections]\ntcp = disabled\n" + "[node]\nreveal-IP-address = false\n", ) - default_connection_handlers, _ = create_connection_handlers(config, mock.Mock(), mock.Mock()) + default_connection_handlers, _ = create_connection_handlers( + config, + ConstantAddresses(handler=object()), + ConstantAddresses(handler=object()), + ) self.assertEqual(default_connection_handlers["tcp"], None) def test_tub_location_auto(self): @@ -258,7 +281,14 @@ class Privacy(unittest.TestCase): ) with self.assertRaises(PrivacyError) as ctx: - create_main_tub(config, {}, {}, {}, mock.Mock(), mock.Mock()) + create_main_tub( + config, + tub_options={}, + default_connection_handlers={}, + foolscap_connection_handlers={}, + i2p_provider=ConstantAddresses(), + tor_provider=ConstantAddresses(), + ) self.assertEqual( str(ctx.exception), "tub.location uses AUTO", From 3d82ca0d25a6bf6facaac0896d9f9e938c103d78 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 16 Dec 2020 11:50:35 -0500 Subject: [PATCH 18/32] Use boring old dependency injection to replace mocks in this test --- src/allmydata/test/test_connections.py | 30 +++++++++++++------ src/allmydata/util/tor_provider.py | 41 ++++++++++++++------------ 2 files changed, 43 insertions(+), 28 deletions(-) diff --git a/src/allmydata/test/test_connections.py b/src/allmydata/test/test_connections.py index 59652730f..107d99dc6 100644 --- a/src/allmydata/test/test_connections.py +++ b/src/allmydata/test/test_connections.py @@ -190,16 +190,28 @@ class Connections(unittest.TestCase): self.assertEqual(default_connection_handlers["i2p"], "i2p") def test_tor_unimportable(self): - with mock.patch("allmydata.util.tor_provider._import_tor", - return_value=None): - self.config = config_from_string( - "fake.port", - "no-basedir", - BASECONFIG + "[connections]\ntcp = tor\n", + """ + If the configuration calls for substituting Tor for TCP and + ``foolscap.connections.tor`` is not importable then + ``create_connection_handlers`` raises ``ValueError`` with a message + explaining this makes Tor unusable. + """ + self.config = config_from_string( + "fake.port", + "no-basedir", + BASECONFIG + "[connections]\ntcp = tor\n", + ) + tor_provider = create_tor_provider( + reactor, + self.config, + import_tor=lambda: None, + ) + with self.assertRaises(ValueError) as ctx: + default_connection_handlers, _ = create_connection_handlers( + self.config, + i2p_provider=ConstantAddresses(handler=object()), + tor_provider=tor_provider, ) - with self.assertRaises(ValueError) as ctx: - tor_provider = create_tor_provider(reactor, self.config) - default_connection_handlers, _ = create_connection_handlers(self.config, mock.Mock(), tor_provider) self.assertEqual( str(ctx.exception), "'tahoe.cfg [connections] tcp='" diff --git a/src/allmydata/util/tor_provider.py b/src/allmydata/util/tor_provider.py index 42700b3bc..7b832735d 100644 --- a/src/allmydata/util/tor_provider.py +++ b/src/allmydata/util/tor_provider.py @@ -17,23 +17,7 @@ from ..interfaces import ( IAddressFamily, ) -def create(reactor, config): - """ - Create a new _Provider service (this is an IService so must be - hooked up to a parent or otherwise started). - - If foolscap.connections.tor or txtorcon are not installed, then - Provider.get_tor_handler() will return None. If tahoe.cfg wants - to start an onion service too, then this `create()` method will - throw a nice error (and startService will throw an ugly error). - """ - provider = _Provider(config, reactor) - provider.check_onion_config() - return provider - - def _import_tor(): - # this exists to be overridden by unit tests try: from foolscap.connections import tor return tor @@ -47,6 +31,25 @@ def _import_txtorcon(): except ImportError: # pragma: no cover return None +def create(reactor, config, import_tor=None, import_txtorcon=None): + """ + Create a new _Provider service (this is an IService so must be + hooked up to a parent or otherwise started). + + If foolscap.connections.tor or txtorcon are not installed, then + Provider.get_tor_handler() will return None. If tahoe.cfg wants + to start an onion service too, then this `create()` method will + throw a nice error (and startService will throw an ugly error). + """ + if import_tor is None: + import_tor = _import_tor + if import_txtorcon is None: + import_txtorcon = _import_txtorcon + provider = _Provider(config, reactor, import_tor(), import_txtorcon()) + provider.check_onion_config() + return provider + + def data_directory(private_dir): return os.path.join(private_dir, "tor-statedir") @@ -217,14 +220,14 @@ def create_config(reactor, cli_config): @implementer(IAddressFamily) class _Provider(service.MultiService): - def __init__(self, config, reactor): + def __init__(self, config, reactor, tor, txtorcon): service.MultiService.__init__(self) self._config = config self._tor_launched = None self._onion_ehs = None self._onion_tor_control_proto = None - self._tor = _import_tor() - self._txtorcon = _import_txtorcon() + self._tor = tor + self._txtorcon = txtorcon self._reactor = reactor def _get_tor_config(self, *args, **kwargs): From 9f28ccb2a4babf7a1ec9cef6a70c5049ff1f1033 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 16 Dec 2020 12:07:54 -0500 Subject: [PATCH 19/32] Move the last three mock-using tests to test_tor_provider where they can be rewritten later --- src/allmydata/test/test_connections.py | 44 ------------------------- src/allmydata/test/test_tor_provider.py | 40 ++++++++++++++++++++++ 2 files changed, 40 insertions(+), 44 deletions(-) diff --git a/src/allmydata/test/test_connections.py b/src/allmydata/test/test_connections.py index 107d99dc6..7a24ac794 100644 --- a/src/allmydata/test/test_connections.py +++ b/src/allmydata/test/test_connections.py @@ -1,8 +1,6 @@ -import mock from twisted.trial import unittest from twisted.internet import reactor -from twisted.internet.interfaces import IStreamClientEndpoint from foolscap.connections import tcp @@ -66,48 +64,6 @@ class CreateConnectionHandlersTests(SyncTestCase): class Tor(unittest.TestCase): - def test_socksport_unix_endpoint(self): - h1 = mock.Mock() - with mock.patch("foolscap.connections.tor.socks_endpoint", - return_value=h1) as f: - config = config_from_string( - "fake.port", - "no-basedir", - BASECONFIG + "[tor]\nsocks.port = unix:/var/lib/fw-daemon/tor_socks.socket\n", - ) - tor_provider = create_tor_provider(reactor, config) - h = tor_provider.get_tor_handler() - self.assertTrue(IStreamClientEndpoint.providedBy(f.mock_calls[0][1][0])) - self.assertIdentical(h, h1) - - def test_socksport_endpoint(self): - h1 = mock.Mock() - with mock.patch("foolscap.connections.tor.socks_endpoint", - return_value=h1) as f: - config = config_from_string( - "fake.port", - "no-basedir", - BASECONFIG + "[tor]\nsocks.port = tcp:127.0.0.1:1234\n", - ) - tor_provider = create_tor_provider(reactor, config) - h = tor_provider.get_tor_handler() - self.assertTrue(IStreamClientEndpoint.providedBy(f.mock_calls[0][1][0])) - self.assertIdentical(h, h1) - - def test_socksport_endpoint_otherhost(self): - h1 = mock.Mock() - with mock.patch("foolscap.connections.tor.socks_endpoint", - return_value=h1) as f: - config = config_from_string( - "no-basedir", - "fake.port", - BASECONFIG + "[tor]\nsocks.port = tcp:otherhost:1234\n", - ) - tor_provider = create_tor_provider(reactor, config) - h = tor_provider.get_tor_handler() - self.assertTrue(IStreamClientEndpoint.providedBy(f.mock_calls[0][1][0])) - self.assertIdentical(h, h1) - def test_socksport_bad_endpoint(self): config = config_from_string( "fake.port", diff --git a/src/allmydata/test/test_tor_provider.py b/src/allmydata/test/test_tor_provider.py index c6d950632..f5dd2e29c 100644 --- a/src/allmydata/test/test_tor_provider.py +++ b/src/allmydata/test/test_tor_provider.py @@ -369,6 +369,46 @@ class Provider(unittest.TestCase): tor.socks_endpoint.assert_called_with(ep) self.assertIs(h, handler) + def test_handler_socks_unix_endpoint(self): + """ + ``socks.port`` can be configured as a UNIX client endpoint. + """ + tor = mock.Mock() + handler = object() + tor.socks_endpoint = mock.Mock(return_value=handler) + ep = object() + cfs = mock.Mock(return_value=ep) + reactor = object() + + with mock_tor(tor): + p = tor_provider.create(reactor, + FakeConfig(**{"socks.port": "unix:path"})) + with mock.patch("allmydata.util.tor_provider.clientFromString", cfs): + h = p.get_tor_handler() + cfs.assert_called_with(reactor, "unix:path") + tor.socks_endpoint.assert_called_with(ep) + self.assertIs(h, handler) + + def test_handler_socks_tcp_endpoint(self): + """ + ``socks.port`` can be configured as a UNIX client endpoint. + """ + tor = mock.Mock() + handler = object() + tor.socks_endpoint = mock.Mock(return_value=handler) + ep = object() + cfs = mock.Mock(return_value=ep) + reactor = object() + + with mock_tor(tor): + p = tor_provider.create(reactor, + FakeConfig(**{"socks.port": "tcp:127.0.0.1:1234"})) + with mock.patch("allmydata.util.tor_provider.clientFromString", cfs): + h = p.get_tor_handler() + cfs.assert_called_with(reactor, "tcp:127.0.0.1:1234") + tor.socks_endpoint.assert_called_with(ep) + self.assertIs(h, handler) + def test_handler_control_endpoint(self): tor = mock.Mock() handler = object() From 8bb4d10d7f91a5d388621c120f626d200ed8432b Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 16 Dec 2020 12:28:29 -0500 Subject: [PATCH 20/32] news fragment --- newsfragments/3529.minor | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 newsfragments/3529.minor diff --git a/newsfragments/3529.minor b/newsfragments/3529.minor new file mode 100644 index 000000000..e69de29bb From 83ebaef86cb90aa01b1216a007b171b8a92ced52 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 16 Dec 2020 15:24:33 -0500 Subject: [PATCH 21/32] Stop mocking safe_load The comment implies this will cause something to break on some platform. Let's find out. --- src/allmydata/test/test_introducer.py | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/src/allmydata/test/test_introducer.py b/src/allmydata/test/test_introducer.py index b14b66ffb..ac587e51b 100644 --- a/src/allmydata/test/test_introducer.py +++ b/src/allmydata/test/test_introducer.py @@ -15,7 +15,7 @@ from six import ensure_binary, ensure_text import os, re, itertools from base64 import b32decode import json -from mock import Mock, patch +from mock import Mock from testtools.matchers import ( Is, @@ -94,17 +94,10 @@ class Node(testutil.SignalMixin, testutil.ReallyEqualMixin, AsyncTestCase): f.write(u'---\n') os.chmod(yaml_fname, 0o000) self.addCleanup(lambda: os.chmod(yaml_fname, 0o700)) - # just mocking the yaml failure, as "yamlutil.safe_load" only - # returns None on some platforms for unreadable files - with patch("allmydata.client.yamlutil") as p: - p.safe_load = Mock(return_value=None) - - fake_tub = Mock() - config = read_config(basedir, "portnum") - - with self.assertRaises(EnvironmentError): - create_introducer_clients(config, fake_tub) + config = read_config(basedir, "portnum") + with self.assertRaises(EnvironmentError): + create_introducer_clients(config, Tub()) @defer.inlineCallbacks def test_furl(self): From 3513e9b4fce500a358f5c517708cc33837b4abf7 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 16 Dec 2020 15:25:11 -0500 Subject: [PATCH 22/32] news fragment --- newsfragments/3534.minor | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 newsfragments/3534.minor diff --git a/newsfragments/3534.minor b/newsfragments/3534.minor new file mode 100644 index 000000000..e69de29bb From 60e401ca697abfd910dc841c900025524b90846f Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 16 Dec 2020 16:19:33 -0500 Subject: [PATCH 23/32] Make ObserverList synchronous, reentrant, and exception safe with tests --- src/allmydata/test/test_observer.py | 40 +++++++++++++++++++++++++++++ src/allmydata/util/observer.py | 15 ++++++++--- 2 files changed, 52 insertions(+), 3 deletions(-) diff --git a/src/allmydata/test/test_observer.py b/src/allmydata/test/test_observer.py index 0db13db58..73eece98e 100644 --- a/src/allmydata/test/test_observer.py +++ b/src/allmydata/test/test_observer.py @@ -101,3 +101,43 @@ class Observer(unittest.TestCase): d.addCallback(_step2) d.addCallback(_check2) return d + + def test_observer_list_reentrant(self): + """ + ``ObserverList`` is reentrant. + """ + observed = [] + + def observer_one(): + obs.unsubscribe(observer_one) + + def observer_two(): + observed.append(None) + + obs = observer.ObserverList() + obs.subscribe(observer_one) + obs.subscribe(observer_two) + obs.notify() + + self.assertEqual([None], observed) + + def test_observer_list_observer_errors(self): + """ + An error in an earlier observer does not prevent notification from being + delivered to a later observer. + """ + observed = [] + + def observer_one(): + raise Exception("Some problem here") + + def observer_two(): + observed.append(None) + + obs = observer.ObserverList() + obs.subscribe(observer_one) + obs.subscribe(observer_two) + obs.notify() + + self.assertEqual([None], observed) + self.assertEqual(1, len(self.flushLoggedErrors(Exception))) diff --git a/src/allmydata/util/observer.py b/src/allmydata/util/observer.py index 432aabb87..ad55e65a5 100644 --- a/src/allmydata/util/observer.py +++ b/src/allmydata/util/observer.py @@ -16,6 +16,9 @@ if PY2: import weakref from twisted.internet import defer from foolscap.api import eventually +from twisted.logger import ( + Logger, +) """The idiom we use is for the observed object to offer a method named 'when_something', which returns a deferred. That deferred will be fired when @@ -97,7 +100,10 @@ class LazyOneShotObserverList(OneShotObserverList): self._fire(self._get_result()) class ObserverList(object): - """A simple class to distribute events to a number of subscribers.""" + """ + Immediately distribute events to a number of subscribers. + """ + _logger = Logger() def __init__(self): self._watchers = [] @@ -109,8 +115,11 @@ class ObserverList(object): self._watchers.remove(observer) def notify(self, *args, **kwargs): - for o in self._watchers: - eventually(o, *args, **kwargs) + for o in self._watchers[:]: + try: + o(*args, **kwargs) + except: + self._logger.failure("While notifying {o!r}", o=o) class EventStreamObserver(object): """A simple class to distribute multiple events to a single subscriber. From b2c9296f6befc5e0046c165bb5b0d1b628cadf69 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 16 Dec 2020 16:20:00 -0500 Subject: [PATCH 24/32] Use ObserverList instead of an ad hoc reimplementation --- src/allmydata/introducer/client.py | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/src/allmydata/introducer/client.py b/src/allmydata/introducer/client.py index fa1e1efe8..60a06ce2d 100644 --- a/src/allmydata/introducer/client.py +++ b/src/allmydata/introducer/client.py @@ -24,6 +24,9 @@ from allmydata.introducer.common import sign_to_foolscap, unsign_from_foolscap,\ get_tubid_string_from_ann from allmydata.util import log, yamlutil, connection_status from allmydata.util.rrefutil import add_version_to_remote_reference +from allmydata.util.observer import ( + ObserverList, +) from allmydata.crypto.error import BadSignature from allmydata.util.assertutil import precondition @@ -64,8 +67,7 @@ class IntroducerClient(service.Service, Referenceable): self._publisher = None self._since = None - self._local_subscribers = [] # (servicename,cb,args,kwargs) tuples - self._subscribed_service_names = set() + self._local_subscribers = {} # {servicename: ObserverList} self._subscriptions = set() # requests we've actually sent # _inbound_announcements remembers one announcement per @@ -179,21 +181,21 @@ class IntroducerClient(service.Service, Referenceable): return log.msg(*args, **kwargs) def subscribe_to(self, service_name, cb, *args, **kwargs): - self._local_subscribers.append( (service_name,cb,args,kwargs) ) - self._subscribed_service_names.add(service_name) + obs = self._local_subscribers.setdefault(service_name, ObserverList()) + obs.subscribe(lambda key_s, ann: cb(key_s, ann, *args, **kwargs)) self._maybe_subscribe() for index,(ann,key_s,when) in list(self._inbound_announcements.items()): precondition(isinstance(key_s, bytes), key_s) servicename = index[0] if servicename == service_name: - eventually(cb, key_s, ann, *args, **kwargs) + obs.notify(key_s, ann) def _maybe_subscribe(self): if not self._publisher: self.log("want to subscribe, but no introducer yet", level=log.NOISY) return - for service_name in self._subscribed_service_names: + for service_name in self._local_subscribers: if service_name in self._subscriptions: continue self._subscriptions.add(service_name) @@ -272,7 +274,7 @@ class IntroducerClient(service.Service, Referenceable): precondition(isinstance(key_s, bytes), key_s) self._debug_counts["inbound_announcement"] += 1 service_name = str(ann["service-name"]) - if service_name not in self._subscribed_service_names: + if service_name not in self._local_subscribers: self.log("announcement for a service we don't care about [%s]" % (service_name,), level=log.UNUSUAL, umid="dIpGNA") self._debug_counts["wrong_service"] += 1 @@ -343,9 +345,10 @@ class IntroducerClient(service.Service, Referenceable): def _deliver_announcements(self, key_s, ann): precondition(isinstance(key_s, bytes), key_s) service_name = str(ann["service-name"]) - for (service_name2,cb,args,kwargs) in self._local_subscribers: - if service_name2 == service_name: - eventually(cb, key_s, ann, *args, **kwargs) + + obs = self._local_subscribers.get(service_name) + if obs is not None: + obs.notify(key_s, ann) def connection_status(self): assert self.running # startService builds _introducer_reconnector From 98000c2b66a7a4a5b68e0abddb41abfc2ea9544d Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 16 Dec 2020 16:20:38 -0500 Subject: [PATCH 25/32] re-implement test_unsigned_announcement without mock and to make assertions about public behavior instead of private implementation details --- src/allmydata/test/test_introducer.py | 57 +++++++++++++++++++++------ 1 file changed, 45 insertions(+), 12 deletions(-) diff --git a/src/allmydata/test/test_introducer.py b/src/allmydata/test/test_introducer.py index ac587e51b..668d577db 100644 --- a/src/allmydata/test/test_introducer.py +++ b/src/allmydata/test/test_introducer.py @@ -15,7 +15,12 @@ from six import ensure_binary, ensure_text import os, re, itertools from base64 import b32decode import json -from mock import Mock +from operator import ( + setitem, +) +from functools import ( + partial, +) from testtools.matchers import ( Is, @@ -84,7 +89,8 @@ class Node(testutil.SignalMixin, testutil.ReallyEqualMixin, AsyncTestCase): def test_introducer_clients_unloadable(self): """ - Error if introducers.yaml exists but we can't read it + ``create_introducer_clients`` raises ``EnvironmentError`` if + ``introducers.yaml`` exists but we can't read it. """ basedir = u"introducer.IntroducerNode.test_introducer_clients_unloadable" os.mkdir(basedir) @@ -1030,23 +1036,50 @@ class Signatures(SyncTestCase): unsign_from_foolscap, (bad_msg, sig, b"v999-key")) def test_unsigned_announcement(self): - ed25519.verifying_key_from_string(b"pub-v0-wodst6ly4f7i7akt2nxizsmmy2rlmer6apltl56zctn67wfyu5tq") - mock_tub = Mock() + """ + An incorrectly signed announcement is not delivered to subscribers. + """ + private_key, public_key = ed25519.create_signing_keypair() + public_key_str = ed25519.string_from_verifying_key(public_key) + ic = IntroducerClient( - mock_tub, + Tub(), "pb://", u"fake_nick", "0.0.0", "1.2.3", (0, u"i am a nonce"), - "invalid", + FilePath(self.mktemp()), + ) + received = {} + ic.subscribe_to("good-stuff", partial(setitem, received)) + + # Deliver a good message to prove our test code is valid. + ann = {"service-name": "good-stuff", "payload": "hello"} + ann_t = sign_to_foolscap(ann, private_key) + ic.got_announcements([ann_t]) + + self.assertEqual( + {public_key_str[len("pub-"):]: ann}, + received, + ) + received.clear() + + # Now deliver one without a valid signature and observe that it isn't + # delivered to the subscriber. + ann = {"service-name": "good-stuff", "payload": "bad stuff"} + (msg, sig, key) = sign_to_foolscap(ann, private_key) + # Invalidate the signature a little + sig = sig.replace(b"2", b"3") + ann_t = (msg, sig, key) + ic.got_announcements([ann_t]) + + # The received announcements dict should remain empty because we + # should not receive the announcement with the invalid signature. + self.assertEqual( + {}, + received, ) - self.assertEqual(0, ic._debug_counts["inbound_announcement"]) - ic.got_announcements([ - (b"message", b"v0-aaaaaaa", b"v0-wodst6ly4f7i7akt2nxizsmmy2rlmer6apltl56zctn67wfyu5tq") - ]) - # we should have rejected this announcement due to a bad signature - self.assertEqual(0, ic._debug_counts["inbound_announcement"]) # add tests of StorageFarmBroker: if it receives duplicate announcements, it From b200075246ee8d69e19d852fd44cdaa87b117908 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 16 Dec 2020 16:23:05 -0500 Subject: [PATCH 26/32] whitespace --- src/allmydata/introducer/client.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/allmydata/introducer/client.py b/src/allmydata/introducer/client.py index 60a06ce2d..af84d12c3 100644 --- a/src/allmydata/introducer/client.py +++ b/src/allmydata/introducer/client.py @@ -345,7 +345,6 @@ class IntroducerClient(service.Service, Referenceable): def _deliver_announcements(self, key_s, ann): precondition(isinstance(key_s, bytes), key_s) service_name = str(ann["service-name"]) - obs = self._local_subscribers.get(service_name) if obs is not None: obs.notify(key_s, ann) From 4117beba6a3eb2f142e089c55fc04a840d1d1e24 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 16 Dec 2020 16:25:51 -0500 Subject: [PATCH 27/32] remove unused import yaaay --- src/allmydata/introducer/client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/allmydata/introducer/client.py b/src/allmydata/introducer/client.py index af84d12c3..de24f9cf3 100644 --- a/src/allmydata/introducer/client.py +++ b/src/allmydata/introducer/client.py @@ -16,7 +16,7 @@ from six import ensure_text import time from zope.interface import implementer from twisted.application import service -from foolscap.api import Referenceable, eventually +from foolscap.api import Referenceable from allmydata.interfaces import InsufficientVersionError from allmydata.introducer.interfaces import IIntroducerClient, \ RIIntroducerSubscriberClient_v2 From a223f6bb60a9e9c3e7a25613c479c36d2d2ca74f Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 16 Dec 2020 17:31:06 -0500 Subject: [PATCH 28/32] More reliably corrupt the signature --- src/allmydata/test/test_introducer.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/allmydata/test/test_introducer.py b/src/allmydata/test/test_introducer.py index 668d577db..1ba928257 100644 --- a/src/allmydata/test/test_introducer.py +++ b/src/allmydata/test/test_introducer.py @@ -1069,8 +1069,11 @@ class Signatures(SyncTestCase): # delivered to the subscriber. ann = {"service-name": "good-stuff", "payload": "bad stuff"} (msg, sig, key) = sign_to_foolscap(ann, private_key) - # Invalidate the signature a little - sig = sig.replace(b"2", b"3") + # Drop a base32 word from the middle of the key to invalidate the + # signature. + sig_l = list(sig) + sig_l[20:22] = [] + sig = b"".join(sig_l) ann_t = (msg, sig, key) ic.got_announcements([ann_t]) From 895ba55cf7759ed7a76eb77b435bbd038fe6a759 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 16 Dec 2020 18:17:14 -0500 Subject: [PATCH 29/32] Python 3 compatibility --- src/allmydata/test/test_introducer.py | 6 +++--- src/allmydata/util/base32.py | 4 +++- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/allmydata/test/test_introducer.py b/src/allmydata/test/test_introducer.py index 1ba928257..0475d3f6c 100644 --- a/src/allmydata/test/test_introducer.py +++ b/src/allmydata/test/test_introducer.py @@ -1071,9 +1071,9 @@ class Signatures(SyncTestCase): (msg, sig, key) = sign_to_foolscap(ann, private_key) # Drop a base32 word from the middle of the key to invalidate the # signature. - sig_l = list(sig) - sig_l[20:22] = [] - sig = b"".join(sig_l) + sig_a = bytearray(sig) + sig_a[20:22] = [] + sig = bytes(sig_a) ann_t = (msg, sig, key) ic.got_announcements([ann_t]) diff --git a/src/allmydata/util/base32.py b/src/allmydata/util/base32.py index 287d214ea..41c0f0413 100644 --- a/src/allmydata/util/base32.py +++ b/src/allmydata/util/base32.py @@ -140,7 +140,9 @@ def a2b(cs): # Add padding back, to make Python's base64 module happy: while (len(cs) * 5) % 8 != 0: cs += b"=" - return base64.b32decode(cs) + # Let newbytes come through and still work on Python 2, where the base64 + # module gets confused by them. + return base64.b32decode(backwardscompat_bytes(cs)) __all__ = ["b2a", "a2b", "b2a_or_none", "BASE32CHAR_3bits", "BASE32CHAR_1bits", "BASE32CHAR", "BASE32STR_anybytes", "could_be_base32_encoded"] From 0ffbc7870ee4f0f319f594acd8cb47aa289d51e7 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 16 Dec 2020 20:32:04 -0500 Subject: [PATCH 30/32] Okay, let KeyboardInterrupt through --- src/allmydata/test/test_observer.py | 13 +++++++++++++ src/allmydata/util/observer.py | 2 +- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/src/allmydata/test/test_observer.py b/src/allmydata/test/test_observer.py index 73eece98e..134876be3 100644 --- a/src/allmydata/test/test_observer.py +++ b/src/allmydata/test/test_observer.py @@ -141,3 +141,16 @@ class Observer(unittest.TestCase): self.assertEqual([None], observed) self.assertEqual(1, len(self.flushLoggedErrors(Exception))) + + def test_observer_list_propagate_keyboardinterrupt(self): + """ + ``KeyboardInterrupt`` escapes ``ObserverList.notify``. + """ + def observer_one(): + raise KeyboardInterrupt() + + obs = observer.ObserverList() + obs.subscribe(observer_one) + + with self.assertRaises(KeyboardInterrupt): + obs.notify() diff --git a/src/allmydata/util/observer.py b/src/allmydata/util/observer.py index ad55e65a5..4a39fe014 100644 --- a/src/allmydata/util/observer.py +++ b/src/allmydata/util/observer.py @@ -118,7 +118,7 @@ class ObserverList(object): for o in self._watchers[:]: try: o(*args, **kwargs) - except: + except Exception: self._logger.failure("While notifying {o!r}", o=o) class EventStreamObserver(object): From 9e83343335a683d348cf68d523dbd35f4681e663 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Tue, 29 Dec 2020 10:47:58 -0500 Subject: [PATCH 31/32] news fragment --- newsfragments/3575.minor | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 newsfragments/3575.minor diff --git a/newsfragments/3575.minor b/newsfragments/3575.minor new file mode 100644 index 000000000..e69de29bb From 30b37e17dd44559f05242ec66925f9f6b2905d79 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Tue, 29 Dec 2020 10:48:03 -0500 Subject: [PATCH 32/32] More of a storage_index_hash test --- src/allmydata/test/test_hashutil.py | 30 +++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/src/allmydata/test/test_hashutil.py b/src/allmydata/test/test_hashutil.py index abcd4f0fb..6ec861c9f 100644 --- a/src/allmydata/test/test_hashutil.py +++ b/src/allmydata/test/test_hashutil.py @@ -102,9 +102,35 @@ class HashUtilTests(unittest.TestCase): got_a = base32.b2a(got) self.failUnlessEqual(got_a, expected_a) - def test_known_answers(self): - # assert backwards compatibility + def test_storage_index_hash_known_answers(self): + """ + Verify backwards compatibility by comparing ``storage_index_hash`` outputs + for some well-known (to us) inputs. + """ + # This is a marginal case. b"" is not a valid aes 128 key. The + # implementation does nothing to avoid producing a result for it, + # though. self._testknown(hashutil.storage_index_hash, b"qb5igbhcc5esa6lwqorsy7e6am", b"") + + # This is a little bit more realistic though clearly this is a poor key choice. + self._testknown(hashutil.storage_index_hash, b"wvggbrnrezdpa5yayrgiw5nzja", b"x" * 16) + + # Here's a much more realistic key that I generated by reading some + # bytes from /dev/urandom. I computed the expected hash value twice. + # First using hashlib.sha256 and then with sha256sum(1). The input + # string given to the hash function was "43:," + # in each case. + self._testknown( + hashutil.storage_index_hash, + b"aarbseqqrpsfowduchcjbonscq", + base32.a2b(b"2ckv3dfzh6rgjis6ogfqhyxnzy"), + ) + + def test_known_answers(self): + """ + Verify backwards compatibility by comparing hash outputs for some + well-known (to us) inputs. + """ self._testknown(hashutil.block_hash, b"msjr5bh4evuh7fa3zw7uovixfbvlnstr5b65mrerwfnvjxig2jvq", b"") self._testknown(hashutil.uri_extension_hash, b"wthsu45q7zewac2mnivoaa4ulh5xvbzdmsbuyztq2a5fzxdrnkka", b"") self._testknown(hashutil.plaintext_hash, b"5lz5hwz3qj3af7n6e3arblw7xzutvnd3p3fjsngqjcb7utf3x3da", b"")