From 9556dfac5a00114f2dc09f1a86f4fd5f127626a6 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Wed, 11 May 2016 10:44:40 -0700 Subject: [PATCH 01/19] Add a way to update registrations Fixes #1215 --- certbot/cli.py | 10 ++++++++-- certbot/main.py | 28 ++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/certbot/cli.py b/certbot/cli.py index 97b1a5399..b4460f681 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -265,8 +265,9 @@ class HelpfulArgumentParser(object): self.VERBS = {"auth": main.obtain_cert, "certonly": main.obtain_cert, "config_changes": main.config_changes, "run": main.run, "install": main.install, "plugins": main.plugins_cmd, - "renew": main.renew, "revoke": main.revoke, - "rollback": main.rollback, "everything": main.run} + "register": main.register, "renew": main.renew, + "revoke": main.revoke, "rollback": main.rollback, + "everything": main.run} # List of topics for which additional help can be provided HELP_TOPICS = ["all", "security", "paths", "automation", "testing"] + list(self.VERBS) @@ -591,6 +592,11 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): "certificates. Updates to the Subscriber Agreement will still " "affect you, and will be effective 14 days after posting an " "update to the web site.") + helpful.add( + None, "--update-registration", action="store_true", + help="With the register verb, indicates that details associated " + "with an existing registration, such as the e-mail address, " + "should be updated, rather than registering a new account.") helpful.add(None, "-m", "--email", help=config_help("email")) # positional arg shadows --domains, instead of appending, and # --domains is useful, because it can be stored in config diff --git a/certbot/main.py b/certbot/main.py index 309889e8e..08a5a3ea4 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -10,6 +10,7 @@ import traceback import zope.component +from acme import errors as acme_errors from acme import jose import certbot @@ -363,6 +364,33 @@ def _init_le_client(config, authenticator, installer): return client.Client(config, acc, authenticator, installer, acme=acme) +def register(config, unused_plugins): + """Create or modify accounts on the server.""" + + # Currently, only --update-registration is implemented. Issue #2446 + # calls for a fuller register verb, to allow better separation of + # account management from obtaining certs. + if not config.update_registration: + return "Currently, only register --update-registration is implemented." + if config.email is None: + return ("Currently, --update-registration can only change the e-mail " + "address\nassociated with an account. A new e-mail address is " + "required\n(hint: --email)") + acc, acme = _determine_account(config) + acme_client = client.Client(config, acc, None, None, acme=acme) + try: + updated_reg = client.messages.Registration.from_data(email=config.email) + acme_client.acme.update_registration(acme_client.account.regr, + updated_reg) + except acme_errors.UnexpectedUpdate: + # We expect the unexpected update! + pass + query_data = acme_client.acme.query_registration(acme_client.account.regr) + # We rely on an ACME exception to interrupt this process if it didn't work. + print("Registration change succeeded. New registration data:\n") + print(query_data) + + def install(config, plugins): """Install a previously obtained cert in a server.""" # XXX: Update for renewer/RenewableCert From 70912be5a9a1000e235ff74d9935cfc2aef9b3c9 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Wed, 18 May 2016 14:57:31 -0700 Subject: [PATCH 02/19] Associate --update-registration with register help topic --- certbot/cli.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/certbot/cli.py b/certbot/cli.py index d42d77412..7f3779e5b 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -88,7 +88,8 @@ More detailed help: the available topics are: all, automation, paths, security, testing, or any of the subcommands or - plugins (certonly, install, nginx, apache, standalone, webroot, etc) + plugins (certonly, install, register, nginx, apache, standalone, webroot, + etc.) """ @@ -615,7 +616,7 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): "affect you, and will be effective 14 days after posting an " "update to the web site.") helpful.add( - None, "--update-registration", action="store_true", + "register", "--update-registration", action="store_true", help="With the register verb, indicates that details associated " "with an existing registration, such as the e-mail address, " "should be updated, rather than registering a new account.") From 2ba5ce9217433cb913edf408477c96a710a91e22 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Wed, 18 May 2016 15:55:25 -0700 Subject: [PATCH 03/19] Mention register subcommand in main help --- certbot/cli.py | 1 + 1 file changed, 1 insertion(+) diff --git a/certbot/cli.py b/certbot/cli.py index 7f3779e5b..f8be642d8 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -63,6 +63,7 @@ cert. Major SUBCOMMANDS are: install Install a previously obtained cert in a server renew Renew previously obtained certs that are near expiry revoke Revoke a previously obtained certificate + register Perform tasks related to registering with the CA rollback Rollback server configuration changes made during install config_changes Show changes made to server config during installation plugins Display information about installed plugins From 3105fe8a34618f22b906ef8f4875dbf5f1695dca Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Sat, 21 May 2016 13:26:59 -0700 Subject: [PATCH 04/19] Disable too-many-statements on parser setup --- certbot/cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot/cli.py b/certbot/cli.py index 7898c4c0b..356e71c56 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -567,7 +567,7 @@ class HelpfulArgumentParser(object): return dict([(t, t == chosen_topic) for t in self.help_topics]) -def prepare_and_parse_args(plugins, args, detect_defaults=False): +def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: disable=too-many-statements """Returns parsed command line arguments. :param .PluginsRegistry plugins: available plugins From fab9f8db78a52cd7019867a4bf5748cf8f932097 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Sat, 21 May 2016 13:27:12 -0700 Subject: [PATCH 05/19] Complete register functionality (for now) --- certbot/main.py | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/certbot/main.py b/certbot/main.py index 7b50efd0f..fc54c8615 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -370,11 +370,27 @@ def _init_le_client(config, authenticator, installer): def register(config, unused_plugins): """Create or modify accounts on the server.""" - # Currently, only --update-registration is implemented. Issue #2446 - # calls for a fuller register verb, to allow better separation of - # account management from obtaining certs. + # Portion of _determine_account logic to see whether accounts already + # exist or not. + account_storage = account.AccountFileStorage(config) + accounts = account_storage.find_all() + + # registering a new account if not config.update_registration: - return "Currently, only register --update-registration is implemented." + 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: return ("Currently, --update-registration can only change the e-mail " "address\nassociated with an account. A new e-mail address is " @@ -392,6 +408,7 @@ def register(config, unused_plugins): # We rely on an ACME exception to interrupt this process if it didn't work. print("Registration change succeeded. New registration data:\n") print(query_data) + return def install(config, plugins): From e7c8b73083a33cc942edeb9d69774b18f4895c9b Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Sun, 22 May 2016 09:50:27 -0700 Subject: [PATCH 06/19] Test coverage for register verb --- certbot/tests/cli_test.py | 67 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/certbot/tests/cli_test.py b/certbot/tests/cli_test.py index d7965a24e..1b6d032a8 100644 --- a/certbot/tests/cli_test.py +++ b/certbot/tests/cli_test.py @@ -14,6 +14,7 @@ import mock import six from six.moves import reload_module # pylint: disable=import-error +from acme import errors as acme_errors from acme import jose from certbot import account @@ -888,6 +889,72 @@ class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods self._call(['-c', test_util.vector_path('cli.ini')]) self.assertTrue(mocked_run.called) + def test_register(self): + with mock.patch('certbot.main.client') as mocked_client: + acc = mock.MagicMock() + acc.id = "imaginary_account" + mocked_client.register.return_value = (acc, "worked") + self._call_no_clientmock(["register", "--email", "user@example.org"]) + # TODO: It would be more correct to explicitly check that + # _determine_account() gets called in the above case, + # but coverage statistics should also show that it did. + 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 = ["an account"] + x = self._call_no_clientmock(["register", "--email", "user@example.org"]) + assert "There is an existing account" in x[0] + + def test_update_registration_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( + ["register", "--update-registration", "--email", + "user@example.org"]) + assert "Could not find an existing account" in x[0] + + def test_update_registration_no_email(self): + # This test will become obsolete when register --update-registration + # supports updating something other than the e-mail address! + # 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 = ["an account"] + x = self._call_no_clientmock(["register", "--update-registration"]) + assert "can only change the e-mail" in x[0] + + def test_update_registration_with_email(self): + with mock.patch('certbot.main.client') as mocked_client: + with mock.patch('certbot.main.account') as mocked_account: + with mock.patch('certbot.main._determine_account') as mocked_det: + 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 = ("a", "b") + acme_client = mock.MagicMock() + mocked_client.Client.return_value = acme_client + # Currently the update_registration() call always + # raises a harmless acme_errors.UnexpectedUpdate. + # If this is fixed, we should get rid of both this + # side effect and the corresponding try/catch in + # main.register(). + uu = acme_errors.UnexpectedUpdate + acme_client.acme.update_registration.side_effect = uu + x = self._call_no_clientmock( + ["register", "--update-registration", "--email", + "user@example.org"]) + # When registration change succeeds, the return value + # of register() is None + assert x[0] is None + # and we got far enough to query the registration from + # the server + assert acme_client.acme.query_registration.call_count == 1 + class DetermineAccountTest(unittest.TestCase): """Tests for certbot.cli._determine_account.""" From 0b691d1b561c0bb5b068664935fb4fe4ed19dbdc Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Wed, 25 May 2016 14:11:12 -0700 Subject: [PATCH 07/19] Use better update_registration invocation, and reporter interface --- certbot/main.py | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/certbot/main.py b/certbot/main.py index fa5797483..3a3218a49 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -10,7 +10,6 @@ import traceback import zope.component -from acme import errors as acme_errors from acme import jose import certbot @@ -397,17 +396,12 @@ def register(config, unused_plugins): "required\n(hint: --email)") acc, acme = _determine_account(config) acme_client = client.Client(config, acc, None, None, acme=acme) - try: - updated_reg = client.messages.Registration.from_data(email=config.email) - acme_client.acme.update_registration(acme_client.account.regr, - updated_reg) - except acme_errors.UnexpectedUpdate: - # We expect the unexpected update! - pass - query_data = acme_client.acme.query_registration(acme_client.account.regr) - # We rely on an ACME exception to interrupt this process if it didn't work. - print("Registration change succeeded. New registration data:\n") - print(query_data) + data = acme_client.acme.update_registration(acc.regr.update( + body=acc.regr.body.update(contact=('mailto:' + config.email,)))) + # We rely on an exception to interrupt this process if it didn't work. + reporter_util = zope.component.getUtility(interfaces.IReporter) + msg = "Your e-mail address was updated to {0}.".format(config.email) + reporter_util.add_message(msg, reporter_util.HIGH_PRIORITY) return From 6ceaca4a9d26c87c732329b149a7f7301fe22ad0 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Wed, 25 May 2016 14:11:43 -0700 Subject: [PATCH 08/19] Minor test updates for update_registration call change --- certbot/tests/cli_test.py | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/certbot/tests/cli_test.py b/certbot/tests/cli_test.py index 4dfc78792..007675759 100644 --- a/certbot/tests/cli_test.py +++ b/certbot/tests/cli_test.py @@ -947,25 +947,18 @@ class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods mocked_storage = mock.MagicMock() mocked_account.AccountFileStorage.return_value = mocked_storage mocked_storage.find_all.return_value = ["an account"] - mocked_det.return_value = ("a", "b") + mocked_det.return_value = (mock.MagicMock(), "foo") acme_client = mock.MagicMock() mocked_client.Client.return_value = acme_client - # Currently the update_registration() call always - # raises a harmless acme_errors.UnexpectedUpdate. - # If this is fixed, we should get rid of both this - # side effect and the corresponding try/catch in - # main.register(). - uu = acme_errors.UnexpectedUpdate - acme_client.acme.update_registration.side_effect = uu x = self._call_no_clientmock( ["register", "--update-registration", "--email", "user@example.org"]) # When registration change succeeds, the return value # of register() is None assert x[0] is None - # and we got far enough to query the registration from + # and we got supposedly did update the registration from # the server - assert acme_client.acme.query_registration.call_count == 1 + assert acme_client.acme.update_registration.call_count == 1 class DetermineAccountTest(unittest.TestCase): From 2268dbf48987e902bf01580d3748853afc6b2778 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 25 May 2016 14:55:44 -0700 Subject: [PATCH 09/19] delint --- certbot/main.py | 2 +- certbot/tests/cli_test.py | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/certbot/main.py b/certbot/main.py index 3a3218a49..b7dc88839 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -396,7 +396,7 @@ def register(config, unused_plugins): "required\n(hint: --email)") acc, acme = _determine_account(config) acme_client = client.Client(config, acc, None, None, acme=acme) - data = acme_client.acme.update_registration(acc.regr.update( + acme_client.acme.update_registration(acc.regr.update( body=acc.regr.body.update(contact=('mailto:' + config.email,)))) # We rely on an exception to interrupt this process if it didn't work. reporter_util = zope.component.getUtility(interfaces.IReporter) diff --git a/certbot/tests/cli_test.py b/certbot/tests/cli_test.py index 007675759..92caf8f04 100644 --- a/certbot/tests/cli_test.py +++ b/certbot/tests/cli_test.py @@ -14,7 +14,6 @@ import mock import six from six.moves import reload_module # pylint: disable=import-error -from acme import errors as acme_errors from acme import jose from certbot import account From 29822aad9d2a76780178d2e1f493ef4b5966d40f Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 25 May 2016 14:57:01 -0700 Subject: [PATCH 10/19] denit --- certbot/main.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/certbot/main.py b/certbot/main.py index b7dc88839..56d09c078 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -401,8 +401,7 @@ def register(config, unused_plugins): # We rely on an exception to interrupt this process if it didn't work. reporter_util = zope.component.getUtility(interfaces.IReporter) msg = "Your e-mail address was updated to {0}.".format(config.email) - reporter_util.add_message(msg, reporter_util.HIGH_PRIORITY) - return + reporter_util.add_message(msg, reporter_util.MEDIUM_PRIORITY) def install(config, plugins): From 14e2ea92ee21fa1bea1b7b5ae07b47a7a29dade0 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 25 May 2016 16:32:03 -0700 Subject: [PATCH 11/19] Add a way to only save registration resources --- certbot/account.py | 23 ++++++++++++++++++----- certbot/tests/account_test.py | 10 ++++++++++ 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/certbot/account.py b/certbot/account.py index cc50a6ea6..5e0d4f2fd 100644 --- a/certbot/account.py +++ b/certbot/account.py @@ -186,16 +186,29 @@ class AccountFileStorage(interfaces.AccountStorage): return acc def save(self, account): + self._save(account, regr_only=False) + + def save_regr(self, account): + """Save the registration resource. + + :param Account account: account whose regr should be saved + + """ + self._save(account, regr_only=True) + + def _save(self, account, regr_only): account_dir_path = self._account_dir_path(account.id) le_util.make_or_verify_dir(account_dir_path, 0o700, os.geteuid(), self.config.strict_permissions) try: with open(self._regr_path(account_dir_path), "w") as regr_file: regr_file.write(account.regr.json_dumps()) - with le_util.safe_open(self._key_path(account_dir_path), - "w", chmod=0o400) as key_file: - key_file.write(account.key.json_dumps()) - with open(self._metadata_path(account_dir_path), "w") as metadata_file: - metadata_file.write(account.meta.json_dumps()) + if not regr_only: + with le_util.safe_open(self._key_path(account_dir_path), + "w", chmod=0o400) as key_file: + key_file.write(account.key.json_dumps()) + with open(self._metadata_path( + account_dir_path), "w") as metadata_file: + metadata_file.write(account.meta.json_dumps()) except IOError as error: raise errors.AccountStorageError(error) diff --git a/certbot/tests/account_test.py b/certbot/tests/account_test.py index a96e57507..4cd2bfebf 100644 --- a/certbot/tests/account_test.py +++ b/certbot/tests/account_test.py @@ -137,6 +137,16 @@ class AccountFileStorageTest(unittest.TestCase): # restore self.assertEqual(self.acc, self.storage.load(self.acc.id)) + def test_save_regr(self): + self.storage.save_regr(self.acc) + account_path = os.path.join(self.config.accounts_dir, self.acc.id) + self.assertTrue(os.path.exists(account_path)) + self.assertTrue(os.path.exists(os.path.join( + account_path, "regr.json"))) + for file_name in "meta.json", "private_key.json": + self.assertFalse(os.path.exists( + os.path.join(account_path, file_name))) + def test_find_all(self): self.storage.save(self.acc) self.assertEqual([self.acc], self.storage.find_all()) From 5531c156e8511c0528101452c7a5f71d3952175f Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 25 May 2016 16:41:19 -0700 Subject: [PATCH 12/19] Save the updated registration --- certbot/main.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/certbot/main.py b/certbot/main.py index 56d09c078..3491f44a6 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -396,9 +396,10 @@ def register(config, unused_plugins): "required\n(hint: --email)") acc, acme = _determine_account(config) acme_client = client.Client(config, acc, None, None, acme=acme) - acme_client.acme.update_registration(acc.regr.update( - body=acc.regr.body.update(contact=('mailto:' + config.email,)))) # We rely on an exception to interrupt this process if it didn't work. + acc.regr = acme_client.acme.update_registration(acc.regr.update( + body=acc.regr.body.update(contact=('mailto:' + config.email,)))) + account_storage.save_regr(account) reporter_util = zope.component.getUtility(interfaces.IReporter) msg = "Your e-mail address was updated to {0}.".format(config.email) reporter_util.add_message(msg, reporter_util.MEDIUM_PRIORITY) From a87df33de69c9105888e8ec857e36af5c000b58d Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 25 May 2016 16:41:39 -0700 Subject: [PATCH 13/19] Update register tests --- certbot/tests/cli_test.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/certbot/tests/cli_test.py b/certbot/tests/cli_test.py index 92caf8f04..fbb822490 100644 --- a/certbot/tests/cli_test.py +++ b/certbot/tests/cli_test.py @@ -914,7 +914,7 @@ class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods mocked_account.AccountFileStorage.return_value = mocked_storage mocked_storage.find_all.return_value = ["an account"] x = self._call_no_clientmock(["register", "--email", "user@example.org"]) - assert "There is an existing account" in x[0] + self.assertTrue("There is an existing account" in x[0]) def test_update_registration_no_existing_accounts(self): # with mock.patch('certbot.main.client') as mocked_client: @@ -924,8 +924,8 @@ class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods mocked_storage.find_all.return_value = [] x = self._call_no_clientmock( ["register", "--update-registration", "--email", - "user@example.org"]) - assert "Could not find an existing account" in x[0] + "user@example.org"]) + self.assertTrue("Could not find an existing account" in x[0]) def test_update_registration_no_email(self): # This test will become obsolete when register --update-registration @@ -936,7 +936,7 @@ class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods mocked_account.AccountFileStorage.return_value = mocked_storage mocked_storage.find_all.return_value = ["an account"] x = self._call_no_clientmock(["register", "--update-registration"]) - assert "can only change the e-mail" in x[0] + self.assertTrue("can only change the e-mail" in x[0]) def test_update_registration_with_email(self): with mock.patch('certbot.main.client') as mocked_client: @@ -951,13 +951,16 @@ class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods mocked_client.Client.return_value = acme_client x = self._call_no_clientmock( ["register", "--update-registration", "--email", - "user@example.org"]) + "user@example.org"]) # When registration change succeeds, the return value # of register() is None - assert x[0] is None + self.assertTrue(x[0] is None) # and we got supposedly did update the registration from # the server - assert acme_client.acme.update_registration.call_count == 1 + self.assertTrue( + acme_client.acme.update_registration.called) + # and we saved the updated registration on disk + self.assertTrue(mocked_storage.save_regr.called) class DetermineAccountTest(unittest.TestCase): From 1819b22ebc11b917cc59ea503001596657f00234 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 25 May 2016 16:47:05 -0700 Subject: [PATCH 14/19] Fix typo --- certbot/main.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot/main.py b/certbot/main.py index 3491f44a6..20b9a7ce5 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -399,7 +399,7 @@ def register(config, unused_plugins): # We rely on an exception to interrupt this process if it didn't work. acc.regr = acme_client.acme.update_registration(acc.regr.update( body=acc.regr.body.update(contact=('mailto:' + config.email,)))) - account_storage.save_regr(account) + account_storage.save_regr(acc) reporter_util = zope.component.getUtility(interfaces.IReporter) msg = "Your e-mail address was updated to {0}.".format(config.email) reporter_util.add_message(msg, reporter_util.MEDIUM_PRIORITY) From 6598bcb53eda7e16975e6129f3700bd1944270db Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 25 May 2016 21:53:20 -0700 Subject: [PATCH 15/19] refactor get_email --- certbot/client.py | 2 +- certbot/display/ops.py | 69 +++++++++++++++++++------------ certbot/tests/display/ops_test.py | 15 +++---- 3 files changed, 49 insertions(+), 37 deletions(-) diff --git a/certbot/client.py b/certbot/client.py index ba31f8760..a885d0426 100644 --- a/certbot/client.py +++ b/certbot/client.py @@ -150,7 +150,7 @@ def perform_registration(acme, config): return acme.register(messages.NewRegistration.from_data(email=config.email)) except messages.Error as e: if e.typ == "urn:acme:error:invalidEmail": - config.namespace.email = display_ops.get_email(more=True, invalid=True) + config.namespace.email = display_ops.get_email(invalid=True) return perform_registration(acme, config) else: raise diff --git a/certbot/display/ops.py b/certbot/display/ops.py index 6752bf0c1..16c44a881 100644 --- a/certbot/display/ops.py +++ b/certbot/display/ops.py @@ -15,41 +15,56 @@ logger = logging.getLogger(__name__) z_util = zope.component.getUtility -def get_email(more=False, invalid=False): +def get_email(invalid=False, optional=True): """Prompt for valid email address. - :param bool more: explain why the email is strongly advisable, but how to - skip it - :param bool invalid: true if the user just typed something, but it wasn't - a valid-looking email + :param bool invalid: True if an invalid was provided by the user + :param bool optional: True if the user can use + --register-unsafely-without-email to avoid providing an e-mail - :returns: Email or ``None`` if cancelled by user. + :returns: e-mail address :rtype: str - """ - msg = "Enter email address (used for urgent notices and lost key recovery)" - if invalid: - msg = "There seem to be problems with that address. " + msg - if more: - msg += ('\n\nIf you really want to skip this, you can run the client with ' - '--register-unsafely-without-email but make sure you backup your ' - 'account key from /etc/letsencrypt/accounts\n\n') - try: - code, email = zope.component.getUtility(interfaces.IDisplay).input(msg) - except errors.MissingCommandlineFlag: - msg = ("You should register before running non-interactively, or provide --agree-tos" - " and --email flags") - raise errors.MissingCommandlineFlag(msg) + :raises errors.Error: if the user cancels - if code == display_util.OK: - if le_util.safe_email(email): - return email + """ + invalid_prefix = "There seem to be problems with that address. " + msg = "Enter email address (used for urgent notices and lost key recovery)" + unsafe_suggestion = ("\n\nIf you really want to skip this, you can run " + "the client with --register-unsafely-without-email " + "but make sure you backup your account key from " + "/etc/letsencrypt/accounts\n\n") + if optional: + if invalid: + msg += unsafe_suggestion else: - # TODO catch the server's ACME invalid email address error, and - # make a similar call when that happens - return get_email(more=True, invalid=(email != "")) + suggest_unsafe = True else: - return None + suggest_unsafe = False + + while True: + try: + code, email = z_util(interfaces.IDisplay).input( + invalid_prefix + msg if invalid else msg) + except errors.MissingCommandlineFlag: + msg = ("You should register before running non-interactively, " + "or provide --agree-tos and --email flags") + raise errors.MissingCommandlineFlag(msg) + + if code != display_util.OK: + if optional: + raise errors.Error( + "An e-mail address or " + "--register-unsafely-without-email must be provided.") + else: + raise errors.Error("An e-mail address must be provided.") + elif le_util.safe_email(email): + return email + elif suggest_unsafe: + msg += unsafe_suggestion + suggest_unsafe = False # add this message at most once + + invalid = bool(email) def choose_account(accounts): diff --git a/certbot/tests/display/ops_test.py b/certbot/tests/display/ops_test.py index 05cb6b12d..be496a42a 100644 --- a/certbot/tests/display/ops_test.py +++ b/certbot/tests/display/ops_test.py @@ -12,6 +12,7 @@ from acme import jose from acme import messages from certbot import account +from certbot import errors from certbot import interfaces from certbot.display import util as display_util @@ -37,7 +38,7 @@ class GetEmailTest(unittest.TestCase): def test_cancel_none(self): self.input.return_value = (display_util.CANCEL, "foo@bar.baz") - self.assertTrue(self._call() is None) + self.assertRaises(errors.Error, self._call) def test_ok_safe(self): self.input.return_value = (display_util.OK, "foo@bar.baz") @@ -52,7 +53,7 @@ class GetEmailTest(unittest.TestCase): self.assertTrue(self._call() is "foo@bar.baz") def test_more_and_invalid_flags(self): - more_txt = "--register-unsafely-without-email" + optional_txt = "--register-unsafely-without-email" invalid_txt = "There seem to be problems" base_txt = "Enter email" self.input.return_value = (display_util.OK, "foo@bar.baz") @@ -60,16 +61,12 @@ class GetEmailTest(unittest.TestCase): mock_safe_email.return_value = True self._call() msg = self.input.call_args[0][0] - self.assertTrue(more_txt not in msg) + self.assertTrue(optional_txt not in msg) self.assertTrue(invalid_txt not in msg) self.assertTrue(base_txt in msg) - self._call(more=True) + self._call(invalid=True) msg = self.input.call_args[0][0] - self.assertTrue(more_txt in msg) - self.assertTrue(invalid_txt not in msg) - self._call(more=True, invalid=True) - msg = self.input.call_args[0][0] - self.assertTrue(more_txt in msg) + self.assertTrue(optional_txt in msg) self.assertTrue(invalid_txt in msg) self.assertTrue(base_txt in msg) From 8d802ef6fd3699e4ee1f695cdf985336606fe9fc Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 25 May 2016 22:12:06 -0700 Subject: [PATCH 16/19] Fix up display/ops.py coverage --- certbot/tests/display/ops_test.py | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/certbot/tests/display/ops_test.py b/certbot/tests/display/ops_test.py index be496a42a..84a4ada3f 100644 --- a/certbot/tests/display/ops_test.py +++ b/certbot/tests/display/ops_test.py @@ -39,6 +39,7 @@ class GetEmailTest(unittest.TestCase): def test_cancel_none(self): self.input.return_value = (display_util.CANCEL, "foo@bar.baz") self.assertRaises(errors.Error, self._call) + self.assertRaises(errors.Error, self._call, optional=False) def test_ok_safe(self): self.input.return_value = (display_util.OK, "foo@bar.baz") @@ -52,23 +53,24 @@ class GetEmailTest(unittest.TestCase): mock_safe_email.side_effect = [False, True] self.assertTrue(self._call() is "foo@bar.baz") - def test_more_and_invalid_flags(self): - optional_txt = "--register-unsafely-without-email" + def test_invalid_flag(self): invalid_txt = "There seem to be problems" - base_txt = "Enter email" self.input.return_value = (display_util.OK, "foo@bar.baz") with mock.patch("certbot.display.ops.le_util.safe_email") as mock_safe_email: mock_safe_email.return_value = True self._call() - msg = self.input.call_args[0][0] - self.assertTrue(optional_txt not in msg) - self.assertTrue(invalid_txt not in msg) - self.assertTrue(base_txt in msg) + self.assertTrue(invalid_txt not in self.input.call_args[0][0]) self._call(invalid=True) - msg = self.input.call_args[0][0] - self.assertTrue(optional_txt in msg) - self.assertTrue(invalid_txt in msg) - self.assertTrue(base_txt in msg) + self.assertTrue(invalid_txt in self.input.call_args[0][0]) + + def test_optional_flag(self): + self.input.return_value = (display_util.OK, "foo@bar.baz") + with mock.patch("certbot.display.ops.le_util.safe_email") as mock_safe_email: + mock_safe_email.side_effect = [False, True] + self._call(optional=False) + for call in self.input.call_args_list: + self.assertTrue( + "--register-unsafely-without-email" not in call[0][0]) class ChooseAccountTest(unittest.TestCase): From 89279a72bd03c5f05b8b00813e7ea3801059aa4d Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 25 May 2016 22:23:36 -0700 Subject: [PATCH 17/19] Interatively ask for e-mail with register verb --- certbot/main.py | 9 ++++++--- certbot/tests/cli_test.py | 11 ++++++----- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/certbot/main.py b/certbot/main.py index 20b9a7ce5..c048d9277 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -391,9 +391,12 @@ def register(config, unused_plugins): if len(accounts) == 0: return "Could not find an existing account to update." if config.email is None: - return ("Currently, --update-registration can only change the e-mail " - "address\nassociated with an account. A new e-mail address is " - "required\n(hint: --email)") + if config.register_unsafely_without_email: + return ("--register-unsafely-without-email provided, however, a " + "new e-mail address must\ncurrently be provided when " + "updating a registration.") + config.namespace.email = display_ops.get_email(optional=False) + acc, acme = _determine_account(config) acme_client = client.Client(config, acc, None, None, acme=acme) # We rely on an exception to interrupt this process if it didn't work. diff --git a/certbot/tests/cli_test.py b/certbot/tests/cli_test.py index b348e8ea7..b6103c2e8 100644 --- a/certbot/tests/cli_test.py +++ b/certbot/tests/cli_test.py @@ -942,16 +942,17 @@ class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods "user@example.org"]) self.assertTrue("Could not find an existing account" in x[0]) - def test_update_registration_no_email(self): + def test_update_registration_unsafely(self): # This test will become obsolete when register --update-registration - # supports updating something other than the e-mail address! - # with mock.patch('certbot.main.client') as mocked_client: + # supports removing an e-mail address from the account 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 = ["an account"] - x = self._call_no_clientmock(["register", "--update-registration"]) - self.assertTrue("can only change the e-mail" in x[0]) + x = self._call_no_clientmock( + "register --update-registration " + "--register-unsafely-without-email".split()) + self.assertTrue("--register-unsafely-without-email" in x[0]) def test_update_registration_with_email(self): with mock.patch('certbot.main.client') as mocked_client: From 14b45b795a0a37af9b039a10728d290b6a113874 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 25 May 2016 22:45:57 -0700 Subject: [PATCH 18/19] give register full test coverage --- certbot/tests/cli_test.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/certbot/tests/cli_test.py b/certbot/tests/cli_test.py index b6103c2e8..0bf34eb10 100644 --- a/certbot/tests/cli_test.py +++ b/certbot/tests/cli_test.py @@ -954,7 +954,11 @@ class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods "--register-unsafely-without-email".split()) self.assertTrue("--register-unsafely-without-email" in x[0]) - def test_update_registration_with_email(self): + @mock.patch('certbot.main.display_ops.get_email') + @mock.patch('certbot.main.zope.component.getUtility') + def test_update_registration_with_email(self, mock_utility, mock_email): + email = "user@example.com" + mock_email.return_value = email with mock.patch('certbot.main.client') as mocked_client: with mock.patch('certbot.main.account') as mocked_account: with mock.patch('certbot.main._determine_account') as mocked_det: @@ -966,8 +970,7 @@ class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods acme_client = mock.MagicMock() mocked_client.Client.return_value = acme_client x = self._call_no_clientmock( - ["register", "--update-registration", "--email", - "user@example.org"]) + ["register", "--update-registration"]) # When registration change succeeds, the return value # of register() is None self.assertTrue(x[0] is None) @@ -977,6 +980,8 @@ class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods acme_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]) def test_conflicting_args(self): args = ['renew', '--dialog', '--text'] From c1e4b57d374201e1eb19a32357485df11a368a92 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Tue, 31 May 2016 15:53:40 -0700 Subject: [PATCH 19/19] Tiny documentation fixes --- certbot/display/ops.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/certbot/display/ops.py b/certbot/display/ops.py index 16c44a881..a91c2d747 100644 --- a/certbot/display/ops.py +++ b/certbot/display/ops.py @@ -18,7 +18,7 @@ z_util = zope.component.getUtility def get_email(invalid=False, optional=True): """Prompt for valid email address. - :param bool invalid: True if an invalid was provided by the user + :param bool invalid: True if an invalid address was provided by the user :param bool optional: True if the user can use --register-unsafely-without-email to avoid providing an e-mail @@ -32,7 +32,7 @@ def get_email(invalid=False, optional=True): msg = "Enter email address (used for urgent notices and lost key recovery)" unsafe_suggestion = ("\n\nIf you really want to skip this, you can run " "the client with --register-unsafely-without-email " - "but make sure you backup your account key from " + "but make sure you then backup your account key from " "/etc/letsencrypt/accounts\n\n") if optional: if invalid: