Add A Novice One-Liner For Ease Of Use!

[vthg2themax]

Add A Novice One-Liner For Ease Of Use so that someone just jumping in would be able to run this when they grab the crate down.

[steffengy]

The implementation you copied from the example is not exactly production ready.
It has print statements, a configuration that is not as secure as possible, missing error handling, ...
So it serves as short example that shows how to use it in principle but nothing that I would be comfortable having as default in winauth itself as-is.

[vthg2themax]

Thank you very much for your constructive feedback! I have attempted to address the deficiencies in the code submitted. Please let me know if it's still not good, and if so, what I may do to improve it further please. Thanks again!

[steffengy]

It might make sense to rewrite the macro as function to address the following shortcomings.
There really isn't a reason this is a macro, besides it was quicker writable for demo purposes.

The most important shortcomings:

1. The error handling should not panic, but expose an Result that the user can handle and decide what to do with the error.
2. The NtlmSspiBuilder cannot be customized.
   It also doesn't take multi-platform support into account, the macro e.g. only works on windows.
3. For versioning a dependency of winauth to reqwest has the downside of preventing us to upgrade to a newer reqwest version without bumping the version of winauth itself.
   I'm myself not sure what the best approach here is, maybe a winauth-reqwest crate.
   We could also just decide to not care and include reqwest support in winauth as a easy starter, but we cannot do so for more implementations (hyper, ...) as it eventually becomes chaotic version-wise.

[vthg2themax]

I think keeping it as a macro would be the best option for that very reason. If it were to become a function, then the project would depend on reqwest, even if another library becomes a better option. Could changing the Macro's name be a better solution instead, since at that point it would be assumed that it is a simple reqwest being made? I can definitely take care of points 1, and 2. Thanks again for your excellent code critiques!

[steffengy]

It depends on reqwest in any case.
With a macro, the dependency is implicit - which actually is worse, since the user has to ensure his Cargo.toml has a dependency (of the right version) to reqwest.
So with a macro we're actually unable to update the underlying reqwest version or use another library, since that would be a backwards-compatibility breaking change.
Exposing a "blackbox" function hiding reqwest on the other hand does not have this downside.

[vthg2themax]

I appreciate you explaining this to me, but does what you are saying mean that the current version of winauth-rs has an implicit dependency on reqwest? Thanks, Vince Pike

…

On 2019-10-09 11:26 a.m., Steffen Butzer wrote: It depends on reqwest in any case. With a macro, the dependency is implicit - which actually is worse, since the user has to ensure his Cargo.toml has a dependency (of the right version) to reqwest. So with a macro we're actually unable to update the underlying reqwest version or use another library, since that would be a backwards-compatibility breaking change. Exposing a "blackbox" function hiding reqwest on the other hand does not have this downside. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#7?email_source=notifications&email_token=AF63BZ7NOMLN2USY3GB3SU3QNXZYXA5CNFSM4I42E47KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAYIZGY#issuecomment-540052635>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF63BZZDA3CFT56ICY2PRV3QNXZYXANCNFSM4I42E47A>.

[steffengy]

No, since reqwest is not used in winauth but only examples, winauth doesn't have any dependency to reqwest. My point is that if we expose a simple function that allows doing http requests for convenience it should be with an an explicit dependency, so we can atleast somewhat guarantee backwards compatibility.

[vthg2themax]

Oh, okay. I appreciate you helping me to understand how that part works. I was thinking that since it was being used in an example, that it would then require reqwest in order to use winauth-rs at all. I would be willing to explicitly write the submitted code as a function instead of as a macro. However, if you think that adding that dependency isn't worth it (in terms of adding a dependency), I understand as well. Let me know what you think. Thanks again for your wonderful crate! Thanks, Vince Pike

…

On 2019-10-15 12:29 p.m., Steffen Butzer wrote: No, since reqwest is not used in winauth but only examples, winauth doesn't have any dependency to reqwest. My point is that if we expose a simple function that allows doing http requests for convenience it should be with an an explicit dependency, so we can atleast somewhat guarantee backwards compatibility. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#7?email_source=notifications&email_token=AF63BZYOVCAXTG5TF27AU4DQOXVVVA5CNFSM4I42E47KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBJM42Q#issuecomment-542297706>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AF63BZZPKTGKBTC6X3H7YLDQOXVVVANCNFSM4I42E47A>.

[steffengy]

In priciple having a convenience interface that just allows users to do authenticated NTLM requests is something I'm fine with. Getting the API right is another story though ;)

A single function probably won't do, since you'll need to specify http method, headers, etc. - probably something like a wrapper around reqwest::RequestBuilder, but we can use types from the http crate in theory.
(To decouple our public API from reqwest, so if reqwest updates a version we can update the version of reqwest we use, without breaking backwards compatibility).
==> This is much more work than it looks like