Making the https_only client configuration option editable

[Gaelik-git]

Hey !

I would like to use Wiremock-rs to mock the usage of this lib in my app, the wiremock-rs does not support https mock server for now (LukeMathWalker/wiremock-rs#58)

With the use_endpoint method already existing, this default https_only option is the only piece missing to be able to mock the lib. Do you think this change fits ?

// Start mock
let mock_server = MockServer::start().await;

// Create Graph client with token pointing to mock server
let mut graph: Graph = GraphClientConfiguration::new()
    .access_token(token)
    .https_only(false)
    .into();
graph.use_endpoint(format!("{}/graph", mock_server.uri()).as_str());

// set the reqest to expect and value to respond with
Mock::given(method("GET"))
        .and(path("/graph/users"))
        .respond_with(ResponseTemplate::new(200).set_body_json(json!({
            "value": []
        })))
        .expect(1)
        .mount(&mock_server)
        .await;

let mock_result = client
  .users()
  .list_user()
  .paging()
  .json::<Value>()
  .await
  .unwrap();

[sreeise]

Hey @Gaelik-git,

So I think this is a pretty good idea and a good use case. My only concern is that it won't work for a future release thats already planned for updating the oauth/openid impl which will update the client to only allow the national clouds that are available for Microsoft Graph meaning the only URLs that could actually be used to call the Graph API. The reason there is not currently a way to set https only to false is well because its a security issue but also it probably just woudn't ever work since the Graph API requires it.

But I do like the use case for what your wanting to do. And so I am thinking that this would work well as a feature flag. That way users of the crate must opt in to a feature in order to set https only to false. And in the updates I am making for oauth/openid I will also add the ability to set a custom endpoint that is not a national cloud and provide that method under the same feature flag.

So, if your willing to compromise I think what could be done right now is to add a feature flag to graph-http in the Cargo.toml below the other features here with this line:

test = []

For the https_only method add the cfg feature annotation:

    #[cfg(feature = "test")]
    pub fn https_only(mut self, https_only: bool) -> GraphClientConfiguration {
        self.config.https_only = https_only;
        self
    }

Then add the same feature flag to the main Cargo.toml, with it also enabling the graph-http feature:

test = ["graph-http/test"]

And when you use the SDK of course just enable the feature:

graph-rs-sdk = { version =  "1.1.2", features = ["test"] }

And like I mentioned before, in future versions when we start validating the URLs for the national clouds, I will add a method to the client under that same feature that will enable setting the endpoint to a custom value. Obviously, for now you can just continue to use the use_endpoint method.

Does that work?

[Gaelik-git]

I understand your idea ! I think it would fit my need.
I don't know a lot about rust feature system, but with your inputs I should be able to do this.
And it's probably a good thing for me to learn this

Do you think the feature should be named test or something more specifc like https_not_required ?

[Gaelik-git]

I implemented your suggestion with the feature name https-not-required
Tell me if you want to rename it

[Gaelik-git]

Another solution would be to just update the default value of https_only ?

impl ClientConfiguration {
    pub fn new() -> ClientConfiguration {
        let mut headers: HeaderMap<HeaderValue> = HeaderMap::with_capacity(2);
        headers.insert(ACCEPT, HeaderValue::from_static("*/*"));

        ClientConfiguration {
            access_token: None,
            headers,
            referer: true,
            timeout: None,
            connect_timeout: None,
            connection_verbose: false,
            #[cfg(feature = "http-allowed")]
            https_only: false,
            #[cfg(not(feature = "http-allowed"))]
            https_only: true,
            /// TLS 1.2 required to support all features in Microsoft Graph
            /// See [Reliability and Support](https://learn.microsoft.com/en-us/graph/best-practices-concept#reliability-and-support)
            min_tls_version: Version::TLS_1_2,
        }
    }
}

Something like this should be ok ?

[sreeise]

I implemented your suggestion with the feature name https-not-required Tell me if you want to rename it

I think it should be named test or test-util because its a feature meant for testing and its likely that other testing utilities, meaning types meant for testing using the crate itself, will be added under the feature flag at a later point in time. test-util might make the feature flag name more clear in this case.

The feature flag is meant to enable utilities used when testing and the name should be related to that basically. In this case it wouldn't make sense that the name would be https-not-required because its too specific to just one thing or method: disabling or enabling https only. Whereas something like test or test-util is more broad and makes it so that we can add onto it in the future. The disabling of https shouldn't be something that would be done outside of testing regardless just because it will cause the http requests to fail if you do try to hit the graph api.

Doing the feature this way is simlar to how Tokio has a feature called test-util which enables the testing utilities for tokio such as the #[tokio::test] macro

[Gaelik-git]

I renamed the feature to test-util
Any other feedback on this ?

[sreeise]

I renamed the feature to test-util Any other feedback on this ?

That works for me. Thanks for the work on this 🚀

[Gaelik-git]

Thanks for the support on this. Any idea when will be the next release ?

[sreeise]

Thanks for the support on this. Any idea when will be the next release ?

I'll try to see if I can get a minor version published here within a day or two. The tests seem to be failing for something unrelated to this so I need to take a look at it and id rather not publish before I know for sure. I don't see how anything in this PR could cause failures so nothing major, but I don't like to publish if the build tag on the README says the tests are failing.

[sreeise]

Thanks for the support on this. Any idea when will be the next release ?

@Gaelik-git

1.1.3 https://crates.io/crates/graph-rs-sdk