from_authorized_user_file always returns invalid credentials

[busunkim96]

from_user_info currently always sets the token to None. So a credential created from a JSON is always invalid and must be refreshed.

>>> from google.oauth2 import credentials
>>> TOKENS = 'tokens.json' # OAuth2 token storage
>>> creds = credentials.Credentials.from_authorized_user_file(TOKENS)
>>> creds.valid
False
>>> creds.expiry
>>> creds.expired
False

google-auth-library-python/google/oauth2/credentials.py

Lines 235 to 244 in 772dac6

	return cls(
	None, # No access token, must be refreshed.
	refresh_token=info["refresh_token"],
	token_uri=_GOOGLE_OAUTH2_TOKEN_ENDPOINT,
	scopes=scopes,
	client_id=info["client_id"],
	client_secret=info["client_secret"],
	quota_project_id=info.get(
	"quota_project_id"
	), # quota project may not exist

If a token is available, from_user_info should use it. info.get('token', None)

[alvyjudy]

I wonder if this was intentional because the access token is usually set to expire within a short period of time? It is likely expired in this context if it were spun up again from a JSON file.

[busunkim96]

@alvyjudy I believe the token is valid for ~1 hour. If the token is expired the refresh logic should kick in, so I don't it's harmful to use token from the JSON.

google-auth-library-python/google/auth/credentials.py

Lines 69 to 76 in 772dac6

	@property
	def valid(self):
	"""Checks the validity of the credentials.
	This is True if the credentials have a :attr:`token` and the token
	is not :attr:`expired`.
	"""
	return self.token is not None and not self.expired

google-auth-library-python/google/auth/credentials.py

Lines 106 to 125 in 772dac6

	def before_request(self, request, method, url, headers):
	"""Performs credential-specific before request logic.
	Refreshes the credentials if necessary, then calls :meth:`apply` to
	apply the token to the authentication header.
	Args:
	request (google.auth.transport.Request): The object used to make
	HTTP requests.
	method (str): The request's HTTP method or the RPC method being
	invoked.
	url (str): The request's URI or the RPC service's URI.
	headers (Mapping): The request's headers.
	"""
	# pylint: disable=unused-argument
	# (Subclasses may use these arguments to ascertain information about
	# the http request.)
	if not self.valid:
	self.refresh(request)
	self.apply(headers)

[alvyjudy]

@busunkim96 I feel the way self.expired and self.valid work wouldn't allow us to just pass in the token to from_authorized_user_info

google-auth-library-python/google/auth/credentials.py

Lines 53 to 67 in 772dac6

	@property
	def expired(self):
	"""Checks if the credentials are expired.
	Note that credentials can be invalid but not expired because
	Credentials with :attr:`expiry` set to None is considered to never
	expire.
	"""
	if not self.expiry:
	return False
	# Remove 5 minutes from expiry to err on the side of reporting
	# expiration early so that we avoid the 401-refresh-retry loop.
	skewed_expiry = self.expiry - _helpers.CLOCK_SKEW
	return _helpers.utcnow() >= skewed_expiry

Since self.expiry won't be set when a credential is freshly constructed, self.expired will always return False.

This will cause self.valid to always return True if token is passed in

google-auth-library-python/google/auth/credentials.py

Lines 69 to 76 in 772dac6

	@property
	def valid(self):
	"""Checks the validity of the credentials.
	This is True if the credentials have a :attr:`token` and the token
	is not :attr:`expired`.
	"""
	return self.token is not None and not self.expired

and self.refresh won't be called even if the token did expire. Proceeding with the code will result in a invalid token response when the token is actually valid but expired.

Please correct me if I had made a mistake interpreting the code (I'm quite new here:)

[alvyjudy]

A simpler fix would be just to add a line of code to refresh the credential before returning it. I can open a PR if that's desired

[wescpy]

I feel that will change our "contract" with the developer. When there's a valid access token stored, users will expect no refresh until after the 3600s have passed for it. It'd be better if we take that access token verbatim and ensure all the flags are set appropriately for however much longer that token's TTL is good for.

[alvyjudy]

@wescpy I agree with what you said. It's just that currently to_json method would strip away the expiry information making it hard to keep track of the TTL. Would it be a better idea to pickle that information in to_json and parse it too when picking up the token in from_authorized_user_info?

[wescpy]

That sounds much better. Was there a particular reason why it (expiry) wasn't included when 1st implemented (to better understand the design decision at the time... wasn't a tz issue was it)?

[busunkim96]

@wescpy I don't think so. I believe it was modeled closely after oauth2client, so we might have to take another look there to see if there was a reason for exclusion.

[wescpy]

SG. (Of course) oauth2client didn't have any issues w/being able to process both active access tokens or run the refresh process if needed. Since it managed the tokens and everyone used that code, it was probably fairly "hardened" (hence why I think at some point in the future, we should also add that feature to this library or google_auth_oauthlib [as well as the client libs for the other languages]... it's much better UX if developers didn't have to worry about token timeout issues which are orthogonal to the solutions they're building).

[dd-ssc]

I stumbled across this while working my way from the GMail Python Quickstart towards a production app. That sample app saves/loads the token to/from a local token.pickle binary Python pickle file, including the access token.

In our projects, we store all secrets in a HashiCorp Vault instance, so a non-binary, text representation is needed for persistence. As a first step towards that, I replaced the local pickle file by a local json file.

When I run the app subsquently during development, the access token is still perfectly valid; there is no reason to refresh. Still, the credentials returned by from_authorized_user_info() are invalid.
As a result, it isn't even attempted to refresh the access token by means of the refresh token - the entire user auth process is started every time: Please visit this URL to authorize this application in Terminal, Browser window opens, user logs into their GMail / GSuite account, etc.

Also, why would from_authorized_user_file (or from_authorized_user_info it uses internally) make any decision about the token validity ? IMO, the job of persistence in general is to freeze information into storage and thaw it back into the app as needed - and the info that comes back out should obviously be equal to what was put in. The access token may have expired since it was persisted, but that's not for from_authorized_user_file to decide, is it ?

I think I can work around this for the time being by just re-adding the access token to the credentials, but from my point of view, this just feels like a bug.

[dd-ssc]

Update: The workaround for this issue is trivial, posting it here in case anyone else runs into it:

import json
from google.oauth2.credentials import Credentials

def load_user_secrets_from_local(user_secrets_file):
    if exists(user_secrets_file):
        with open(user_secrets_file, 'r') as stream:
            creds_json = json.load(stream)
        creds = Credentials.from_authorized_user_info(creds_json)
        # workaround for
        # https://github.com/googleapis/google-auth-library-python/issues/501
        creds.token = creds_json['token']
        return creds
    return None

[wescpy]

A workaround is good but not a permanent fix for this bug, so I spent the day working on it and submitted for review as </pull/589>. (A couple of supporting tweaks also needed to be made.) Thanks to @dd-ssc for the inspiration!

[wescpy]

While the PR has been merged, this isn't officially "fixed" until the next release. However, those who are being bitten by this can at least grab the latest google/oauth2/credentials.py and replace their faulty one.

[busunkim96]

Closed by #589