Sorry guys for late reply, just got back from trip. I'll take a look at the patch flow again.
Meanwhile we can just keep the patch since no one has raised concern or complained about it. - Henry On Tue, May 29, 2012 at 9:10 AM, Rich Thompson <[email protected]> wrote: > How would it find the OAuth credentials to use if it doesn't authorize to > the Shindig server? > > [image: Inactive hide details for "Henry Saputra" ---05/29/2012 12:06:41 > PM---> On 2012-05-25 07:11:50, Henry Saputra wrote: > > Sorry]"Henry > Saputra" ---05/29/2012 12:06:41 PM---> On 2012-05-25 07:11:50, Henry > Saputra wrote: > > Sorry for late reply but this patch basically cha > > From: "Henry Saputra" <[email protected]> > To: Dan Dumont/Westford/IBM@Lotus, "Stanton Sievers" <[email protected]>, > Brian Lillie/Austin/IBM@IBMUS, "Ryan Baxter" <[email protected]>, > Cc: "Xiao Feng Yu" <[email protected]>, "Henry Saputra" < > [email protected]>, "shindig" <[email protected]> > Date: 05/29/2012 12:06 PM > Subject: Re: Review Request: Add OAuth support in content proxy to access > protected resources with content > ------------------------------ > > > > > > > On 2012-05-25 07:11:50, Henry Saputra wrote: > > > 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? > > > > Stanton Sievers wrote: > > 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. > > > > Henry Saputra wrote: > > 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. > > > > > > Stanton Sievers wrote: > > 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. > > 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? > > > - Henry > > > ----------------------------------------------------------- > > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5112/#review8097 > ----------------------------------------------------------- > > > > On 2012-05-22 18:20:43, Xiao Feng Yu wrote: > > > > ----------------------------------------------------------- > > This is an automatically generated e-mail. To reply, visit: > > > https://reviews.apache.org/r/5112/ > > > ----------------------------------------------------------- > > > > (Updated 2012-05-22 18:20:43) > > > > > > Review request for shindig, Ryan Baxter, Dan Dumont, Stanton Sievers, > and Brian Lillie. > > > > > > Summary > > ------- > > > > > 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. > > > > > > This addresses bug Shindig-1773. > > https://issues.apache.org/jira/browse/Shindig-1773 > > > > > > > Diffs > > ----- > > > > http://svn.apache.org/repos/asf/shindig/trunk/config/container.js 1341572 > > > http://svn.apache.org/repos/asf/shindig/trunk/config/oauth2.json 1341572 > > > > http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/commoncontainer/gadgetCollections.json > 1340019 > > > > http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/oauth2/oauth2_spring_proxy.xml > PRE-CREATION > > > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/core.io/io.js > 1341572 > > > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js > 1341572 > > > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyServlet.java > 1341572 > > > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriBase.java > 1341572 > > > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriManager.java > 1341572 > > > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriCommon.java > 1341572 > > > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java > 1341572 > > > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyServletTest.java > 1341572 > > > > http://svn.apache.org/repos/asf/shindig/trunk/java/server-resources/src/main/webapp/WEB-INF/web.xml > 1341572 > > > > > Diff: https://reviews.apache.org/r/5112/diff > > > > > > Testing > > ------- > > > > > > Thanks, > > > > Xiao Feng > > > > > > >
