----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2396/#review2673 -----------------------------------------------------------
Ship it! Found a few more small items -- noted them in the review -- but otherwise LGTM. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/AbstractSecurityToken.java <https://reviews.apache.org/r/2396/#comment6024> Should you be using getMaxTokenTTL() here instead of the MAX_TOKEN_TTL value directly so that subclasses can override the TTL value? It seems like that was probably your intent since OAuthCallbackStateToken overrides this method and returns a different value than the default. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/crypto/BlobCrypter.java <https://reviews.apache.org/r/2396/#comment6028> Doesn't timestamp anymore http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/crypto/BlobExpiredException.java <https://reviews.apache.org/r/2396/#comment6030> /was until/was valid until/ http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthClientState.java <https://reviews.apache.org/r/2396/#comment6031> CLIENT_STATE_MAX_AGE_SECS is no longer used. - Jesse On 2011-10-19 18:40:11, Dan Dumont wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/2396/ > ----------------------------------------------------------- > > (Updated 2011-10-19 18:40:11) > > > Review request for shindig, Matt Marum, Ryan Baxter, Eric Woods, li xu, Jesse > Ciancetta, and Stanton Sievers. > > > Summary > ------- > > Long diffs... but before I make any more progress, I want to make sure that > everyone agrees that this cleanup is sound. > > Major things of note: > SecurityToken interface has token expiration built into it, yet crypters were > timestamping maps before encoding them and throwing exceptions during decrypt > if the timestamp was too old. This bypassed the meaning of the expiration > values in the tokens. It also seemed like the crypter was a terrible place > to be doing this. Now, all tokens that claim support for the expire key will > now have actual expire times instead of timestamps tacked on to the tokens. > > Expiration setting and checking has been moved into the token. > Codecs are now responsible for enforcing token expirations, not crypters. > AbstractSecurityToken has gotten a major face-lift. It now includes an Enum > for various popular token keys and many utility methods for handling token to > Map and Map to token conversions. > > Some OAuth code changes were necessary as they depended on former crypter > token timestamping. For the most part, I tried to leave the code alone > because I'm not familiar with it. This has meant that an awkward, but > functional implementation of an OAuthClientState is actually an extention of > AbstractSecurityToken... It seemed to almost make sense for all the work it > was doing... > > I'll make a jira before this gets submitted, as I'm not sure how close to the > final design this will be. > > > This addresses bug SHINDIG-1645. > https://issues.apache.org/jira/browse/SHINDIG-1645 > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/config/ShindigAuthConfigContributor.java > 1184753 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthCallbackState.java > 1184753 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthCallbackStateToken.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth/OAuthClientStateTest.java > 1184753 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth2/MockUtils.java > 1184753 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java > 1184753 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerTest.java > 1184753 > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth/OAuthSecurityToken.java > 1184753 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java > 1184753 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthClientState.java > 1184753 > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/common/testing/FakeGadgetToken.java > 1184753 > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/common/crypto/BlobCrypterTest.java > 1184753 > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/DefaultSecurityTokenCodecTest.java > 1184753 > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/UrlParameterAuthenticationHandlerTest.java > 1184753 > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenTest.java > 1184753 > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/crypto/BlobExpiredException.java > 1184753 > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/BasicSecurityTokenCodecTest.java > 1184753 > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodecTest.java > 1184753 > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/crypto/BlobCrypter.java > 1184753 > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/DefaultSecurityTokenCodec.java > 1184753 > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/SecurityToken.java > 1184753 > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/SecurityTokenCodec.java > 1184753 > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/crypto/BasicBlobCrypter.java > 1184753 > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java > 1184753 > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityToken.java > 1184753 > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BasicSecurityTokenCodec.java > 1184753 > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BasicSecurityToken.java > 1184753 > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/AnonymousSecurityToken.java > 1184753 > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/AbstractSecurityToken.java > 1184753 > > Diff: https://reviews.apache.org/r/2396/diff > > > Testing > ------- > > Tests have been updated with the interface changes and all currently pass. > > Tests that tested crypter expiration/timestamp enforcement have been removed. > > > Thanks, > > Dan > >
