Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
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 ourterms of service andprivacy 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
chris13524 wants to merge6 commits intomakarski:main
base:main
Choose a base branch
Loading
fromreown-com:feat/key-not-from-file

Conversation

chris13524
Copy link

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

  • Adds interior mutability using a tokioRwLock to enable cloningServiceAccount while re-using the current access token across threads/instances. Also allowsServiceAccount to be used without needing to bemut.
  • MakesServiceAccountKey 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. readingproject_id necessary for FCM)
  • AddsServiceAccountBuilder to enable the use of a variety of constructors, including the ability to re-usereqwest::Client across instantiations which enables sharing connection pools. Existingfrom_file() anduser_email() methods continue to work for backwards-compatibility, but the builder pattern is generally a better alternative
  • access_token() now returnsAccessToken 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 aResult type since file reading and parsing is performed in this function, rather than lazily inaccess_token(). Also now accepts any path-compatible value, rather than just&str.
  • StoresRsaKeyPair inJwtToken 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

dkong reacted with heart emoji
@chris13524chris13524 changed the titlefeat: key not from filefeat: key not from file & other improvementsApr 18, 2024
@chris13524chris13524 marked this pull request as ready for reviewApril 19, 2024 16:31
Copy link
Owner

@makarskimakarski 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 to0.9, since there is a breaking change toaccess_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

chris13524 reacted with thumbs up emoji
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

chris13524 reacted with thumbs up emoji
}

// 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

makarski reacted with thumbs up emoji

@chris13524chris13524 marked this pull request as draftMay 13, 2024 18:15
@makarski
Copy link
Owner

makarski commentedFeb 25, 2025
edited
Loading

@chris13524 I liked the idea - should we try finishing this?

@chris13524
Copy link
Author

@makarski unfortunately I don't really have the bandwidth/focus from my company to revisit and properly upstream these changes. We are just happy with using the fork for now.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@makarskimakarskimakarski requested changes

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

Successfully merging this pull request may close these issues.

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

[8]ページ先頭

©2009-2025 Movatter.jp