From dbda499b08722da8311fb2352bf423318772192e Mon Sep 17 00:00:00 2001 From: ohemorange Date: Tue, 31 Mar 2020 12:12:14 -0700 Subject: [PATCH 1/2] Remove interactive redirect ask (#7861) Fixes #7594. Removes the code asking interactively if the user would like to add a redirect. * Remove interactive redirect ask * display.enhancements is no longer used, so remove it. * update changelog * remove references to removed display.enhancements * add redirect_default flag to enhance_config to conditionally set default for redirect value * Update default in help text. --- certbot/CHANGELOG.md | 2 +- certbot/certbot/_internal/cli/__init__.py | 6 +- certbot/certbot/_internal/client.py | 14 ++--- .../certbot/_internal/display/enhancements.py | 63 ------------------- certbot/certbot/_internal/main.py | 2 +- certbot/tests/client_test.py | 19 ++---- certbot/tests/display/enhancements_test.py | 57 ----------------- 7 files changed, 14 insertions(+), 149 deletions(-) delete mode 100644 certbot/certbot/_internal/display/enhancements.py delete mode 100644 certbot/tests/display/enhancements_test.py diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md index c6f6c95a3..88a4ab190 100644 --- a/certbot/CHANGELOG.md +++ b/certbot/CHANGELOG.md @@ -17,7 +17,7 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). ### Changed -* +* Stop asking interactively if the user would like to add a redirect. ### Fixed diff --git a/certbot/certbot/_internal/cli/__init__.py b/certbot/certbot/_internal/cli/__init__.py index 96dfb163e..2f83edcc8 100644 --- a/certbot/certbot/_internal/cli/__init__.py +++ b/certbot/certbot/_internal/cli/__init__.py @@ -321,12 +321,14 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): "--redirect", action="store_true", dest="redirect", default=flag_default("redirect"), help="Automatically redirect all HTTP traffic to HTTPS for the newly " - "authenticated vhost. (default: Ask)") + "authenticated vhost. (default: redirect enabled for install and run, " + "disabled for enhance)") helpful.add( "security", "--no-redirect", action="store_false", dest="redirect", default=flag_default("redirect"), help="Do not automatically redirect all HTTP traffic to HTTPS for the newly " - "authenticated vhost. (default: Ask)") + "authenticated vhost. (default: redirect enabled for install and run, " + "disabled for enhance)") helpful.add( ["security", "enhance"], "--hsts", action="store_true", dest="hsts", default=flag_default("hsts"), diff --git a/certbot/certbot/_internal/client.py b/certbot/certbot/_internal/client.py index 526f4200f..a9bf946cc 100644 --- a/certbot/certbot/_internal/client.py +++ b/certbot/certbot/_internal/client.py @@ -28,7 +28,6 @@ from certbot._internal import constants from certbot._internal import eff from certbot._internal import error_handler from certbot._internal import storage -from certbot._internal.display import enhancements from certbot._internal.plugins import selection as plugin_selection from certbot.compat import os from certbot.display import ops as display_ops @@ -521,12 +520,13 @@ class Client(object): # sites may have been enabled / final cleanup self.installer.restart() - def enhance_config(self, domains, chain_path, ask_redirect=True): + def enhance_config(self, domains, chain_path, redirect_default=True): """Enhance the configuration. :param list domains: list of domains to configure :param chain_path: chain file path :type chain_path: `str` or `None` + :param redirect_default: boolean value that the "redirect" flag should default to :raises .errors.Error: if no installer is specified in the client. @@ -548,14 +548,8 @@ class Client(object): for config_name, enhancement_name, option in enhancement_info: config_value = getattr(self.config, config_name) if enhancement_name in supported: - if ask_redirect: - if config_name == "redirect" and config_value is None: - config_value = enhancements.ask(enhancement_name) - if not config_value: - logger.warning("Future versions of Certbot will automatically " - "configure the webserver so that all requests redirect to secure " - "HTTPS access. You can control this behavior and disable this " - "warning with the --redirect and --no-redirect flags.") + if config_name == "redirect" and config_value is None: + config_value = redirect_default if config_value: self.apply_enhancement(domains, enhancement_name, option) enhanced = True diff --git a/certbot/certbot/_internal/display/enhancements.py b/certbot/certbot/_internal/display/enhancements.py deleted file mode 100644 index ce6470708..000000000 --- a/certbot/certbot/_internal/display/enhancements.py +++ /dev/null @@ -1,63 +0,0 @@ -"""Certbot Enhancement Display""" -import logging - -import zope.component - -from certbot import errors -from certbot import interfaces -from certbot.display import util as display_util - -logger = logging.getLogger(__name__) - -# Define a helper function to avoid verbose code -util = zope.component.getUtility - - -def ask(enhancement): - """Display the enhancement to the user. - - :param str enhancement: One of the - :const:`~certbot.plugins.enhancements.ENHANCEMENTS` enhancements - - :returns: True if feature is desired, False otherwise - :rtype: bool - - :raises .errors.Error: if the enhancement provided is not supported - - """ - try: - # Call the appropriate function based on the enhancement - return DISPATCH[enhancement]() - except KeyError: - logger.error("Unsupported enhancement given to ask(): %s", enhancement) - raise errors.Error("Unsupported Enhancement") - - -def redirect_by_default(): - """Determines whether the user would like to redirect to HTTPS. - - :returns: True if redirect is desired, False otherwise - :rtype: bool - - """ - choices = [ - ("No redirect", "Make no further changes to the webserver configuration."), - ("Redirect", "Make all requests redirect to secure HTTPS access. " - "Choose this for new sites, or if you're confident your site works on HTTPS. " - "You can undo this change by editing your web server's configuration."), - ] - - code, selection = util(interfaces.IDisplay).menu( - "Please choose whether or not to redirect HTTP traffic to HTTPS, removing HTTP access.", - choices, default=1, - cli_flag="--redirect / --no-redirect", force_interactive=True) - - if code != display_util.OK: - return False - - return selection == 1 - - -DISPATCH = { - "redirect": redirect_by_default -} diff --git a/certbot/certbot/_internal/main.py b/certbot/certbot/_internal/main.py index 4a57dd78d..94ccb8b5e 100644 --- a/certbot/certbot/_internal/main.py +++ b/certbot/certbot/_internal/main.py @@ -927,7 +927,7 @@ def enhance(config, plugins): config.chain_path = lineage.chain_path if oldstyle_enh: le_client = _init_le_client(config, authenticator=None, installer=installer) - le_client.enhance_config(domains, config.chain_path, ask_redirect=False) + le_client.enhance_config(domains, config.chain_path, redirect_default=False) if enhancements.are_requested(config): enhancements.enable(lineage, domains, installer, config) diff --git a/certbot/tests/client_test.py b/certbot/tests/client_test.py index 7232ed84b..3e0e5b212 100644 --- a/certbot/tests/client_test.py +++ b/certbot/tests/client_test.py @@ -577,8 +577,7 @@ class EnhanceConfigTest(ClientTestCommon): self.assertRaises( errors.Error, self.client.enhance_config, [self.domain], None) - @mock.patch("certbot._internal.client.enhancements") - def test_unsupported(self, mock_enhancements): + def test_unsupported(self): self.client.installer = mock.MagicMock() self.client.installer.supported_enhancements.return_value = [] @@ -588,7 +587,6 @@ class EnhanceConfigTest(ClientTestCommon): self.client.enhance_config([self.domain], None) self.assertEqual(mock_logger.warning.call_count, 1) self.client.installer.enhance.assert_not_called() - mock_enhancements.ask.assert_not_called() @mock.patch("certbot._internal.client.logger") def test_already_exists_header(self, mock_log): @@ -612,14 +610,11 @@ class EnhanceConfigTest(ClientTestCommon): self._test_with_already_existing() self.assertFalse(mock_log.warning.called) - @mock.patch("certbot._internal.client.enhancements.ask") @mock.patch("certbot._internal.client.logger") - def test_warn_redirect(self, mock_log, mock_ask): + def test_no_warn_redirect(self, mock_log): self.config.redirect = None - mock_ask.return_value = False - self._test_with_already_existing() - self.assertTrue(mock_log.warning.called) - self.assertTrue("disable" in mock_log.warning.call_args[0][0]) + self._test_with_all_supported() + self.assertFalse(mock_log.warning.called) def test_no_ask_hsts(self): self.config.hsts = True @@ -670,12 +665,6 @@ class EnhanceConfigTest(ClientTestCommon): self.client.installer = installer self._test_error_with_rollback() - @mock.patch("certbot._internal.client.enhancements.ask") - def test_ask(self, mock_ask): - self.config.redirect = None - mock_ask.return_value = True - self._test_with_all_supported() - def _test_error_with_rollback(self): self._test_error() self.assertTrue(self.client.installer.restart.called) diff --git a/certbot/tests/display/enhancements_test.py b/certbot/tests/display/enhancements_test.py deleted file mode 100644 index edace29b1..000000000 --- a/certbot/tests/display/enhancements_test.py +++ /dev/null @@ -1,57 +0,0 @@ -"""Module for enhancement UI.""" -import logging -import unittest - -import mock - -from certbot import errors -from certbot.display import util as display_util - - -class AskTest(unittest.TestCase): - """Test the ask method.""" - def setUp(self): - logging.disable(logging.CRITICAL) - - def tearDown(self): - logging.disable(logging.NOTSET) - - @classmethod - def _call(cls, enhancement): - from certbot._internal.display.enhancements import ask - return ask(enhancement) - - @mock.patch("certbot._internal.display.enhancements.util") - def test_redirect(self, mock_util): - mock_util().menu.return_value = (display_util.OK, 1) - self.assertTrue(self._call("redirect")) - - def test_key_error(self): - self.assertRaises(errors.Error, self._call, "unknown_enhancement") - - -class RedirectTest(unittest.TestCase): - """Test the redirect_by_default method.""" - @classmethod - def _call(cls): - from certbot._internal.display.enhancements import redirect_by_default - return redirect_by_default() - - @mock.patch("certbot._internal.display.enhancements.util") - def test_secure(self, mock_util): - mock_util().menu.return_value = (display_util.OK, 1) - self.assertTrue(self._call()) - - @mock.patch("certbot._internal.display.enhancements.util") - def test_cancel(self, mock_util): - mock_util().menu.return_value = (display_util.CANCEL, 1) - self.assertFalse(self._call()) - - @mock.patch("certbot._internal.display.enhancements.util") - def test_easy(self, mock_util): - mock_util().menu.return_value = (display_util.OK, 0) - self.assertFalse(self._call()) - - -if __name__ == "__main__": - unittest.main() # pragma: no cover From bc3088121b74158ca4b24bc380d043f187196957 Mon Sep 17 00:00:00 2001 From: Adrien Ferrand Date: Tue, 31 Mar 2020 22:12:42 +0200 Subject: [PATCH 2/2] Add a step to check powershell version in vs2017-win2016 (#7870) Following discussion at https://github.com/certbot/certbot/pull/7539#issuecomment-572318805, this PR adds a check for Powershell version: we expect that the `vs2017-win2016` node that will test the installer has Powershell 5.x, and nothing else. This ensure that at least one node of the pipeline is testing the installer with the lowest Powershell version supported by Certbot. One full pipeline success can be seen here: https://dev.azure.com/adferrand/certbot/_build/results?buildId=713 I also create on purpose a failing pipeline, that would check that Powershell 6.x is installed. Its result can be seen here: https://dev.azure.com/adferrand/certbot/_build/results?buildId=714 --- .azure-pipelines/templates/installer-tests.yml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/.azure-pipelines/templates/installer-tests.yml b/.azure-pipelines/templates/installer-tests.yml index ea2101792..ebadcb2dc 100644 --- a/.azure-pipelines/templates/installer-tests.yml +++ b/.azure-pipelines/templates/installer-tests.yml @@ -31,6 +31,13 @@ jobs: pool: vmImage: $(imageName) steps: + - powershell: | + $currentVersion = $PSVersionTable.PSVersion + if ($currentVersion.Major -ne 5) { + throw "Powershell version is not 5.x" + } + condition: eq(variables['imageName'], 'vs2017-win2016') + displayName: Check Powershell 5.x is used in vs2017-win2016 - task: UsePythonVersion@0 inputs: versionSpec: 3.8