Repository: GoogleCloudPlatform/google-auth-library-python (python)

Issue:
Add a five minute buffer to all expiration checks

Pull request description:
Add a consistent 5 minute clock skew accomodation
Resolves #141.

/cc @vchudnov-g

Patch:
diff --git a/google/auth/_helpers.py b/google/auth/_helpers.py
index de86bee..860b827 100644
--- a/google/auth/_helpers.py
+++ b/google/auth/_helpers.py
@@ -22,6 +22,10 @@ import six
 from six.moves import urllib
 
 
+CLOCK_SKEW_SECS = 300  # 5 minutes in seconds
+CLOCK_SKEW = datetime.timedelta(seconds=CLOCK_SKEW_SECS)
+
+
 def copy_docstring(source_class):
     """Decorator that copies a method's docstring from another class.
 
diff --git a/google/auth/credentials.py b/google/auth/credentials.py
index 8570957..6fb89d8 100644
--- a/google/auth/credentials.py
+++ b/google/auth/credentials.py
@@ -56,8 +56,10 @@ class Credentials(object):
         Note that credentials can be invalid but not expired becaue Credentials
         with :attr:`expiry` set to None is considered to never expire.
         """
-        now = _helpers.utcnow()
-        return self.expiry is not None and self.expiry <= now
+        # Err on the side of reporting expiration early so that we avoid
+        # the 403-refresh-retry loop.
+        adjusted_now = _helpers.utcnow() - _helpers.CLOCK_SKEW
+        return self.expiry is not None and self.expiry <= adjusted_now
 
     @property
     def valid(self):
diff --git a/google/auth/jwt.py b/google/auth/jwt.py
index 506ba0e..412f122 100644
--- a/google/auth/jwt.py
+++ b/google/auth/jwt.py
@@ -52,8 +52,7 @@ from google.auth import crypt
 import google.auth.credentials
 
 
-_DEFAULT_TOKEN_LIFETIME_SECS = 3600  # 1 hour in sections
-_CLOCK_SKEW_SECS = 300  # 5 minutes in seconds
+_DEFAULT_TOKEN_LIFETIME_SECS = 3600  # 1 hour in seconds
 
 
 def encode(signer, payload, header=None, key_id=None):
