don't superfluously ask whether to renew, when changing key type (#9421)

* dont superfluously ask whether to renew, when changing key type

* reorder conditions

this prevents "Certificate not yet due for renewal" being printed

* and replace superfluous mock

* mock renewal.should_renew
This commit is contained in:
alexzorin 2022-10-07 08:29:58 +11:00 committed by GitHub
parent a0b8a2cc62
commit f5e7d16303
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 23 additions and 5 deletions

View file

@ -147,19 +147,21 @@ def _get_and_save_cert(le_client: client.Client, config: configuration.Namespace
def _handle_unexpected_key_type_migration(config: configuration.NamespaceConfig,
cert: storage.RenewableCert) -> None:
cert: storage.RenewableCert) -> bool:
"""
This function ensures that the user will not implicitly migrate an existing key
from one type to another in the situation where a certificate for that lineage
already exist and they have not provided explicitly --key-type and --cert-name.
:param config: Current configuration provided by the client
:param cert: Matching certificate that could be renewed
:returns: Whether a key type migration is going ahead.
:rtype: `bool`
"""
new_key_type = config.key_type.upper()
cur_key_type = cert.private_key_type.upper()
if new_key_type == cur_key_type:
return
return False
# If both --key-type and --cert-name are provided, we consider the user's intent to
# be unambiguous: to change the key type of this lineage.
@ -172,7 +174,7 @@ def _handle_unexpected_key_type_migration(config: configuration.NamespaceConfig,
yes_label='Update key type', no_label='Keep existing key type',
default=False, force_interactive=False,
):
return
return True
# If --key-type was set on the CLI but the user did not confirm the key type change using
# one of the two above methods, their intent is ambiguous. Error out.
@ -188,6 +190,7 @@ def _handle_unexpected_key_type_migration(config: configuration.NamespaceConfig,
# default value. The user is not asking for a key change: keep the key type of the existing
# lineage.
config.key_type = cur_key_type.lower()
return False
def _handle_subset_cert_request(config: configuration.NamespaceConfig,
@ -254,11 +257,11 @@ def _handle_identical_cert_request(config: configuration.NamespaceConfig,
:rtype: `tuple` of `str`
"""
_handle_unexpected_key_type_migration(config, lineage)
is_key_type_changing = _handle_unexpected_key_type_migration(config, lineage)
if not lineage.ensure_deployed():
return "reinstall", lineage
if renewal.should_renew(config, lineage):
if is_key_type_changing or renewal.should_renew(config, lineage):
return "renew", lineage
if config.reinstall:
# Set with --reinstall, force an identical certificate to be

View file

@ -57,6 +57,21 @@ class TestHandleCerts(unittest.TestCase):
self.assertEqual(ret, ("reinstall", mock_lineage))
self.assertTrue(mock_handle_migration.called)
@mock.patch('certbot._internal.renewal.should_renew')
@mock.patch("certbot.display.util.menu")
@mock.patch("certbot._internal.main._handle_unexpected_key_type_migration")
def test_handle_identical_cert_key_type_change(self, mock_handle_migration, mock_menu,
mock_should_renew):
mock_handle_migration.return_value = True
mock_lineage = mock.Mock()
mock_lineage.ensure_deployed.return_value = True
mock_should_renew.return_value = False
ret = main._handle_identical_cert_request(mock.MagicMock(verb="run", reinstall=False),
mock_lineage)
self.assertTrue(mock_handle_migration.called)
self.assertFalse(mock_menu.called)
self.assertEqual(ret, ("renew", mock_lineage))
@mock.patch("certbot._internal.main._handle_unexpected_key_type_migration")
def test_handle_subset_cert_request(self, mock_handle_migration):
mock_config = mock.Mock()