[2.4.x] Upgrade async-http-client to version 1.9.29#4826
[2.4.x] Upgrade async-http-client to version 1.9.29#4826marcospereira wants to merge 1 commit intoplayframework:2.4.xfrom marcospereira:upgrade-ahc-play-2.4.x
Conversation
|
My oauth signatures totally broke in a 2.3 to 2.4 migration. Tried this fix by manually updating my async client, and it did not help me at all, still getting invalid oauth returned from multiple oauth 1 providers. |
|
@brianwawok can you put together a test case reproducing the problem? |
|
@wsargent not sure I can provide a test case scala-test style, as I am not super familiar with what is going on in the encoding level of http async. Here is a test case though hitting up the etsy api.. (https://www.etsy.com/developers/) I cannot use the inbuilt play oauth thingy because I need to pass params (perms here), so I can't do it the way twitter does it in the docs. val redirectUrl = "http://localhost/processToken"
val etsyKey = ConsumerKey("myapiKey", "mysharedSecret") //replace with etsy api keys
val requestToken = RequestToken("", "") //this is really blank for a request token request
val perms = "listings_r listings_w listings_d transactions_r transactions_w shops_rw"
val requestUrl = "https://openapi.etsy.com/v2/oauth/request_token"
val nonce = "abcdefg" //really do something better
WS.url(requestUrl).sign(OAuthCalculator(etsyKey, requestToken))
.withRequestTimeout(10000)
.withHeaders("Content-Type" -> "application/x-www-form-urlencoded", "Accept" -> "application/json")
.withQueryString("scope" -> perms)
.post(
s"oauth_callback=$redirectUrl&" +
s"oauth_nonce=${nonce}&" +
s"oauth_timestamp=${DateTime.now(DateTimeZone.UTC).getMillis / 1000}&" +
s"oauth_version=1.0")Works in 2.3, in 2.4 I get back from etsy api |
|
@brianwawok this looks good enough to me. I will try to use your scenario to replicate the problem and see if it is a problem with Play or async-http-client. Can't do it right now, but will try later today. Thanks! |
|
I also experience the same problem. My oauth 1.0a request to Yelp APIs used to work in 2.3, and now I get invalid signature error. My code is as follows: I asked this question on StackOverflow. http://stackoverflow.com/questions/31253491/oauth-1-0a-stops-working-after-upgrading-from-play-2-3-to-play-2-4. Thanks. |
|
@brianwawok and @coolsuntraveler, Directly using async-http-client also results in a import org.joda.time._
import com.ning.http.client._
import com.ning.http.client.oauth._
val redirectUrl = "http://localhost/processToken"
val consumerKey = new ConsumerKey("...", "...") //replace with etsy api keys
val requestToken = new RequestToken("", "") //this is really blank for a request token request
val perms = "listings_r listings_w listings_d transactions_r transactions_w shops_rw"
val requestUrl = "https://openapi.etsy.com/v2/oauth/request_token"
val nonce = "abcdefg" //really do something better
val client = new AsyncHttpClient()
val response = client
.preparePost(requestUrl)
.setSignatureCalculator(new OAuthSignatureCalculator(consumerKey, requestToken))
.setRequestTimeout(10000)
.addHeader("Content-Type", "application/x-www-form-urlencoded")
.addHeader("Accept", "application/json")
.addQueryParam("scope", perms)
.addFormParam("oauth_callback", requestUrl)
.addFormParam("oauth_nonce", nonce)
.addFormParam("oauth_timestamp", String.valueOf(DateTime.now(DateTimeZone.UTC).getMillis / 1000))
.addFormParam("oauth_version", "1.0")
.execute()Which generates a request with the following headers: And the following response: (Of course, I've omitted sensible information here). So, we either have a problem with async-http-client itself, or the request has some problem. I don't know oAuth 1.0 enough. Do you think there is some problem in the code above? The following request returns a val response = client
.preparePost(requestUrl)
.setSignatureCalculator(new OAuthSignatureCalculator(consumerKey, requestToken))
.setRequestTimeout(10000)
.addHeader("Content-Type", "application/x-www-form-urlencoded")
.addHeader("Accept", "application/json")
.addQueryParam("scope", perms)
.execute()What do you guys think? |
|
@marcospereira I do not know if the code above is valid oauth (like you oauth1 makes me scratch my head a little).. But I do know that it worked in Play 2.3, then stopped in 2.4 (which it looks like to blame is the async-http-client change). I would be curious if the code above works with an old async-http-client, but I suspect it would. Your request that got a 200 response looks the exact same, but with no form params? Which means it wouldn't really work, as we have to at least set the oauth_callback up for our next request, or the client would never make it back to our server after approving us. So it seems like there has been some regression in how async-http-client signs form params... |
|
This is what play 2.3 does |
|
@brianwawok, can you please tell me how to get a http request printout like your last comment? |
|
@coolsuntraveler put logging to debug, com.ning.http.client will do it for you |
|
Thanks @brianwawok. I compared yelp api requests for Play 2.3 and 2.4, and they look the same to me, other than the values of nonce, timestamp, and signature. I tried to use async-http-client directly, and it doesn't work for even 1.5.0 and 1.8.0. Either I am using it wrong, or oauth never worked for async-http-client. This is the code for 1.8.0. |
|
I found if I don't encode the parameters, Yelp API works with async-http-client 1.9.29. But this still gets invalid signature in Play 2.4. |
|
so to further make things interesting, I did this in play 2.4.
And it worked. So something is wrong with the signature calculator... hand calculator signature "works" |
|
Ok, after some investigation, I don't think this is a regression in either async-http-client or play itself, but just the lack of backward compatibility. Turns out async-http-client handles This is the code that works as expected: val redirectUrl = "http://localhost/processToken"
val etsyKey = ConsumerKey("...", "...") //replace with etsy api keys
val requestToken = RequestToken("", "") //this is really blank for a request token request
val perms = "listings_r listings_w listings_d transactions_r transactions_w shops_rw"
val requestUrl = "https://openapi.etsy.com/v2/oauth/request_token"
val client = NingWSClient()
client.url(requestUrl)
.sign(OAuthCalculator(etsyKey, requestToken))
.withRequestTimeout(10000)
.withHeaders("Content-Type" -> "application/x-www-form-urlencoded", "Accept" -> "application/json")
.withQueryString("scope" -> perms)
.post(s"oauth_callback=$redirectUrl")It also generates signatures which are compatible with the page linked by @brianwawok. Manually posting @coolsuntraveler I was not able to reproduce your scenario. Could you please submit the logs generated when you execute the code samples you posted here? |
|
I agree this is not a http client thing, as I played with older clients with no success. Also agree we don't need to add oauth stuff, as it gets auto added now. (I think maybe it got auto added before, but there was logic to not auto add it if it was already in the request). However the big problem - I cannot get your example to work in Play WS syntax. Why does this not work? val redirectUrl = "http://localhost/processToken"
val etsyKey = ConsumerKey("...", "...") //replace with etsy api keys
val requestToken = RequestToken("", "") //this is really blank for a request token request
val perms = "listings_r listings_w listings_d transactions_r transactions_w shops_rw"
val requestUrl = "https://openapi.etsy.com/v2/oauth/request_token"
WS.url(requestUrl)
.sign(OAuthCalculator(etsyKey, requestToken))
.withRequestTimeout(10000)
.withHeaders("Content-Type" -> "application/x-www-form-urlencoded", "Accept" -> "application/json")
.withQueryString("scope" -> perms)
.post(s"oauth_callback=$redirectUrl") |
|
@brianwawok what do you mean by not working? Adapting the code sample I've sent to use val redirectUrl = "http://localhost/processToken"
val etsyKey = ConsumerKey("...", "...") //replace with etsy api keys
val requestToken = RequestToken("", "") //this is really blank for a request token request
val perms = "listings_r listings_w listings_d transactions_r transactions_w shops_rw"
val requestUrl = "https://openapi.etsy.com/v2/oauth/request_token"
WS.url(requestUrl)
.sign(OAuthCalculator(etsyKey, requestToken))
.withRequestTimeout(10000)
.withHeaders("Content-Type" -> "application/x-www-form-urlencoded", "Accept" -> "application/json")
.withQueryString("scope" -> perms)
.post(s"oauth_callback=$redirectUrl")This generates a |
|
@marcospereira Got it, with removing oauth stuff AND upgrading async to 1.9.29, life is good. Maybe worth a mention in the migration docs that sending your own oauth params will not result in an invalid signaute? Seems kind of weird still, to just calc the signature wrong on including some oauth params. |
|
@marcospereira |
|
@coolsuntraveler could you please also post the response body? |
|
@coolsuntraveler also, did you tried with async-http-client 1.9.29? |
|
Response body: I am not familiar with this. I tried with Play 2.4.2. How do I upgrade async-http-client to 1.9.29? Thanks. |
|
@coolsuntraveler add this line to your libraryDependencies += "com.ning" % "async-http-client" % "1.9.29" |
|
@marcospereira It works after upgrading to 1.9.29. Thanks for your help! |
|
LGTM. @wsargent, do you want to review? |
|
@richdougherty and @wsargent any news here? |
There was a problem hiding this comment.
This seems fishy since it's casing a long into an int
There was a problem hiding this comment.
Mmm, I realise that since none pointed this out I must be missing something :-)
There was a problem hiding this comment.
I don't like it either, but this was done to keep backward compatibility.
There was a problem hiding this comment.
The biggest thing I'd be worried about here is whether this has changed from seconds to milliseconds. You can't use an int for a cookie max age in milliseconds because the longest max age you would be able to specify then would be 24 days. But if you change it to long, you can use milliseconds.
There was a problem hiding this comment.
Based on this test and also on the fact that max-age header is represented in seconds, I think (although I'm not sure) AHC is expecting seconds here.
There was a problem hiding this comment.
Maybe this is an issue that doesn't exist, but what if the returned Long is greater than Int.MaxValue?
One more question about binary compatibility, is there a compelling reason for merging this in 2.4.x?
There was a problem hiding this comment.
Maybe this is an issue that doesn't exist, but what if the returned Long is greater than Int.MaxValue?
Said otherwise, I'm wondering why not having Long.min(ahcCookie.getMaxAge(), Integer.MAX_VALUE).intValue() to avoid any potential overflow.
There was a problem hiding this comment.
One more question about binary compatibility, is there a compelling reason for merging this in 2.4.x?
It fixes some problems with oAuth1 signatures.
Said otherwise, I'm wondering why not having
Long.min(ahcCookie.getMaxAge(), Integer.MAX_VALUE).intValue()to avoid any potential overflow.
We can do that, which is choose between overflowing or returning the wrong value. I prefer to keep this as a int because it is changing less things.
There was a problem hiding this comment.
We can do that, which is choose between overflowing or returning the wrong value. I prefer to keep this as a int because it is changing less things.
Ok. My rationale was that if ahcCookie.getMaxAge() would return (Integer.MAX_VALUE + 1), I thought it was better to return Integer.MAX_VALUE, rather than Integer.MIN_VALUE because it overflowed.
There was a problem hiding this comment.
I got that. My point was that returning the "wrong/unexpected value" (Integer.MAX_VALUE) is no better than the overflow. Anyway, I don't have a strong opinion here. Both have their own problems.
|
@dotta, thanks for reviewing this PR. :-) |
|
@dotta oauth is broke without this merge, isn't a regression a compelling reason to merge this into 2.4? |
|
@brianwawok introducing another regression (breaking binary compatibility) to solve a regression is not a good thing. |
|
This PR has stayed open for several months now, and there hasn't been any further follow up since August 21th. If someone is going to investigate how oauth can be fixed without breaking binary compatibility, please feel free to reopen the PR or create a new one. |
|
Fun fact: I will need to use the workaround outlined in this PR / issue and reference this bug in the upcoming release of Reactive Web Applications. It's not pretty but it doesn't look like I have an alternative... |
|
@manuelbernhardt Maybe Play 2.5 will be released before your book is published? |
|
@dotta It will be a tie, the book will remain on 2.4. It will likely come out in March and will go to production (= all the processing needed to get it to print) very soon, which means the content cannot be changed anymore. Also there are a few dependencies on third-party libraries, so waiting for 2.5 would mean waiting for at least 3-4 months longer, which I'm not too much interested in. |
This PR backports #4778 to branch 2.4.x.