Faraday #998
Faraday #998
Conversation
…r ntlm auth handshakes
…e's test runner; use a mocked faraday response
… deprecated options; save body to locals
…building a faraday request requires accessing internal apis and is very nasty; solve ntlm handshake; convert various httpi fields to their faraday equivalents and mark any that don't have one
|
Thanks for putting this together. planning to take a look at it all this weekend |
|
Thanks again for this PR. Here's a quick plan:
thoughts? let me know if I can help with any part in particular |
|
The biggest concern i have is whether NTLM is going to function correctly, i have a local server i can use to integration test it though so pre-release sounds like the best course of action. Upgrade path is going to be interesting, for the most part the biggest thing is going to be for anyone who actually uses the encrypted cert file functionality, they'll need to open the cert file using openssl and provide it that way. We could provide a helper internally that does this in order to preserve that API though. I guess also people using custom adapters would need to rebuild their adapter on top of faraday's API. I tried to preserve as much public api as i could. |
|
looks like code climate is happy now. |
|
@pcai do have a quick question for you, regarding the implementation of cookie parsing. Currently I have it set up so you can pass array'd cookie values under Implemented at Can you think of a better way to handle this? |
|
I suppose we could just assign to nil {
'some-cookie': 'choc-chip'
'HttpOnly': nil
} |
|
Checking in on the status of this PR. I maintain a Rails application that uses I think this PR might resolve our issues and allow us to upgrade. Just curious. |
|
Honestly forgotten about, i need to fix the cookie approach, I'll look into getting that done asap, thanks for the reminder. (Timeframe is probably within the next 3 or so days.) |
|
@pcai Looks like the test application relies on Rack::Auth::Digest::MD5 which is gone, so i'll have to update that too. Seems like rack's opinion on the subject is that the upgrade path for digest auth is that there is no upgrade path for digest auth, and any tooling using it should simply drop support. I'm going to go that route for now... |
|
looks like 3.4 removes httpclient from stdlib... guess i'll add that to our gemspec as a dev gem. |
|
@pcai Alright, at this point we're probably ready to cut a prerelease. The deprecation specs just fail on 3.4 because of ascii encoding differences, and fixing that spec on 3.4 would break it on every other version. |
|
thx @LukeIGS i will take a look today @eclectic-coding please make sure you force I plan on making a minor bugfix release this wk to mainline to force httpi 4.x+ |
There was a problem hiding this comment.
thank you for the work on this. I have inline thoughts on DeprecatedOptionError - but here are some more general ones:
- It's not immediately clear to me why all the metaprogramming is really needed
- Most of it is to support an
optionattribute but I don't really see the value of it, since its just a variable used in an error string builder. Why not just build the error string directly? Sure its easier to test, but the code doesn't need so much testing that it should be architected to prioritize it
| @locals[:soap_action] ||= soap_action | ||
| @globals[:endpoint] ||= endpoint | ||
| def handle_ntlm(connection) | ||
| ntlm_message = Net::NTLM::Message |
|
Unless I'm reading the logs wrong, its more than just an encoding difference - ruby-head returns the full namespace in addition to the method name
|
…he api changes with 3.4
What kind of change is this?
Conversion to faraday full of breaking changes
Did you add tests for your changes?
Yes
Summary of changes
#992
So far the biggest changes of note are that faraday doesn't support setting cert files except the ca, it doesn't support using an alternative ssl cipher and since it doesn't open cert files, it doesn't worry about using a password to decrypt them. The end user is expected to open and decrypt the client key using openssl and provide the result to faraday instead. As such I went ahead and deprecated those globals for now. If we wanted to keep this functionality, savon would have to handle those logic flows.
Faraday also uses various middlewares to enhance its auth capabilities. Currently I'm solving for NTLM in savon proper, but it might make more sense to write a middleware to handle NTLM auth, along the lines of how digest is handled.
Since these middlewares are not included out of box with faraday, I've added them as development dependencies and added errors when trying to load them if they aren't present. I used the same approach for NTLM auth, though rubyntlm is such a light weight gem, adding it as a solid dep would probably be acceptable too.
Worth noting that CI will not be able to build this unless the associated wasabi version is built and available, since it also relies on some very minor changes to support faraday.
see: savonrb/wasabi#116
The API for adapters is also slightly different since savon's adapters are often built using parameters, so I just take an array of options to forward