Add error message to account registration error (#9233)

* Add  message to account reg. error

* Changelog

* Remove forced lowercase first char

* Catch errors raised by acme library

* Fix mypy and add some comments

* Add some tests

* Move changelog entry to current version

* Address comments

* Address additional comments

Put everything in this commit instead of using the "Commit suggestion"
feat on Github, which would resolve in 4 different tiny commits.

certbot/CHANGELOG.md

@@ -22,6 +22,9 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
   agree with a non-existent Terms of Service ("None"). This bug is now fixed, so
   that if an ACME server does not provide any Terms of Service to agree with, the
   user is not asked to agree to a non-existent Terms of Service any longer.
+* If account registration fails, Certbot did not relay the error from the ACME server
+  back to the user. This is now fixed: the error message from the ACME server is now
+  presented to the user when account registration fails.
 
 More details about these changes can be found on our GitHub repo.
 

certbot/certbot/_internal/main.py

@@ -21,6 +21,7 @@ import zope.interface
 
 from acme import client as acme_client
 from acme import errors as acme_errors
+from acme import messages as acme_messages
 import certbot
 from certbot import configuration
 from certbot import crypto_util
@@ -726,10 +727,14 @@ def _determine_account(config: configuration.NamespaceConfig
                 display_util.notify("Account registered.")
             except errors.MissingCommandlineFlag:
                 raise
-            except errors.Error:
+            except (errors.Error, acme_messages.Error) as err:
                 logger.debug("", exc_info=True)
+                if acme_messages.is_acme_error(err):
+                    err_msg = f"Error returned by the ACME server: {str(err)}"
+                else:
+                    err_msg = str(err)
                 raise errors.Error(
-                    "Unable to register an account with ACME server")
+                    f"Unable to register an account with ACME server. {err_msg}")
 
     config.account = acc.id
     return acc, acme

certbot/tests/main_test.py

@@ -16,6 +16,7 @@ import unittest
 import josepy as jose
 import pytz
 
+from acme.messages import Error as acme_error
 from certbot import crypto_util, configuration
 from certbot import errors
 from certbot import interfaces
@@ -593,6 +594,17 @@ class DetermineAccountTest(test_util.ConfigTestCase):
             mock_storage.return_value = self.account_storage
             return _determine_account(self.config)
 
+    @mock.patch('certbot._internal.client.register')
+    @mock.patch('certbot._internal.client.display_ops.get_email')
+    def _register_error_common(self, err_msg, exception, mock_get_email, mock_register):
+        mock_get_email.return_value = 'foo@bar.baz'
+        mock_register.side_effect = exception
+        try:
+            self._call()
+        except errors.Error as err:
+            self.assertEqual(f"Unable to register an account with ACME server. {err_msg}",
+                             str(err))
+
     def test_args_account_set(self):
         self.account_storage.save(self.accs[1], self.mock_client)
         self.config.account = self.accs[1].id
@@ -617,6 +629,16 @@ class DetermineAccountTest(test_util.ConfigTestCase):
         self.assertEqual(self.accs[1].id, self.config.account)
         self.assertIsNone(self.config.email)
 
+    @mock.patch('certbot._internal.client.display_ops.choose_account')
+    def test_multiple_accounts_canceled(self, mock_choose_accounts):
+        for acc in self.accs:
+            self.account_storage.save(acc, self.mock_client)
+        mock_choose_accounts.return_value = None
+        try:
+            self._call()
+        except errors.Error as err:
+            self.assertIn("No account has been chosen", str(err))
+
     @mock.patch('certbot._internal.client.display_ops.get_email')
     @mock.patch('certbot._internal.main.display_util.notify')
     def test_no_accounts_no_email(self, mock_notify, mock_get_email):
@@ -641,6 +663,24 @@ class DetermineAccountTest(test_util.ConfigTestCase):
         self.assertEqual(self.accs[1].id, self.config.account)
         self.assertEqual('other email', self.config.email)
 
+    def test_register_error_certbot(self):
+        err_msg = "Some error message raised by Certbot"
+        self._register_error_common(err_msg, errors.Error(err_msg))
+
+    def test_register_error_acme_type_and_detail(self):
+        err_msg = ("Error returned by the ACME server: urn:ietf:params:acme:"
+                   "error:malformed :: The request message was malformed :: "
+                   "must agree to terms of service")
+        exception = acme_error(typ = "urn:ietf:params:acme:error:malformed",
+                               detail = "must agree to terms of service")
+        self._register_error_common(err_msg, exception)
+
+    def test_register_error_acme_type_only(self):
+        err_msg = ("Error returned by the ACME server: urn:ietf:params:acme:"
+                   "error:serverInternal :: The server experienced an internal error")
+        exception = acme_error(typ = "urn:ietf:params:acme:error:serverInternal")
+        self._register_error_common(err_msg, exception)
+
 
 class MainTest(test_util.ConfigTestCase):
     """Tests for different commands."""