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: fix expiry for to_json() #589

Merged
merged 7 commits into from
Sep 17, 2020
Merged

fix: fix expiry for to_json() #589

merged 7 commits into from
Sep 17, 2020

Conversation

wescpy
Copy link
Contributor

@wescpy wescpy commented Aug 10, 2020

This patch for </issues/501> includes the following fixes:

  • The access token is always set to None, so the fix involves using (the access) token from the saved JSON credentials file.
  • For refresh needs, expiry also needs to be saved via to_json().
    • DUMP: As expiry is a datetime.datetime object, serialize to datetime.isoformat() in the same oauth2client format for consistency.
    • LOAD: Add code to restore expiry back to datetime.datetime object when imported.
    • LOAD: If expiry was unsaved, automatically set it as expired so refresh takes place.
  • Minor scopes updates
    • DUMP: Add property for scopes so to_json() can grab it
    • LOAD: scopes may be saved as a string instead of a JSON array (Python list), so ensure it is Sequence[str] when imported.

- The access token is always set to `None`, so the fix involves using (the access) `token` from the saved JSON credentials file.
- For refresh needs, `expiry` also needs to be saved via `to_json()`.
    - DUMP: As `expiry` is a `datetime.datetime` object, serialize to `datetime.isoformat()` in the same [`oauth2client` format](https://github.com/googleapis/oauth2client/blob/master/oauth2client/client.py#L55) for consistency.
    - LOAD: Add code to restore `expiry` back to `datetime.datetime` object when imported.
    - LOAD: If `expiry` was unsaved, automatically set it as expired so refresh takes place.
- Minor `scopes` updates
    - DUMP: Add property for `scopes` so `to_json()` can grab it
    - LOAD: `scopes` may be saved as a string instead of a JSON array (Python list), so ensure it is Sequence[str] when imported.
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 10, 2020
@busunkim96 busunkim96 added automerge Merge the pull request once unit tests and other checks pass. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Aug 11, 2020
@busunkim96
Copy link
Contributor

@wescpy Thank you!

@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 11, 2020
@busunkim96 busunkim96 changed the title Fixes so to_json() works properly fix: fix expiry for to_json() Aug 11, 2020
@gcf-merge-on-green
Copy link

Your PR has attempted to merge for 3 hours. Please check that all required checks have passed, you have an automerge label, and that all your reviewers have approved the PR

@gcf-merge-on-green
Copy link

Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, or one of your required reviews was not approved. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot.

@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Aug 12, 2020
@wescpy
Copy link
Contributor Author

wescpy commented Aug 12, 2020

Thanks for looking at this! Not all the tests are passing (lint & cover), so I'm happy to help tweak/fix or work collaboratively to get them passing (unless you determine it's not necessary). In particular, I used this script to test various scenarios with this PR:

  1. Unpatched, you always get "Re-run flow", even with valid but expired access tokens and valid NON-expired access tokens (because they're always set to None):
$ python bug501.py 
** Re-run flow: creds [True], valid? [False], expired [False]
  1. With the access token fix in-place but no expiry, the access token would never be "expired" (the default), so w/o a timestamp, the client library makes it always "Valid" (incorrectly). A Google API would reject it as 400-something (invalid/expired), but the client library is smart and does a "refresh" under the hood. Unfortunately that "effort" is never saved back to the JSON file, so all calls to the API would continually refresh, which sux:
$ python bug501.py 
** "Valid" creds: creds [True], valid? [True], expired [False]
  1. With the expiry patch in-place, a valid but expired access token give you "Auto-refresh" which is correct, and a valid NON-expired access token gives you "Valid", which is actually correct:
$ python bug501.py  # BEFORE refresh
** Auto-refresh: got creds? [True], valid? [False], expired? [True]
$ python bug501.py  # AFTER refresh
** "Valid" creds: creds [True], valid? [True], expired [False]

So while this script isn't a unit test, I'm going to try to roll this into the existing one </tests/oauth2/test_credentials.py>. LMK how else I can help in the meantime and stay tuned for another patch that should pass the unit tests.

@busunkim96 busunkim96 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 17, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 17, 2020
@wescpy
Copy link
Contributor Author

wescpy commented Aug 17, 2020

Fixed the black/linter reports; confused about pytest coverage output:

$ coverage report -m --fail-under=100
Name                               Stmts   Miss Branch BrPart  Cover   Missing
------------------------------------------------------------------------------
google/oauth2/__init__.py              1      0      0      0   100%
google/oauth2/_client.py              75      0      9      0   100%
google/oauth2/credentials.py         107      0     22      2    98%   264->267, 319->323
google/oauth2/id_token.py             48      0      6      0   100%
google/oauth2/service_account.py     117      0      6      0   100%
------------------------------------------------------------------------------
TOTAL                                348      0     43      2    99%
Coverage failure: total of 99 is less than fail-under=100
  1. Code block 362-364 of test/oauth2/test_credentials.py covers 264-267 of google/oauth2/credentials.py, and ...
  2. Code block 394-408 covers 319-323... what am I missing? Thanks!

@busunkim96
Copy link
Contributor

@wescpy Apologies for leaving this unattended for so long. I will take a closer look at coverage this week and get back to you.

@wescpy
Copy link
Contributor Author

wescpy commented Aug 31, 2020

SGTM @busunkim96 ... I've been on vaca anyway (back tmrw). :-)

@busunkim96 busunkim96 added kokoro:force-run Add this label to force Kokoro to re-run the tests. automerge Merge the pull request once unit tests and other checks pass. labels Sep 17, 2020
@yoshi-kokoro yoshi-kokoro removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Sep 17, 2020
@busunkim96 busunkim96 merged commit d0e0aba into googleapis:master Sep 17, 2020
busunkim96 added a commit that referenced this pull request Oct 22, 2020
* refactor: split 'with_quota_project' into separate base class (#561)

Co-authored-by: Tres Seaver <tseaver@palladion.com>

* fix: dummy commit to trigger a auto release (#597)

* chore: release 1.21.1 (#599)

* chore: updated CHANGELOG.md [ci skip]

* chore: updated setup.cfg [ci skip]

* chore: updated setup.py

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

* fix: migrate signBlob to iamcredentials.googleapis.com (#600)

Migrate signBlob from iam.googleapis.com to iamcredentials.googleapis.com.

This API is deprecated and will be shutdown in one year.

This is used google.auth.iam.Signer.
Added a system_test to sanity check the implementation.

* chore: release 1.21.2 (#601)

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

* fix: fix expiry for `to_json()` (#589)

* This patch for </issues/501> includes the following fixes:

- The access token is always set to `None`, so the fix involves using (the access) `token` from the saved JSON credentials file.
- For refresh needs, `expiry` also needs to be saved via `to_json()`.
    - DUMP: As `expiry` is a `datetime.datetime` object, serialize to `datetime.isoformat()` in the same [`oauth2client` format](https://github.com/googleapis/oauth2client/blob/master/oauth2client/client.py#L55) for consistency.
    - LOAD: Add code to restore `expiry` back to `datetime.datetime` object when imported.
    - LOAD: If `expiry` was unsaved, automatically set it as expired so refresh takes place.
- Minor `scopes` updates
    - DUMP: Add property for `scopes` so `to_json()` can grab it
    - LOAD: `scopes` may be saved as a string instead of a JSON array (Python list), so ensure it is Sequence[str] when imported.

* chore: add default CODEOWNERS (#609)

* chore: release 1.21.3 (#607)

* feat: add asyncio based auth flow (#612)

* feat: asyncio http request logic and asynchronous credentials logic  (#572)

Co-authored-by: Anirudh Baddepudi <43104821+anibadde@users.noreply.github.com>

* chore: release 1.22.0 (#615)

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

* fix: move aiohttp to extra as it is currently internal surface (#619)

Fix #618. Removes aiohttp from required dependencies to lessen dependency tree for google-auth.

This will need to be looked at again as more folks use aiohttp and once the surfaces goes to public visibility.

* chore: release 1.22.1 (#620)

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

* fix: remove checks for ancient versions of Cryptography (#596)

Refs #595 (comment) 

I see no point in checking whether someone is running a version of https://github.com/pyca/cryptography/ from 2014 that doesn't even compile against modern versions of OpenSSL anymore.

* chore: sync to master

Syncs to master.
Fixes broken unit tests in Python 3.6 and 3.7.
Aligns test_identity_pool.py with test_aws.py.

Co-authored-by: Bu Sun Kim <8822365+busunkim96@users.noreply.github.com>
Co-authored-by: Tres Seaver <tseaver@palladion.com>
Co-authored-by: arithmetic1728 <58957152+arithmetic1728@users.noreply.github.com>
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Co-authored-by: wesley chun <wescpy@gmail.com>
Co-authored-by: Christopher Wilcox <crwilcox@google.com>
Co-authored-by: Anirudh Baddepudi <43104821+anibadde@users.noreply.github.com>
Co-authored-by: Aarni Koskela <akx@iki.fi>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge the pull request once unit tests and other checks pass. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants