----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2396/ -----------------------------------------------------------
(Updated 2011-10-19 18:38:20.724229) Review request for shindig, Matt Marum, Ryan Baxter, Eric Woods, li xu, Jesse Ciancetta, and Stanton Sievers. Changes ------- Adding the dev list. Was talking this over with Jesse on the phone. I've removed the minimum time check on the token's expiration time, as it could potentially cause problems. I'm not convinced that the thing the minimum check prevented against was a real problem. The old calculation was to ensure that the current time was between the earliest possible creation time and the maximum expires time. Before any of this patch, the minimum time was the (timestamp - clock skew) before this latest update the minimum time was (expiry - TTL - clock skew). Now there is no minimum time check, we just ensure the current time is less than (expiry + clock skew). Please let me know if you think there will be an issue with this. 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. Diffs (updated) ----- 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
