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 <ebportnoy@gmail.com>

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'),

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
 

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

certbot/certbot/_internal/cli/report_config_interaction.py

@@ -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)

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),

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.

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:

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'