fixes to address comments

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.")

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

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.")

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

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):

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__':

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)