Hi Jesse, Sorry I havent gotten chance to review the patch but I agree sending 0 for moduleId in common container seems like a bug.
We havent moved to common container so I am honestly havent spend a lot of time tracing the impl. Maybe Mike and/ or John could help review the patch too. Thanks to Ryan and Stanton that already help in this thread discussion. - Henry On Wed, Oct 5, 2011 at 5:05 AM, Ciancetta, Jesse E. <jc...@mitre.org> wrote: > 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 >>> >>> > >
