Add autorenew option to renew subcommand (#5911)

* Add autorenew option to `renew` subcommand.

* Change default value for 'autorenew' cli option.

* Update certbot.cli.prepare_and_parse_args (autorenew)

Set `default` for --autorenew and --no-autorenew.

* Update certbot.storage.RenewableCert.should_autorenew.

- Remove `interactive` argument in RenewableCert.should_autorenew.
- Update certbot.renewal.should_renew.

* Move autorenew enable/disable check to certbot.storage.

- Remove autorenew enable/disable check in
  `certbot.renewal.handle_renewal_request`.
- Fix RenewableCert.autorenewal_is_enabled; autorenew is stored in
  'renewalparams'.
- Add autorenew enable/disable check in
  `RenewableCert.should_autorenew`.
- Update tests test_time_interval_judgments,
  test_autorenewal_is_enabled, test_should_autorenew tests in
  storage_test.py

* certbot: Update RenewableCert.should_autorenew

Remove block that sets autorenew option in the renewal configuration
file.

* certbot: Update prepare_and_parse_args.

Remove --autorenew option.

* certbot: Update CLI_DEFAULTS.

Set default of `autorenew` to True.

* Remove unused imports in certbot.storage.

certbot/cli.py

@@ -1220,6 +1220,10 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False):  # pylint: dis
         " when the user executes \"certbot renew\", regardless of if the certificate"
         " is renewed. This setting does not apply to important TLS configuration"
         " updates.")
+    helpful.add(
+        "renew", "--no-autorenew", action="store_false",
+        default=flag_default("autorenew"), dest="autorenew",
+        help="Disable auto renewal of certificates.")
 
     helpful.add_deprecated_argument("--agree-dev-preview", 0)
     helpful.add_deprecated_argument("--dialog", 0)

certbot/constants.py

@@ -37,6 +37,7 @@ CLI_DEFAULTS = dict(
     expand=False,
     renew_by_default=False,
     renew_with_new_domains=False,
+    autorenew=True,
     allow_subset_of_names=False,
     tos=False,
     account=None,

certbot/renewal.py

@@ -36,7 +36,8 @@ STR_CONFIG_ITEMS = ["config_dir", "logs_dir", "work_dir", "user_agent",
                     "pre_hook", "post_hook", "tls_sni_01_address",
                     "http01_address"]
 INT_CONFIG_ITEMS = ["rsa_key_size", "tls_sni_01_port", "http01_port"]
-BOOL_CONFIG_ITEMS = ["must_staple", "allow_subset_of_names", "reuse_key"]
+BOOL_CONFIG_ITEMS = ["must_staple", "allow_subset_of_names", "reuse_key",
+                     "autorenew"]
 
 CONFIG_ITEMS = set(itertools.chain(
     BOOL_CONFIG_ITEMS, INT_CONFIG_ITEMS, STR_CONFIG_ITEMS, ('pref_challs',)))
@@ -261,7 +262,7 @@ def should_renew(config, lineage):
     if config.renew_by_default:
         logger.debug("Auto-renewal forced with --force-renewal...")
         return True
-    if lineage.should_autorenew(interactive=True):
+    if lineage.should_autorenew():
         logger.info("Cert is due for renewal, auto-renewing...")
         return True
     if config.dry_run:

certbot/storage.py

@@ -920,10 +920,10 @@ class RenewableCert(object):
         :rtype: bool
 
         """
-        return ("autorenew" not in self.configuration or
-                self.configuration.as_bool("autorenew"))
+        return ("autorenew" not in self.configuration["renewalparams"] or
+                self.configuration["renewalparams"].as_bool("autorenew"))
 
-    def should_autorenew(self, interactive=False):
+    def should_autorenew(self):
         """Should we now try to autorenew the most recent cert version?
 
         This is a policy question and does not only depend on whether
@@ -934,16 +934,12 @@ class RenewableCert(object):
         Note that this examines the numerically most recent cert version,
         not the currently deployed version.
 
-        :param bool interactive: set to True to examine the question
-            regardless of whether the renewal configuration allows
-            automated renewal (for interactive use). Default False.
-
         :returns: whether an attempt should now be made to autorenew the
             most current cert version in this lineage
         :rtype: bool
 
         """
-        if interactive or self.autorenewal_is_enabled():
+        if self.autorenewal_is_enabled():
             # Consider whether to attempt to autorenew this cert now
 
             # Renewals on the basis of revocation

certbot/tests/storage_test.py

@@ -383,8 +383,9 @@ class RenewableCertTests(BaseRenewableCertTest):
         os.unlink(self.test_rc.cert)
         self.assertRaises(errors.CertStorageError, self.test_rc.names)
 
+    @mock.patch("certbot.storage.cli")
     @mock.patch("certbot.storage.datetime")
-    def test_time_interval_judgments(self, mock_datetime):
+    def test_time_interval_judgments(self, mock_datetime, mock_cli):
         """Test should_autodeploy() and should_autorenew() on the basis
         of expiry time windows."""
         test_cert = test_util.load_vector("cert_512.pem")
@@ -399,6 +400,8 @@ class RenewableCertTests(BaseRenewableCertTest):
             f.write(test_cert)
 
         mock_datetime.timedelta = datetime.timedelta
+        mock_cli.set_by_cli.return_value = False
+        self.test_rc.configuration["renewalparams"] = {}
 
         for (current_time, interval, result) in [
                 # 2014-12-13 12:00:00+00:00 (about 5 days prior to expiry)
@@ -451,22 +454,25 @@ class RenewableCertTests(BaseRenewableCertTest):
         self.assertFalse(self.test_rc.should_autodeploy())
 
     def test_autorenewal_is_enabled(self):
+        self.test_rc.configuration["renewalparams"] = {}
         self.assertTrue(self.test_rc.autorenewal_is_enabled())
-        self.test_rc.configuration["autorenew"] = "1"
+        self.test_rc.configuration["renewalparams"]["autorenew"] = "True"
         self.assertTrue(self.test_rc.autorenewal_is_enabled())
 
-        self.test_rc.configuration["autorenew"] = "0"
+        self.test_rc.configuration["renewalparams"]["autorenew"] = "False"
         self.assertFalse(self.test_rc.autorenewal_is_enabled())
 
+    @mock.patch("certbot.storage.cli")
     @mock.patch("certbot.storage.RenewableCert.ocsp_revoked")
-    def test_should_autorenew(self, mock_ocsp):
+    def test_should_autorenew(self, mock_ocsp, mock_cli):
         """Test should_autorenew on the basis of reasons other than
         expiry time window."""
         # pylint: disable=too-many-statements
+        mock_cli.set_by_cli.return_value = False
         # Autorenewal turned off
-        self.test_rc.configuration["autorenew"] = "0"
+        self.test_rc.configuration["renewalparams"] = {"autorenew": "False"}
         self.assertFalse(self.test_rc.should_autorenew())
-        self.test_rc.configuration["autorenew"] = "1"
+        self.test_rc.configuration["renewalparams"]["autorenew"] = "True"
         for kind in ALL_FOUR:
             self._write_out_kind(kind, 12)
         # Mandatory renewal on the basis of OCSP revocation