Issue 3816/revamp register subcommand (#6006)

* address issue #3816

* formatting update

* remove unused variable

* address pylint trailing whitespace error

* revert whitespace add

* update boulder ci test for new update_registration verb

* address code review comments

* Issue 3816: Revert renaming '...update_regristration...' tests to '...update_account...'. Fix removing update_registration default argument value.

* Issue 3816: Fix '--update-registration' not referring to 'update_registration' default as opposed to 'update_account'.

* Issue 3816: delint tox output.

* Issue 3816: Change @example.org domain to @domain.org in boulder test script

* Issue 3816: Update CHANGELOG.md for Issue 3816 and remove extraneous space in main.py

* Issue 3816: Remove extraneous default variable.

CHANGELOG.md

@@ -6,11 +6,14 @@ Certbot adheres to [Semantic Versioning](http://semver.org/).
 
 ### Added
 
-*
+* Added the `update_account` subcommand for account management commands.
 
 ### Changed
 
-*
+* Copied account management functionality from the `register` subcommand
+  to the `update_account` subcommand.
+* Marked usage `register --update-registration` for deprecation and 
+  removal in a future release.
 
 ### Fixed
 

certbot/cli.py

@@ -101,6 +101,7 @@ manage certificates:
 
 manage your account with Let's Encrypt:
     register        Create a Let's Encrypt ACME account
+    update_account  Update a Let's Encrypt ACME account
   --agree-tos       Agree to the ACME server's Subscriber Agreement
    -m EMAIL         Email address for important account notifications
 """
@@ -397,9 +398,14 @@ VERB_HELP = [
     }),
     ("register", {
         "short": "Register for account with Let's Encrypt / other ACME server",
-        "opts": "Options for account registration & modification",
+        "opts": "Options for account registration",
         "usage": "\n\n  certbot register --email user@example.com [options]\n\n"
     }),
+    ("update_account", {
+        "short": "Update existing account with Let's Encrypt / other ACME server",
+        "opts": "Options for account modification",
+        "usage": "\n\n  certbot update_account --email updated_email@example.com [options]\n\n"
+    }),
     ("unregister", {
         "short": "Irrevocably deactivate your account",
         "opts": "Options for account deactivation.",
@@ -465,6 +471,7 @@ class HelpfulArgumentParser(object):
             "install": main.install,
             "plugins": main.plugins_cmd,
             "register": main.register,
+            "update_account": main.update_account,
             "unregister": main.unregister,
             "renew": main.renew,
             "revoke": main.revoke,
@@ -993,21 +1000,21 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False):  # pylint: dis
              "certificates. Updates to the Subscriber Agreement will still "
              "affect you, and will be effective 14 days after posting an "
              "update to the web site.")
+    # TODO: When `certbot register --update-registration` is fully deprecated,
+    # delete following helpful.add
     helpful.add(
         "register", "--update-registration", action="store_true",
-        default=flag_default("update_registration"),
+        default=flag_default("update_registration"), dest="update_registration",
-        help="With the register verb, indicates that details associated "
+        help=argparse.SUPPRESS)
-             "with an existing registration, such as the e-mail address, "
-             "should be updated, rather than registering a new account.")
     helpful.add(
-        ["register", "unregister", "automation"], "-m", "--email",
+        ["register", "update_account", "unregister", "automation"], "-m", "--email",
         default=flag_default("email"),
         help=config_help("email"))
-    helpful.add(["register", "automation"], "--eff-email", action="store_true",
+    helpful.add(["register", "update_account", "automation"], "--eff-email", action="store_true",
                 default=flag_default("eff_email"), dest="eff_email",
                 help="Share your e-mail address with EFF")
-    helpful.add(["register", "automation"], "--no-eff-email", action="store_false",
+    helpful.add(["register", "update_account", "automation"], "--no-eff-email",
-                default=flag_default("eff_email"), dest="eff_email",
+                action="store_false", default=flag_default("eff_email"), dest="eff_email",
                 help="Don't share your e-mail address with EFF")
     helpful.add(
         ["automation", "certonly", "run"],

certbot/main.py

@@ -654,7 +654,45 @@ def unregister(config, unused_plugins):
 
 
 def register(config, unused_plugins):
-    """Create or modify accounts on the server.
+    """Create accounts on the server.
+
+    :param config: Configuration object
+    :type config: interfaces.IConfig
+
+    :param unused_plugins: List of plugins (deprecated)
+    :type unused_plugins: `list` of `str`
+
+    :returns: `None` or a string indicating and error
+    :rtype: None or str
+
+    """
+    # TODO: When `certbot register --update-registration` is fully deprecated,
+    # delete the true case of if block
+    if config.update_registration:
+        msg = ("Usage 'certbot register --update-registration' is deprecated.\n"
+               "Please use 'cerbot update_account [options]' instead.\n")
+        logger.warning(msg)
+        return update_account(config, unused_plugins)
+
+    # Portion of _determine_account logic to see whether accounts already
+    # exist or not.
+    account_storage = account.AccountFileStorage(config)
+    accounts = account_storage.find_all()
+
+    if len(accounts) > 0:
+        # TODO: add a flag to register a duplicate account (this will
+        #       also require extending _determine_account's behavior
+        #       or else extracting the registration code from there)
+        return ("There is an existing account; registration of a "
+                "duplicate account with this command is currently "
+                "unsupported.")
+    # _determine_account will register an account
+    _determine_account(config)
+    return
+
+
+def update_account(config, unused_plugins):
+    """Modify accounts on the server.
 
     :param config: Configuration object
     :type config: interfaces.IConfig
@@ -673,20 +711,6 @@ def register(config, unused_plugins):
     reporter_util = zope.component.getUtility(interfaces.IReporter)
     add_msg = lambda m: reporter_util.add_message(m, reporter_util.MEDIUM_PRIORITY)
 
-    # registering a new account
-    if not config.update_registration:
-        if len(accounts) > 0:
-            # TODO: add a flag to register a duplicate account (this will
-            #       also require extending _determine_account's behavior
-            #       or else extracting the registration code from there)
-            return ("There is an existing account; registration of a "
-                    "duplicate account with this command is currently "
-                    "unsupported.")
-        # _determine_account will register an account
-        _determine_account(config)
-        return
-
-    # --update-registration
     if len(accounts) == 0:
         return "Could not find an existing account to update."
     if config.email is None:

certbot/tests/main_test.py

@@ -1398,7 +1398,20 @@ class MainTest(test_util.ConfigTestCase):  # pylint: disable=too-many-public-met
                 x = self._call_no_clientmock(["register", "--email", "user@example.org"])
                 self.assertTrue("There is an existing account" in x[0])
 
-    def test_update_registration_no_existing_accounts(self):
+    def test_update_account_no_existing_accounts(self):
+        # with mock.patch('certbot.main.client') as mocked_client:
+        with mock.patch('certbot.main.account') as mocked_account:
+            mocked_storage = mock.MagicMock()
+            mocked_account.AccountFileStorage.return_value = mocked_storage
+            mocked_storage.find_all.return_value = []
+            x = self._call_no_clientmock(
+                ["update_account", "--email",
+                 "user@example.org"])
+            self.assertTrue("Could not find an existing account" in x[0])
+
+    # TODO: When `certbot register --update-registration` is fully deprecated,
+    # delete the following test
+    def test_update_registration_no_existing_accounts_deprecated(self):
         # with mock.patch('certbot.main.client') as mocked_client:
         with mock.patch('certbot.main.account') as mocked_account:
             mocked_storage = mock.MagicMock()
@@ -1409,7 +1422,9 @@ class MainTest(test_util.ConfigTestCase):  # pylint: disable=too-many-public-met
                  "user@example.org"])
             self.assertTrue("Could not find an existing account" in x[0])
 
-    def test_update_registration_unsafely(self):
+    # TODO: When `certbot register --update-registration` is fully deprecated,
+    # delete the following test
+    def test_update_registration_unsafely_deprecated(self):
         # This test will become obsolete when register --update-registration
         # supports removing an e-mail address from the account
         with mock.patch('certbot.main.account') as mocked_account:
@@ -1423,7 +1438,39 @@ class MainTest(test_util.ConfigTestCase):  # pylint: disable=too-many-public-met
 
     @mock.patch('certbot.main.display_ops.get_email')
     @test_util.patch_get_utility()
-    def test_update_registration_with_email(self, mock_utility, mock_email):
+    def test_update_account_with_email(self, mock_utility, mock_email):
+        email = "user@example.com"
+        mock_email.return_value = email
+        with mock.patch('certbot.eff.handle_subscription') as mock_handle:
+            with mock.patch('certbot.main._determine_account') as mocked_det:
+                with mock.patch('certbot.main.account') as mocked_account:
+                    with mock.patch('certbot.main.client') as mocked_client:
+                        mocked_storage = mock.MagicMock()
+                        mocked_account.AccountFileStorage.return_value = mocked_storage
+                        mocked_storage.find_all.return_value = ["an account"]
+                        mocked_det.return_value = (mock.MagicMock(), "foo")
+                        cb_client = mock.MagicMock()
+                        mocked_client.Client.return_value = cb_client
+                        x = self._call_no_clientmock(
+                            ["update_account"])
+                        # When registration change succeeds, the return value
+                        # of register() is None
+                        self.assertTrue(x[0] is None)
+                        # and we got supposedly did update the registration from
+                        # the server
+                        self.assertTrue(
+                            cb_client.acme.update_registration.called)
+                        # and we saved the updated registration on disk
+                        self.assertTrue(mocked_storage.save_regr.called)
+                        self.assertTrue(
+                            email in mock_utility().add_message.call_args[0][0])
+                        self.assertTrue(mock_handle.called)
+
+    # TODO: When `certbot register --update-registration` is fully deprecated,
+    # delete the following test
+    @mock.patch('certbot.main.display_ops.get_email')
+    @test_util.patch_get_utility()
+    def test_update_registration_with_email_deprecated(self, mock_utility, mock_email):
         email = "user@example.com"
         mock_email.return_value = email
         with mock.patch('certbot.eff.handle_subscription') as mock_handle:

tests/certbot-boulder-integration.sh

@@ -207,10 +207,16 @@ common unregister
 
 common register --email ex1@domain.org,ex2@domain.org
 
+# TODO: When `certbot register --update-registration` is fully deprecated, delete the two following deprecated uses
+
 common register --update-registration --email ex1@domain.org
 
 common register --update-registration --email ex1@domain.org,ex2@domain.org
 
+common update_account --email example@domain.org
+
+common update_account --email ex1@domain.org,ex2@domain.org
+
 common plugins --init --prepare | grep webroot
 
 # We start a server listening on the port for the