Add confirmation before certificate delete (#8349)

* Ask confirmation before deleting cert

* Changelog

* Fix lint and preserve non-interactively deletetion

* Improve English

* Integrate message into yesno() without logger

* Reduce if/else into oneliner

* Expand "certificate(s)" in `get_certnames`

* Address comments

* Update certbot/certbot/_internal/cert_manager.py

certbot/CHANGELOG.md

@@ -25,6 +25,7 @@ More details about these changes can be found on our GitHub repo.
 
 * `--preconfigured-renewal` flag, for packager use only.
   See the [packaging guide](https://certbot.eff.org/docs/packaging.html).
+* Confirmation when deleting certificates
 
 ### Changed
 

certbot/certbot/_internal/cert_manager.py

@@ -90,9 +90,16 @@ def certificates(config):
 def delete(config):
     """Delete Certbot files associated with a certificate lineage."""
     certnames = get_certnames(config, "delete", allow_multiple=True)
+    disp = zope.component.getUtility(interfaces.IDisplay)
+    msg = ["The following certificate(s) are selected for deletion:\n"]
+    for certname in certnames:
+        msg.append("  * " + certname)
+    msg.append("\nAre you sure you want to delete the above certificate(s)?")
+    if not disp.yesno("\n".join(msg), default=True):
+        logger.info("Deletion of certificate(s) canceled.")
+        return
     for certname in certnames:
         storage.delete_files(config, certname)
-        disp = zope.component.getUtility(interfaces.IDisplay)
         disp.notification("Deleted all files relating to certificate {0}."
             .format(certname), pause=False)
 

certbot/tests/cert_manager_test.py

@@ -116,10 +116,11 @@ class DeleteTest(storage_test.BaseRenewableCertTest):
     @test_util.patch_get_utility()
     @mock.patch('certbot._internal.cert_manager.lineage_for_certname')
     @mock.patch('certbot._internal.storage.delete_files')
-    def test_delete_from_config(self, mock_delete_files, mock_lineage_for_certname,
-        unused_get_utility):
+    def test_delete_from_config_yes(self, mock_delete_files, mock_lineage_for_certname,
+        mock_util):
         """Test delete"""
         mock_lineage_for_certname.return_value = self.test_rc
+        mock_util().yesno.return_value = True
         self.config.certname = "example.org"
         self._call()
         mock_delete_files.assert_called_once_with(self.config, "example.org")
@@ -127,27 +128,65 @@ class DeleteTest(storage_test.BaseRenewableCertTest):
     @test_util.patch_get_utility()
     @mock.patch('certbot._internal.cert_manager.lineage_for_certname')
     @mock.patch('certbot._internal.storage.delete_files')
-    def test_delete_interactive_single(self, mock_delete_files, mock_lineage_for_certname,
+    def test_delete_from_config_no(self, mock_delete_files, mock_lineage_for_certname,
+        mock_util):
+        """Test delete"""
+        mock_lineage_for_certname.return_value = self.test_rc
+        mock_util().yesno.return_value = False
+        self.config.certname = "example.org"
+        self._call()
+        self.assertEqual(mock_delete_files.call_count, 0)
+
+    @test_util.patch_get_utility()
+    @mock.patch('certbot._internal.cert_manager.lineage_for_certname')
+    @mock.patch('certbot._internal.storage.delete_files')
+    def test_delete_interactive_single_yes(self, mock_delete_files, mock_lineage_for_certname,
         mock_util):
         """Test delete"""
         mock_lineage_for_certname.return_value = self.test_rc
         mock_util().checklist.return_value = (display_util.OK, ["example.org"])
+        mock_util().yesno.return_value = True
         self._call()
         mock_delete_files.assert_called_once_with(self.config, "example.org")
 
     @test_util.patch_get_utility()
     @mock.patch('certbot._internal.cert_manager.lineage_for_certname')
     @mock.patch('certbot._internal.storage.delete_files')
-    def test_delete_interactive_multiple(self, mock_delete_files, mock_lineage_for_certname,
+    def test_delete_interactive_single_no(self, mock_delete_files, mock_lineage_for_certname,
+        mock_util):
+        """Test delete"""
+        mock_lineage_for_certname.return_value = self.test_rc
+        mock_util().checklist.return_value = (display_util.OK, ["example.org"])
+        mock_util().yesno.return_value = False
+        self._call()
+        self.assertEqual(mock_delete_files.call_count, 0)
+
+    @test_util.patch_get_utility()
+    @mock.patch('certbot._internal.cert_manager.lineage_for_certname')
+    @mock.patch('certbot._internal.storage.delete_files')
+    def test_delete_interactive_multiple_yes(self, mock_delete_files, mock_lineage_for_certname,
         mock_util):
         """Test delete"""
         mock_lineage_for_certname.return_value = self.test_rc
         mock_util().checklist.return_value = (display_util.OK, ["example.org", "other.org"])
+        mock_util().yesno.return_value = True
         self._call()
         mock_delete_files.assert_any_call(self.config, "example.org")
         mock_delete_files.assert_any_call(self.config, "other.org")
         self.assertEqual(mock_delete_files.call_count, 2)
 
+    @test_util.patch_get_utility()
+    @mock.patch('certbot._internal.cert_manager.lineage_for_certname')
+    @mock.patch('certbot._internal.storage.delete_files')
+    def test_delete_interactive_multiple_no(self, mock_delete_files, mock_lineage_for_certname,
+        mock_util):
+        """Test delete"""
+        mock_lineage_for_certname.return_value = self.test_rc
+        mock_util().checklist.return_value = (display_util.OK, ["example.org", "other.org"])
+        mock_util().yesno.return_value = False
+        self._call()
+        self.assertEqual(mock_delete_files.call_count, 0)
+
 
 class CertificatesTest(BaseCertManagerTest):
     """Tests for certbot._internal.cert_manager.certificates