Addressing review comments.

dhermes · dhermes · commit b59ac6eca249 · 2016-02-03T13:45:58.000-08:00
diff --git a/oauth2client/_pure_python_crypt.py b/oauth2client/_pure_python_crypt.py
@@ -39,8 +39,12 @@
     >   openssl rsa > key.pem
 """
 
-
 _POW2 = (128, 64, 32, 16, 8, 4, 2, 1)
+_PKCS1_MARKER = ('-----BEGIN RSA PRIVATE KEY-----',
+                 '-----END RSA PRIVATE KEY-----')
+_PKCS8_MARKER = ('-----BEGIN PRIVATE KEY-----',
+                 '-----END PRIVATE KEY-----')
+_PKCS8_SPEC = PrivateKeyInfo()
 
 
 def _bit_list_to_bytes(bit_list):
@@ -88,8 +92,8 @@ def verify(self, message, signature):
         except (ValueError, rsa.pkcs1.VerificationError):
             return False
 
-    @staticmethod
-    def from_string(key_pem, is_x509_cert):
+    @classmethod
+    def from_string(cls, key_pem, is_x509_cert):
         """Construct a Verified instance from a string.
 
         Args:
@@ -119,7 +123,7 @@ def from_string(key_pem, is_x509_cert):
             pubkey = rsa.PublicKey.load_pkcs1(key_bytes, 'DER')
         else:
             pubkey = rsa.PublicKey.load_pkcs1(key_pem, 'PEM')
-        return RsaVerifier(pubkey)
+        return cls(pubkey)
 
 
 class RsaSigner(object):
@@ -129,12 +133,6 @@ class RsaSigner(object):
         pkey: rsa.key.PrivateKey (or equiv), The private key to sign with.
     """
 
-    _PKCS1_MARKER = ('-----BEGIN RSA PRIVATE KEY-----',
-                     '-----END RSA PRIVATE KEY-----')
-    _PKCS8_MARKER = ('-----BEGIN PRIVATE KEY-----',
-                     '-----END PRIVATE KEY-----')
-    _PKCS8_SPEC = PrivateKeyInfo()
-
     def __init__(self, pkey):
         self._key = pkey
 
@@ -167,14 +165,14 @@ def from_string(cls, key, password='notasecret'):
         """
         key = _from_bytes(key)  # pem expects str in Py3
         marker_id, key_bytes = pem.readPemBlocksFromFile(
-            six.StringIO(key), cls._PKCS1_MARKER, cls._PKCS8_MARKER)
+            six.StringIO(key), _PKCS1_MARKER, _PKCS8_MARKER)
 
         if marker_id == 0:
             pkey = rsa.key.PrivateKey.load_pkcs1(key_bytes,
                                                  format='DER')
         elif marker_id == 1:
             key_info, remaining = decoder.decode(
-                key_bytes, asn1Spec=cls._PKCS8_SPEC)
+                key_bytes, asn1Spec=_PKCS8_SPEC)
             if remaining != b'':
                 raise ValueError('Unused bytes', remaining)
             pkey_info = key_info.getComponentByName('privateKey')
diff --git a/oauth2client/client.py b/oauth2client/client.py
@@ -1243,7 +1243,7 @@ def from_json(cls, json_str):
         # possible return type of GoogleCredentials.get_application_default()
         if (data['_module'] == 'oauth2client.service_account' and
             data['_class'] == 'ServiceAccountCredentials'):
