----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4947/#review7480 -----------------------------------------------------------
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/BasicOAuth2Store.java <https://reviews.apache.org/r/4947/#comment16551> The call sequence here appears to be BasicOAuth2Store.removeToken( OAuth2Token ) that disaggregates into the removeToken( token parts ), where we do a cache lookup by parts to find a token (that we already had) so that we can do a remove by OAuth2Token. Seems like this could be streamlined with a remove method on the cache by a token's component parts (since the MapCache disassembles the token into its component parts to build the key) http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/OAuth2CallbackState.java <https://reviews.apache.org/r/4947/#comment16553> If there is no crypter, then the stateBlob won't be parsed. If there is no intent to handle a null crypter in the class, then it seems like that should be an exception condition. Seems like no crypter might be useful in some debug conditions http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/OAuth2CallbackState.java <https://reviews.apache.org/r/4947/#comment16552> There is no toString in the superclasses, so if there is no crypter, the string will be unusable http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/persistence/OAuth2Client.java <https://reviews.apache.org/r/4947/#comment16550> This changes the toString method result. I didn't see a corresponding change in this patch to the org.apache.shindig.gadgets.oauth2.persistence.OAuth2ClientTest class for test method testOAuth2Client_1 - BrianLillie On 2012-05-01 15:12:43, Adam Clarke wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4947/ > ----------------------------------------------------------- > > (Updated 2012-05-01 15:12:43) > > > Review request for shindig, Ryan Baxter and Brian Lillie. > > > Summary > ------- > > When the original OAuth2 Consumer was added the patch was quite large and it > was tough to get a comprehensive review. It was expected that there would > be a couple of revisions to address problems. > > This patch contains the changes for Jira 173. Due to time constraints it > also contains a number of other fixes/enhancements found in internal OAuth2 > Consumer reviews and testing. > > Some of these changes are interface changes/additions/deletions and could > effect custom consumer extensions. (Probably unlikely because nobody has > complained about them yet.) > > > Patch Includes: > 0) More standard formatting and checkstyle in modified files. > > 1) Jira 1732 for restricted OAuth2 endpoints. > > 2) Rework of OAuth2Cache and the default InMemoryCache. Tried to get too > cute with the original which made it very hard to implement. New version is > much easier to implement with your own Maps. > > 3) OAuth2 State encryption/decryption. Wasn't absolutely necessary for > OAuth2, but added it for consistency with OAuth1 and for the peace-of-mind of > security types. > > 4) BasicOAuth2Request no longer sends expired tokens. The previous impl > relied on the OAuth2 Service Provider returning 401 when a expired token was > used. This led to a nasty user experience when a service provider violated > the spec and returned something other than 401. OAuth2 Consumer no longer > sends expired tokens and reacts as if the server returned the 401. > > 5) OAuth2Persister had an unnecessary method to create a token. This is now > handled in the OAuth2Store. > > 6) Caching and OAuth2Store.init() fixes for better behavior in a clustered > environment. > > 7) Properly handle the URL %scheme% for the redirect uri (aka callback url) > > > This addresses bug SHINDIG-1732. > https://issues.apache.org/jira/browse/SHINDIG-1732 > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/conf/shindig.properties > 1332636 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/BasicOAuth2Accessor.java > 1332636 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/BasicOAuth2Request.java > 1332636 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/BasicOAuth2Store.java > 1332636 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/OAuth2Accessor.java > 1332636 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/OAuth2CallbackState.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/OAuth2CallbackStateToken.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/OAuth2Error.java > 1332636 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/OAuth2FetcherConfig.java > 1332636 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/OAuth2Message.java > 1332636 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/OAuth2Module.java > 1332636 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/OAuth2Store.java > 1332636 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/OAuth2Token.java > 1332636 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/handler/CodeAuthorizationResponseHandler.java > 1332636 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/handler/CodeGrantTypeHandler.java > 1332636 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/handler/OAuth2HandlerModule.java > 1332636 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/handler/TokenAuthorizationResponseHandler.java > 1332636 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/persistence/MapCache.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/persistence/OAuth2Cache.java > 1332636 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/persistence/OAuth2Client.java > 1332636 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/persistence/OAuth2Persister.java > 1332636 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/persistence/OAuth2TokenPersistence.java > 1332636 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/persistence/sample/InMemoryCache.java > 1332636 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/persistence/sample/JSONOAuth2Persister.java > 1332636 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/MakeRequestHandler.java > 1332636 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/OAuth2CallbackServlet.java > 1332636 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth2/MockUtils.java > 1332636 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth2/handler/CodeGrantTypeHandlerTest.java > 1332636 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth2/handler/TokenAuthorizationResponseHandlerTest.java > 1332636 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth2/persistence/sample/InMemoryCacheTest.java > 1332636 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth2/persistence/sample/JSONOAuth2PersisterTest.java > 1332636 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/MakeRequestHandlerTest.java > 1332636 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/resources/org/apache/shindig/gadgets/oauth2/oauth2_test.json > 1332636 > > Diff: https://reviews.apache.org/r/4947/diff > > > Testing > ------- > > All test cases pass. > > > Thanks, > > Adam > >
