Stanton has given this an initial nod code-wise, but it seems we're still struggling for consensus on the need to/use cases for making this change in the first place -- so before I continue working on this it would be very nice if at least one other person could confirm that we should indeed be making this change.
I'm as sure as I can be myself that this is a bug that should be fixed, but the fact that I've been working on this and sending mail to this list for over a month now and haven’t received anything from the community to confirm that this is something that should be fixed has me a little concerned that I'm somehow missing something. So just at the very highest level -- if I could get someone else to please confirm that common container using 0 for the moduleId in the security token for all gadgets is a bug that needs to be resolved I would very much appreciate it. Here is a link to the review for more detail: https://reviews.apache.org/r/1632/ Thanks! >-----Original Message----- >From: Jesse Ciancetta [mailto:jc...@mitre.org] >Sent: Tuesday, October 04, 2011 1:28 PM >To: shindig; Ciancetta, Jesse E.; Stanton Sievers >Subject: Re: Review Request: Common container currently doesnt include the >siteId (moduleId) in any of it's security token processing/handling > > > >> On 2011-10-04 16:28:14, Stanton Sievers wrote: >> > Overall this looks good. It does lead me to wonder what your use cases >are for the module ID. In the Shindig Java code I only ever see >org.apache.shindig.auth.SecurityToken.getModuleId() used in >org.apache.shindig.gadgets.http.AbstractHttpCache.getInstanceId(HttpReque >st). What are the intentions of the module ID and do you have new use cases >for its use? > >Thanks for taking a look Stanton! > >Here are the use cases I am aware of: > >-- Within Shindig oAuth tokens are stored on a per user/per gadget instance >basis, and moduleId is used as the gadget instance identifier -- see >BasicOAuthStore.makeBasicOAuthStoreTokenIndex(...) > >-- I've seen samples (although I cant seem to find them any longer) of using >the moduleId to scope appdata to a particular instance of a gadget. Since >appdata is stored on a per application basis as opposed to a per application >instance basis, if you want to store appdata for a particular application >instance you need some persistent identifier to use in the schema of the >appdata that you store -- and moduleId is how I think people typically do it. >If >anyone our there is doing this or can find the sample showing this usage >please reply. > >-- Third party sites interested in how many users are using their gadget. I >believe that the moduleId is one of the parameters passed to third parties >when shindig sends a signed makeRequest, so I could envision gadgets that >wanted to track how many instances are being used in the wild using signed >makeRequests to fetch their data -- then on their side the third party could >keep track of how many unique userId/moduleId combinations they see >coming across the wire to get a pretty good idea of how many gadget >instances are being used. > >If anyone has any other use cases please feel free to share. > > >> On 2011-10-04 16:28:14, Stanton Sievers wrote: >> > >http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/ex >amples/commoncontainer/assembler.js, line 28 >> > <https://reviews.apache.org/r/1632/diff/3/?file=46287#file46287line28> >> > >> > I'd rather we not set this by default and if we do, I'd rather it be a >> > bit >bigger, e.g., 60 seconds or more. > >Agreed -- I've got a comment above this saying it is only there to facilitate >testing and shouldn't be included in a final patch. > > >- Jesse > > >----------------------------------------------------------- >This is an automatically generated e-mail. To reply, visit: >https://reviews.apache.org/r/1632/#review2310 >----------------------------------------------------------- > > >On 2011-09-28 19:36:42, Jesse Ciancetta wrote: >> >> ----------------------------------------------------------- >> This is an automatically generated e-mail. To reply, visit: >> https://reviews.apache.org/r/1632/ >> ----------------------------------------------------------- >> >> (Updated 2011-09-28 19:36:42) >> >> >> 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/features/src/main/javascript/ >features/container/service.js 1176946 >> >http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/ >features/container.util/util.js 1176946 >> >http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/ >features/container/container.js 1176946 >> >http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/ex >amples/commoncontainer/assembler.js 1176946 >> >http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/ >features/container.gadget/gadget_site.js 1176946 >> >http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/f >eatures/container/gadget_site_test.js 1176946 >> >http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/f >eatures/container/service_test.js 1176946 >> >http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/ >org/apache/shindig/gadgets/servlet/GadgetsHandler.java 1176946 >> >http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/ >org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java 1176946 >> >http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/ >org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java 1176946 >> >> Diff: https://reviews.apache.org/r/1632/diff >> >> >> Testing >> ------- >> >> >> Thanks, >> >> Jesse >> >>
