> On 2011-09-27 17:46:35, Ryan Baxter wrote: > > A few general comments in addition to the ones below. > > > > In several files there are white spaces, please go through and remove > > these, its best to start with a clean slate. > > > > My major concern here is testing. It looks like you added a lot of new > > code but I see a little in the way of unit tests for it. Also in the unit > > tests you did modify I did not see any new tests exercising the new code > > paths. > > > > There is a lot of code here and it would be best to get as many eyes on > > this as possible.
Agreed. Will clean up the white space. Next drop (today or tomorrow) will have the unit tests in, this will be the last drop with major changes. Yes lot's of code, thanks for taking a look. > On 2011-09-27 17:46:35, Ryan Baxter wrote: > > /trunk/config/oauth2.json, line 1 > > <https://reviews.apache.org/r/1947/diff/3/?file=45512#file45512line1> > > > > If there any way we can combine this file with the oauth file that > > already exists in shindig. Does that make sense? I would rather not have > > another file which does the same thing. We considered using the existing oauth.json but there are three major reasons we added a new one 1) OAuth2 terminology is different, OAuth2 is a replacement for OAuth1 not additions to it. 2) There are some key differences in the structure as well. binding->client->provider which don't existing in oauth.json. Would probably just confuse people accustomed to oauth1, or worse break implementations that were already using it. 3) Since OAuth1 and OAuth2 persistence is separate they can both be changed in different ways. For instance, a user may have a custom OAuth1 persistence going to a database and have no need for oauth.json, but they would still need oauth2.json to kick the tires on OAuth2. > On 2011-09-27 17:46:35, Ryan Baxter wrote: > > /trunk/content/samplecontainer/examples/oauth2/oauth2_facebook.xml, line 20 > > <https://reviews.apache.org/r/1947/diff/3/?file=45514#file45514line20> > > > > It looks like this gadget is using jquery but I don't see a script > > import for it.... I guess that notation is strange. Honestly I just copied the gadget from the current oauth.xml and that's what they did. I will change it to avoid confusion. > On 2011-09-27 17:46:35, Ryan Baxter wrote: > > /trunk/content/samplecontainer/examples/oauth2/oauth2_windowslive.xml, line > > 14 > > <https://reviews.apache.org/r/1947/diff/3/?file=45516#file45516line14> > > > > The tabs here look to be off, should be 2 spaces will be fixed in next drop > On 2011-09-27 17:46:35, Ryan Baxter wrote: > > /trunk/content/samplecontainer/examples/oauth2/shindig_authorization.xml, > > line 42 > > <https://reviews.apache.org/r/1947/diff/3/?file=45517#file45517line42> > > > > Is this neccessary to point out ideally this the oauth2 provider patch > > will be there This demo was originally done to showcase a complete OAuth2 solution with the shindig consumer working with the shindig provider. Since the two are being reviewed separately we'll just remove these demo gadgets until both consumer and provider are available. > On 2011-09-27 17:46:35, Ryan Baxter wrote: > > /trunk/content/samplecontainer/examples/oauth2/shindig_authorization.xml, > > line 70 > > <https://reviews.apache.org/r/1947/diff/3/?file=45517#file45517line70> > > > > again not sure where the jquery is coming from addressed above > On 2011-09-27 17:46:35, Ryan Baxter wrote: > > /trunk/java/common/conf/shindig.properties, line 42 > > <https://reviews.apache.org/r/1947/diff/3/?file=45521#file45521line42> > > > > Comments on what these properties do would be good added for next drop > On 2011-09-27 17:46:35, Ryan Baxter wrote: > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/config/XhrwrapperConfigContributor.java, > > line 81 > > <https://reviews.apache.org/r/1947/diff/3/?file=45523#file45523line81> > > > > Log the exception This was based on the existing implementation for OAuth 1 and was duplicated for OAuth 2. I'm hesitant to change it without knowing why it was originally like this. > On 2011-09-27 17:46:35, Ryan Baxter wrote: > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/BasicOAuth2Request.java, > > line 306 > > <https://reviews.apache.org/r/1947/diff/3/?file=45528#file45528line306> > > > > Any need to check for null here? No. realRequest and securityToken are validated upon entry into the fetch() method. The only entry point. > On 2011-09-27 17:46:35, Ryan Baxter wrote: > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/BasicOAuth2Request.java, > > line 374 > > <https://reviews.apache.org/r/1947/diff/3/?file=45528#file45528line374> > > > > Could we just handle this exception in the outer try? I don't see this in my current code base anymore, so I must have agreed with you :) > On 2011-09-27 17:46:35, Ryan Baxter wrote: > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/BasicOAuth2Request.java, > > line 407 > > <https://reviews.apache.org/r/1947/diff/3/?file=45528#file45528line407> > > > > Any need to check for null here? Added checks for null fetcherConfig and getTokenStore. Everything else was checked on fetch() entry. > On 2011-09-27 17:46:35, Ryan Baxter wrote: > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/BasicOAuth2Request.java, > > line 445 > > <https://reviews.apache.org/r/1947/diff/3/?file=45528#file45528line445> > > > > Use Maps.newHashMap(...) Replaced all occurences. > On 2011-09-27 17:46:35, Ryan Baxter wrote: > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/BasicOAuth2Store.java, > > line 43 > > <https://reviews.apache.org/r/1947/diff/3/?file=45529#file45529line43> > > > > It would be nice if it is at all possible to use the existing oauth > > store Requires a new store. > On 2011-09-27 17:46:35, Ryan Baxter wrote: > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/GadgetOAuth2TokenStore.java, > > line 40 > > <https://reviews.apache.org/r/1947/diff/3/?file=45530#file45530line40> > > > > spacing here fixed in next drop > On 2011-09-27 17:46:35, Ryan Baxter wrote: > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/GadgetOAuth2TokenStore.java, > > line 45 > > <https://reviews.apache.org/r/1947/diff/3/?file=45530#file45530line45> > > > > It would be great if you could javadoc these public methods agreed > On 2011-09-27 17:46:35, Ryan Baxter wrote: > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/GadgetOAuth2TokenStore.java, > > line 97 > > <https://reviews.apache.org/r/1947/diff/3/?file=45530#file45530line97> > > > > Document the exception done > On 2011-09-27 17:46:35, Ryan Baxter wrote: > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/GadgetOAuth2TokenStore.java, > > line 138 > > <https://reviews.apache.org/r/1947/diff/3/?file=45530#file45530line138> > > > > Log the exception done > On 2011-09-27 17:46:35, Ryan Baxter wrote: > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/OAuth2Accessor.java, > > line 36 > > <https://reviews.apache.org/r/1947/diff/3/?file=45531#file45531line36> > > > > Please make sure the javadoc is complete agreed > On 2011-09-27 17:46:35, Ryan Baxter wrote: > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/OAuth2Arguments.java, > > line 36 > > <https://reviews.apache.org/r/1947/diff/3/?file=45532#file45532line36> > > > > Make sure the javadoc is complete agreed > On 2011-09-27 17:46:35, Ryan Baxter wrote: > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/OAuth2Arguments.java, > > line 61 > > <https://reviews.apache.org/r/1947/diff/3/?file=45532#file45532line61> > > > > I believe you can use the Maps API to create a new TreeMap fixed in next drop > On 2011-09-27 17:46:35, Ryan Baxter wrote: > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/OAuth2Error.java, > > line 31 > > <https://reviews.apache.org/r/1947/diff/3/?file=45533#file45533line31> > > > > Make sure the javadoc is complete agreed > On 2011-09-27 17:46:35, Ryan Baxter wrote: > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/OAuth2Error.java, > > line 101 > > <https://reviews.apache.org/r/1947/diff/3/?file=45533#file45533line101> > > > > At the very least you probably want to log this exception. fixed in next drop, static code analysis tool caught all these and I fixed them > On 2011-09-27 17:46:35, Ryan Baxter wrote: > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/OAuth2Message.java, > > line 35 > > <https://reviews.apache.org/r/1947/diff/3/?file=45536#file45536line35> > > > > Make sure there are descriptions in the javadoc agreed - Adam ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1947/#review2092 ----------------------------------------------------------- On 2011-09-23 19:54:08, li xu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/1947/ > ----------------------------------------------------------- > > (Updated 2011-09-23 19:54:08) > > > Review request for shindig. > > > Summary > ------- > > OAuth 2.0 client implementation in Apache Shindig from Adam Clarke, Eric > Woods, Jeff Hoy, Li Xu and Matthew Marum. > > > Documentation wiki: > http://docs.opensocial.org/display/OSD/OAuth+2.0+Consumer+Implementation+in+Apache+Shindig > > > JIRA issue: https://issues.apache.org/jira/browse/SHINDIG-1624 > > OAuth2 test gadgets are added to common container test page and can be tested > by using > http://localhost:8080/samplecontainer/examples/commoncontainer/index.html > > You will need OAuth2 service provider implementation to test following two > gadgets: > OAuth2 demo with Shindig Provider (Authorization Code) > OAuth2 demo with Shindig Provider (Client credential ) > > You will need to have your own google/facebook client to test following two > gadgets > OAuth2 demo with Google Provider > OAuth2 demo with Facebook Provider > Once it's registered with Google/Facebook, you can register the client-id/pwd > under > /config/oauth2.json > > > This addresses bug shindig-1624. > https://issues.apache.org/jira/browse/shindig-1624 > > > Diffs > ----- > > /trunk/config/oauth2.json PRE-CREATION > > /trunk/content/samplecontainer/examples/commoncontainer/gadgetCollections.json > 1173772 > /trunk/content/samplecontainer/examples/oauth2/oauth2_facebook.xml > PRE-CREATION > /trunk/content/samplecontainer/examples/oauth2/oauth2_google.xml > PRE-CREATION > /trunk/content/samplecontainer/examples/oauth2/oauth2_windowslive.xml > PRE-CREATION > /trunk/content/samplecontainer/examples/oauth2/shindig_authorization.xml > PRE-CREATION > > /trunk/content/samplecontainer/examples/oauth2/shindig_client_credentials.xml > PRE-CREATION > /trunk/features/src/main/javascript/features/core.io/io.js 1173772 > > /trunk/features/src/main/javascript/features/shindig.xhrwrapper/xhrwrapper.js > 1173772 > /trunk/java/common/conf/shindig.properties 1173772 > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/AuthType.java > 1173772 > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/config/XhrwrapperConfigContributor.java > 1173772 > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java > 1173772 > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultRequestPipeline.java > 1173772 > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpRequest.java > 1173772 > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/BasicOAuth2Accessor.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/BasicOAuth2Request.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/BasicOAuth2Store.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/GadgetOAuth2TokenStore.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/OAuth2Accessor.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/OAuth2Arguments.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/OAuth2Error.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/OAuth2FetcherConfig.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/OAuth2GadgetContext.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/OAuth2Message.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/OAuth2Module.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/OAuth2Request.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/OAuth2RequestException.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/OAuth2ResponseParams.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/OAuth2Store.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/OAuth2Token.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/OAuth2Utils.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/handler/AuthorizationEndpointResponseHandler.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/handler/ClientAuthenticationHandler.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/handler/GrantRequestHandler.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/handler/OAuth2HandlerError.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/handler/ResourceRequestHandler.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/handler/TokenEndpointResponseHandler.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/handler/sample/BasicAuthenticationHandler.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/handler/sample/BearerTokenHandler.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/handler/sample/ClientCredentialsGrantTypeHandler.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/handler/sample/CodeAuthorizationResponseHandler.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/handler/sample/CodeGrantTypeHandler.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/handler/sample/MACTokenHandler.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/handler/sample/OAuth2HandlerModule.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/handler/sample/StandardAuthenticationHandler.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/handler/sample/TokenAuthorizationResponseHandler.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/persistence/OAuth2Cache.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/persistence/OAuth2CacheException.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/persistence/OAuth2Client.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/persistence/OAuth2Encrypter.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/persistence/OAuth2EncryptionException.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/persistence/OAuth2PersistenceException.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/persistence/OAuth2Persister.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/persistence/OAuth2TokenPersistence.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/persistence/sample/InMemoryCache.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/persistence/sample/JSONOAuth2Persister.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/persistence/sample/NoOpEncrypter.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/persistence/sample/OAuth2GadgetBinding.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/persistence/sample/OAuth2PersistenceModule.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/persistence/sample/OAuth2Provider.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/sample/BasicOAuth2Message.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/sample/OAuth2MessageModule.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/preload/HttpPreloader.java > 1173772 > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/ProxyRenderer.java > 1173772 > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HttpRequestHandler.java > 1173772 > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/MakeRequestHandler.java > 1173772 > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/OAuth2CallbackServlet.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/ModulePrefs.java > 1173772 > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/OAuth2Service.java > PRE-CREATION > > /trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/OAuth2Spec.java > PRE-CREATION > > /trunk/java/gadgets/src/main/resources/org/apache/shindig/gadgets/oauth2/resource.properties > PRE-CREATION > > /trunk/java/gadgets/src/main/resources/org/apache/shindig/gadgets/oauth2/resource_en_US.properties > PRE-CREATION > > /trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/AuthTypeTest.java > 1173772 > > /trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/DefaultInvalidationServiceTest.java > 1173772 > > /trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/http/DefaultRequestPipelineTest.java > 1173772 > > /trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/RewriteModuleTest.java > 1173772 > > /trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/StyleTagProxyEmbeddedUrlsVisitorTest.java > 1173772 > > /trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/templates/tags/TemplateBasedTagHandlerTest.java > 1173772 > /trunk/java/server/pom.xml 1173772 > /trunk/java/server/src/main/webapp/WEB-INF/web.xml 1173772 > > /trunk/java/server/src/test/java/org/apache/shindig/server/endtoend/EndToEndServer.java > 1173772 > > Diff: https://reviews.apache.org/r/1947/diff > > > Testing > ------- > > Yes, passed all JUnit tests with mantis build. tested with new OAuth2 demo > pages. > > > Thanks, > > li > >
