Add Proxy Support

[visig9]

Hello, I just found this crate this morning and consider use it for my project recently. But after quick skim it seem like no proxy support (which is necessary part for me). So I try to add it.

I'm not familiar current codebase so some decision may be little weird. Here is something I'm not so sure:

• Custom Proxy struct as thin wrapper of reqwest::Proxy, because...
  • Seem this crate try to hide reqwest as implement detail, so I follow that.
  • Some request::Proxy methods may not suit for this case (e.g., request::Proxy::no_proxy())
• Proxy::new() return an Option but not Result, because ...
  • I'm not sure which Error type should be used for Client creation phase. So just use Option for now but this may not good enough.

How your feel? Please let me know. Thanks.

BTW, thanks for your cool project!

CAUTION: I believe this code can works but currently it only pass cargo check and I'm not actual run it yet. Because I don't have test environment right now. Feel free to test it by youself or I will test it later.

[visig9]

I test it today manually and it works.

User can create a client with proxy like this:

use graph_rs_sdk::{
    identity::ConfidentialClientApplication, GraphClient, GraphClientConfiguration, Proxy,
};

fn main() {
    const TANENT_ID: &str = "xxx";
    const CLIENT_ID: &str = "yyy";
    const CLIENT_SECRET: &str = "zzz";

    let confidential_client_application = ConfidentialClientApplication::builder(CLIENT_ID)
        .with_client_secret(CLIENT_SECRET)
        .with_tenant(TANENT_ID)
        .build();

    let proxy = Proxy::new("http://myporxy.com:3128").unwrap(); // <- here

    let graph_client: GraphClient = GraphClientConfiguration::new()
        .client_application(confidential_client_application)
        .proxy(proxy)  // <- here
        .into();

    // do something with graph_client ...
}

[sreeise]

I test it today manually and it works.

User can create a client with proxy like this:

use graph_rs_sdk::{
    identity::ConfidentialClientApplication, GraphClient, GraphClientConfiguration, Proxy,
};

fn main() {
    const TANENT_ID: &str = "xxx";
    const CLIENT_ID: &str = "yyy";
    const CLIENT_SECRET: &str = "zzz";

    let confidential_client_application = ConfidentialClientApplication::builder(CLIENT_ID)
        .with_client_secret(CLIENT_SECRET)
        .with_tenant(TANENT_ID)
        .build();

    let proxy = Proxy::new("http://myporxy.com:3128").unwrap(); // <- here

    let graph_client: GraphClient = GraphClientConfiguration::new()
        .client_application(confidential_client_application)
        .proxy(proxy)  // <- here
        .into();

    // do something with graph_client ...
}

Nice, i'll take a look at your PR and review it hopefully within a couple of days but it may be a little longer. Thanks for taking the initiative.

[sreeise]

Hey, thanks for the PR. This looks good except for a few minor changes.

As far as abstracting the reqwest client. I mean yes, there are some controls for the reqwest client that the crate would like to abstract for various reasons. One of them being there are features that we would maybe like to enable, such as a socks proxy, but there are also features that we don't want to enable. Thats not the only reason but its an example. So to a certain extent there needs to be some abstraction and limits on what can be done with the http client from the perspective of someone using this crate.

But, as far as using another crates types for things in this case its completley fine. Its the same as reexporting the header types from reqwest for others to use. That may not always be the case with every type though, so it depends on a case by case basis.

[sreeise]

This isn't really needed. In the GraphClientConfiguration method just use the reqwest proxy as the type.

[visig9]

Removed.

[sreeise]

This should be under the http mod. You can add it to some of the other reqwest reexports here:

graph-rs-sdk/src/lib.rs

Line 258 in 7aa756a

pub use reqwest::tls::Version;

[visig9]

Thanks for point out, now I expose reqwest::{Proxy, NoProxy} here.

[sreeise]

I'm not familiar current codebase so some decision may be little weird.

The fact that you just found this crate and were able to find where to add the proxy support and the features for socks means your doing good haha.

Some request::Proxy methods may not suit for this case (e.g., request::Proxy::no_proxy())

Yeah that one im not sure what the use case would be. But regardless I don't think... its an issue? I could see this being a use case if someone wants to proxy specific requests to one proxy while others may go to a different proxy. So they setup an exclusion list of proxy servers for some requests. I've havn't used proxies much so thats just a guess on my end. Regardless I think its fine that they can do that.

I'm not sure which Error type should be used for Client creation phase. So just use Option for now but this may
not good enough.

Yeah I think this one is solved by allowing the user to pass the reqwest::Proxy object instead as I mentioned in the pr review. That way the user has to deal with any issues regarding the proxy before adding to the configuration.

BTW, thanks for your cool project!

Thanks! I appreciate it.

[visig9]

Thanks for suggestions. I force push a new commit to fix all issues you list here.

Here is new examples:

use graph_rs_sdk::{
    identity::ConfidentialClientApplication, GraphClient, GraphClientConfiguration, http::Proxy,
};

fn main() {
    const TANENT_ID: &str = "xxx";
    const CLIENT_ID: &str = "yyy";
    const CLIENT_SECRET: &str = "zzz";

    let confidential_client_application = ConfidentialClientApplication::builder(CLIENT_ID)
        .with_client_secret(CLIENT_SECRET)
        .with_tenant(TANENT_ID)
        .build();

    let proxy = Proxy::all("http://myproxy.com:3128").unwrap(); // <- here

    let graph_client: GraphClient = GraphClientConfiguration::new()
        .client_application(confidential_client_application)
        .proxy(proxy)  // <- here
        .into();

    // do something with graph_client ...
}

This new commit also change feature flag socks-proxy to socks to match how you mapping reqwest feature names to local feature names.

[sreeise]

@visig9 Looks good. Thanks for the PR!

By the way you don't need to force push changes to your PR branch. You can just make new commits and push them up. At least not when making changes to this repository. I know of other repositories on GitHub who do request that.

[sreeise]

@visig9 Released in crate version 2.0.1 https://crates.io/crates/graph-rs-sdk/2.0.1