incompatibility to reqwest 0.13

[geoHeil]

https://github.com/ramosbugs/oauth2-rs/blob/main/Cargo.toml#L55 currently oauth2-rs is not compatible to the new version of reqwest

[9SMTM6]

Note that this of course also blocks consumers via openidconnect-rs.

And the feature rustls-tls of reqwest was renamed to just rustls. Maybe that should also be accounted for, like renaming the features on these crates etc.

[ramosbugs]

see #334 (comment) for my plans here. I'll keep this issue open to track those changes (copied below):

TL;DR I'm planning to release a separate crate that provides a reqwest 0.13 client for oauth2, and similar for other HTTP clients going forward to avoid the need for frequent breaking changes to this crate.

since reqwest is part of this crate's public API, bumping it to a new major version would also be a breaking change for this crate and require a major version bump. I experimented with supporting the new version behind a feature flag or by relaxing the version range in Cargo.toml, but I don't believe it's possible due to differences in Cargo feature names between the two versions of reqwest.

I would prefer not to release a new major version of this crate (and openidconnect) every time one of its public dependencies has a new major release. doing so would inconvenience all users of this crate, whether or not they use reqwest, or impose a maintenance burden to support multiple release branches on an ongoing basis.

as mentioned in the docs, any user of this crate can bring their own HTTP client by providing either a closure or an implementation of the AsyncHttpClient/SyncHttpClient traits. this solution can also be used to support a different version of reqwest/ureq/etc. if you need to upgrade to reqwest 0.13 urgently, I suggest taking that route.

as a more maintainable solution, I'm currently working on providing an updated reqwest 0.13 client in a separate crate. I'll follow a similar model for other HTTP clients going forward so that their versions can be bumped independently of oauth2's version. it's likely I'll remove the existing clients from oauth2's public interface in the next major version.

[9SMTM6]

@ramosbugs sounds like a good plan. When I realized that this crate and reqwest are version locked after using this feature I was already a bit surprised, and wondered if there isn't a better way, and this seems to be it.

[ramosbugs]

I just released oauth2-reqwest 0.1.0-alpha.1 to gather feedback before making a stable release. Please try it and post feedback either here or in new GitHub issues. See documentation for usage instructions.

[GodTamIt]

I just released oauth2-reqwest 0.1.0-alpha.1 to gather feedback before making a stable release. Please try it and post feedback either here or in new GitHub issues. See documentation for usage instructions.

I just gave it a shot and integrated it into my existing project.

From an ergonomics standpoint, I'm not sure that taking a reference to a reqwest::Client is great, especially if one has ownership of a reqwest client and just wants to use it for OAuth. oauth2_reqwest::ReqwestClient has to be temporarily constructed via ReqwestClient::from every time the user wants to use it. My current use-case:

use oauth2_reqwest::ReqwestClient as OAuthReqwestClient;

struct Inner {
    /// The AWS secrets manager instance to grab credentials from.
    pub secrets_manager: SecretsManager,

    /// The secret ID that contains the OAuth credentials.
    pub secret_id: String,

    /// The client to make OAuth requests with.
    // pub oauth_http_client: OAuthReqwestClient, // <-- I can't do this without painful self-ref workarounds or inserting a lifetime to my type :(

    // The other alternative is to store the raw client and construct every time.
    pub oauth_http_client: reqwest::Client,

    /// The URL for OAuth token authentication.
    pub auth_url: String,

    /// The OAuth access token.
    pub token: AsyncRwLock<Arc<String>>,
}

#[derive(Clone)]
pub struct OAuthManager {
    inner: Arc<Inner>,
}


impl OAuthManager {
  // ...
}

#[async_trait::async_trait]
impl reqwest_middleware::Middleware for OAuthManager {
    async fn handle(
        &self,
        mut req: reqwest::Request,
        extensions: &mut http::Extensions,
        next: reqwest_middleware::Next<'_>,
    ) -> reqwest_middleware::Result<reqwest::Response> {
        let creds = get_api_credentials(&self.inner.secrets_manager, &self.inner.secret_id)
            .await
            .map_err(reqwest_middleware::Error::Middleware)?;
        let token = self.get_token().await;

        req.headers_mut().insert(
            HeaderName::from_static("x-api-key"),
            HeaderValue::from_str(creds.api_key.as_str())
                .map_err(reqwest_middleware::Error::middleware)?,
        );
        req.headers_mut().insert(
            HeaderName::from_static("authorization"),
            HeaderValue::from_str(format!("Bearer {}", token).as_str())
                .map_err(reqwest_middleware::Error::middleware)?,
        );

        next.run(req, extensions).await
    }
}

Cloning reqwest::Client is just cloning an Arc which is relatively cheap. If we want to avoid the overhead of cloning/pointer accounting, we COULD have owned and borrowed versions.

