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

Added automatic access_token refresh when they expire. #346

Merged
merged 8 commits into from
Dec 13, 2024

Conversation

stevenpince
Copy link
Contributor

Problem
The added functionality in #326 (automatic refresh after specific HTTP status code in Zou's response) is wasteful as it puts the responsibility of tracking the access_token's expiration on the Zou server.

Solution
I added a properties to the Client object, that makes access to "access_token" and "refresh_token" easier, as well as the property "access_token_has_expired", that decodes the given JWT access token to determine it's expiration date.

The property is checked in the make_auth_header call, which I have also move to the Client object for a more clear separation of concerns.

If the client's automatic_refresh_token is set to True, the refresh_token (renamed to "refresh_authentication_tokens") method is called to refresh authentication before creating the header - thus theoretically bypassing any possible future NotAuthenticatedError.

This does add the python module "jwt" to the requirements of Gazu - which I haven't added yet to this change.
As far as I can tell, this module should be available from Python 3.6+ - let's check the options

Unittests have been added to showcase the use of these changes and all checks are green.

@stevenpince stevenpince marked this pull request as draft November 21, 2024 22:16
@stevenpince stevenpince force-pushed the auto_jwt_refresh branch 2 times, most recently from 9d75d30 to 8b92cf5 Compare November 21, 2024 22:18
@stevenpince
Copy link
Contributor Author

Since checks are turning red due to the missing jwt library, am trying to add the correct one.

@stevenpince
Copy link
Contributor Author

I added https://pyjwt.readthedocs.io , which I import as jwt - I'm not sure how to declare it properly in the requirements file.

@frankrousseau
Copy link
Contributor

Thank you for your contribution @stevenpince. We are pretty busy at the moment but your code will be merged as soon as we have availability.

@EvanBldy please include this merge in your todo-list.

@EvanBldy
Copy link
Member

EvanBldy commented Nov 27, 2024

Hi @stevenpince, good idea thanks!

For pyjwt:
Gazu is compatible with python 2.7 and python>=3.6. So, requirements need the same.
To make it flexible I propose the following:
pyjwt==1.7.1; python_version == '2.7' pyjwt>=2.4.0; python_version >= '3.6'

@steven-pi
Copy link

Ok, I am traveling at the moment - will incorporate these changes.

@stevenpince
Copy link
Contributor Author

Ok, looks like Python 2.7 is breaking because datetime.datetime.timestamp is not available. Let me check if there's a clean way to resolve. Last version of 2.7 is 5.5 years old at this point. What software packages still ship it?

@EvanBldy
Copy link
Member

EvanBldy commented Dec 3, 2024

We need to keep Gazu compatible with Python 2.7 for now, some of our clients use some DCC/pipeline that need it.

@stevenpince
Copy link
Contributor Author

I understand you want to stay Python 2.7 compatible, but datetime.datetime.timestamp was added in 3.3. I tried running a local Python 2.7 environment, but it was just breaking too much for me (requests_mock depends on urllib.parse, which is also not available).

I've gated the functionality for Python3 only. It won't break Python 2.

@frankrousseau
Copy link
Contributor

@EvanBldy Can you review the last changes?

@EvanBldy EvanBldy force-pushed the auto_jwt_refresh branch 5 times, most recently from 1ea8413 to c55e0a7 Compare December 13, 2024 14:43
@EvanBldy
Copy link
Member

I'm sorry, but I finally don't get the point of parsing the JWT expiration date. It breaks what I made before and knowing from the server if the JWT is expired is not a problem (see here: https://datatracker.ietf.org/doc/html/rfc6749#section-1.5). Also, this is not compatible with Python 2.7 and it will parse the token / check for expiration at each Gazu request.
I prefer that the server tells that the JWT is expired this is just basic and less complex.

Nevertheless I kept some interesting part of the code.

@EvanBldy EvanBldy marked this pull request as ready for review December 13, 2024 14:48
@EvanBldy EvanBldy merged commit 32b80b7 into cgwire:main Dec 13, 2024
12 checks passed
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.

4 participants