----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11299/#review20878 -----------------------------------------------------------
In general I'd like to see some tests using the other supported HMAC types instead of only testing SHA-1. http://svn.apache.org/repos/asf/shindig/trunk/java/common/conf/shindig.properties <https://reviews.apache.org/r/11299/#comment43010> It is not clear to me why these are lists. The code that reads these seems to always take the first item in the list. Is there a reason to make this a comma-separated field? http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/crypto/BasicBlobCrypter.java <https://reviews.apache.org/r/11299/#comment43003> Small style nit. Is there a reason the @Named annotation isn't simply being imported to avoid the fully qualified reference here? http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/crypto/BasicBlobCrypter.java <https://reviews.apache.org/r/11299/#comment42998> I believe there is a @Nullable annotation that would be good to use on hmacType in this situation http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/crypto/BasicBlobCrypter.java <https://reviews.apache.org/r/11299/#comment42999> This is a common pattern between the two constructors. Can this code be shared? http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/crypto/BasicBlobCrypter.java <https://reviews.apache.org/r/11299/#comment43000> Most of the Shindig code base uses the Splitter.on() method to do this type of work. http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/crypto/BasicBlobCrypter.java <https://reviews.apache.org/r/11299/#comment43001> Can hmacType be annotated with @Nullable? http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/crypto/BasicBlobCrypter.java <https://reviews.apache.org/r/11299/#comment43002> Can hmacType be annotated with @Nullable? http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/crypto/Crypto.java <https://reviews.apache.org/r/11299/#comment43007> Is this public only to support testing? http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/crypto/Crypto.java <https://reviews.apache.org/r/11299/#comment43008> Is this public only to support testing? http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/util/GenericDigestUtils.java <https://reviews.apache.org/r/11299/#comment43009> This class seems to have a lot of duplication with DigestUtils, which it extends. Why can't this delegate to DigestUtils for the static methods instead of re-writing them? http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/util/GenericDigestUtils.java <https://reviews.apache.org/r/11299/#comment43004> Small style nit. Is there a reason the @Named annotation isn't simply being imported to avoid the fully qualified reference here? http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/util/HMACType.java <https://reviews.apache.org/r/11299/#comment43005> Small nits on whitespace in this file http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/common/crypto/CryptoTest.java <https://reviews.apache.org/r/11299/#comment43006> Can you explain why you are setting this? Shouldn't test tearDown be setting this back to the default? Otherwise will this cascade between tests? - Stanton Sievers On May 21, 2013, 8:27 a.m., Zhi Hong Yang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/11299/ > ----------------------------------------------------------- > > (Updated May 21, 2013, 8:27 a.m.) > > > Review request for shindig, Ryan Baxter, Dan Dumont, Stanton Sievers, and > Rich Thompson. > > > Description > ------- > > > the following setting are added to support different algorithms: > > 1) shindig.crypo.preferredHashAlgorithms = SHA, SHA-256, SHA-384, SHA-512 > > this setting is used to set string hash algorithm, all supported hash > Algorithms is listed and the first one is supported by default. > > 2) shindig.crypo.preferredHMACAlgorithms = > HMACSHA1,HMACSHA256,HMACSHA384,HMACSHA512 > > this setting is used to set string encrypt/decrypt algorithm, all supported > HMA SHA algorithms is listed and the first one is supported by default > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/conf/shindig.properties > 1484375 > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java > 1484375 > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/crypto/BasicBlobCrypter.java > 1484375 > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/crypto/Crypto.java > 1484375 > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/util/DigestType.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/util/GenericDigestUtils.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/util/HMACType.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodecTest.java > 1484375 > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenTest.java > 1484375 > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/common/crypto/BlobCrypterTest.java > 1484375 > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/common/crypto/CryptoTest.java > 1484375 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/DefaultGuiceModule.java > 1484375 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/HashLockedDomainService.java > 1484375 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthRequest.java > 1484375 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/handler/MacTokenHandler.java > 1484375 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/HashShaLockedDomainPrefixGenerator.java > 1484375 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth/testing/FakeOAuthServiceProvider.java > 1484375 > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth/OAuthAuthenticationHandler.java > 1484375 > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/test/java/org/apache/shindig/social/core/oauth/FakeOAuthRequest.java > 1484375 > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/test/java/org/apache/shindig/social/core/oauth/OAuthAuthenticationHanderTest.java > 1484375 > > Diff: https://reviews.apache.org/r/11299/diff/ > > > Testing > ------- > > Done. > > > Thanks, > > Zhi Hong Yang > >
