Quick refactor

letsencrypt/client.py

@@ -258,8 +258,8 @@ class Client(object):
             params, config, cli_config)
         return lineage
 
-    def save_certificate(self, certr, chain_cert, cert_path, chain_path, fullchain_path):
-        # pylint: disable=no-self-use,too-many-locals
+    def save_certificate(self, certr, chain_cert,
+                         cert_path, chain_path, fullchain_path):
         """Saves the certificate received from the ACME server.
 
         :param certr: ACME "certificate" resource.
@@ -270,7 +270,8 @@ class Client(object):
         :param str chain_path: Candidate path to a certificate chain.
         :param str fullchain_path: Candidate path to a full cert chain.
 
-        :returns: cert_path, chain_path, fullchain_path (absolute paths to the actual files)
+        :returns: cert_path, chain_path, and fullchain_path as absolute
+            paths to the actual files
         :rtype: `tuple` of `str`
 
         :raises IOError: If unable to find room to write the cert files
@@ -281,13 +282,9 @@ class Client(object):
                 os.path.dirname(path), 0o755, os.geteuid(),
                 self.config.strict_permissions)
 
-        # try finally close
-        cert_chain_abspath = None
-        fullchain_abspath = None
-        cert_file, act_cert_path = le_util.unique_file(cert_path, 0o644)
-        # TODO: Except
         cert_pem = OpenSSL.crypto.dump_certificate(
             OpenSSL.crypto.FILETYPE_PEM, certr.body)
+        cert_file, act_cert_path = le_util.unique_file(cert_path, 0o644)
         try:
             cert_file.write(cert_pem)
         finally:
@@ -295,32 +292,13 @@ class Client(object):
         logger.info("Server issued certificate; certificate written to %s",
                     act_cert_path)
 
-        # TODO too long, refactor... either split this into another function
-        # up one level (though it needs a copy of cert_pem) or find a way to
-        # reuse machinery from storage.py
+        cert_chain_abspath = None
+        fullchain_abspath = None
         if chain_cert:
-            chain_file, act_chain_path = le_util.unique_file(chain_path, 0o644)
-            # TODO: Except
             chain_pem = crypto_util.dump_pyopenssl_chain(chain_cert)
-            try:
-                chain_file.write(chain_pem)
-            finally:
-                chain_file.close()
-
-            logger.info("Cert chain written to %s", act_chain_path)
-
-            # This expects a valid chain file
-            cert_chain_abspath = os.path.abspath(act_chain_path)
-
-            # fullchain is cert + chain
-            fullchain_file, act_fullchain_path = le_util.unique_file(
-                 fullchain_path, 0o644)
-            try:
-                fullchain_file.write(cert_pem + chain_pem)
-            finally:
-                fullchain_file.close()
-            logger.info("Cert chain written to %s", act_fullchain_path)
-            fullchain_abspath = os.path.abspath(act_fullchain_path)
+            cert_chain_abspath = _save_chain(chain_pem, chain_path)
+            fullchain_abspath = _save_chain(cert_pem + chain_pem,
+                                            fullchain_path)
 
         return os.path.abspath(act_cert_path), cert_chain_abspath, fullchain_abspath
 
@@ -479,3 +457,25 @@ def view_config_changes(config):
     rev = reverter.Reverter(config)
     rev.recovery_routine()
     rev.view_config_changes()
+
+
+def _save_chain(chain_pem, chain_path):
+    """Saves chain_pem at a unique path based on chain_path.
+
+    :param str chain_pem: certificate chain in PEM format
+    :param str chain_path: candidate path for the cert chain
+
+    :returns: absolute path to saved cert chain
+    :rtype: str
+
+    """
+    chain_file, act_chain_path = le_util.unique_file(chain_path, 0o644)
+    try:
+        chain_file.write(chain_pem)
+    finally:
+        chain_file.close()
+
+    logger.info("Cert chain written to %s", act_chain_path)
+
+    # This expects a valid chain file
+    return os.path.abspath(act_chain_path)

letsencrypt/tests/client_test.py

@@ -114,20 +114,20 @@ class ClientTest(unittest.TestCase):
             mock.sentinel.key, domains, self.config.csr_dir)
         self._check_obtain_certificate()
 
-    def test_save_certificate(self): # pylint: disable=too-many-locals
+    def test_save_certificate(self):
         certs = ["matching_cert.pem", "cert.pem", "cert-san.pem"]
         tmp_path = tempfile.mkdtemp()
         os.chmod(tmp_path, 0o755)  # TODO: really??
 
         certr = mock.MagicMock(body=test_util.load_cert(certs[0]))
-        cert1 = test_util.load_cert(certs[1])
-        cert2 = test_util.load_cert(certs[2])
+        chain_cert = [test_util.load_cert(certs[1]),
+                      test_util.load_cert(certs[2])]
         candidate_cert_path = os.path.join(tmp_path, "certs", "cert.pem")
         candidate_chain_path = os.path.join(tmp_path, "chains", "chain.pem")
         candidate_fullchain_path = os.path.join(tmp_path, "chains", "fullchain.pem")
 
         cert_path, chain_path, fullchain_path = self.client.save_certificate(
-            certr, [cert1, cert2], candidate_cert_path, candidate_chain_path,
+            certr, chain_cert, candidate_cert_path, candidate_chain_path,
             candidate_fullchain_path)
 
         self.assertEqual(os.path.dirname(cert_path),