async API #46

legokichi · 2018-10-26T12:59:09Z

legokichi · 2018-10-26T13:31:18Z

tests/lib.rs

@@ -273,15 +276,18 @@ fn test_exchange_code_successful_with_complete_json_response() {
 fn test_exchange_client_credentials_with_basic_auth() {
    let mock = mock("POST", "/token")
        // base64(urlencode("aaa/;&") + ":" + urlencode("bbb/;&"))
-        .match_header("Authorization", "Basic YWFhJTJGJTNCJTI2OmJiYiUyRiUzQiUyNg==")
+        .match_header("Authorization", "basic YWFhJTJGJTNCJTI2OmJiYiUyRiUzQiUyNg==")


reqwest puts lowercase letters.
https://docs.rs/reqwest/0.9.3/src/reqwest/async_impl/request.rs.html#171

thanks for the reference! looking at https://tools.ietf.org/html/rfc7235#section-2.1, the auth scheme is indeed supposed to be case insensitive. hopefully this won't cause any compatibility issues with off-spec server implementations.

legokichi · 2018-10-26T15:45:16Z

I'm thinking of using hyper instead of reqwest.
reqwest does not have HttpConnector API -
https://docs.rs/hyper/0.12.12/hyper/client/struct.HttpConnector.html

ex.

ramosbugs · 2018-10-26T15:55:30Z

thanks for the PR! I'm traveling this weekend, but I'll take a look next week.

legokichi · 2018-10-26T16:03:14Z

src/lib.rs

@@ -236,6 +237,24 @@ use prelude::*;

 const CONTENT_TYPE_JSON: &str = "application/json";

+#[derive(Clone)]
+/// This struct is used to maintain the strict URI encoding standard as proposed by RFC 3986
+struct StrictEncodeSet;


from here https://github.com/rusoto/rusoto/blob/7640ca3de1e6486b0b25bce395810a9434a66ee5/rusoto/core/src/signature.rs#L593

I'm finding the chain of specs referenced by https://tools.ietf.org/html/rfc6749#appendix-B to be a bit hard to follow, so it's not super clear to me exactly which characters need to be escaped (especially since we're dealing with an HTTP header here, rather than a URI). This code does seem to maintain the current behavior as implemented in https://github.com/curl/curl/blob/master/lib/escape.c#L40, but I wonder if it would suffice to use
USERINFO_ENCODE_SET, which seems to be intended for username and password escaping (according to https://github.com/dtolnay/url/blob/master/UPGRADING.md).

@DeeUnderscore do you have any thoughts here, as the author of #41? (see usage below)

By my reading, this adheres to application/x-www-form-urlencoded—everything outside of the permitted bytes is encoded. This WHATWG spec seems to be essentially equivalent to what RFC6749 wanted when it described the encoding in Appendix B.

URLs can use forms of percent encoding which aren't application/x-www-form-urlencoded, since they permit more byte ranges (presumably to make URLs more readable). USERINFO_ENCODE_SET (WHATWG ref), in particular, is intended for the userinfo component of a URL, ie. the user:password in https://user:password@example.org/.

In practical terms, you could probably get away with a less strict encoding, and I suspect most OAuth implementations in the wild avoid using characters which could cause problems anyway. But, for closest adherence to the standard I'd recommend what this PR is doing.

Thanks for the reference and rationale! That makes sense; I agree with the recommendation.

The encoding here does seem to deviate ever so slightly from the WHATWG spec, though:

spaces encoded with %20 instead of +

asterisks (0x2A) excluded from the encoding set but mentioned by WHATWG

tildes (0x7E) included in encoding set but excluded by WHATWG

None of these deviations seems particularly problematic, though it's probably safer for interoperability to err on the side of percent-encoding more values rather than fewer. Unless there's an error in the WHATWG spec, we should probably remove tildes from this set.

Seems reasonable. While the RFC specifically gives spaces as + in an example, a properly implemented decoder should consume %20 fine, too.

legokichi · 2018-10-26T16:26:19Z

src/lib.rs

-    ) -> Result<RequestTokenResponse, curl::Error> {
-        let mut easy = Easy::new();
+    ) -> impl Future<Item = RequestTokenResponse, Error = reqwest::Error> + Send + 'static {
+        let mut req_builder = ::reqwest::async::Client::new().post(&token_url.to_string()[..]);


I think add hyper::Client to oauth2::Client structure as member and create oauth2::Client::with_connector static method.

ramosbugs · 2018-11-04T00:05:28Z

Sorry for the delay! Looking this PR over now.

I'm thinking of using hyper instead of reqwest.
reqwest does not have HttpConnector API -
https://docs.rs/hyper/0.12.12/hyper/client/struct.HttpConnector.html

I think it's definitely useful (often required) for clients to be able to specify connection parameters, including TLS settings (trusted CA certs, cipher suites, etc.) and proxy configs. This functionality has been missing from this crate so far, so thank you for bringing it up!

However, it doesn't look like hyper::client::HttpConnector really lets us specify these parameters. Rather, that interface seems to deal with lower level network options. What sort of customization were you hoping to support via HttpConnector?

As an alternative, reqwest::async::ClientBuilder does seem to allow significant customization: https://docs.rs/reqwest/0.9.4/reqwest/async/struct.ClientBuilder.html. What if we add a set_http_client method to oauth2::Client so that users can pass in their own reqwest::async::Clients?

https://actix.rs/actix-web/actix_web/client/struct.ClientConnector.html#method.with_connector

it looks like this one refers to a different interface (openssl's SslConnector struct rather than hyper's Connect trait)

I'll submit comments on the rest of the PR soon.

ramosbugs

Thanks again for the PR!

In addition to supporting the async API, I think we should still provide a sync API. That should reduce the boilerplate for callers that want sync behavior, and in particular make this breaking change easier for existing code to adopt.

We probably want both sync and async versions of Client, similar to what reqwest does. What I'm not sure of is which interface should be the default. We could either have oauth2::Client be sync and oauth2::async::Client be async or vice versa (oauth2::Client is async and oauth2::sync::Client is sync).

The former is nice because the default interface matches the underlying implementation, while the latter is nice because it avoids making a breaking change altogether (I think). What do you think?

ramosbugs · 2018-11-03T20:34:13Z

Cargo.toml

@@ -18,3 +19,4 @@ url = "1.0"

 [dev-dependencies]
 mockito = "0.8.2"
+tokio = "0.1"


let's add a newline at the end here

ramosbugs · 2018-11-03T20:38:28Z

src/lib.rs

@@ -211,21 +211,22 @@
 //!

 extern crate base64;
-extern crate curl;
+extern crate futures;
+extern crate reqwest;


nit: sort imports

Suggested change

extern crate reqwest;

src/lib.rs

ramosbugs · 2018-11-03T20:39:38Z

src/lib.rs

 use std::fmt::{Debug, Display, Error as FormatterError, Formatter};
 use std::marker::{Send, Sync, PhantomData};
 use std::ops::Deref;
 use std::time::Duration;

-use curl::easy::Easy;
+use url::percent_encoding::{utf8_percent_encode, EncodeSet};


Suggested change

use url::percent_encoding::{utf8_percent_encode, EncodeSet};

src/lib.rs

ramosbugs · 2018-11-04T00:36:00Z

src/lib.rs

+            let content_type = res
+                .headers()
+                .get(::reqwest::header::CONTENT_TYPE)
+                .map(|o| o.to_str().unwrap().to_string());


a panic is probably undesirable here. it seems like we have a few options:

return an Err if the header contains non-ASCII chars

set content_type to None in this case

change the type of RequestTokenResponse::content_type to Option<reqwest::header::HeaderValue>

I think the last option is probably the cleanest here, but the first seems fine too.

ramosbugs · 2018-11-04T01:21:25Z

src/lib.rs

+        let token_url = self.token_url.as_ref().unwrap();
+        let fut = self.post_request_token(token_url, params).map_err(RequestTokenError::Request)
+        .and_then(|token_response|{
+            Ok(()).and_then(|()| -> Result<_, _> {


I think we can simplify this to:

.and_then(|token_response| { if token_response.http_status != 200 { .... } ... })

In other words, the inner closure is already returning a Result, which transforms automatically into FutureResult courtesy of impl From<Result<T, E>> for FutureResult<T, E>. This lets us remove the redundant:

Ok(()).and_then(|| { ... }).into_future()

ramosbugs · 2018-11-04T02:05:25Z

src/lib.rs

@@ -236,6 +237,24 @@ use prelude::*;

 const CONTENT_TYPE_JSON: &str = "application/json";

+#[derive(Clone)]
+/// This struct is used to maintain the strict URI encoding standard as proposed by RFC 3986
+struct StrictEncodeSet;


I'm finding the chain of specs referenced by https://tools.ietf.org/html/rfc6749#appendix-B to be a bit hard to follow, so it's not super clear to me exactly which characters need to be escaped (especially since we're dealing with an HTTP header here, rather than a URI). This code does seem to maintain the current behavior as implemented in https://github.com/curl/curl/blob/master/lib/escape.c#L40, but I wonder if it would suffice to use
USERINFO_ENCODE_SET, which seems to be intended for username and password escaping (according to https://github.com/dtolnay/url/blob/master/UPGRADING.md).

@DeeUnderscore do you have any thoughts here, as the author of #41? (see usage below)

ramosbugs · 2018-11-04T02:07:40Z

tests/lib.rs

@@ -202,8 +203,9 @@ fn test_exchange_code_successful_with_minimal_json_response() {
            Url::parse(&(SERVER_URL.to_string() + "/token")).unwrap(),
        )),
    );
-    let token = client
-        .exchange_code(AuthorizationCode::new("ccc".to_string()))
+    let token = ::tokio::runtime::Runtime::new().unwrap().block_on(


since each test needs to do this, let's define a function that takes a future, creates a runtime, blocks on it, and returns the result. it's probably worth trying to minimize the test boilerplate.

legokichi · 2018-11-08T06:11:33Z

src/lib.rs

+    }
+
+    /// async edition
+    pub fn exchange_code_async(


futures trait also changes greatly in Rust 2018.
How about doing this as a provisional response for the past few months?

exchange_code_async

exchange_password_async

exchange_client_credentials_async

exchange_refresh_token_async

Alternative choice: just constructs and returns http::request::Request object.
We can chose own http request library e.g. curl, reqwest, hyper, actix-web ...etc.

I think it's fine to put the pre-Rust 2018 async functionality into this crate. However, I think we should do this via a separate Client implementation (i.e., oauth2::async::Client). Then, when we're ready to add support for Rust 2018 async, we can call that oauth2::async2018::Client or something. Each of these can present the same exchange_* functions, just with different return types. This change will probably require some refactoring to share as much code as possible between the different Client implementations.

also, the reason I don't think returning Request objects would work well is that we'd then have to offer an interface for callers to pass the response back in for us to parse

legokichi · 2018-11-08T06:21:17Z

Here are the implementations of https_connector.

ramosbugs · 2018-12-09T00:29:36Z

src/lib.rs

+        let underscore = byte == 0x5f;
+        let tilde = byte == 0x7e;
+        let period = byte == 0x2e;
+        !(upper || lower || numeric || hyphen || underscore || tilde || period)


based on the conversation above, I think we should probably remove tilde

ramosbugs · 2018-12-09T00:38:52Z

src/lib.rs

+    }
+
+    /// async edition
+    pub fn exchange_code_async(


I think it's fine to put the pre-Rust 2018 async functionality into this crate. However, I think we should do this via a separate Client implementation (i.e., oauth2::async::Client). Then, when we're ready to add support for Rust 2018 async, we can call that oauth2::async2018::Client or something. Each of these can present the same exchange_* functions, just with different return types. This change will probably require some refactoring to share as much code as possible between the different Client implementations.

ramosbugs

Looks like there are some merge conflicts now based some other PRs I merged

ramosbugs · 2018-12-09T01:24:48Z

src/lib.rs

+        }
+        let token_url = self.token_url.as_ref().unwrap();
+        let fut = self.post_request_token(token_url, params).map_err(RequestTokenError::Request)
+        .and_then(|token_response|{


Added some minor cleanup here to 3ca27ff. Mind pulling in those changes, or giving me permissions to update this PR?

src/lib.rs

ramosbugs · 2018-12-09T01:28:48Z

src/lib.rs

+    }
+
+    /// async edition
+    pub fn exchange_code_async(


also, the reason I don't think returning Request objects would work well is that we'd then have to offer an interface for callers to pass the response back in for us to parse

ramosbugs · 2018-12-09T01:41:29Z

Here are the implementations of https_connector.

native-tls: https://github.com/hyperium/hyper-tls

rustls: https://github.com/ctz/hyper-rustls

https://github.com/pimeys/hyper-alpn

https://github.com/sfackler/hyper-native-tls

https://github.com/sfackler/hyper-openssl

Ohh, I see. So it looks like what we really want to accept is a struct that implements the hyper::client::connect::Connect trait, which can do anything it wants as long as it yields a (hyper::client::connect::Connect::Transport, hyper::client::connect::Connected). Something like what you mentioned above should work, e.g.:

pub struct Client<C: Connect, EF: ExtraTokenFields, TT: TokenType, TE: ErrorResponseType> {
    connector: C,
    ...
}

// new() sets a sensible default (and secure) connector (require HTTPS, etc.)

pub fn with_connector(mut self, connector: C) -> Self {
    self.connector = connector;

    self
}

bachp · 2018-12-27T22:34:27Z

I would like to use this in the context of actix-web. So I would be very happy if there is a way to specify the client library to use (reqwest, actix, ...). Did you already give any toughts on how this could be done?

If it would be desired to have this as a feature I might be able to come up with a PR that allows to select actix-web as a replacement for reqwest.

ramosbugs · 2018-12-28T04:15:54Z

I would like to use this in the context of actix-web. So I would be very happy if there is a way to specify the client library to use (reqwest, actix, ...). Did you already give any toughts on how this could be done?

I think the simplest approach to supporting multiple HTTP clients is probably via a trait. Since the hyper::client::connect::Connect trait is quite low level, I think it's a good candidate. This also lets people use hyper by default, which seems to have a significantly larger volume of downloads on crates.io than reqwest or actix-web. Other client libraries could potentially be used by writing wrappers that implement the hyper::client::connect::Connect trait on top of those libraries.

Alternatively, we could probably do something at compile time based on feature flags, but I haven't looked into that much, and it feels like that would be harder to maintain over time.

Was there specific functionality from actix-web that you were looking to use in conjunction with this crate? Understanding the use case a bit more might help illuminate the right solution.

I think it's also probably worth filing a separate Github issue for this so that we can refocus #44 on the async discussion, and the new issue on the need for a generic HTTP client interface.

ramosbugs · 2019-06-09T08:11:08Z

latest master now features an async API with pluggable HTTP clients

legokichi added 2 commits October 26, 2018 21:57

change curl to async reqwest

1b93a12

Basic to basic

594cee2

legokichi commented Oct 26, 2018

View reviewed changes

add encodeURIComponent EncodeSet

e98a0c7

legokichi commented Oct 26, 2018

View reviewed changes

add http_client field to Client struct

686dc4d

ramosbugs reviewed Nov 4, 2018

View reviewed changes

revert sync api

9337ba1

legokichi commented Nov 8, 2018

View reviewed changes

remove unwrap when non-ascii header value

0755447

legokichi force-pushed the async-reqwest branch from 3146148 to 0755447 Compare November 8, 2018 06:14

ramosbugs reviewed Dec 9, 2018

View reviewed changes

bachp mentioned this pull request Dec 28, 2018

Support multiple http backends #49

Closed

ramosbugs closed this Jun 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

async API #46

async API #46

legokichi commented Oct 26, 2018 •

edited

Loading

legokichi Oct 26, 2018

ramosbugs Nov 3, 2018

legokichi commented Oct 26, 2018 •

edited

Loading

ramosbugs commented Oct 26, 2018

legokichi Oct 26, 2018 •

edited

Loading

ramosbugs Nov 4, 2018

DeeUnderscore Nov 4, 2018

ramosbugs Nov 4, 2018

DeeUnderscore Nov 5, 2018

legokichi Oct 26, 2018

ramosbugs commented Nov 4, 2018

ramosbugs left a comment

ramosbugs Nov 3, 2018

ramosbugs Nov 3, 2018

ramosbugs Nov 3, 2018

ramosbugs Nov 4, 2018

ramosbugs Nov 4, 2018

ramosbugs Nov 4, 2018

ramosbugs Nov 4, 2018

legokichi Nov 8, 2018

legokichi Nov 8, 2018 •

edited

Loading

ramosbugs Dec 9, 2018

ramosbugs Dec 9, 2018

legokichi commented Nov 8, 2018 •

edited

Loading

ramosbugs Dec 9, 2018

ramosbugs Dec 9, 2018

ramosbugs left a comment

ramosbugs Dec 9, 2018

ramosbugs Dec 9, 2018

ramosbugs commented Dec 9, 2018

bachp commented Dec 27, 2018

ramosbugs commented Dec 28, 2018

ramosbugs commented Jun 9, 2019

async API #46

async API #46

Conversation

legokichi commented Oct 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legokichi commented Oct 26, 2018 • edited Loading

ramosbugs commented Oct 26, 2018

legokichi Oct 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramosbugs commented Nov 4, 2018

ramosbugs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legokichi Nov 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legokichi commented Nov 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramosbugs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramosbugs commented Dec 9, 2018

bachp commented Dec 27, 2018

ramosbugs commented Dec 28, 2018

ramosbugs commented Jun 9, 2019

legokichi commented Oct 26, 2018 •

edited

Loading

legokichi commented Oct 26, 2018 •

edited

Loading

legokichi Oct 26, 2018 •

edited

Loading

legokichi Nov 8, 2018 •

edited

Loading

legokichi commented Nov 8, 2018 •

edited

Loading