[Rust] Implement minimal auth support by euank · Pull Request #7338 · swagger-api/swagger-codegen

euank · 2018-01-08T02:56:15Z

PR checklist

Read the contribution guidelines.
Ran the shell script under ./bin/ to update Petstore sample
Filed the PR against the correct branch: master
Copied the technical committee (pending this being ready or review)

Description of the PR

This PR implements the minimum required to implement oauth or basic auth on the client side

euank · 2018-01-08T03:46:23Z

I've manually verified Basic Auth works as I expect via generating a hydra client (based on this definition) and calling the .introspect_o_auth2_token api with correct and incorrect basic-auth credentials.

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.

euank · 2018-03-19T00:07:41Z

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

frol

The code seems to be working, so it can be merged as is even though it can take a little polishing.

frol · 2018-03-19T09:50:58Z

modules/swagger-codegen/src/main/resources/rust/api.mustache

+        };
+{{/isApiKey}}
+{{#isBasic}}
+        {


Why do you need these extra blocks around matches?

No reason it turns out. I think I was worried about some lifetime thing, but it compiles fine without them on stable. Removed.

frol · 2018-03-19T09:54:59Z

modules/swagger-codegen/src/main/resources/rust/api.mustache

+                    );
+                    auth_headers.insert("Authorization".to_owned(), format!("{}", auth));
+                }
+                None => {}


We could use configuration.basic_auth.and_then(|auth_conf| ...) instead of match + Some / None.

Or indeed if let ... (as above)

frol · 2018-03-19T09:58:06Z

modules/swagger-codegen/src/main/resources/rust/api.mustache

+                            password: auth_conf.1.to_owned(),
+                        }
+                    );
+                    auth_headers.insert("Authorization".to_owned(), format!("{}", auth));


auth.to_string() should express the intention better than format!("{}", ...).

Authorization doesn't have a to_string method, just a fmt::Display impl. I couldn't find a cleaner way to do this.

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 the ToString 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.

Worked just fine, thanks for the suggestion/pointer!

frol · 2018-03-19T10:07:06Z

modules/swagger-codegen/src/main/resources/rust/api.mustache

+                query.append_pair(key, val);
+            }
+{{/hasAuthMethods}} 
+            format!("?{}", query.finish())


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.

bjgill

Thanks for the PR. A few comments, but nothing major - the latter three are more suggestions, and hence optional.

bjgill · 2018-03-26T10:23:34Z

modules/swagger-codegen/src/main/resources/rust/api.mustache

+{{#isApiKey}}
+        match configuration.api_key {
+            None => (),
+            Some(ref apikey) => {


A minor style point - might this be clearer as if let Some(ref apikey) = configuration.api_key { ...?

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.

bjgill · 2018-03-26T10:24:15Z

modules/swagger-codegen/src/main/resources/rust/api.mustache

+            {{/isKeyInHeader}}
+            {{#isKeyInQuery}}
+                auth_query.insert("{{keyParamName}}".to_owned(), val);
+            {{/isKeyInQuery}}


I assume isKeyInHeader and isKeyInQuery are guaranteed to be mutually exclusive?

As I understand it yes, but these parameters aren't really well documented so I could be wrong.

bjgill · 2018-03-26T10:25:26Z

modules/swagger-codegen/src/main/resources/rust/api.mustache

+                    );
+                    auth_headers.insert("Authorization".to_owned(), format!("{}", auth));
+                }
+                None => {}


Or indeed if let ... (as above)

bjgill · 2018-03-26T10:29:26Z

modules/swagger-codegen/src/main/resources/rust/configuration.mustache

  pub base_path: String,
  pub user_agent: Option<String>,
  pub client: hyper::client::Client<C>,
+


Would be nice not to have this empty line here.

bjgill · 2018-03-26T10:31:15Z

modules/swagger-codegen/src/main/resources/rust/configuration.mustache

@@ -1,10 +1,23 @@
 {{>partial_header}}
 use hyper;
+use std::collections::HashMap;


Is this not going to yield warnings? It might be nice to allow the warning or only add this use statement if necessary.

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.

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.

euank · 2018-03-27T04:43:36Z

Addressed comments (excepting the fact that there are still warnings, I'd like to punt on that per my reply)

bjgill

Thanks.

bjgill · 2018-03-27T08:26:53Z

modules/swagger-codegen/src/main/resources/rust/api.mustache

+                    token: token.to_owned(),
+                }
+            );
+            auth_headers.insert("Authorization".to_owned(), format!("{}", auth));


Minor niggle - should this be auth.to_string() for consistency with the previous block?

frol · 2018-03-27T10:26:27Z

modules/swagger-codegen/src/main/resources/rust/api.mustache

+                    token: token.to_owned(),
+                }
+            );
+            auth_headers.insert("Authorization".to_owned(), format!("{}", auth));


Should this format! also become auth.to_string()?

wing328 · 2018-04-02T02:36:05Z

@euank did you have a chance to review the feedback by @bjgill and @frol?

euank · 2018-04-02T06:13:05Z

Thanks for the ping; fixed that last nit, sorry for not getting it properly the first time. Thanks for the review!

frol · 2018-04-02T06:54:50Z

LGTM

@euank Thanks for the improvement!

euank force-pushed the rust-auth branch from e50fa9b to 86b9a9d Compare January 8, 2018 03:43

[Rust] Implement minimal auth support

9b2d208

This is pretty much the bare minimum needed to get v2 auth working. This is partly based on the Go implementation.

euank force-pushed the rust-auth branch from 8cbd28d to 0bec79c Compare March 19, 2018 00:03

euank changed the title ~~[WIP] [Rust] Implement minimal auth support~~ [Rust] Implement minimal auth support Mar 19, 2018

[Rust] properly format query string

dced4c2

euank force-pushed the rust-auth branch from 0bec79c to 80c592c Compare March 19, 2018 03:55

frol approved these changes Mar 19, 2018

View reviewed changes

bjgill reviewed Mar 26, 2018

View reviewed changes

euank force-pushed the rust-auth branch from 16d2a1f to 3d25e48 Compare March 27, 2018 04:42

bjgill approved these changes Mar 27, 2018

View reviewed changes

frol reviewed Mar 27, 2018

View reviewed changes

wing328 added Client: Rust Enhancement: General labels Apr 2, 2018

wing328 added this to the v2.4.0 milestone Apr 2, 2018

euank added 2 commits April 1, 2018 23:11

[Rust] Improve auth formatting

1d3f087

[Rust] Regenerate petstore sample

6f53010

euank force-pushed the rust-auth branch from 3d25e48 to 6f53010 Compare April 2, 2018 06:12

wing328 merged commit b443573 into swagger-api:master Apr 2, 2018

Conversation

euank commented Jan 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Description of the PR

Uh oh!

euank commented Jan 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

euank commented Mar 19, 2018

Uh oh!

frol left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjgill left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

euank commented Mar 27, 2018

Uh oh!

bjgill left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wing328 commented Apr 2, 2018

Uh oh!

euank commented Apr 2, 2018

Uh oh!

frol commented Apr 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

euank commented Jan 8, 2018 •

edited

Loading

euank commented Jan 8, 2018 •

edited

Loading