Review Board 1.7.22


Add OAuth support in content proxy to access protected resources with content

Review Request #5112 - Created May 14, 2012 and submitted

Xiao Feng Yu
Shindig-1773
Reviewers
shindig
brianlil, ddumont, rbaxter, ssievers
shindig
Couple of changes are included in this patch.
1) On the client side, the getProxyUrl is updated to add auth parameter to specify the auth scheme used, also check for the AUTHORIZATION and OAUTH_SERVICE setting and add them in proxy url.
2) On the server side, proxy servlet will pass additional HttpServletRequest to ProxyHandler to build the HttpRequest object, in the ProxyHandler additional information as security token, auth type, oauth service and gadget will be used to construct a HttpRequest to pass to the DefaultRequestPipeline for handling.

 
Review request changed
Updated (May 22, 2012, 6:20 p.m.)
updated the format
Ship it!
Posted (May 22, 2012, 9:27 p.m.)
LGTM.  The sparklr sample works great and helps to tie together a compelling use case for the API.
Posted (May 23, 2012, 1:52 a.m.)
I will do a more in depth review tomorrow, but I noticed one small nit in the sample gadget.
make the target="_blank" in this anchor tag so we dont navigate inside the iframe
Ship it!
Posted (May 23, 2012, 10:27 p.m.)
LGTM! Thanks.
Ship it!
Posted (May 23, 2012, 11:13 p.m.)
Committed revision 1342085.  I went ahead an made the target="_blank" change that Ryan mentioned.

Please close this review.
Posted (May 25, 2012, 7:11 a.m.)
Sorry for late reply but this patch basically change the open proxy mechanism to have authentication check. I believe there was a discussion in dev thread before about why it was intended as open proxy.

I believe you can achieve this via makeRequest with OAuth2? Why would you need OAuth for open proxy endpoint?
  1. I don't recall the exact discussion Henry, but I do recall talking about how to access protected images, which is what this patch is solving.
    
    The issue with makeRequest is that you cannot take the response and stick it in an img tag, for instance.  There are limitations on putting base64 encoded data directly into an img, mostly with IE, I think.  
    
    This patch is only changing the behavior of the content proxy if the extra parameters are added, otherwise, there is no impact.
  2. Apparently the discussion was about protecting the open proxy itself.
    
    I was just worry about adding default /gadgets/proxy to the auth servlet filter that could add impression that this endpoint is auth protected.
    
  3. Well, it technically is auth protected just by adding the endpoint to the filter mapping, right?  The auth filter just looks for a security token.  Is that overly concerning?
    
    The concern I remember hearing the most was that adding a security token to the content proxy was that you lose cacheability.  That's only an issue in this one case, otherwise, the requests look like they used to and will be cached the same way.
  4. The reason I ask bc adding the proxy filter to the auth filter means that it's trying to auth access to the Shindig auth servlet itself rather than the target proxied content, which I think is the intention of this patch?