Cleaning up code based on bmw's comments

letsencrypt/auth_handler.py

@@ -66,31 +66,27 @@ class AuthHandler(object):
 
         self._choose_challenges(domains)
 
-        failed_domains = set()
 
         # While there are still challenges remaining...
         while self.achalls:
             resp = self._solve_challenges()
             logger.info("Waiting for verification...")
 
-            # Send all Responses - this modifies dv_c and cont_c
-            response = self._respond(resp, best_effort)
-
-            if response:
-                failed_domains = failed_domains.union(response)
-                for domain in failed_domains:
-                    logger.warning(
-                        "Challenge failed for domain %s",
-                        domain)
-
-        returnDomains = [domain for domain in domains
-                        if domain not in failed_domains]
+            # Send all Responses - this modifies achalls
+            self._respond(resp, best_effort)
 
         # Just make sure all decisions are complete.
         self.verify_authzr_complete()
+
         # Only return valid authorizations
-        return [authzr for authzr in self.authzr.values()
-                if authzr.body.status == messages.STATUS_VALID], returnDomains
+        retVal = [authzr for authzr in self.authzr.values()
+                if authzr.body.status == messages.STATUS_VALID]
+
+        if len(retVal) <= 0:
+            logger.critical("Challenges failed for all domains")
+            raise
+
+        return retVal
 
     def _choose_challenges(self, domains):
         """Retrieve necessary challenges to satisfy server."""
@@ -134,13 +130,11 @@ class AuthHandler(object):
 
         # Check for updated status...
         try:
-            failed_domains = self._poll_challenges(chall_update, best_effort)
+            self._poll_challenges(chall_update, best_effort)
         finally:
             # This removes challenges from self.achalls
             self._cleanup_challenges(active_achalls)
 
-        return failed_domains
-
     def _send_responses(self, achalls, resps, chall_update):
         """Send responses and make sure errors are handled.
 
@@ -172,7 +166,6 @@ class AuthHandler(object):
         """Wait for all challenge results to be determined."""
         dom_to_check = set(chall_update.keys())
         comp_domains = set()
-        failed_domains = set()
         rounds = 0
 
         while dom_to_check and rounds < max_rounds:
@@ -191,7 +184,10 @@ class AuthHandler(object):
                 # We failed some challenges... damage control
                 else:
                     if best_effort:
-                        failed_domains.add(domain)
+                        comp_domains.add(domain)
+                        logger.warning(
+                            "Challenge failed for domain %s",
+                            domain)
                     else:
                         all_failed_achalls.update(
                             updated for _, updated in failed_achalls)
@@ -200,12 +196,10 @@ class AuthHandler(object):
                 _report_failed_challs(all_failed_achalls)
                 raise errors.FailedChallenges(all_failed_achalls)
 
-            dom_to_check -= comp_domains.union(failed_domains)
+            dom_to_check -= comp_domains
             comp_domains.clear()
             rounds += 1
 
-        return failed_domains
-
     def _handle_check(self, domain, achalls):
         """Returns tuple of ('completed', 'failed')."""
         completed = []

letsencrypt/cli.py

@@ -694,7 +694,7 @@ def obtain_cert(config, plugins, lineage=None):
     if config.csr is not None:
         assert lineage is None, "Did not expect a CSR with a RenewableCert"
         csr, typ = config.actual_csr
-        certr, chain = le_client.obtain_certificate_from_csr(config.domains, csr, typ, authzr=False)
+        certr, chain = le_client.obtain_certificate_from_csr(config.domains, csr, typ)
         if config.dry_run:
             logger.info(
                 "Dry run: skipping saving certificate to %s", config.cert_path)
@@ -1621,10 +1621,11 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False):
         help="Require that all configuration files are owned by the current "
              "user; only needed if your config is somewhere unsafe like /tmp/")
     helpful.add(
-        "automation", "--allow-subset-of-names", dest="allow_subset_of_names",
-        action="store_true", default=False,
-        help="Allow subsets of domain names in a single lineage to fail "
-             "validation without exiting.")
+        "automation", "--allow-subset-of-names",
+        action="store_true",
+        help="When performing domain validation, do not consider it a failure "
+             "if authorizations can not be obtained for a strict subset of "
+             "the requested domains. This option cannot be used with --csr.")
 
     helpful.add_group(
         "renew", description="The 'renew' subcommand will attempt to renew all"

letsencrypt/client.py

@@ -189,7 +189,7 @@ class Client(object):
             self.auth_handler = None
 
     def obtain_certificate_from_csr(self, domains, csr,
-        typ=OpenSSL.crypto.FILETYPE_ASN1, authzr=False):
+        typ=OpenSSL.crypto.FILETYPE_ASN1, authzr=None):
         """Obtain certificate.
 
         Internal function with precondition that `domains` are
