happydpc / goauth2

Automatically exported from code.google.com/p/goauth2
BSD 3-Clause "New" or "Revised" License
0 stars 0 forks source link

Request does not work when token needs to be refreshed #2

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
What steps will reproduce the problem?
1. Get a token (and gob it to the disk for testing)
2. Wait for AccessToken to expire
3. Make request

What is the expected output? What do you see instead?
Expected: oauth will use the RefreshToken to obtain a new AccessToken and 
reissue the request with the new AccessToken.

Observed: Request will fail with the exact results depending on the server of 
the requested url.  The for example google spreadsheets server eventually times 
out with 502 Bad Gateway and some error message.

What version of the product are you using? On what operating system?
latest tip d7c746aa73cb on OSX

As far as I can tell, there are two issues:
1) After the initial failed request, the http.Request object is reused in a 2nd 
attempt.  Unfortunately the http.Request.Body will be all used up, so the 
proper data will not be sent.
2) After a new access token is retrieved, the request still has the 
Authorization header set to the old AccessToken.

I have attached a patch with fixes for these two issues, but I'm not sure 
you're going to like my approach with the readCloseDupper.  Another approach 
might be to proactively request a new AccessToken when it is going to expire 
soon before trying to make a request which will consume the Body.

After applying this patch, it works as I expect it to.

Original issue reported on code.google.com by tarmi...@gmail.com on 19 Jun 2011 at 11:25

Attachments:

GoogleCodeExporter commented 9 years ago
I ran into the same problem, although with an empty-bodied request, so I didn't 
run into the consumption problem.  

I've got another point to add to this:  I've observed that the server I'm 
interacting with (google storage for developers) returns 403 rather than 401 on 
a bad token.  

As nice as the transparent retry / refresh logic is, I think it's a bad idea to 
keep it at all.  The behavior is explicitly called out as invalid in the go 
http package doc:

    // RoundTrip executes a single HTTP transaction, returning
    // the Response for the request req.  RoundTrip should not
    // attempt to interpret the response.  In particular,
    // RoundTrip must return err == nil if it obtained a response,
    // regardless of the response's HTTP status code.  A non-nil
    // err should be reserved for failure to obtain a response.

The idea of duplicating the body for every request terrifies me.  This will 
carry substantial cost for applications which routinenely PUT large files (like 
mine).

Original comment by ip...@google.com on 28 Jul 2011 at 7:53