impl(auth): add SigningProvider trait #3969
Conversation
|
See #3828 for big picture |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3969 +/- ##
==========================================
- Coverage 95.31% 95.26% -0.05%
==========================================
Files 164 165 +1
Lines 6252 6274 +22
==========================================
+ Hits 5959 5977 +18
- Misses 293 297 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
coryan
left a comment
coryan
left a comment
There was a problem hiding this comment.
In addition to the stuff around async_trait: I am not sure what is the minimum bar for test coverage, but 0% is not it. Please add some tests.
src/auth/src/signer.rs
Outdated
|
|
||
| /// A trait for types that can sign content. | ||
| #[async_trait::async_trait] | ||
| pub trait SigningProvider: Send + Sync + std::fmt::Debug { |
There was a problem hiding this comment.
Typically we expose public traits like this so the type (in this case Signer) can be mocked, but I don't see a clear need, nor a way to initial a signer with a mock. Thoughts?
There was a problem hiding this comment.
Customers can provide their own way of signing blob (not sure yet how this is used, but the other SDKs provide that), so the use case is both for mocking like you mentioned, but also for letting customers implement their own logic to sign things.
There was a problem hiding this comment.
Customers can provide their own way of signing blob (not sure yet how this is used, but the other SDKs provide that), so the use case is both for mocking like you mentioned, but also for letting customers implement their own logic to sign things.
Fine, but I still cannot create a Signer from a signer provide, can I? That is, if I write:
impl SigningProvider for MyCoolSigner { ... }how do I get that inside a Signer so I can use it to sign URLs or something?
alvarowolfx
left a comment
alvarowolfx
left a comment
There was a problem hiding this comment.
Initially I haven't added tests because this PR is not adding any SigningProvider impl, but I added some basic tests mocking the trait.
src/auth/src/signer.rs
Outdated
|
|
||
| /// A trait for types that can sign content. | ||
| #[async_trait::async_trait] | ||
| pub trait SigningProvider: Send + Sync + std::fmt::Debug { |
There was a problem hiding this comment.
Customers can provide their own way of signing blob (not sure yet how this is used, but the other SDKs provide that), so the use case is both for mocking like you mentioned, but also for letting customers implement their own logic to sign things.
| impl<T> std::convert::From<T> for Signer | ||
| where | ||
| T: SigningProvider + Send + Sync + 'static, | ||
| { |
There was a problem hiding this comment.
Ah, I see the From thing. Yup.
| /// The content is typically a string-to-sign generated by the caller. | ||
| /// Returns the signature as a base64 encoded string (or other format depending on implementation, | ||
| /// but typically hex or base64). |
| /// It is typically the Google service account client email address from the Google Developers Console | ||
| /// in the form of "xxx@developer.gserviceaccount.com". Required. |
|
@coryan I'll fix the comments on coming PRs |
3 participants
Towards #3645