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 misbehaviour in store_oauth2_credentials #242

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tobbez
Copy link

@tobbez tobbez commented Mar 14, 2016

The previous implementation was both needlessly complicated, and also had a
misbehaviour.

When writing new credentials to an already existing file, the file would
not get correctly truncated. This caused some previous data to remain if
the new data to be written was shorter than the current file contents.

It would then cause gmvault to fail with the following error upon the
next invocation:

=== Exception traceback ===
Traceback (most recent call last):
File "/usr/lib/python2.7/site-packages/gmv/credential_utils.py", line 147, in read_oauth2_tok_sec
oauth_result = json.load(oauth_file)
File "/usr/lib64/python2.7/json/init.py", line 291, in load
**kw)
File "/usr/lib64/python2.7/json/init.py", line 339, in loads
return _default_decoder.decode(s)
File "/usr/lib64/python2.7/json/decoder.py", line 367, in decode
raise ValueError(errmsg("Extra data", s, end, len(s)))
ValueError: Extra data: line 1 column 225 - line 1 column 237 (char 224 - 236)

@tobbez
Copy link
Author

tobbez commented Aug 19, 2016

It would be nice if the master branch was kept relatively up to date, since it is what you ask people to fork when contributing. Back when I made this pull request in March, HEAD of master was at a759623, but since then, a lot of fairly old commits were merged in, among them commits from August 2015 (72fd16f, 72fd16f, 09f9834) that fixed the issue this pull request originally aimed to fix.

Anyway, I've rewritten this change against master again. While the current code isn't broken, the proposed change would make the code clearer and easier to maintain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant