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

feat: key not from file & other improvements #29

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

chris13524
Copy link

This adds support for constructing a ServiceAccount directly from a ServiceAccountKey, rather than from a file. It also:

  • Adds interior mutability using a tokio RwLock to enable cloning ServiceAccount while re-using the current access token across threads/instances. Also allows ServiceAccount to be used without needing to be mut.
  • Makes ServiceAccountKey and its fields public to allow parsing or constructing the struct from any source (e.g. string, bytes, etc.) and also allows the fields inside to be re-used by other modules (e.g. reading project_id necessary for FCM)
  • Adds ServiceAccountBuilder to enable the use of a variety of constructors, including the ability to re-use reqwest::Client across instantiations which enables sharing connection pools. Existing from_file() and user_email() methods continue to work for backwards-compatibility, but the builder pattern is generally a better alternative
  • access_token() now returns AccessToken instead of e.g. "Bearer token" which allows the use of reqwest's .bearer_token() method without needing to parse-out the token. Also enables the invoker to see when the token will expire.
  • Asserts that the returned token is indeed a bearer token
  • from_file() retained as a helper method and now returns a Result type since file reading and parsing is performed in this function, rather than lazily in access_token(). Also now accepts any path-compatible value, rather than just &str.
  • Stores RsaKeyPair in JwtToken rather than inputs to improve efficiency
  • Catch and re-throw any non-200 status codes when requesting an access token. Also generally improve error handling by returning the original error, rather than converting into a string.
  • Resolves various clippy errors

Resolves #28

@chris13524 chris13524 force-pushed the feat/key-not-from-file branch from d3ed7c9 to c0a45bc Compare April 18, 2024 20:11
@chris13524 chris13524 changed the title feat: key not from file feat: key not from file & other improvements Apr 18, 2024
@chris13524 chris13524 marked this pull request as ready for review April 19, 2024 16:31
Copy link
Owner

@makarski makarski left a comment

Choose a reason for hiding this comment

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

Hey @chris13524. Thanks for the PR. I managed to spot a few minor things, hope this is not too much of a hustle to do the changes. Also would be great to bump up the version in Cargo to 0.9, since there is a breaking change to access_token return type.

HttpRequest(reqwest::Error),

#[error("failed to send request")]
HttpRequestUnsuccessful(StatusCode, std::result::Result<String, reqwest::Error>),
Copy link
Owner

Choose a reason for hiding this comment

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

std::result::Result could be shorted to StdResult, since there is already an import on line 3

HttpJson(reqwest::Error),

#[error("response returned non-Bearer auth access token: {0}")]
AccessTokenNotBeaarer(String),
Copy link
Owner

Choose a reason for hiding this comment

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

Looks like there is a typo in Beaarer

}

// Account for clock skew or time to receive or process the response
const LEEWAY: Duration = Duration::seconds(30);
Copy link
Owner

Choose a reason for hiding this comment

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

This piece seems not to compile for me. Here is the error message that I am get

error[E0015]: cannot call non-const fn `chrono::Duration::seconds` in constants
   --> src/serv_account/mod.rs:108:34
    |
108 |         const LEEWAY: Duration = Duration::seconds(30);
    |                                  ^^^^^^^^^^^^^^^^^^^^^
    |
    = note: calls in constants are limited to constant functions, tuple structs and tuple variants
    ```


Ok(token)
let expires_at = Utc::now() + Duration::seconds(json.expires_in) - LEEWAY;
Copy link
Owner

Choose a reason for hiding this comment

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

One of the options to resolve the issue with Duration above would be to keep it simple and use ints:
Utc::now().timestamp() as u64 + json.expires_in - 30

@chris13524
Copy link
Author

Thanks for the review! I'm working on some fixes for the above as well as some other tweaks

@chris13524 chris13524 marked this pull request as draft May 13, 2024 18:15
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.

Support for creating ServiceAccount from string instead of file path
2 participants