Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix GCE creds serialization w/ oauth2client >= 4.0 #195

Merged
merged 2 commits into from
Dec 15, 2017

Conversation

houglum
Copy link
Contributor

@houglum houglum commented Dec 12, 2017

When updating gsutil to use oauth2client 4.1.2 and apitools 0.5.19,
testing on GCE instances showed that GceAssertionCredentials were not
being written to our credential storage file. Debugging showed that this
was because of a KeyError when looking for the 'scope' attribute in a
serialized credential (no longer included as of oauth2client 3.0). The
'scope' value was also mistakenly being passed in as an argument for
'email'. This change maintains backward compatibility for oauth2client
versions that used the 'scope' attribute while also working for current
versions that don't include 'scope' when constructing a
GceAssertionCredentials object.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 88.92% when pulling 9443b3e on houglum:master into b1c9c71 on google:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 88.92% when pulling 2505e18 on houglum:master into b1c9c71 on google:master.

When updating gsutil to use oauth2client 4.1.2 and apitools 0.5.19,
testing on GCE instances showed that GceAssertionCredentials were not
being written to our credential storage file. Debugging showed that this
was because of a KeyError when looking for the 'scope' attribute in a
serialized credential (no longer included as of oauth2client 3.0). This
change maintains backward compatibility for oauth2client versions that
used the 'scope' attribute while also working for current versions that
don't include 'scope' when constructing a GceAssertionCredentials
object.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 88.92% when pulling 4e27d6c on houglum:master into b1c9c71 on google:master.

@houglum
Copy link
Contributor Author

houglum commented Dec 12, 2017

FYI: Fixed the linter errors and ensured credential serialization still works on GCE.

@houglum houglum requested a review from thobrla December 14, 2017 18:18
@thobrla
Copy link
Contributor

thobrla commented Dec 14, 2017

Is it possible emulate each version (serializable/unserializable) of GceAssertionCredentials in a unit test to ensure to_json/from_json both work?

@houglum
Copy link
Contributor Author

houglum commented Dec 14, 2017

Not sure how/why to test a version that doesn't support to_json(), but I've added a unit test for calling to_json() on GceAssertionCredentials.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 88.936% when pulling 7748c6a on houglum:master into b1c9c71 on google:master.

@thobrla
Copy link
Contributor

thobrla commented Dec 14, 2017

LGTM pending fixing the Travis CI lint errors.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 88.936% when pulling f2a9947 on houglum:master into b1c9c71 on google:master.

@houglum
Copy link
Contributor Author

houglum commented Dec 15, 2017

D'oh. Fixed.

@houglum houglum merged commit 91a8932 into google:master Dec 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants