> On May 22, 2013, 12:55 a.m., Stanton Sievers wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/conf/shindig.properties, > > lines 228-229 > > <https://reviews.apache.org/r/11299/diff/2/?file=295178#file295178line228> > > > > 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? > > Dan Dumont wrote: > I agree. If this is a List, then it should keep trying to find a > supported algorithm until the list is exhausted. Otherwise, I think the > property would be better defined as a single value. > > Rich Thompson wrote: > The choice of list was driven by the need to enhance the OAuth support. > The OAuth spec says the the Provider specifies in its metadata what > algorithms (ordered list) are supported, but my quick read said that Shindig > just ignored this and always used SHA-1. This needs to be enhanced (separate > effort, not currently planned) to determine the first preferred consumer > algorithm that is in the Provider list. One reason not to push this too hard > now is the lack of complaints about the behavior ... pretty clear all > Providers people have tried support SHA-1. > > Ryan Baxter wrote: > Rich the algorithm used is specified in the oauth.json file in shindig. > I know SmartCloud only supports plain text, I ran into that a while ago.
the setting is telling user what sha algorithms can be supported. and the first one is the sha algorithms in using. > On May 22, 2013, 12:55 a.m., Stanton Sievers wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/crypto/BasicBlobCrypter.java, > > line 149 > > <https://reviews.apache.org/r/11299/diff/2/?file=295180#file295180line149> > > > > Most of the Shindig code base uses the Splitter.on() method to do this > > type of work. Fixed > On May 22, 2013, 12:55 a.m., Stanton Sievers wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/crypto/BasicBlobCrypter.java, > > line 156 > > <https://reviews.apache.org/r/11299/diff/2/?file=295180#file295180line156> > > > > Can hmacType be annotated with @Nullable? Fixed > On May 22, 2013, 12:55 a.m., Stanton Sievers wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/crypto/BasicBlobCrypter.java, > > line 178 > > <https://reviews.apache.org/r/11299/diff/2/?file=295180#file295180line178> > > > > Can hmacType be annotated with @Nullable? Fixed > On May 22, 2013, 12:55 a.m., Stanton Sievers wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/util/GenericDigestUtils.java, > > line 35 > > <https://reviews.apache.org/r/11299/diff/2/?file=295183#file295183line35> > > > > 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? private static byte[] digest(MessageDigest digest, InputStream data)in DigestUtils is invisible, so have to re-writing it. > On May 22, 2013, 12:55 a.m., Stanton Sievers wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/crypto/BasicBlobCrypter.java, > > line 81 > > <https://reviews.apache.org/r/11299/diff/2/?file=295180#file295180line81> > > > > Small style nit. Is there a reason the @Named annotation isn't simply > > being imported to avoid the fully qualified reference here? not quite understand it, can you give more explaination? > On May 22, 2013, 12:55 a.m., Stanton Sievers wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/crypto/Crypto.java, > > line 52 > > <https://reviews.apache.org/r/11299/diff/2/?file=295181#file295181line52> > > > > Is this public only to support testing? Fixed > On May 22, 2013, 12:55 a.m., Stanton Sievers wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/crypto/Crypto.java, > > line 77 > > <https://reviews.apache.org/r/11299/diff/2/?file=295181#file295181line77> > > > > Is this public only to support testing? Fixed > On May 22, 2013, 12:55 a.m., Stanton Sievers wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/util/HMACType.java, > > line 26 > > <https://reviews.apache.org/r/11299/diff/2/?file=295184#file295184line26> > > > > Small nits on whitespace in this file fixed - Zhi Hong ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11299/#review20878 ----------------------------------------------------------- On June 6, 2013, 2:41 a.m., Zhi Hong Yang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/11299/ > ----------------------------------------------------------- > > (Updated June 6, 2013, 2:41 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/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 > >
