Fix credentials usage in BatchHTTPRequest (#376)

Author: Jon Wayne Parrott

googleapiclient/_auth.py

@@ -14,6 +14,8 @@
 
 """Helpers for authentication using oauth2client or google-auth."""
 
+import httplib2
+
 try:
     import google.auth
     import google.auth.credentials
@@ -29,8 +31,6 @@
 except ImportError:  # pragma: NO COVER
     HAS_OAUTH2CLIENT = False
 
-from googleapiclient.http import build_http
-
 
 def default_credentials():
     """Returns Application Default Credentials."""
@@ -84,9 +84,49 @@ def authorized_http(credentials):
         Union[httplib2.Http, google_auth_httplib2.AuthorizedHttp]: An
             authorized http client.
     """
+    from googleapiclient.http import build_http
+
     if HAS_GOOGLE_AUTH and isinstance(
             credentials, google.auth.credentials.Credentials):
         return google_auth_httplib2.AuthorizedHttp(credentials,
                                                    http=build_http())
     else:
         return credentials.authorize(build_http())
+
+
+def refresh_credentials(credentials):
+    # Refresh must use a new http instance, as the one associated with the
+    # credentials could be a AuthorizedHttp or an oauth2client-decorated
+    # Http instance which would cause a weird recursive loop of refreshing
+    # and likely tear a hole in spacetime.
+    refresh_http = httplib2.Http()
+    if HAS_GOOGLE_AUTH and isinstance(
+            credentials, google.auth.credentials.Credentials):
+        request = google_auth_httplib2.Request(refresh_http)
+        return credentials.refresh(request)
+    else:
+        return credentials.refresh(refresh_http)
+
+
+def apply_credentials(credentials, headers):
+    # oauth2client and google-auth have the same interface for this.
+    return credentials.apply(headers)
+
+
+def is_valid(credentials):
+    if HAS_GOOGLE_AUTH and isinstance(
+            credentials, google.auth.credentials.Credentials):
+        return credentials.valid
+    else:
+        return not credentials.access_token_expired
+
+
+def get_credentials_from_http(http):
+    if http is None:
+        return None
+    elif hasattr(http.request, 'credentials'):
+        return http.request.credentials
+    elif hasattr(http, 'credentials'):
+        return http.credentials
+    else:
+        return None

googleapiclient/http.py

@@ -62,6 +62,7 @@
 except ImportError:
   from oauth2client import _helpers as util
 
+from googleapiclient import _auth
 from googleapiclient import mimeparse
 from googleapiclient.errors import BatchError
 from googleapiclient.errors import HttpError
@@ -1126,21 +1127,25 @@ def _refresh_and_apply_credentials(self, request, http):
     # If there is no http per the request then refresh the http passed in
     # via execute()
     creds = None
-    if request.http is not None and hasattr(request.http.request,
-        'credentials'):
-      creds = request.http.request.credentials
-    elif http is not None and hasattr(http.request, 'credentials'):
-      creds = http.request.credentials
+    request_credentials = False
+
+    if request.http is not None:
+      creds = _auth.get_credentials_from_http(request.http)
+      request_credentials = True
+
+    if creds is None and http is not None:
+      creds = _auth.get_credentials_from_http(http)
+
     if creds is not None:
       if id(creds) not in self._refreshed_credentials:
-        creds.refresh(http)
+        _auth.refresh_credentials(creds)
         self._refreshed_credentials[id(creds)] = 1
 
     # Only apply the credentials if we are using the http object passed in,
     # otherwise apply() will get called during _serialize_request().
-    if request.http is None or not hasattr(request.http.request,
-        'credentials'):
-      creds.apply(request.headers)
+    if request.http is None or not request_credentials:
+      _auth.apply_credentials(creds, request.headers)
+
 
   def _id_to_header(self, id_):
     """Convert an id to a Content-ID header value.
@@ -1200,9 +1205,10 @@ def _serialize_request(self, request):
     msg = MIMENonMultipart(major, minor)
     headers = request.headers.copy()
 
-    if request.http is not None and hasattr(request.http.request,
-        'credentials'):
-      request.http.request.credentials.apply(headers)
+    if request.http is not None:
+      credentials = _auth.get_credentials_from_http(request.http)
+      if credentials is not None:
+        _auth.apply_credentials(credentials, headers)
 
     # MIMENonMultipart adds its own Content-Type header.
     if 'content-type' in headers:
@@ -1409,11 +1415,11 @@ def execute(self, http=None):
 
     # Special case for OAuth2Credentials-style objects which have not yet been
     # refreshed with an initial access_token.
-    if getattr(http.request, 'credentials', None) is not None:
-      creds = http.request.credentials
-      if not getattr(creds, 'access_token', None):
+    creds = _auth.get_credentials_from_http(http)
+    if creds is not None:
+      if not _auth.is_valid(creds):
         LOGGER.info('Attempting refresh to obtain initial access_token')
-        creds.refresh(http)
+        _auth.refresh_credentials(creds)
 
     self._execute(http, self._order, self._requests)
 

tests/test_http.py

@@ -62,12 +62,17 @@
 
 class MockCredentials(Credentials):
   """Mock class for all Credentials objects."""
-  def __init__(self, bearer_token):
+  def __init__(self, bearer_token, expired=False):
     super(MockCredentials, self).__init__()
     self._authorized = 0
     self._refreshed = 0
     self._applied = 0
     self._bearer_token = bearer_token
+    self._access_token_expired = expired
+
+  @property
+  def access_token_expired(self):
+    return self._access_token_expired
 
   def authorize(self, http):
     self._authorized += 1
@@ -1128,10 +1133,7 @@ def test_execute_request_body(self):
   def test_execute_initial_refresh_oauth2(self):
     batch = BatchHttpRequest()
     callbacks = Callbacks()
-    cred = MockCredentials('Foo')
-
-    # Pretend this is a OAuth2Credentials object
-    cred.access_token = None
+    cred = MockCredentials('Foo', expired=True)
 
     http = HttpMockSequence([
       ({'status': '200',

tox.ini

@@ -19,4 +19,4 @@ deps =
        coverage>=3.6,<3.99
        unittest2
        mock
-commands = nosetests --with-coverage --cover-package=googleapiclient --nocapture --cover-erase --cover-tests --cover-branches --cover-min-percentage=85
+commands = nosetests --with-coverage --cover-package=googleapiclient --nocapture --cover-erase --cover-tests --cover-branches --cover-min-percentage=85 []