Furthermore, I think you'll need ReqwestClient to satisfy a Higher-Ranked Trait Bound in case your AsyncHttpClient (i.e. call) lifetime is not exactly the same as the ReqwestClient's lifetime. Maybe something like:

impl<'c, 'a> AsyncHttpClient<'a> for ReqwestClient<'c>
where
    'c: 'a,
{
    type Error = HttpClientError<reqwest::Error>;

    #[cfg(target_arch = "wasm32")]
    type Future = Pin<Box<dyn Future<Output = Result<HttpResponse, Self::Error>> + 'a>>;
    #[cfg(not(target_arch = "wasm32"))]
    type Future =
        Pin<Box<dyn Future<Output = Result<HttpResponse, Self::Error>> + Send + Sync + 'a>>;

    fn call(&'a self, request: HttpRequest) -> Self::Future {
        Box::pin(async move {
            let response = self
                .0
                .execute(request.try_into().map_err(Box::new)?)
                .await
                .map_err(Box::new)?;

            let mut builder = http::Response::builder().status(response.status());

            #[cfg(not(target_arch = "wasm32"))]
            {
                builder = builder.version(response.version());
            }

            for (name, value) in response.headers().iter() {
                builder = builder.header(name, value);
            }

            builder
                .body(response.bytes().await.map_err(Box::new)?.to_vec())
                .map_err(HttpClientError::Http)
        })
    }
}

[ramosbugs]

@GodTamIt: thanks for the quick feedback!

From an ergonomics standpoint, I'm not sure that taking a reference to a reqwest::Client is great, especially if one has ownership of a reqwest client and just wants to use it for OAuth. oauth2_reqwest::ReqwestClient has to be temporarily constructed via ReqwestClient::from every time the user wants to use it.

I used a reference to avoid requiring users to give up ownership of the reqwest client in case they needed it for other purposes, but I missed that it's cheaply clone-able. In that case, I agree it makes more sense to take ownership of the client and allow users to keep it around if desired. This approach might not work for other HTTP clients in future crates, but I think it's fine for each wrapper API to be tailored to the underlying client API.

Furthermore, I think you'll need ReqwestClient to satisfy a Higher-Ranked Trait Bound in case your AsyncHttpClient (i.e. call) lifetime is not exactly the same as the ReqwestClient's lifetime.

Good point! I originally implemented it that way before making a bad assumption and removing the extra lifetime. In any case, I think this problem goes away if we get rid of the reference.

I'm planning to wait a bit to see if others have feedback before incorporating these changes and cutting another release.

[vsiles]

Furthermore, I think you'll need ReqwestClient to satisfy a Higher-Ranked Trait Bound in case your AsyncHttpClient (i.e. call) lifetime is not exactly the same as the ReqwestClient's lifetime. Maybe something like:

Hi ! I just tried to use your new crate and right away ran into that problem:

error: implementation of `oauth2::AsyncHttpClient` is not general enough
   --> token-provider/src/oauth2/provider.rs:539:5
    |
539 |     async fn device_code_login(&self, timeout: Duration) -> Result<Token> {
    |     ^^^^^ implementation of `oauth2::AsyncHttpClient` is not general enough
    |
    = note: `oauth2::AsyncHttpClient<'0>` would have to be implemented for the type `oauth2_reqwest::ReqwestClient<'_>`, for any lifetime `'0`...
    = note: ...but `oauth2::AsyncHttpClient<'1>` is actually implemented for the type `oauth2_reqwest::ReqwestClient<'1>`, for some specific lifetime `'1`

[9SMTM6]

@ramosbugs Just making sure you're aware that the readme on crates.io as published is the one from oauth2-rs. You've set this explicitly in its Cargo.toml, probably an oversight.

And then regarding ergonomics. In fact I seem to be unable to make this crate work with CodeTokenRequest::request_async.

At first I thought maybe I was doing something else problematic, so I did a (needed) cleanup of my OIDC code that allowed me to hold ReqwestClient's over minimal await points. But now I'm able to do a minimal change to adopt your crate in an update, and this ends up only holding ReqwestClient in the OIDC callback only over the one await point from CodeTokenRequest::request_async, which is needed. And I don't get any compilation errors directly from your crates. But still I'm unable to get this to compile, since it seems to break axum in some way: The handler that holds ReqwestClient over the await point from CodeTokenRequest::request_async suddenly doesn't implement axum::handler::Handler any longer, applying #[axum::debug_handler] doesn't provide any useful info.

None of the axum crates got updated in the change, I checked the Cargo.lock.

Honestly what exactly is going wrong there is currently beyond me. I need to read up on the axum documentation a bit more as to what they require.

But the change to ownership proposed should probably solve it.