@@ -199,6 +199,8 @@ class Client(object):
         :param .le_util.CSR csr: DER-encoded Certificate Signing
             Request. The key used to generate this CSR can be different
             than `authkey`.
+        :param dict authzr: ACME Authorization Resource dict where keys are
+            domains and values are :class:`acme.messages.AuthorizationResource`
 
         :returns: `.CertificateResource` and certificate chain (as
             returned by `.fetch_chain`).
@@ -215,10 +217,8 @@ class Client(object):
 
         logger.debug("CSR: %s, domains: %s", csr, domains)
 
-        if authzr is False:
-            authzr, _ = self.auth_handler.get_authorizations(
-                            domains,
-                            self.config.allow_subset_of_names)
+        if authzr is None:
+            authzr = self.auth_handler.get_authorizations(domains)
 
         certr = self.acme.request_issuance(
             jose.ComparableX509(
@@ -240,15 +240,20 @@ class Client(object):
         :rtype: tuple
 
         """
-        authzr, domains = self.auth_handler.get_authorizations(domains,
-                                     self.config.allow_subset_of_names)
+        authzr = self.auth_handler.get_authorizations(
+                domains,
+                self.config.allow_subset_of_names)
+
+        domains = [a.body.identifier.value.encode('ascii', 'ignore')
+                                                    for a in authzr]
 
         # Create CSR from names
         key = crypto_util.init_save_key(
             self.config.rsa_key_size, self.config.key_dir)
         csr = crypto_util.init_save_csr(key, domains, self.config.csr_dir)
 
-        return self.obtain_certificate_from_csr(domains, csr, authzr=authzr) + (key, csr)
+        return (self.obtain_certificate_from_csr(domains, csr, authzr=authzr)
+                                                                + (key, csr))
 
     def obtain_and_enroll_certificate(self, domains):
         """Obtain and enroll certificate.

letsencrypt/tests/auth_handler_test.py

@@ -87,7 +87,7 @@ class GetAuthorizationsTest(unittest.TestCase):
 
         mock_poll.side_effect = self._validate_all
 
-        authzr, _ = self.handler.get_authorizations(["0"])
+        authzr = self.handler.get_authorizations(["0"])
 
         self.assertEqual(self.mock_net.answer_challenge.call_count, 1)
 
@@ -127,9 +127,7 @@ class GetAuthorizationsTest(unittest.TestCase):
             self.assertTrue(achall.typ in ["tls-sni-01", "http-01", "dns"])
 
         # Length of authorizations list
-        self.assertEqual(len(authzr[0]), 1)
-        # Length of valid domains list
-        self.assertEqual(len(authzr[1]), 1)
+        self.assertEqual(len(authzr), 1)
 
     @mock.patch("letsencrypt.auth_handler.AuthHandler._poll_challenges")
     def test_name3_tls_sni_01_3(self, mock_poll):
@@ -138,7 +136,7 @@ class GetAuthorizationsTest(unittest.TestCase):
 
         mock_poll.side_effect = self._validate_all
 
-        authzr, _ = self.handler.get_authorizations(["0", "1", "2"])
+        authzr = self.handler.get_authorizations(["0", "1", "2"])
 
         self.assertEqual(self.mock_net.answer_challenge.call_count, 3)
 

letsencrypt/tests/client_test.py

@@ -124,7 +124,7 @@ class ClientTest(unittest.TestCase):
             self.eg_domains,
             self.config.allow_subset_of_names)
 
-        authzr, _ = self.client.auth_handler.get_authorizations()
+        authzr = self.client.auth_handler.get_authorizations()
 
         self.acme.request_issuance.assert_called_once_with(
             jose.ComparableX509(OpenSSL.crypto.load_certificate_request(
@@ -158,7 +158,7 @@ class ClientTest(unittest.TestCase):
             self.assertRaises(errors.ConfigurationError,
                 cli.HelpfulArgumentParser.handle_csr, mock_parser, mock_parsed_args)
 
-            authzr, _ = self.client.auth_handler.get_authorizations(self.eg_domains, False)
+            authzr = self.client.auth_handler.get_authorizations(self.eg_domains, False)
 
             self.assertEqual(
                 (mock.sentinel.certr, mock.sentinel.chain),
@@ -190,7 +190,17 @@ class ClientTest(unittest.TestCase):
         # return_value is essentially set to (None, None) in
         # _mock_obtain_certificate(), which breaks this test.
         # Thus fixed by the next line.
-        self.client.auth_handler.get_authorizations.return_value = (None, domains)
+
+        authzr = []
+
+        for domain in domains:
+            authzr.append(
+                mock.MagicMock(
+                    body=mock.MagicMock(
+                        identifier=mock.MagicMock(
+                            value=domain))))
+
+        self.client.auth_handler.get_authorizations.return_value = authzr
 
         self.assertEqual(
             self.client.obtain_certificate(domains),