@@ -161,21 +160,25 @@ def _verify_iat_and_exp(payload):
     """
     now = _helpers.datetime_to_secs(_helpers.utcnow())
 
-    # Make sure the iat and exp claims are present
+    # Make sure the iat and exp claims are present.
     for key in ('iat', 'exp'):
         if key not in payload:
             raise ValueError(
                 'Token does not contain required claim {}'.format(key))
 
-    # Make sure the token wasn't issued in the future
+    # Make sure the token wasn't issued in the future.
     iat = payload['iat']
-    earliest = iat - _CLOCK_SKEW_SECS
+    # Err on the side of accepting a token that is slightly early to account
+    # for clock skew.
+    earliest = iat - _helpers.CLOCK_SKEW_SECS
     if now < earliest:
         raise ValueError('Token used too early, {} < {}'.format(now, iat))
 
-    # Make sure the token wasn't issue in the past
+    # Make sure the token wasn't issued in the past.
     exp = payload['exp']
-    latest = exp + _CLOCK_SKEW_SECS
+    # Err on the side of accepting a token that is slightly out of date
+    # to account for clow skew.
+    latest = exp + _helpers.CLOCK_SKEW_SECS
     if latest < now:
         raise ValueError('Token expired, {} < {}'.format(latest, now))
 


Test patch:
diff --git a/tests/compute_engine/test_credentials.py b/tests/compute_engine/test_credentials.py
index 568aec7..2564da9 100644
--- a/tests/compute_engine/test_credentials.py
+++ b/tests/compute_engine/test_credentials.py
@@ -17,6 +17,7 @@ import datetime
 import mock
 import pytest
 
+from google.auth import _helpers
 from google.auth import exceptions
 from google.auth.compute_engine import credentials
 
@@ -38,7 +39,8 @@ class TestCredentials(object):
         assert self.credentials.service_account_email == 'default'
 
     @mock.patch(
-        'google.auth._helpers.utcnow', return_value=datetime.datetime.min)
+        'google.auth._helpers.utcnow',
+        return_value=datetime.datetime.min + _helpers.CLOCK_SKEW)
     @mock.patch('google.auth.compute_engine._metadata.get')
     def test_refresh_success(self, get_mock, now_mock):
         get_mock.side_effect = [{
@@ -57,7 +59,7 @@ class TestCredentials(object):
         # Check that the credentials have the token and proper expiration
         assert self.credentials.token == 'token'
         assert self.credentials.expiry == (
-            datetime.datetime.min + datetime.timedelta(seconds=500))
+            now_mock() + datetime.timedelta(seconds=500))
 
         # Check the credential info
         assert (self.credentials.service_account_email ==
diff --git a/tests/oauth2/test_credentials.py b/tests/oauth2/test_credentials.py
index fa86a16..b117ad4 100644
--- a/tests/oauth2/test_credentials.py
+++ b/tests/oauth2/test_credentials.py
@@ -53,7 +53,8 @@ class TestCredentials(object):
 
     @mock.patch('google.oauth2._client.refresh_grant', autospec=True)
     @mock.patch(
-        'google.auth._helpers.utcnow', return_value=datetime.datetime.min)
+        'google.auth._helpers.utcnow',
+        return_value=datetime.datetime.min + _helpers.CLOCK_SKEW)
     def test_refresh_success(self, now_mock, refresh_grant_mock):
         token = 'token'
         expiry = _helpers.utcnow() + datetime.timedelta(seconds=500)
diff --git a/tests/test_app_engine.py b/tests/test_app_engine.py
index d3a79d5..136a914 100644
--- a/tests/test_app_engine.py
+++ b/tests/test_app_engine.py
@@ -17,6 +17,7 @@ import datetime
 import mock
 import pytest
 
+from google.auth import _helpers
 from google.auth import app_engine
 
 
@@ -111,7 +112,7 @@ class TestCredentials(object):
 
     @mock.patch(
         'google.auth._helpers.utcnow',
-        return_value=datetime.datetime.min)
+        return_value=datetime.datetime.min + _helpers.CLOCK_SKEW)
     def test_refresh(self, now_mock, app_identity_mock):
         token = 'token'
         ttl = 100
@@ -124,7 +125,7 @@ class TestCredentials(object):
             credentials.scopes, credentials._service_account_id)
         assert credentials.token == token
         assert credentials.expiry == (
-            datetime.datetime.min + datetime.timedelta(seconds=ttl))
+            now_mock() + datetime.timedelta(seconds=ttl))
         assert credentials.valid
         assert not credentials.expired
 
diff --git a/tests/test_credentials.py b/tests/test_credentials.py
index 814369e..c2ff844 100644
--- a/tests/test_credentials.py
+++ b/tests/test_credentials.py
@@ -14,6 +14,7 @@
 
 import datetime
 
+from google.auth import _helpers
 from google.auth import credentials
 
 
@@ -37,8 +38,21 @@ def test_expired_and_valid():
     assert credentials.valid
     assert not credentials.expired
 
+    # Set the expiration in the past, but because of clock skew accomodation
+    # the credentials should still be valid.
     credentials.expiry = (
-        datetime.datetime.utcnow() - datetime.timedelta(seconds=60))
+        datetime.datetime.utcnow() -
+        datetime.timedelta(seconds=1))
+
+    assert credentials.valid
+    assert not credentials.expired
+
+    # Set the credentials far enough in the past to exceed the clock skew
+    # accomodation. They should now be expired.
+    credentials.expiry = (
+        datetime.datetime.utcnow() -
+        _helpers.CLOCK_SKEW -
+        datetime.timedelta(seconds=1))
 
     assert not credentials.valid
     assert credentials.expired