add TokenFetcher struct for automatic token refresh

[adelbertc]

Closes #12 .

• Add a get_token_with_client variant that allows clients to pass in their own reqwest::Client
• Modify get_token (and therefore get_token_blocking and friends) to call into get_token_with_client
• Add a TokenFetcher struct that will automatically refresh the token for each fetch_token call
  • There was some trickiness here to figure out when exactly to refresh. One choice in theory on the client side is to keep using the token until you get 403'd and refresh then, but that is tricky / complex to do in a library - those who want to do so can build their own using get_token_with_client. What I decided to do instead is to allow clients to specify a refresh_buffer and for each fetch_token request check "is now() >= + ( - <refresh_buffer>)"? If so, refresh.

EDIT: Figured out how to get tests in!

EDIT 2: I switched from using a mutable field to using arc_swap::ArcSwap since token refresh seems to be a perfect use case for that (often read, seldom updated). However this does incur an arc-swap dependency.. I can put this behind a feature flag if you don't want to incur this dependency, just LMK!

One open question for this PR is there is some slightly non-trivial logic here with when we decide to refresh or not refresh which I would like to test ideally. Doing so in the current design would require being able to hijack the Token response from the HTTP class so we can shove in custom expires_in values and see what happens. Anecdotally I have used the mockito library, but only in contexts where I require clients to pass in a URL explicitly anyways so it is easy to pass in mockito::server_url() just in the tests and keep mockito as a dev-dependencies. In this case since the URL is dictated on the user side we would need to do the trick they mention in the docs with something like:

#[cfg(test)]
use mockito;

pub async fn get_token(
    jwt: &Jwt<JwtClaims>,
    credentials: &Credentials,
) -> Result<Token> {
    let final_jwt = jwt.finalize()?;
    let request_body = form_body(&final_jwt);

    #[cfg(not(test))]
    let url = credentials.token_uri();

    #[cfg(test)]
    let url = mockito::server_url();

    let client = Client::new();
    let response = client
        .post(&url)
        .form(&request_body)
        .send().await?;
    
    let token = response.json::<Token>().await?;
    Ok(token)
}

But this would require incurring a mockito dependency on all users which seems.. strange. Curious to get your take on this.

[durch]

@adelbertc thank you for a speedy PR :), it looks really great.

I have a few suggestions, would like to get your take on them :)

1. Could we make TokenFetcher.refresh_buffer Option<Duration>, and have a default of Duration::new(0, 0). That way TokenFetcher::new() would have a simpler. signature, and you could add a TokenFetcher::new_with_refresh_buffer() to allow for setting the refresh_buffer.
2. I really like the TokenFetcher abstraction, it would be great if you could also update the README so that the snippet there shows TokenFetcher fetch and refresh functionality.
3. Ideally you'd also add a blocking version of fetch_token, that way we could slowly deprecate get_token_* calls in favour of TokenFetcher

[adelbertc]

@durch Thanks for the quick review! I've added some docs to the README, here are my thoughts on the other two bits of feedback:

Could we make TokenFetcher.refresh_buffer Option, and have a default of Duration::new(0, 0). That way TokenFetcher::new() would have a simpler. signature, and you could add a TokenFetcher::new_with_refresh_buffer() to allow for setting the refresh_buffer.

I like this idea of having a cleaner signature but ran into two issues:

• Since we also want to allow people to pass in their own reqwest::Client, would we have four constructors for 1) no client, no buffer 2) yes client, no buffer 3) no client, yes buffer 4) yes client, yes buffer? Alternatively I think we can do builder pattern here as well.
• The more serious design choice is one of the reasons that led me to introduce a configurable "refresh buffer" is that there is a possible race if we just go off the expires_in field entirely. For instance say we get the token at t=0s, and expires_in is in 3s. At say 2.9s, the user asks for the token to use in a subsequent Google API call. It gets the same token because 2s < 3s, but by the time it makes the API call it wants to make the token is now considered expired. Now this may be fine for some people but for high traffic servers I can see how this innocent default might surprise some people when the token is often being fetched and used for API calls. What do you think?

Ideally you'd also add a blocking version of fetch_token, that way we could slowly deprecate get_token_* calls in favour of TokenFetcher

I started doing this too before realizing we need to provide a Runtime - IDK how I would do this cleanly in the current abstraction, and it also adds another field users would want to insert their own Runtime implementation into, much like reqwest::Client. To solve this I think most libraries have a separate blocking version , e.g. reqwest::blocking::Client which initially I thought we could just pass in a Runtime too but even that seems a bit unergonomic (e.g. how do folks who use #[tokio::main] pass a Runtime through, and do they have to thread it all the way down to whatever needs a hypothetical goauth::blocking::TokenFetcher? Looking at reqwest::blocking::Client it seems that have some fancier ways of doing this: https://docs.rs/reqwest/0.10.8/src/reqwest/blocking/client.rs.html#714 .

[adelbertc]

Just realized for that latter point if we go to blocking::TokenFetcher route we can just wrap a reqwest::blocking::Client - LMK what you think of the former point though since that will inform the blocking version as well and I can do it :-)

[durch]

@adelbertc great stuff :)

On the multiple constructors, I agree its not very ergonomic, so probably better to leave it more configurable. More importantly the point you make around refresh races is excellent, better to have it be explicitly set, at the expens of a few lines of code mode.

As for the blocking, I've previously looked at how reqwest does it, and in the end it seems best fo have an explicit blocking feature that users can opt into. I think jamming blocking in here would needlessly increase scope, the work you did stands on its own. We can always build the blocking feature later :).

All in all thank you for this great contribution

[durch]

Published to crates.io as 0.8.0