Sounds good -- thanks Ryan. Just to clarify though -- I'm not necessarily suggesting that these changes actually get applied as is -- I'm really more looking to start a discussion around this as one possible solution to the problem. This is the simplest set of changes I could come up with to get moduleId's into common container, but with it comes some additional baggage in the form of new nested callbacks when navigating a gadget.
A better way to resolve it might be to refactor the rendering process further to do any potentially XHR causing work (like fetching gadget metadata and gadget tokens) in the same place during rendering to eliminate the deeper nest of chained callbacks that the current patch introduces, but of course that's more work with more significant changes and I'm not sure which way the community would prefer to go with this. Looking forward to feedback and discussion from Stanton and others. --Jesse From: Ryan J Baxter [mailto:rjbax...@us.ibm.com] Sent: Wednesday, September 28, 2011 5:55 PM To: dev@shindig.apache.org; Stanton Sievers Cc: Ciancetta, Jesse E. Subject: Re: Review Request: Common container currently doesnt include the siteId (moduleId) in any of it's security token processing/handling Jesse, I know Stanton has been looking at some of this code lately and might be a good person to take a look at your suggested changes. He is out on vaca until next Tuesday but I can see if he can take a look when he gets back. -Ryan Email: rjbax...@us.ibm.com Phone: 978-899-3041 developerWorks Profile From: "Jesse Ciancetta" <jc...@mitre.org> To: "shindig" <dev@shindig.apache.org>, "Jesse Ciancetta" <jc...@mitre.org>, Date: 09/28/2011 03:39 PM Subject: Re: Review Request: Common container currently doesnt include the siteId (moduleId) in any of it's security token processing/handling ________________________________________ ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1632/ ----------------------------------------------------------- (Updated 2011-09-28 19:36:42.867006) Review request for shindig. Changes ------- I've attached an updated patch which has been realigned with the current shindig trunk. More details can be found in the review description and previous comments. I would very much appreciate some feedback on this patch. Thanks! 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 (updated) ----- http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/service.js1176946 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/util.js1176946 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js1176946 http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/commoncontainer/assembler.js1176946 http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_site.js1176946 http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/gadget_site_test.js1176946 http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/service_test.js1176946 http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java1176946 http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java1176946 http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java1176946 Diff: https://reviews.apache.org/r/1632/diff Testing ------- Thanks, Jesse
