diff --git a/acme/acme/client.py b/acme/acme/client.py index e13b272c7..7ab1eb657 100644 --- a/acme/acme/client.py +++ b/acme/acme/client.py @@ -1,4 +1,7 @@ """ACME client API.""" +# pylint: disable=too-many-lines +# This pylint disable can be deleted once the deprecated ACMEv1 code is +# removed. import base64 import collections import datetime @@ -7,7 +10,9 @@ import heapq import http.client as http_client import logging import re +import sys import time +from types import ModuleType from typing import cast from typing import Dict from typing import List @@ -250,8 +255,6 @@ class Client(ClientBase): URI from which the resource will be downloaded. """ - warnings.warn("acme.client.Client (ACMEv1) is deprecated, " - "use acme.client.ClientV2 instead.", PendingDeprecationWarning) self.key = key if net is None: net = ClientNetwork(key, alg=alg, verify_ssl=verify_ssl) @@ -834,8 +837,6 @@ class BackwardsCompatibleClientV2: """ def __init__(self, net, key, server): - warnings.warn("acme.client.BackwardsCompatibleClientV2 is deprecated, use " - "acme.client.ClientV2 instead.", PendingDeprecationWarning) directory = messages.Directory.from_json(net.get(server).json()) self.acme_version = self._acme_version_from_directory(directory) self.client: Union[Client, ClientV2] @@ -1239,3 +1240,35 @@ class ClientNetwork: response = self._check_response(response, content_type=content_type) self._add_nonce(response) return response + + +# This class takes a similar approach to the cryptography project to deprecate attributes +# in public modules. See the _ModuleWithDeprecation class here: +# https://github.com/pyca/cryptography/blob/91105952739442a74582d3e62b3d2111365b0dc7/src/cryptography/utils.py#L129 +class _ClientDeprecationModule: + """ + Internal class delegating to a module, and displaying warnings when attributes + related to deprecated attributes in the acme.client module. + """ + def __init__(self, module): + self.__dict__['_module'] = module + + def __getattr__(self, attr): + if attr in ('Client', 'BackwardsCompatibleClientV2'): + warnings.warn('The {0} attribute in acme.client is deprecated ' + 'and will be removed soon.'.format(attr), + DeprecationWarning, stacklevel=2) + return getattr(self._module, attr) + + def __setattr__(self, attr, value): # pragma: no cover + setattr(self._module, attr, value) + + def __delattr__(self, attr): # pragma: no cover + delattr(self._module, attr) + + def __dir__(self): # pragma: no cover + return ['_module'] + dir(self._module) + + +# Patching ourselves to warn about deprecation and planned removal of some elements in the module. +sys.modules[__name__] = cast(ModuleType, _ClientDeprecationModule(sys.modules[__name__])) diff --git a/certbot/certbot/_internal/client.py b/certbot/certbot/_internal/client.py index 66a07e842..a217e889d 100644 --- a/certbot/certbot/_internal/client.py +++ b/certbot/certbot/_internal/client.py @@ -39,8 +39,7 @@ def acme_from_config_key(config, key, regr=None): user_agent=determine_user_agent(config)) with warnings.catch_warnings(): - # TODO: full removal of ACMEv1 support: https://github.com/certbot/certbot/issues/6844 - warnings.simplefilter("ignore", PendingDeprecationWarning) + warnings.simplefilter("ignore", DeprecationWarning) client = acme_client.BackwardsCompatibleClientV2(net, key, config.server) if client.acme_version == 1: diff --git a/certbot/tests/auth_handler_test.py b/certbot/tests/auth_handler_test.py index ad09067a1..a94259a79 100644 --- a/certbot/tests/auth_handler_test.py +++ b/certbot/tests/auth_handler_test.py @@ -79,9 +79,9 @@ class HandleAuthorizationsTest(unittest.TestCase): self.mock_auth.perform.side_effect = gen_auth_resp self.mock_account = mock.Mock(key=util.Key("file_path", "PEM")) - self.mock_net = mock.MagicMock(spec=acme_client.Client) + self.mock_net = mock.MagicMock(spec=acme_client.ClientV2) self.mock_net.acme_version = 1 - self.mock_net.retry_after.side_effect = acme_client.Client.retry_after + self.mock_net.retry_after.side_effect = acme_client.ClientV2.retry_after self.handler = AuthHandler( self.mock_auth, self.mock_net, self.mock_account, []) @@ -165,9 +165,6 @@ class HandleAuthorizationsTest(unittest.TestCase): self.assertEqual(len(authzr), 1) def _test_name3_http_01_3_common(self, combos): - self.mock_net.request_domain_challenges.side_effect = functools.partial( - gen_dom_authzr, challs=acme_util.CHALLENGES, combos=combos) - authzrs = [gen_dom_authzr(domain="0", challs=acme_util.CHALLENGES), gen_dom_authzr(domain="1", challs=acme_util.CHALLENGES), gen_dom_authzr(domain="2", challs=acme_util.CHALLENGES)] diff --git a/certbot/tests/client_test.py b/certbot/tests/client_test.py index 4d4c3036f..330fd2852 100644 --- a/certbot/tests/client_test.py +++ b/certbot/tests/client_test.py @@ -1,4 +1,5 @@ """Tests for certbot._internal.client.""" +import contextlib import platform import shutil import tempfile @@ -92,8 +93,17 @@ class RegisterTest(test_util.ConfigTestCase): def _false_mock(): return False + @staticmethod + @contextlib.contextmanager + def _patched_acme_client(): + # This function is written this way to avoid deprecation warnings that + # are raised when BackwardsCompatibleClientV2 is accessed on the real + # acme.client module. + with mock.patch('certbot._internal.client.acme_client') as mock_acme_client: + yield mock_acme_client.BackwardsCompatibleClientV2 + def test_no_tos(self): - with mock.patch("certbot._internal.client.acme_client.BackwardsCompatibleClientV2") as mock_client: + with self._patched_acme_client() as mock_client: mock_client.new_account_and_tos().terms_of_service = "http://tos" mock_client().external_account_required.side_effect = self._false_mock with mock.patch("certbot._internal.eff.prepare_subscription") as mock_prepare: @@ -107,7 +117,7 @@ class RegisterTest(test_util.ConfigTestCase): @test_util.patch_display_util() def test_it(self, unused_mock_get_utility): - with mock.patch("certbot._internal.client.acme_client.BackwardsCompatibleClientV2") as mock_client: + with self._patched_acme_client() as mock_client: mock_client().external_account_required.side_effect = self._false_mock with mock.patch("certbot._internal.eff.handle_subscription"): self._call() @@ -118,7 +128,7 @@ class RegisterTest(test_util.ConfigTestCase): self.config.noninteractive_mode = False msg = "DNS problem: NXDOMAIN looking up MX for example.com" mx_err = messages.Error.with_code('invalidContact', detail=msg) - with mock.patch("certbot._internal.client.acme_client.BackwardsCompatibleClientV2") as mock_client: + with self._patched_acme_client() as mock_client: mock_client().external_account_required.side_effect = self._false_mock with mock.patch("certbot._internal.eff.prepare_subscription") as mock_prepare: mock_client().new_account_and_tos.side_effect = [mx_err, mock.MagicMock()] @@ -131,7 +141,7 @@ class RegisterTest(test_util.ConfigTestCase): self.config.noninteractive_mode = True msg = "DNS problem: NXDOMAIN looking up MX for example.com" mx_err = messages.Error.with_code('invalidContact', detail=msg) - with mock.patch("certbot._internal.client.acme_client.BackwardsCompatibleClientV2") as mock_client: + with self._patched_acme_client() as mock_client: mock_client().external_account_required.side_effect = self._false_mock with mock.patch("certbot._internal.eff.handle_subscription"): mock_client().new_account_and_tos.side_effect = [mx_err, mock.MagicMock()] @@ -144,8 +154,8 @@ class RegisterTest(test_util.ConfigTestCase): @mock.patch("certbot._internal.client.logger") def test_without_email(self, mock_logger): with mock.patch("certbot._internal.eff.prepare_subscription") as mock_prepare: - with mock.patch("certbot._internal.client.acme_client.BackwardsCompatibleClientV2") as mock_clnt: - mock_clnt().external_account_required.side_effect = self._false_mock + with self._patched_acme_client() as mock_client: + mock_client().external_account_required.side_effect = self._false_mock self.config.email = None self.config.register_unsafely_without_email = True self.config.dry_run = False @@ -156,7 +166,7 @@ class RegisterTest(test_util.ConfigTestCase): @mock.patch("certbot._internal.client.display_ops.get_email") def test_dry_run_no_staging_account(self, mock_get_email): """Tests dry-run for no staging account, expect account created with no email""" - with mock.patch("certbot._internal.client.acme_client.BackwardsCompatibleClientV2") as mock_client: + with self._patched_acme_client() as mock_client: mock_client().external_account_required.side_effect = self._false_mock with mock.patch("certbot._internal.eff.handle_subscription"): self.config.dry_run = True @@ -168,7 +178,7 @@ class RegisterTest(test_util.ConfigTestCase): @test_util.patch_display_util() def test_with_eab_arguments(self, unused_mock_get_utility): - with mock.patch("certbot._internal.client.acme_client.BackwardsCompatibleClientV2") as mock_client: + with self._patched_acme_client() as mock_client: mock_client().client.directory.__getitem__ = mock.Mock( side_effect=self._new_acct_dir_mock ) @@ -184,7 +194,7 @@ class RegisterTest(test_util.ConfigTestCase): @test_util.patch_display_util() def test_without_eab_arguments(self, unused_mock_get_utility): - with mock.patch("certbot._internal.client.acme_client.BackwardsCompatibleClientV2") as mock_client: + with self._patched_acme_client() as mock_client: mock_client().external_account_required.side_effect = self._false_mock with mock.patch("certbot._internal.eff.handle_subscription"): target = "certbot._internal.client.messages.ExternalAccountBinding.from_data" @@ -196,7 +206,7 @@ class RegisterTest(test_util.ConfigTestCase): self.assertIs(mock_eab_from_data.called, False) def test_external_account_required_without_eab_arguments(self): - with mock.patch("certbot._internal.client.acme_client.BackwardsCompatibleClientV2") as mock_client: + with self._patched_acme_client() as mock_client: mock_client().client.net.key.public_key = mock.Mock(side_effect=self._public_key_mock) mock_client().external_account_required.side_effect = self._true_mock with mock.patch("certbot._internal.eff.handle_subscription"): @@ -210,7 +220,7 @@ class RegisterTest(test_util.ConfigTestCase): from acme import messages msg = "Test" mx_err = messages.Error.with_code("malformed", detail=msg, title="title") - with mock.patch("certbot._internal.client.acme_client.BackwardsCompatibleClientV2") as mock_client: + with self._patched_acme_client() as mock_client: mock_client().client.directory.__getitem__ = mock.Mock( side_effect=self._new_acct_dir_mock ) @@ -232,9 +242,10 @@ class ClientTestCommon(test_util.ConfigTestCase): self.account = mock.MagicMock(**{"key.pem": KEY}) from certbot._internal.client import Client - with mock.patch("certbot._internal.client.acme_client.BackwardsCompatibleClientV2") as acme: - self.acme_client = acme - self.acme = acme.return_value = mock.MagicMock() + with mock.patch("certbot._internal.client.acme_client") as acme: + self.acme_client = acme.BackwardsCompatibleClientV2 + self.acme = self.acme_client.return_value = mock.MagicMock() + self.client_network = acme.ClientNetwork self.client = Client( config=self.config, account_=self.account, auth=None, installer=None) @@ -255,8 +266,7 @@ class ClientTest(ClientTestCommon): csr_pem=mock.sentinel.csr_pem) def test_init_acme_verify_ssl(self): - net = self.acme_client.call_args[0][0] - self.assertIs(net.verify_ssl, True) + self.assertIs(self.client_network.call_args[1]['verify_ssl'], True) def _mock_obtain_certificate(self): self.client.auth_handler = mock.MagicMock() diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index de202d006..3fd71b153 100644 --- a/certbot/tests/main_test.py +++ b/certbot/tests/main_test.py @@ -323,12 +323,12 @@ class RevokeTest(test_util.TempDirTestCase): self.tmp_cert_path = os.path.abspath(os.path.join(self.tempdir, 'cert_512.pem')) patches = [ - mock.patch('acme.client.BackwardsCompatibleClientV2'), + mock.patch('certbot._internal.client.acme_client'), mock.patch('certbot._internal.client.Client'), mock.patch('certbot._internal.main._determine_account'), mock.patch('certbot._internal.main.display_ops.success_revocation') ] - self.mock_acme_client = patches[0].start() + self.mock_acme_client = patches[0].start().BackwardsCompatibleClientV2 patches[1].start() self.mock_determine_account = patches[2].start() self.mock_success_revoke = patches[3].start() @@ -708,11 +708,10 @@ class MainTest(test_util.ConfigTestCase): @mock.patch('certbot._internal.eff.handle_subscription') @mock.patch('certbot._internal.log.post_arg_parse_setup') @mock.patch('certbot._internal.main._report_new_cert') - @mock.patch('certbot._internal.main.client.acme_client.Client') @mock.patch('certbot._internal.main._determine_account') @mock.patch('certbot._internal.main.client.Client.obtain_and_enroll_certificate') @mock.patch('certbot._internal.main._get_and_save_cert') - def test_user_agent(self, gsc, _obt, det, _client, _, __, ___): + def test_user_agent(self, gsc, _obt, det, _, __, ___): # Normally the client is totally mocked out, but here we need more # arguments to automate it... args = ["--standalone", "certonly", "-m", "none@none.com", @@ -720,7 +719,8 @@ class MainTest(test_util.ConfigTestCase): det.return_value = mock.MagicMock(), None gsc.return_value = mock.MagicMock() - with mock.patch('certbot._internal.main.client.acme_client.ClientNetwork') as acme_net: + with mock.patch('certbot._internal.main.client.acme_client') as acme_client: + acme_net = acme_client.ClientNetwork self._call_no_clientmock(args) os_ver = util.get_os_info_ua() ua = acme_net.call_args[1]["user_agent"] @@ -730,7 +730,8 @@ class MainTest(test_util.ConfigTestCase): if "linux" in plat.lower(): self.assertIn(util.get_os_info_ua(), ua) - with mock.patch('certbot._internal.main.client.acme_client.ClientNetwork') as acme_net: + with mock.patch('certbot._internal.main.client.acme_client') as acme_client: + acme_net = acme_client.ClientNetwork ua = "bandersnatch" args += ["--user-agent", ua] self._call_no_clientmock(args)