-
Notifications
You must be signed in to change notification settings - Fork 33
Fixed configure method for non-refreshable tokens #526
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
Fixed configure method for non-refreshable tokens #526
Conversation
renaudhartert-db
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo fix to the changelogs.
Note: unit tests are great but it would also be good to validate that the code works locally.
|
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
|
Checked the code locally. Browser Authentication is not triggered again (for refreshable and non-refreshable) if the token file already exists. |
What changes are proposed in this pull request?
We recently added support for disabling auto token refresh (#525). The current code is hardcoded to handle refreshable tokens, and it forcefully refreshes the token on every call of
configure()This PR proposed using a token (no matter if it is refreshable or not) if it is valid. Once the ExternalBrowserCredentialsProvider is run, it stores the token on disk. If the application is run again, it checks the token and tries to use it.
How is this tested?
Added Unit tests
NO_CHANGELOG=true