-
Notifications
You must be signed in to change notification settings - Fork 2
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
Major upgrade and refactor #15
Conversation
@@ -12,23 +12,24 @@ approval for the full stack used through the Architecture and Security groups. | |||
```rust | |||
let mauth_info = MAuthInfo::from_default_file().unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this example should include the use
statements or maybe we should have an examples/main.rs
file with something that compiles and works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this is generated from this doc test which is compiled as part of the CI test suite.
Making an example that works is problematic, since that would require an actual MAuth URL and valid private key to be stored in this public repo. I suppose I can modify it to allow the use
statements to be shown though.
Ok(()) | ||
} | ||
|
||
pub(crate) fn set_headers_v2(&self, req: &mut Request, signature: String, timestamp_str: &str) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this need to be available in the whole crate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically yes, the protocol test suite tests need access to it to test that the headers get set correctly. I'm not sure if there's a good way to provide access just to that.
} | ||
|
||
impl MAuthInfo { | ||
pub(crate) async fn validate_request( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to have some test.
These validate functions require an Axum Request, while the signing requests require a reqWest Request.
I know reqWest is the most popular high level client library, not sure about Axum as the community (last time I checked 1 year ago) is fragmented.
Potentially we could use http:request everywhere but I do not know how much of PITA that would be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found it impossible to build a version based on the http crate's request trait because there is no interface provided to allow you to read the contents of the body without consuming it and making it unavailable to the rest of the stack. Constructing a new body with the request data is also not possible, as the concrete type would be unknown and there is no method on the trait to create a new instance. If you can figure out a way to do this, I'd love to know!
Axum is not a dominantly popular Rust web framework. However, to the best of my knowledge, it is the only one integrated with the Tower ecosystem that makes it at least theoretically possible to build reusable and composable middleware like this. To have a working request verifier with any other currently existing Rust web framework would require a complete rewrite anyways to use the interfaces provided by that specific web framework.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm satisfied with the answers, maybe we want to wait for Yohei to give it a last review
Changes include: