> On 2011-12-20 22:27:52, Henry Saputra wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/commoncontainer/assembler.js, > > line 28 > > <https://reviews.apache.org/r/1632/diff/4/?file=64700#file64700line28> > > > > Do you need to take out this line for the common container test example?
Yup -- I've got a comment above it saying it is only there to facilitate testing and shouldn't be included in a final patch. This patch and review are really more just to have people look at and validate the approach before I spend more time finishing up the final bits -- this definitely isn't ready to commit as is (although it should be close enough to trunk to apply to a local deployment and play around with if desired). > On 2011-12-20 22:27:52, Henry Saputra wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/gadget_site_test.js, > > line 76 > > <https://reviews.apache.org/r/1632/diff/4/?file=64705#file64705line76> > > > > I dont think we need to add 's' to the end of metadata? I think I'd added an 's' to the getGadgetToken function (since it returns multiple tokens) so I also added the 's' to getGadgetMetadata too just to keep it consistent (since it returns multiple metadata objects) -- it's not a big deal to me though and would be happy to roll those changes back. > On 2011-12-20 22:27:52, Henry Saputra wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java, > > line 573 > > <https://reviews.apache.org/r/1632/diff/4/?file=64707#file64707line573> > > > > Where does the code change the common container that adds module id at > > the end of the gadget url? The osapi.container.Service.prototype.getGadgetTokens (line 215 of service.js in my patch) takes a request object as one of its parameters which it sends to shindig with the token request -- that request object needs to have the gadgets that we're requesting metadata for specified in the gadgetUrl:moduleId format. You can look at the new osapi.container.Service.prototype.getGadgetToken function (line 313 of service.js in my patch) to see an example of calling getGadgetTokens with the properly formatted request object. Also -- looking back over this code now reminds me why I renamed the existing getGadgetToken function (which returned multiple tokens) to getGadgetTokens -- it was because I wanted to add a function that actually returned a single gadget token (to be used to fetch a single token during gadget rendering if needed). And since I renamed the getGadgetToken function to getGadgetTokens I also renamed the getGadgetMetadata to getGadgetMetadatas for consistency. - Jesse ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1632/#review4021 ----------------------------------------------------------- On 2011-12-14 21:13:40, Jesse Ciancetta wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/1632/ > ----------------------------------------------------------- > > (Updated 2011-12-14 21:13:40) > > > Review request for shindig. > > > Summary > ------- > > Common container currently doesn't include the siteId (moduleId) in any of > it's security token processing/handling (all security tokens in common > container currently get minted with a moduleId of 0). > > This patch is a first (rough) cut at getting moduleId's into security tokens. > I am posting it somewhat prematurely to solicit feedback before I invest any > more time in finishing up the last bits. Here are the things that I know are > still left to do: > > -- Update unit tests on both the JS and Java side -- currently I've been > building and deploying with skipTests=true... > -- Figure out a strategy for dealing with preloaded gadgets. The current > auth-refresh process maintains tokens for preloaded gadgets, however the > preoad JS functions just take a gadgetUrl so there is no concept of a siteId > (moduleId) for them at this time. > -- Figure out how to get the token that is included in the original iframe to > include a moduleId. I think the token in the iframe likely comes back in the > metadata request (although I haven't looked yet to verify), which means that > the call to the metadata service would likely need to include the moduleId as > well. > > I'd greatly appreciate any comments people have on the patch and strategies > for dealing with the outstanding issues noted above. > > Thanks! > > > This addresses bugs RAVE-198 and SHINDIG-1611. > https://issues.apache.org/jira/browse/RAVE-198 > https://issues.apache.org/jira/browse/SHINDIG-1611 > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/commoncontainer/assembler.js > 1214246 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_site.js > 1214246 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/util.js > 1214246 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js > 1214246 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/service.js > 1214246 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/gadget_site_test.js > 1214246 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/service_test.js > 1214246 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java > 1214246 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java > 1214246 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java > 1214246 > > Diff: https://reviews.apache.org/r/1632/diff > > > Testing > ------- > > > Thanks, > > Jesse > >
