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

feat: Add expiry to oauth arguments for refresh #155

Closed
wants to merge 1 commit into from

Conversation

JichaoS
Copy link

@JichaoS JichaoS commented Oct 17, 2023

Currently the oauth flow does not do refreshes if the access token is expired, as Credentails() does't know when the access token expires, so it completely skips refreshing.

Credentails() will only refresh when a expiry datetime object is passed, so here we add an oauth argument for expiry, and pass it to Credentails().

Currently the oauth flow does not do refreshes if the access token is expired, as Credentails() does't know when the access token expires, so it completely skips refreshing.

Credentails() will only refresh when a expiry datetime object is passed, so here we add an oauth argument for expiry, and pass it to Credentails().
@sonarcloud
Copy link

sonarcloud bot commented Oct 17, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@JichaoS JichaoS changed the title Add expiry to oauth arguments for refresh feat: Add expiry to oauth arguments for refresh Oct 17, 2023
@pnadolny13
Copy link
Collaborator

@JichaoS thanks for the PR!

I've only used a service account so I'm not familiar with this flow. Can you help me understand how this works, does the library do the refresh automatically if you provide the expiry parameter? I wasn't able to easily find this in their docs. It sounds like currently the tap would fail if the token expired mid sync, is that right? So after this PR merges, if a expiry is provided, the sync would start and if the token expires mid run it will refresh itself and continue. Is that true or will it only do it on initialization?

@JichaoS
Copy link
Author

JichaoS commented Oct 20, 2023

@pnadolny13 ty for the review!

does the library do the refresh automatically if you provide the expiry parameter?

Yes exactly

tap would fail if the token expired mid sync

I suspect that's the case, but haven't tried it out. FWIW the code here suggests that is the case: https://github.com/googleapis/google-auth-library-python/blob/main/google/auth/transport/requests.py#L552

The case I'm targeting is getting the library to refresh the token when handed an already-expired access token along with a refresh token. The library currently tries to use the access token, and gets back an error saying the oauth token is invalid, even if the user provided a refresh token (and other refresh credentials)

@pnadolny13
Copy link
Collaborator

The case I'm targeting is getting the library to refresh the token when handed an already-expired access token along with a refresh token. The library currently tries to use the access token, and gets back an error saying the oauth token is invalid, even if the user provided a refresh token (and other refresh credentials)

@JichaoS that makes sense, I'm fine with your approach but had another crazy idea, curious what you think - I wonder if hardcoding (or defaulting) the expiry parameter to always be in the past would work. Then the user doesnt need to add a parameter and the token is always assumed to be expired and the refresh flow kicks off. What do you think? Am I missing a piece of how this works?

pnadolny13 added a commit that referenced this pull request Mar 12, 2024
The method that was previously used to manage OAuth tokens had issues
with refreshing (see
#155) and it
expected an access token to be provided which is unnecessary.
@pnadolny13
Copy link
Collaborator

This should now be resolved by #159. The access token is not needed when a refresh token is provided.

@pnadolny13 pnadolny13 closed this Mar 12, 2024
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.

2 participants