-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2396/#review2676
-----------------------------------------------------------


Just some nitpicks.  I'm good with the changes.  

I wish I knew if there was some special motivation in timestamping the blobs as 
the means of enforcing expiration.  If anybody has a problem with changing 
this, now would be a good time to speak up. :)

Matt


http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java
<https://reviews.apache.org/r/2396/#comment6035>

    Import flux.  Eclipse likes to reorder the imports when you organize them.  
Adds unnecessary changes.



http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenTest.java
<https://reviews.apache.org/r/2396/#comment6036>

    Import flux



http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java
<https://reviews.apache.org/r/2396/#comment6033>

    Unused import



http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java
<https://reviews.apache.org/r/2396/#comment6034>

    Unused import


- Matt


On 2011-10-19 19:46:39, Dan Dumont wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/2396/
> -----------------------------------------------------------
> 
> (Updated 2011-10-19 19:46:39)
> 
> 
> 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/common/src/main/java/org/apache/shindig/auth/AbstractSecurityToken.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/BasicSecurityToken.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/BlobCrypterSecurityToken.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/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/common/crypto/BlobCrypter.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/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenTest.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/common/crypto/BlobCrypterTest.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/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/main/java/org/apache/shindig/gadgets/oauth/OAuthClientState.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/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 
> 
> 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
> 
>

Reply via email to