> On 2012-02-22 00:39:19, Ryan Baxter wrote: > > LGTM. This seems reasonable. I took a quick peak at the OAuth 2 spec and > > it didnt seem to go into any details on this. You might want to post > > something on the OAuth mailing list to see if anyone there has any thoughts.
The only details I found were these: http://tools.ietf.org/html/draft-ietf-oauth-v2-bearer-16#section-3.1 It talks about services typically returning 400, 401, 403, and 405 when a request fails. However, the information it provides there also seems like we should really only remove the token if there's a 401. Removing the token from the store in that case would be fulfilling this part of the spec: "The client MAY request a new access token and retry the protected resource request." 403 indicates that the request requires higher privileges than that provided by the access token and that it's not necessarily a problem with the access token itself. I'm not sure if it would be possible to request a new access token with higher privileges since the scope is defined in the gadget from what I've seen. And now the more I think about it, we should really be looking for those error codes like "invalid_token" or "insufficient scope" in the response headers... if those are even available in the flow that I'm looking at in Shindig... I'll investigate more. - Stanton ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3987/#review5255 ----------------------------------------------------------- On 2012-02-21 21:40:31, Stanton Sievers wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3987/ > ----------------------------------------------------------- > > (Updated 2012-02-21 21:40:31) > > > Review request for shindig, li xu and Adam Clarke. > > > Summary > ------- > > From JIRA: > If the url to which a gadget is doing a makeRequest doesn't exist, i.e., > returns a 404 to the Shindig server, the access token is being removed from > the OAuth2 Store. This functionality is implemented here: > org.apache.shindig.gadgets.oauth2.BasicOAuth2Request.fetchFromServer(OAuth2Accessor, > HttpRequest) > > fetchFromServer is checking only if the response code is 4xx, and if so, it > is removing the access token from the store. This seems right for 401 or 403 > return codes, perhaps, but not for 404. The behavior for an end user would > then be that they have to do the OAuth dance again next time the gadget tries > to access a resource. > > The proposal is to change the current implementation to look explicitly for > 401 or 403 response codes in fetchFromServer instead of looking for any 4xx. > > Any other recommendations on what the behavior should be are welcome. > > > This addresses bug SHINDIG-1711. > https://issues.apache.org/jira/browse/SHINDIG-1711 > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/BasicOAuth2Request.java > 1292022 > > Diff: https://reviews.apache.org/r/3987/diff > > > Testing > ------- > > Built and ran existing JUnits. > > > Thanks, > > Stanton > >