-            return ServiceAccountCredentials.from_json(s)
+            return ServiceAccountCredentials.from_json(json_str)
 
         token_expiry = _parse_expiry(data.get('token_expiry'))
         google_credentials = cls(
diff --git a/oauth2client/service_account.py b/oauth2client/service_account.py
@@ -54,6 +54,47 @@
 _PKCS12_KEY = '_private_key_pkcs12'
 
 
+def _get_signer(pkcs8_pem, pkcs12, password):
+    """Get a signer from the ``crypt`` module.
+
+    Will be used by :class:`ServiceAccountCredentials` to sign the
+    header and payload in a JWT.
+
+    Args:
+        pkcs8_pem: bytes or ``NoneType``, The content of a PKCS#8 key
+                               in PEM format.
+        pkcs12: bytes or ``NoneType``, The content of a PKCS#12 key.
+        password: string or ``NoneType``, Password for PKCS#12 private
+                  key. Defaults to ``notasecret``.
+
+    Returns:
+        tuple, A pair of :class:`crypt.Signer` and the potentially updated
+        value of ``password``.
+
+    Raises:
+        ValueError: If both ``pkcs8_pem`` and ``pkcs12`` are set.
+        ValueError: If ``pkcs8_pem`` is set and ``password`` is passed in.
+        ValueError: If ``pkcs12`` is set and ``password`` is not passed in.
+,    """
+    if pkcs8_pem is not None:
+        if pkcs12 is not None:
+            raise ValueError(_AT_LEAST_ONE_KEY, 'Both were passed.')
+        if password is not None:
+            raise ValueError('Private key password can only be used with '
+                             'a PKCS#12 key.')
+        return crypt.Signer.from_string(pkcs8_pem), None
+
+    if pkcs12 is None:
+        raise ValueError(_AT_LEAST_ONE_KEY, 'Neither were passed.')
+    # From here we assume a PKCS#12 key, which is only
+    # supported by pyOpenSSL.
+    if crypt.Signer is not crypt.OpenSSLSigner:
+        raise EnvironmentError(_PKCS12_ERROR)
+    if password is None:
+        password = _PASSWORD_DEFAULT
+    return crypt.Signer.from_string(pkcs12, password), password
+
+
 class ServiceAccountCredentials(AssertionCredentials):
     """Service Account credential for OAuth 2.0 signed JWT grants.
 
@@ -76,8 +117,8 @@ class ServiceAccountCredentials(AssertionCredentials):
     Args:
         service_account_email: string, The email associated with the
                                service account.
-        private_key_pkcs8_pem: bytes, The content of a PKCS#8 key in PEM
-                               format.
+        private_key_pkcs8_pem: bytes, (Optional) The content of a PKCS#8 key
+                               in PEM format.
         private_key_pkcs12: bytes, (Optional) The content of a PKCS#12 key.
         private_key_password: string, (Optional) Password for PKCS#12 private
                               key. Defaults to ``notasecret``.
@@ -97,15 +138,15 @@ class ServiceAccountCredentials(AssertionCredentials):
                     access token.
         _revoke_uri: string, (Optional) The URI to use when revoking an
                      access token.
-        kwargs: dict, Extra key-value pairs to send in the payload body
-                when making an assertion.
+        kwargs: dict, Extra key-value pairs (both strings) to send in the
+                payload body when making an assertion.
 
         Raises:
             ValueError: If both ``private_key_pkcs8_pem`` and
                         ``private_key_pkcs12`` are set.
             ValueError: If ``private_key_pkcs8_pem`` is set and
                         ``private_key_password`` is passed in.
-            ValueError: If ``private_key_pkcs11`` is set and
+            ValueError: If ``private_key_pkcs12`` is set and
                         ``private_key_password`` is not passed in.
     """
 
@@ -138,33 +179,15 @@ def __init__(self,
         self._private_key_pkcs8_pem = private_key_pkcs8_pem
         self._private_key_pkcs12 = private_key_pkcs12
         self._private_key_password = private_key_password
-        self._signer = self._get_signer()
+        self._signer, self._private_key_password = _get_signer(
+            self._private_key_pkcs8_pem, self._private_key_pkcs12,
+            self._private_key_password)
         self._scopes = util.scopes_to_string(scopes)
         self._private_key_id = private_key_id
         self._service_account_id = service_account_id
         self._user_agent = user_agent
         self._kwargs = kwargs
 
-    def _get_signer(self):
-        if self._private_key_pkcs8_pem is not None:
-            if self._private_key_pkcs12 is not None:
-                raise ValueError(_AT_LEAST_ONE_KEY, 'Both were passed.')
-            if self._private_key_password is not None:
-                raise ValueError('Private key password can only be used with '
-                                 'a PKCS#12 key.')
-            return crypt.Signer.from_string(self._private_key_pkcs8_pem)
-
-        if self._private_key_pkcs12 is None:
-            raise ValueError(_AT_LEAST_ONE_KEY, 'Neither were passed.')
-        # From here we assume a PKCS#12 key, which is only
-        # supported by pyOpenSSL.
-        if crypt.Signer is not crypt.OpenSSLSigner:
-            raise EnvironmentError(_PKCS12_ERROR)
-        if self._private_key_password is None:
-            self._private_key_password = _PASSWORD_DEFAULT
-        return crypt.Signer.from_string(self._private_key_pkcs12,
-                                        self._private_key_password)
-
     def _to_json(self, strip, to_serialize=None):
         """Utility function that creates JSON repr. of a Credentials object.
 
diff --git a/tests/test__pure_python_crypt.py b/tests/test__pure_python_crypt.py
@@ -23,6 +23,7 @@
 import unittest2
 
 from oauth2client._helpers import _from_bytes
+from oauth2client import _pure_python_crypt
 from oauth2client.crypt import RsaSigner
 from oauth2client.crypt import RsaVerifier
 
@@ -150,7 +151,8 @@ def test_from_string_pkcs8(self):
     def test_from_string_pkcs8_extra_bytes(self):
         key_bytes = self._load_pkcs8_key_bytes()
         _, pem_bytes = pem.readPemBlocksFromFile(
-            six.StringIO(_from_bytes(key_bytes)), RsaSigner._PKCS8_MARKER)
+            six.StringIO(_from_bytes(key_bytes)),
+            _pure_python_crypt._PKCS8_MARKER)
 
         with mock.patch('pyasn1.codec.der.decoder.decode') as mock_decode:
             key_info, remaining = None, 'extra'
@@ -159,7 +161,7 @@ def test_from_string_pkcs8_extra_bytes(self):
                 RsaSigner.from_string(key_bytes)
             # Verify mock was called.
             mock_decode.assert_called_once_with(
-                pem_bytes, asn1Spec=RsaSigner._PKCS8_SPEC)
+                pem_bytes, asn1Spec=_pure_python_crypt._PKCS8_SPEC)
 
     def test_from_string_pkcs8_unicode(self):
         key_bytes = _from_bytes(self._load_pkcs8_key_bytes())
