From 5ba23a6047a5aefbddb55760bea4b04faa31d040 Mon Sep 17 00:00:00 2001 From: James Kasten Date: Thu, 30 Apr 2015 20:01:32 -0700 Subject: [PATCH] fixes to address comments --- letsencrypt/client/account.py | 10 +-- letsencrypt/client/configuration.py | 2 +- letsencrypt/client/interfaces.py | 4 +- letsencrypt/client/network2.py | 4 +- letsencrypt/client/tests/account_test.py | 82 +++++++++---------- .../client/tests/configuration_test.py | 6 +- letsencrypt/scripts/main.py | 40 ++++----- 7 files changed, 75 insertions(+), 73 deletions(-) diff --git a/letsencrypt/client/account.py b/letsencrypt/client/account.py index a3afa1015..e40b990a4 100644 --- a/letsencrypt/client/account.py +++ b/letsencrypt/client/account.py @@ -55,8 +55,8 @@ class Account(object): """URI link for new registrations.""" if self.regr is not None: return self.regr.uri - else: - return None + + return None @property def new_authzr_uri(self): # pylint: disable=missing-docstring @@ -198,7 +198,7 @@ class Account(object): @classmethod def from_email(cls, config, email): - """Generate an account from an email address. + """Generate a new account from an email address. :param config: Configuration :type config: :class:`letsencrypt.client.interfaces.IConfig` @@ -215,8 +215,8 @@ class Account(object): le_util.make_or_verify_dir( config.account_keys_dir, 0o700, os.geteuid()) key = crypto_util.init_save_key( - config.rsa_key_size, config.account_keys_dir, - cls._get_config_filename(email)) + config.rsa_key_size, config.account_keys_dir, + cls._get_config_filename(email)) return cls(config, key, email) raise errors.LetsEncryptClientError("Invalid email address.") diff --git a/letsencrypt/client/configuration.py b/letsencrypt/client/configuration.py index a0ccdb462..df00ee3aa 100644 --- a/letsencrypt/client/configuration.py +++ b/letsencrypt/client/configuration.py @@ -52,7 +52,7 @@ class NamespaceConfig(object): def accounts_dir(self): #pylint: disable=missing-docstring return os.path.join( self.namespace.config_dir, constants.ACCOUNTS_DIR, - self.namespace.server.partition(":")[0]) + self.namespace.server.replace(':', '-').replace('/', '-')) @property def account_keys_dir(self): #pylint: disable=missing-docstring diff --git a/letsencrypt/client/interfaces.py b/letsencrypt/client/interfaces.py index d432e656e..1d52d854c 100644 --- a/letsencrypt/client/interfaces.py +++ b/letsencrypt/client/interfaces.py @@ -95,6 +95,8 @@ class IConfig(zope.interface.Interface): "be trusted in order to avoid further modifications to the client.") authenticator = zope.interface.Attribute( "Authenticator to use for responding to challenges.") + email = zope.interface.Attribute( + "Email used for registration and recovery contact.") rsa_key_size = zope.interface.Attribute("Size of the RSA key.") config_dir = zope.interface.Attribute("Configuration directory.") @@ -110,7 +112,7 @@ class IConfig(zope.interface.Interface): accounts_dir = zope.interface.Attribute( "Directory where all account information is stored.") account_keys_dir = zope.interface.Attribute( - "Directory where all account keys are stored".) + "Directory where all account keys are stored.") rec_token_dir = zope.interface.Attribute( "Directory where all recovery tokens are saved.") key_dir = zope.interface.Attribute("Keys storage.") diff --git a/letsencrypt/client/network2.py b/letsencrypt/client/network2.py index abe48adb5..59c1d0a10 100644 --- a/letsencrypt/client/network2.py +++ b/letsencrypt/client/network2.py @@ -86,9 +86,7 @@ class Network(object): logging.debug( 'Ignoring wrong Content-Type (%r) for JSON Error', response_ct) - try: - # TODO: This is insufficient or doesn't work as intended. logging.error("Error: %s", jobj) logging.error("Response from server: %s", response.content) raise messages2.Error.from_json(jobj) @@ -328,7 +326,7 @@ class Network(object): body=messages2.ChallengeBody.from_json(response.json())) # TODO: check that challr.uri == response.headers['Location']? if challr.uri != challb.uri: - raise errors.UnexpectedUpdate(challb.uri) + raise errors.UnexpectedUpdate(challr.uri) return challr @classmethod diff --git a/letsencrypt/client/tests/account_test.py b/letsencrypt/client/tests/account_test.py index 2f7bba60a..2855fe1b0 100644 --- a/letsencrypt/client/tests/account_test.py +++ b/letsencrypt/client/tests/account_test.py @@ -23,7 +23,7 @@ class AccountTest(unittest.TestCase): """Tests letsencrypt.client.account.Account.""" def setUp(self): - from letsencrypt.client import account + from letsencrypt.client.account import Account logging.disable(logging.CRITICAL) @@ -51,7 +51,7 @@ class AccountTest(unittest.TestCase): recovery_token="recovery_token", agreement="agreement") ) - self.test_account = account.Account( + self.test_account = Account( self.config, self.key, self.email, None, self.regr) def tearDown(self): @@ -61,27 +61,34 @@ class AccountTest(unittest.TestCase): @mock.patch("letsencrypt.client.account.zope.component.getUtility") @mock.patch("letsencrypt.client.account.crypto_util.init_save_key") def test_prompts(self, mock_key, mock_util): - from letsencrypt.client import account - - displayer = display_util.FileDisplay(sys.stdout) - zope.component.provideUtility(displayer) + from letsencrypt.client.account import Account mock_util().input.return_value = (display_util.OK, self.email) mock_key.return_value = self.key - acc = account.Account.from_prompts(self.config) + acc = Account.from_prompts(self.config) self.assertEqual(acc.email, self.email) self.assertEqual(acc.key, self.key) self.assertEqual(acc.config, self.config) + @mock.patch("letsencrypt.client.account.zope.component.getUtility") + @mock.patch("letsencrypt.client.account.Account.from_email") + def test_prompts_bad_email(self, mock_from_email, mock_util): + from letsencrypt.client.account import Account + + mock_from_email.side_effect = (errors.LetsEncryptClientError, "acc") + mock_util().input.return_value = (display_util.OK, self.email) + + self.assertEqual(Account.from_prompts(self.config), "acc") + + @mock.patch("letsencrypt.client.account.zope.component.getUtility") @mock.patch("letsencrypt.client.account.crypto_util.init_save_key") def test_prompts_empty_email(self, mock_key, mock_util): - displayer = display_util.FileDisplay(sys.stdout) - zope.component.provideUtility(displayer) + from letsencrypt.client.account import Account mock_util().input.return_value = (display_util.OK, "") - acc = account.Account.from_prompts(self.config) + acc = Account.from_prompts(self.config) self.assertTrue(acc.email is None) # _get_config_filename | pylint: disable=protected-access mock_key.assert_called_once_with( @@ -89,17 +96,23 @@ class AccountTest(unittest.TestCase): @mock.patch("letsencrypt.client.account.zope.component.getUtility") def test_prompts_cancel(self, mock_util): - from letsencrypt.client import account + from letsencrypt.client.account import Account mock_util().input.return_value = (display_util.CANCEL, "") - self.assertTrue(account.Account.from_prompts(self.config) is None) + self.assertTrue(Account.from_prompts(self.config) is None) + + def test_from_email(self): + from letsencrypt.client.account import Account + + self.assertRaises(errors.LetsEncryptClientError, + Account.from_email, self.config, "not_valid...email") def test_save_from_existing_account(self): - from letsencrypt.client import account + from letsencrypt.client.account import Account self.test_account.save() - acc = account.Account.from_existing_account(self.config, self.email) + acc = Account.from_existing_account(self.config, self.email) self.assertEqual(acc.key, self.test_account.key) self.assertEqual(acc.email, self.test_account.email) @@ -113,35 +126,22 @@ class AccountTest(unittest.TestCase): self.assertEqual(self.test_account.recovery_token, "recovery_token") def test_partial_properties(self): - from letsencrypt.client import account + from letsencrypt.client.account import Account - partial = account.Account(self.config, self.key) - regr_no_authzr_uri = messages2.RegistrationResource( - uri="uri", - new_authzr_uri=None, - terms_of_service="terms_of_service", - body=messages2.Registration( - recovery_token="recovery_token", agreement="agreement") - ) - partial2 = account.Account( - self.config, self.key, regr=regr_no_authzr_uri) + partial = Account(self.config, self.key) self.assertTrue(partial.uri is None) self.assertTrue(partial.new_authzr_uri is None) self.assertTrue(partial.terms_of_service is None) self.assertTrue(partial.recovery_token is None) - self.assertEqual( - partial2.new_authzr_uri, - "https://letsencrypt-demo.org/acme/new-authz") - def test_partial_account_default(self): - from letsencrypt.client import account + from letsencrypt.client.account import Account - partial = account.Account(self.config, self.key) + partial = Account(self.config, self.key) partial.save() - acc = account.Account.from_existing_account(self.config) + acc = Account.from_existing_account(self.config) self.assertEqual(partial.key, acc.key) self.assertEqual(partial.email, acc.email) @@ -149,33 +149,33 @@ class AccountTest(unittest.TestCase): self.assertEqual(partial.regr, acc.regr) def test_get_accounts(self): - from letsencrypt.client import account + from letsencrypt.client.account import Account - accs = account.Account.get_accounts(self.config) + accs = Account.get_accounts(self.config) self.assertFalse(accs) self.test_account.save() - accs = account.Account.get_accounts(self.config) + accs = Account.get_accounts(self.config) self.assertEqual(len(accs), 1) self.assertEqual(accs[0].email, self.test_account.email) - acc2 = account.Account(self.config, self.key, "testing_email@gmail.com") + acc2 = Account(self.config, self.key, "testing_email@gmail.com") acc2.save() - accs = account.Account.get_accounts(self.config) + accs = Account.get_accounts(self.config) self.assertEqual(len(accs), 2) def test_get_accounts_no_accounts(self): - from letsencrypt.client import account + from letsencrypt.client.account import Account - self.assertEqual(account.Account.get_accounts( + self.assertEqual(Account.get_accounts( mock.Mock(accounts_dir="non-existant")), []) def test_failed_existing_account(self): - from letsencrypt.client import account + from letsencrypt.client.account import Account self.assertRaises( errors.LetsEncryptClientError, - account.Account.from_existing_account, + Account.from_existing_account, self.config, "non-existant@email.org") class SafeEmailTest(unittest.TestCase): diff --git a/letsencrypt/client/tests/configuration_test.py b/letsencrypt/client/tests/configuration_test.py index 9385dbde3..537e26b91 100644 --- a/letsencrypt/client/tests/configuration_test.py +++ b/letsencrypt/client/tests/configuration_test.py @@ -11,7 +11,7 @@ class NamespaceConfigTest(unittest.TestCase): from letsencrypt.client.configuration import NamespaceConfig namespace = mock.MagicMock( config_dir='/tmp/config', work_dir='/tmp/foo', foo='bar', - server='acme-server.org:443') + server='acme-server.org:443/new') self.config = NamespaceConfig(namespace) def test_proxy_getattr(self): @@ -33,10 +33,10 @@ class NamespaceConfigTest(unittest.TestCase): self.config.cert_key_backup, '/tmp/foo/c/acme-server.org') self.assertEqual(self.config.rec_token_dir, '/r') self.assertEqual( - self.config.accounts_dir, '/tmp/config/acc/acme-server.org') + self.config.accounts_dir, '/tmp/config/acc/acme-server.org-443-new') self.assertEqual( self.config.account_keys_dir, - '/tmp/config/acc/acme-server.org/keys') + '/tmp/config/acc/acme-server.org-443-new/keys') if __name__ == '__main__': diff --git a/letsencrypt/scripts/main.py b/letsencrypt/scripts/main.py index 885872623..9d30c916c 100644 --- a/letsencrypt/scripts/main.py +++ b/letsencrypt/scripts/main.py @@ -70,7 +70,7 @@ def create_parser(): add("-k", "--authkey", type=read_file, help="Path to the authorized key file") - add("m", "--email", type=str, + add("-m", "--email", type=str, help="Email address used for account registration.") add("-B", "--rsa-key-size", type=int, default=2048, metavar="N", help=config_help("rsa_key_size")) @@ -176,6 +176,24 @@ def main(): # pylint: disable=too-many-branches, too-many-statements client.rollback(args.rollback, config) sys.exit() + # Prepare for init of Client + if args.email is None: + acc = client.determine_account(config) + else: + try: + # The way to get the default would be args.email = "" + # First try existing account + acc = account.Account.from_existing_account(config, args.email) + except errors.LetsEncryptClientError: + try: + # Try to make an account based on the email address + acc = account.Account.from_email(config, args.email) + except errors.LetsEncryptClientError: + sys.exit(1) + + if acc is None: + sys.exit(0) + if not args.tos: display_eula() @@ -206,26 +224,10 @@ def main(): # pylint: disable=too-many-branches, too-many-statements if not doms: sys.exit(0) - # Prepare for init of Client - if args.email is None: - acc = client.determine_account(config) - else: - try: - # The way to get the default would be args.email = "" - acc = account.from_existing_account(config, args.email) - except errors.LetsEncryptClientError: - try: - acc = account.from_email(config, args.email) - except errors.LetsEncryptClientError: - logging.error("Invalid email address given") - - if acc is None: - sys.exit(0) - acme = client.Client(config, acc, auth, installer) # Validate the key and csr - client.validate_key_csr(account.key) + client.validate_key_csr(acc.key) # This more closely mimics the capabilities of the CLI # It should be possible for reconfig only, install-only, no-install @@ -236,7 +238,7 @@ def main(): # pylint: disable=too-many-branches, too-many-statements acme.register() cert_file, chain_file = acme.obtain_certificate(doms) if installer is not None and cert_file is not None: - acme.deploy_certificate(doms, account.key, cert_file, chain_file) + acme.deploy_certificate(doms, acc.key, cert_file, chain_file) if installer is not None: acme.enhance_config(doms, args.redirect)