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

Token is not getting refreshed after its expired after around 1 hour #109

Closed
praveen-kukreja opened this issue Nov 19, 2022 · 17 comments · Fixed by #117
Closed

Token is not getting refreshed after its expired after around 1 hour #109

praveen-kukreja opened this issue Nov 19, 2022 · 17 comments · Fixed by #117
Labels
bug Something isn't working

Comments

@praveen-kukreja
Copy link

Issue: I've been executing a script to fetch all the objects from cloud storage. There are around a lot of objects present in the bucket.
Observation: The script fetches half of the objects in around 1 hr and then raises the below-mentioned error.
Traceback:
Traceback (most recent call last):

  File "~/aiogoogle/models.py", line 341, in _next_page_generator
    prev_res = await sess.send(next_req, full_res=True)
  File "~/aiogoogle/sessions/aiohttp_session.py", line 183, in send
    results = await schedule_tasks()
  File "~/aiogoogle/sessions/aiohttp_session.py", line 175, in schedule_tasks
    return await asyncio.gather(*tasks, return_exceptions=False)
  File "~/aiogoogle/sessions/aiohttp_session.py", line 157, in get_response
    response.raise_for_status()
  File "~/aiogoogle/models.py", line 453, in raise_for_status
    raise AuthError(msg=self.reason, req=self.req, res=self)
aiogoogle.excs.AuthError: 



Unauthorized

Content:
{'code': 401,
 'errors': [{'domain': 'global',
             'location': 'Authorization',
             'locationType': 'header',
             'message': 'Invalid Credentials',
             'reason': 'authError'}],
 'message': 'Invalid Credentials'}

Request URL:
https://storage.googleapis.com/storage/v1/b/....

Question:
Is there any other way to reset the access token present in the session_factory's send() method? or am I missing a step?

code snippet:

blobs_full_request = await google_client.as_service_account(
    storage_client.objects.list(bucket=BUCKET),
    full_res=True,
)
async for blob_page in blobs_full_request:
    yield blob_page

Possible issue: When pagination starts, if there are more pages to go through, the package fails to look for the validity of the access_token.

@praveen-kukreja praveen-kukreja changed the title Token is not getting refreshed after its expired. Token is not getting refreshed after its expired after around 1 hour Nov 19, 2022
@omarryhan omarryhan added the bug Something isn't working label Nov 19, 2022
@omarryhan
Copy link
Owner

Hey, thanks for the detailed ticket!

Possible issue: When pagination starts, if there are more pages to go through, the package fails to look for the validity of the access_token.

Yes, this indeed seems to be the case.

I'll be grateful if you can create a PR with a fix 🙏 and I'll go ahead and create a new release once you open the PR and it's merged.

Thanks!

@neel004
Copy link
Contributor

neel004 commented Nov 21, 2022

Hey, @omarryhan @praveen-elastic I've raised a PR which fixes this issue for service-accounts.
@omarryhan can you please go ahead and release this, meanwhile I'll raise subsequent PRs for other auth modes.

@omarryhan
Copy link
Owner

Thank you so much @neel004! I've just released v4.4.0

p.s. I think right now we're refreshing tokens on every next page request, ideally, we should check if the access token is expired first to avoid the extra network call. We probably need to work on the Auth.ServiceAccountManager.refresh method. But I guess this isn't urgent, hence why I merged and released it.

@neel004
Copy link
Contributor

neel004 commented Nov 21, 2022

Thanks for the quick merge, I'll raise a PR once I have proper fix ready, till then we can keep the issue open.

@neel004
Copy link
Contributor

neel004 commented Dec 7, 2022

Hey @omarryhan , on a totally different note, is there a way to get the object data in binary or encoded form? I'm encountering
this error when fetching object content using get call with alt=media as a query param.
Traceback (most recent call last):
File "/lib/python3.10/site-packages/aiogoogle/sessions/aiohttp_session.py", line 70, in resolve_response
json = await response.json()
File "/lib/python3.10/site-packages/aiohttp/client_reqrep.py", line 1104, in json
raise ContentTypeError(
aiohttp.client_exceptions.ContentTypeError: 0, message='Attempt to decode JSON with unexpected mimetype: application/msword', url=URL('https://storage.googleapis.com/storage/v1/b/{bucket}/o/{file_name}.doc?alt=media')

@omarryhan
Copy link
Owner

Hey @neel004, if you pass a download_file parameter it will save the binary in a file. If you want to stream the binary response, you can use the pipe_to param, you can find some examples in this issue #98.

@neel004
Copy link
Contributor

neel004 commented Feb 25, 2023

I think right now we're refreshing tokens on every next page request, ideally, we should check if the access token is expired first to avoid the extra network call.

@omarryhan
we do call refresh method for service account but it indeed checks the validity before going for new token[here].

Thinking about the authentication with ApiKeyManager, should the package raise the error it encounters for expired token or you want it to be handled differently?
do let me know your thoughts on this.

@omarryhan
Copy link
Owner

Ah makes sense.

I'm not sure what the current behaviour is. Also, I was under the impression that API keys never expire?

@neel004
Copy link
Contributor

neel004 commented Feb 26, 2023

Also, I was under the impression that API keys never expire?

Ohh, Got it 👍🏻 .

@omarryhan
Copy link
Owner

I guess it's safe to close this issue now? :)

@neel004
Copy link
Contributor

neel004 commented Feb 26, 2023

meanwhile I'll raise subsequent PRs for other auth modes.

I'm working on implementation for other authentication methods, as planned before. Once done we can close the issue 😃.

@omarryhan
Copy link
Owner

Right! My bad. Thank you for your contributions and work!

@neel004
Copy link
Contributor

neel004 commented Apr 2, 2023

Hello @omarryhan, began work on this. Could you could assist me with this tiny problem?
According to the code for the as user oauth2 flow, the creds will only be used for one request.
Can't we refresh the tokens for the next API calls using the configured refresh token from the user creds?
OR is it because we don't store the user creds that are supplied immediately to as user?

@omarryhan
Copy link
Owner

Sure, happy to help.

The purpose of giving the option to pass user creds for only one request is for applications/library users who are handling user creds of multiple users. It would be inconvenient if the Aiogoogle class stores the user creds in the instance's state if it will change with every call anyway.

I recall that the problem was only with pagination, no? If you set user creds as an attribute in the aiogoogle instance, then it should automatically check if the access token is expired and refresh. That only doesn't happen in pagination, right?

@neel004
Copy link
Contributor

neel004 commented Apr 3, 2023

If you set user creds as an attribute in the aiogoogle instance, then it should automatically check if the access token is expired and refresh. That only doesn't happen in pagination, right?

@omarryhan Yes, This is the case for individual requests.
But the concern arises when the API call takes more than an hour to complete the API request(pagination..) and the token gets expires. When this happens the token gets refreshed at this point, and for refreshing the tokens the user_creds can't be available anyhow.

So, as a solution to it, I was thinking of introducing the user_creds as an instance variable to Oauth2Manager.
I'm happy to take any suggestion for this, Do let me know if any thing seems confusing.

@omarryhan
Copy link
Owner

Thanks for the clarification @neel004!

I think we should avoid setting user_creds to auth manager if the user didn't explicitly do so when instantiating the auth manager, otherwise, it will be useless to allow one off user_creds requests.

If it will be easy, maybe we can somehow pass the user_creds object like we pass the auth manager?

@neel004
Copy link
Contributor

neel004 commented Apr 4, 2023

Got your concern about allowing one-off user_creds 👍🏻 .

If it will be easy, maybe we can somehow pass the user_creds object like we pass the auth manager?

🆒 I'll do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants