-
Notifications
You must be signed in to change notification settings - Fork 6k
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
[Rust] Implement minimal auth support #7338
Conversation
I've manually verified Basic Auth works as I expect via generating a hydra client (based on this definition) and calling the Next up, oauth2 testing via more hydra apis. |
This is pretty much the bare minimum needed to get v2 auth working. This is partly based on the Go implementation.
cc @frol @farcaller @bjgill, I think this is ready for review. I've been using this code for an oauth2 flow with success so far |
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.
The code seems to be working, so it can be merged as is even though it can take a little polishing.
}; | ||
{{/isApiKey}} | ||
{{#isBasic}} | ||
{ |
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.
Why do you need these extra blocks around matches?
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.
No reason it turns out. I think I was worried about some lifetime thing, but it compiles fine without them on stable. Removed.
); | ||
auth_headers.insert("Authorization".to_owned(), format!("{}", auth)); | ||
} | ||
None => {} |
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.
We could use configuration.basic_auth.and_then(|auth_conf| ...)
instead of match + Some / None
.
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.
Or indeed if let ...
(as above)
password: auth_conf.1.to_owned(), | ||
} | ||
); | ||
auth_headers.insert("Authorization".to_owned(), format!("{}", auth)); |
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.
auth.to_string()
should express the intention better than format!("{}", ...)
.
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.
Authorization
doesn't have a to_string
method, just a fmt::Display
impl. I couldn't find a cleaner way to do this.
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.
This trait is automatically implemented for any type which implements the
Display
trait. As such,ToString
shouldn't be implemented directly:Display
should be implemented instead, and you get theToString
implementation for free.
https://doc.rust-lang.org/stable/std/string/trait.ToString.html
There might be some black magic involved, but as far as I know, it should have .to_string()
since it implements the Display trait.
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.
Worked just fine, thanks for the suggestion/pointer!
query.append_pair(key, val); | ||
} | ||
{{/hasAuthMethods}} | ||
format!("?{}", query.finish()) |
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.
Since the ?
will end up even for an empty query string, I would just put in the format!
below and just return query.finish()
here.
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.
Thanks for the PR. A few comments, but nothing major - the latter three are more suggestions, and hence optional.
{{#isApiKey}} | ||
match configuration.api_key { | ||
None => (), | ||
Some(ref apikey) => { |
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.
A minor style point - might this be clearer as if let Some(ref apikey) = configuration.api_key { ...
?
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.
Thank you for the suggestion. I agree that's cleaner and I'm not quite sure why I didn't do that the first time.
{{/isKeyInHeader}} | ||
{{#isKeyInQuery}} | ||
auth_query.insert("{{keyParamName}}".to_owned(), val); | ||
{{/isKeyInQuery}} |
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 assume isKeyInHeader
and isKeyInQuery
are guaranteed to be mutually exclusive?
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.
As I understand it yes, but these parameters aren't really well documented so I could be wrong.
); | ||
auth_headers.insert("Authorization".to_owned(), format!("{}", auth)); | ||
} | ||
None => {} |
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.
Or indeed if let ...
(as above)
|
||
pub struct Configuration<C: hyper::client::Connect> { | ||
pub base_path: String, | ||
pub user_agent: Option<String>, | ||
pub client: hyper::client::Client<C>, | ||
|
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.
Would be nice not to have this empty line here.
@@ -1,10 +1,23 @@ | |||
{{>partial_header}} | |||
use hyper; | |||
use std::collections::HashMap; |
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.
Is this not going to yield warnings? It might be nice to allow the warning or only add this use
statement if necessary.
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, but at this point we're too far gone on warnings.
I would like to do a pass dedicated to squashing warnings at some point, which I think will be easiest with a certain amount of refactoring. For now, I'm okay with living with another warning in the sea of them.
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.
To be specific, I think refactoring the shared code (the whole fn operationId
bit) to be one generic function which takes configurable things as parameters would result in more readable code that would be easier to eliminate warnings from... if only because all the code would be there and just not be called necessarily. Each operation would then just be a short function calling the big generic "do_call" style function with appropriate args.
I recall some of the other clients take this approach, but I don't have references handy because I dug into this some months ago.
Addressed comments (excepting the fact that there are still warnings, I'd like to punt on that per my reply) |
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.
Thanks.
token: token.to_owned(), | ||
} | ||
); | ||
auth_headers.insert("Authorization".to_owned(), format!("{}", auth)); |
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.
Minor niggle - should this be auth.to_string()
for consistency with the previous block?
token: token.to_owned(), | ||
} | ||
); | ||
auth_headers.insert("Authorization".to_owned(), format!("{}", auth)); |
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.
Should this format!
also become auth.to_string()
?
Thanks for the ping; fixed that last nit, sorry for not getting it properly the first time. Thanks for the review! |
LGTM @euank Thanks for the improvement! |
PR checklist
./bin/
to update Petstore samplemaster
Description of the PR
This PR implements the minimum required to implement oauth or basic auth on the client side