From 4eb0b560c59cea45a88dc1af5e3a5952e85fc8d0 Mon Sep 17 00:00:00 2001 From: alexzorin Date: Fri, 23 Oct 2020 06:12:54 +1100 Subject: [PATCH] manual: deprecate --manual-public-ip-logging-ok (#8381) * manual: deprecate --manual-public-ip-logging-ok * remove unused cli.report_config_interaction code Co-authored-by: ohemorange --- .../utils/certbot_call.py | 1 - certbot/CHANGELOG.md | 3 ++ certbot/certbot/_internal/cli/__init__.py | 1 - .../cli/report_config_interaction.py | 27 -------------- certbot/certbot/_internal/plugins/manual.py | 19 +--------- certbot/certbot/plugins/common.py | 4 -- certbot/tests/cli_test.py | 37 ------------------- certbot/tests/plugins/manual_test.py | 17 +-------- 8 files changed, 5 insertions(+), 104 deletions(-) delete mode 100644 certbot/certbot/_internal/cli/report_config_interaction.py diff --git a/certbot-ci/certbot_integration_tests/utils/certbot_call.py b/certbot-ci/certbot_integration_tests/utils/certbot_call.py index 2ddaa41c8..a71c610e5 100755 --- a/certbot-ci/certbot_integration_tests/utils/certbot_call.py +++ b/certbot-ci/certbot_integration_tests/utils/certbot_call.py @@ -92,7 +92,6 @@ def _prepare_args_env(certbot_args, directory_url, http_01_port, tls_alpn_01_por '--no-verify-ssl', '--http-01-port', str(http_01_port), '--https-port', str(tls_alpn_01_port), - '--manual-public-ip-logging-ok', '--config-dir', config_dir, '--work-dir', os.path.join(workspace, 'work'), '--logs-dir', os.path.join(workspace, 'logs'), diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md index aff3a7cef..fc83d3b46 100644 --- a/certbot/CHANGELOG.md +++ b/certbot/CHANGELOG.md @@ -13,6 +13,9 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). ### Changed * certbot-auto was deprecated on Debian based systems. +* CLI flag `--manual-public-ip-logging-ok` is now a no-op, generates a + deprecation warning, and will be removed in a future release. +* ### Fixed diff --git a/certbot/certbot/_internal/cli/__init__.py b/certbot/certbot/_internal/cli/__init__.py index e4909840e..f6f648aa9 100644 --- a/certbot/certbot/_internal/cli/__init__.py +++ b/certbot/certbot/_internal/cli/__init__.py @@ -51,7 +51,6 @@ from certbot._internal.cli.cli_utils import ( ) # These imports depend on cli_constants and cli_utils. -from certbot._internal.cli.report_config_interaction import report_config_interaction from certbot._internal.cli.verb_help import VERB_HELP, VERB_HELP_MAP from certbot._internal.cli.group_adder import _add_all_groups from certbot._internal.cli.subparsers import _create_subparsers diff --git a/certbot/certbot/_internal/cli/report_config_interaction.py b/certbot/certbot/_internal/cli/report_config_interaction.py deleted file mode 100644 index 39266e776..000000000 --- a/certbot/certbot/_internal/cli/report_config_interaction.py +++ /dev/null @@ -1,27 +0,0 @@ -"""This is a module that reports config option interaction that should be -checked by set_by_cli""" -import six - -from certbot._internal.cli import VAR_MODIFIERS - - -def report_config_interaction(modified, modifiers): - """Registers config option interaction to be checked by set_by_cli. - - This function can be called by during the __init__ or - add_parser_arguments methods of plugins to register interactions - between config options. - - :param modified: config options that can be modified by modifiers - :type modified: iterable or str (string_types) - :param modifiers: config options that modify modified - :type modifiers: iterable or str (string_types) - - """ - if isinstance(modified, six.string_types): - modified = (modified,) - if isinstance(modifiers, six.string_types): - modifiers = (modifiers,) - - for var in modified: - VAR_MODIFIERS.setdefault(var, set()).update(modifiers) diff --git a/certbot/certbot/_internal/plugins/manual.py b/certbot/certbot/_internal/plugins/manual.py index cc471230c..a2e4bb28e 100644 --- a/certbot/certbot/_internal/plugins/manual.py +++ b/certbot/certbot/_internal/plugins/manual.py @@ -84,8 +84,7 @@ permitted by DNS standards.) help='Path or command to execute for the authentication script') add('cleanup-hook', help='Path or command to execute for the cleanup script') - add('public-ip-logging-ok', action='store_true', - help='Automatically allows public IP logging (default: Ask)') + util.add_deprecated_argument(add, 'public-ip-logging-ok', 0) def prepare(self): # pylint: disable=missing-function-docstring if self.config.noninteractive_mode and not self.conf('auth-hook'): @@ -114,8 +113,6 @@ permitted by DNS standards.) return [challenges.HTTP01, challenges.DNS01] def perform(self, achalls): # pylint: disable=missing-function-docstring - self._verify_ip_logging_ok() - responses = [] for achall in achalls: if self.conf('auth-hook'): @@ -125,20 +122,6 @@ permitted by DNS standards.) responses.append(achall.response(achall.account_key)) return responses - def _verify_ip_logging_ok(self): - if not self.conf('public-ip-logging-ok'): - cli_flag = '--{0}'.format(self.option_name('public-ip-logging-ok')) - msg = ('NOTE: The IP of this machine will be publicly logged as ' - "having requested this certificate. If you're running " - 'certbot in manual mode on a machine that is not your ' - "server, please ensure you're okay with that.\n\n" - 'Are you OK with your IP being logged?') - display = zope.component.getUtility(interfaces.IDisplay) - if display.yesno(msg, cli_flag=cli_flag, force_interactive=True): - setattr(self.config, self.dest('public-ip-logging-ok'), True) - else: - raise errors.PluginError('Must agree to IP logging to proceed') - def _perform_achall_with_script(self, achall, achalls): env = dict(CERTBOT_DOMAIN=achall.domain, CERTBOT_VALIDATION=achall.validation(achall.account_key), diff --git a/certbot/certbot/plugins/common.py b/certbot/certbot/plugins/common.py index 7d21e6d0a..5db75922b 100644 --- a/certbot/certbot/plugins/common.py +++ b/certbot/certbot/plugins/common.py @@ -55,10 +55,6 @@ class Plugin(object): def add_parser_arguments(cls, add): """Add plugin arguments to the CLI argument parser. - NOTE: If some of your flags interact with others, you can - use cli.report_config_interaction to register this to ensure - values are correctly saved/overridable during renewal. - :param callable add: Function that proxies calls to `argparse.ArgumentParser.add_argument` prepending options with unique plugin name prefix. diff --git a/certbot/tests/cli_test.py b/certbot/tests/cli_test.py index 592c40be7..7475e99ea 100644 --- a/certbot/tests/cli_test.py +++ b/certbot/tests/cli_test.py @@ -505,43 +505,6 @@ class SetByCliTest(unittest.TestCase): verb = 'renew' self.assertTrue(_call_set_by_cli('webroot_map', args, verb)) - def test_report_config_interaction_str(self): - cli.report_config_interaction('manual_public_ip_logging_ok', - 'manual_auth_hook') - cli.report_config_interaction('manual_auth_hook', 'manual') - - self._test_report_config_interaction_common() - - def test_report_config_interaction_iterable(self): - cli.report_config_interaction(('manual_public_ip_logging_ok',), - ('manual_auth_hook',)) - cli.report_config_interaction(('manual_auth_hook',), ('manual',)) - - self._test_report_config_interaction_common() - - def _test_report_config_interaction_common(self): - """Tests implied interaction between manual flags. - - --manual implies --manual-auth-hook which implies - --manual-public-ip-logging-ok. These interactions don't actually - exist in the client, but are used here for testing purposes. - - """ - - args = ['--manual'] - verb = 'renew' - for v in ('manual', 'manual_auth_hook', 'manual_public_ip_logging_ok'): - self.assertTrue(_call_set_by_cli(v, args, verb)) - - # https://github.com/python/mypy/issues/2087 - cli.set_by_cli.detector = None # type: ignore - - args = ['--manual-auth-hook', 'command'] - for v in ('manual_auth_hook', 'manual_public_ip_logging_ok'): - self.assertTrue(_call_set_by_cli(v, args, verb)) - - self.assertFalse(_call_set_by_cli('manual', args, verb)) - def _call_set_by_cli(var, args, verb): with mock.patch('certbot._internal.cli.helpful_parser') as mock_parser: diff --git a/certbot/tests/plugins/manual_test.py b/certbot/tests/plugins/manual_test.py index 933c759d6..7318783fd 100644 --- a/certbot/tests/plugins/manual_test.py +++ b/certbot/tests/plugins/manual_test.py @@ -32,8 +32,7 @@ class AuthenticatorTest(test_util.TempDirTestCase): # initialization. self.config = mock.MagicMock( http01_port=0, manual_auth_hook=None, manual_cleanup_hook=None, - manual_public_ip_logging_ok=False, noninteractive_mode=False, - validate_hooks=False, + noninteractive_mode=False, validate_hooks=False, config_dir=os.path.join(self.tempdir, "config_dir"), work_dir=os.path.join(self.tempdir, "work_dir"), backup_dir=os.path.join(self.tempdir, "backup_dir"), @@ -60,19 +59,7 @@ class AuthenticatorTest(test_util.TempDirTestCase): self.assertEqual(self.auth.get_chall_pref('example.org'), [challenges.HTTP01, challenges.DNS01]) - @test_util.patch_get_utility() - def test_ip_logging_not_ok(self, mock_get_utility): - mock_get_utility().yesno.return_value = False - self.assertRaises(errors.PluginError, self.auth.perform, []) - - @test_util.patch_get_utility() - def test_ip_logging_ok(self, mock_get_utility): - mock_get_utility().yesno.return_value = True - self.auth.perform([]) - self.assertTrue(self.config.manual_public_ip_logging_ok) - def test_script_perform(self): - self.config.manual_public_ip_logging_ok = True self.config.manual_auth_hook = ( '{0} -c "from __future__ import print_function;' 'from certbot.compat import os;' @@ -105,7 +92,6 @@ class AuthenticatorTest(test_util.TempDirTestCase): @test_util.patch_get_utility() def test_manual_perform(self, mock_get_utility): - self.config.manual_public_ip_logging_ok = True self.assertEqual( self.auth.perform(self.achalls), [achall.response(achall.account_key) for achall in self.achalls]) @@ -116,7 +102,6 @@ class AuthenticatorTest(test_util.TempDirTestCase): self.assertFalse(kwargs['wrap']) def test_cleanup(self): - self.config.manual_public_ip_logging_ok = True self.config.manual_auth_hook = ('{0} -c "import sys; sys.stdout.write(\'foo\')"' .format(sys.executable)) self.config.manual_cleanup_hook = '# cleanup'