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

Fix installation token cache issue #442

Conversation

matthewgapp
Copy link
Contributor

My application holds an Installation for an extended period of time (longer than 1 hour). This causes a "Bad credentials" error from Github. This PR introduces a fix that changes the installation token cache to only return a token if it's not expired, forcing Octocrab client to refresh the token appropriately.

@matthewgapp matthewgapp marked this pull request as ready for review August 16, 2023 02:57
}

fn valid_token(&self) -> Option<SecretString> {
self.valid_token_with_buffer(chrono::Duration::seconds(30))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consider a token that expires within 30 seconds to be expired by default.

From what I can tell, the expiration is usually set about 1 hour from token mint.

let expiration = token_object
.expires_at
.map(|time| {
DateTime::<Utc>::from_str(&time).map_err(|e| error::Error::Other {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Github doesn't document the timezone but I assumed it was UTC. Also checked a couple responses manually and verified that it was UTC.

@XAMPPRocky
Copy link
Owner

Thank you for your PR! However I need to check with someone to make sure these aren't overlapping (and different) proposals for the same feature.

cc @marcusirgens Is this problem solved by #436 or is it separate?

@marcusirgens
Copy link
Contributor

@XAMPPRocky Yes, I solve this very similarily in #436, making CachedToken::get return None if the token is expired.

I'm not opposed to merging this first and working it into my PR, as it's more focused than my "do many things at once"-request, especially considering that #436 needs some clarifications. :)

https://github.com/marcusirgens/octocrab/blob/store-installation-tokens/src/lib.rs#L811-L828

@matthewgapp
Copy link
Contributor Author

I'm not opposed to merging this first and working it into my PR, as it's more focused than my "do many things at once"-request, especially considering that #436 needs some clarifications. :)

@XAMPPRocky sounds like it makes sense to merge so that @marcusirgens can rebase onto this.

@XAMPPRocky
Copy link
Owner

Thank you for your PR!

@XAMPPRocky XAMPPRocky merged commit 4b9c26e into XAMPPRocky:main Aug 24, 2023
@github-actions github-actions bot mentioned this pull request Aug 24, 2023
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.

3 participants