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.

Sign up for GitHub

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

Jump to bottom

Default token expiry #67

Merged
merged 8 commits into from
Mar 14, 2023
Merged

Default token expiry #67

merged 8 commits into from
Mar 14, 2023

Conversation

MatthewHelmer
Copy link

@MatthewHelmer MatthewHelmer commented Feb 23, 2023 •
edited
Loading

Closes #66
GCloudAuthorizedUser::from_string defaults the token's expiry_time to None, which causes Token::has_expired to always return false. This results in AuthenticationManager::get_token never refreshing a token that was retrieved via a GCloudAuthorizedUser. I have refactored InnerToken to store expiry time as an OffsetDateTime instead of an Option<OffsetDateTime>, and that will default to 1 hour in the future (3600 seconds).

@MatthewHelmer MatthewHelmer changed the title Default token expiry #66 Default token expiry Feb 23, 2023
@MatthewHelmer MatthewHelmer mentioned this pull request Feb 23, 2023
Closed
djc
djc reviewed Feb 24, 2023
View reviewed changes
src/types.rs Outdated Show resolved Hide resolved
djc
djc reviewed Feb 26, 2023
View reviewed changes
src/types.rs Outdated Show resolved Hide resolved
djc
djc reviewed Feb 27, 2023
View reviewed changes
src/types.rs Outdated Show resolved Hide resolved
@MatthewHelmer MatthewHelmer requested a review from djc March 5, 2023 13:54
djc
djc reviewed Mar 13, 2023
View reviewed changes
Copy link
Owner

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry -- a few more nits, I'm pretty sure this is the last round!

src/types.rs Outdated Show resolved Hide resolved
src/types.rs Show resolved Hide resolved
djc
djc approved these changes Mar 14, 2023
View reviewed changes
Copy link
Owner

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for persisting through this review cycle!

@djc djc merged commit 4f03eeb into djc:master Mar 14, 2023
@MatthewHelmer
Copy link
Author

@djc It's no trouble at all - thank you for the rigorous review! I appreciate the commitment to quality. This is the first time I've submitted to an open-source project, so I'm excited to have done it :)

@MatthewHelmer MatthewHelmer deleted the default-token-expiry branch March 14, 2023 14:07
@djc djc mentioned this pull request Jun 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djc djc djc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Tokens fetched via a GCloudAuthorizedUser do not refresh
2 participants
@MatthewHelmer @djc