http://codereview.appspot.com/28075/diff/23/1015 File ../trunk/java/common/src/main/java/org/apache/shindig/auth/AuthenticationServletFilter.java (right):
http://codereview.appspot.com/28075/diff/23/1015#newcode124 Line 124: final HttpServletRequest wrapped; On 2009/03/24 15:12:34, beaton wrote:
don't need this variable
Done. http://codereview.appspot.com/28075/diff/23/1010 File ../trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthCommandLine.java (right): http://codereview.appspot.com/28075/diff/23/1010#newcode58 Line 58: * --bodySigning hash|legacy On 2009/03/24 15:12:34, beaton wrote:
add |none
Done. http://codereview.appspot.com/28075/diff/23/1010#newcode107 Line 107: bodySigningEnum = BodySigning.valueOf(paramLocation); On 2009/03/24 15:12:34, beaton wrote:
paramLocation? should be bodySigning.
doh! http://codereview.appspot.com/28075/diff/23/1014 File ../trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth/OAuthAuthenticationHandler.java (right): http://codereview.appspot.com/28075/diff/23/1014#newcode57 Line 57: private OAuthDataStore store; On 2009/03/24 15:12:34, beaton wrote:
these can be final
Done. http://codereview.appspot.com/28075/diff/23/1014#newcode88 Line 88: if (allowLegacyBodySigning && !request.getContentType().contains(OAuth.FORM_ENCODED)) { On 2009/03/24 15:12:34, beaton wrote:
Comment about meaning of allowLegacyBodySigning would be good here, or somewhere.
Done. http://codereview.appspot.com/28075/diff/23/1014#newcode101 Line 101: OAuthEntry entry = null; On 2009/03/24 15:12:34, beaton wrote:
This function is kind of long, could it be broken up a bit?
Split out entry and consumer lookup http://codereview.appspot.com/28075/diff/23/1014#newcode106 Line 106: throw new InvalidAuthenticationException("No oauth entry for token", null); On 2009/03/24 15:12:34, beaton wrote:
It's weird this isn't a checked exception.
It is, isnt it. Fixed. http://codereview.appspot.com/28075/diff/23/1014#newcode115 Line 115: OAuthConsumer authConsumer = store.getConsumer(consumerKey); On 2009/03/24 15:12:34, beaton wrote:
Should die a horrible and verbose death if authConsumer is null.
Done. http://codereview.appspot.com/28075/diff/23/1014#newcode134 Line 134: On 2009/03/24 15:12:34, beaton wrote:
I suspect people are going to need to customize the latter parts of
this
function, might be good to split it out into a protected method
SecurityToken getTokenFromVerifiedRequest(OAuthMessage msg, OAuthEntry
entry); Agreed, added consumer as well for more context http://codereview.appspot.com/28075/diff/23/1018 File ../trunk/java/social-api/src/test/java/org/apache/shindig/social/core/oauth/FakeOAuthRequest.java (right): http://codereview.appspot.com/28075/diff/23/1018#newcode61 Line 61: String url; On 2009/03/24 15:12:34, beaton wrote:
private static final
no need to be private in test land, and certainly not static. added final http://codereview.appspot.com/28075/diff/23/1018#newcode154 Line 154: static String getAuthorizationHeader(List<Map.Entry<String, String>> oauthParams) { On 2009/03/24 15:12:34, beaton wrote:
these methods can be private.
Done. http://codereview.appspot.com/28075/diff/23/1011 File ../trunk/java/social-api/src/test/java/org/apache/shindig/social/core/oauth/OAuthAuthenticationHanderTest.java (right): http://codereview.appspot.com/28075/diff/23/1011#newcode43 Line 43: * Verify behavior of 3-legged OAuth handler On 2009/03/24 15:12:34, beaton wrote:
These tests look great. Should change comment to mention that it
handles both
2-legged and 3-legged.
Done. http://codereview.appspot.com/28075
