I haven't really looked at the tests. I'll review them once you make a decision about whether it's a good idea to move more functionality up into BaseOAuthRequestHandler.
http://codereview.appspot.com/28075/diff/1/7 File ../trunk/java/common/src/main/java/org/apache/shindig/auth/AuthenticationServletFilter.java (right): http://codereview.appspot.com/28075/diff/1/7#newcode120 Line 120: static class StashedBodyRequestwrapper extends HttpServletRequestWrapper { Why is this package private? Seems like it should be either private or public. http://codereview.appspot.com/28075/diff/1/7#newcode122 Line 122: HttpServletRequest wrapped; private and final for some of these? http://codereview.appspot.com/28075/diff/1/7#newcode135 Line 135: public ServletInputStream getInputStream() throws IOException { Might be nice to have the precondition check right at the top of the function: if (reader != null) { throw new IllegalStateException(...); } Same for getReader http://codereview.appspot.com/28075/diff/1/7#newcode140 Line 140: return rawStream.read(); Is the efficiency of this code acceptable? http://codereview.appspot.com/28075/diff/1/7#newcode157 Line 157: encoding = "UTF8"; prefer CharsetUtil.UTF8 to String charsets http://codereview.appspot.com/28075/diff/1/5 File ../trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth/BaseOAuthRequestHandler.java (right): http://codereview.appspot.com/28075/diff/1/5#newcode38 Line 38: public static final String OAUTH_BODY_HASH = "oauth_body_hash"; One of these days we need to stick some of these constants in common, or merge them into the oauth.net libraries. http://codereview.appspot.com/28075/diff/1/5#newcode39 Line 39: public static final String FORM_URLENCODED = "application/x-www-form-urlencoded"; This one already is in oauth.net, can use OAuth.FORM_ENCODED http://codereview.appspot.com/28075/diff/1/5#newcode57 Line 57: } I think you could move all of the OAuth crypto stuff into this class, so subclasses could just worry about the semantics of things like oauth_token vs xoauth_requestor_id vs opensocial_viewer_id. Something like this maybe? public OAuthMessage verifyOAuthRequest(HttpServletRequest request) { OAuthMessage msg = OAuthServlet.getMessage(request); String consumer = msg.getParameter("oauth_consumer"); String consumerSecret = oauthStore.getConsumerSecret(consumer); String token = msg.getParameter("oauth_token"); String tokenSecret = null; if (token != null) { tokenSecret = oauthStore.getTokenSecret(token); } ... create OAuthAccessor and OAuthConsumer ... validator.validateMessage(msg); verifyBodyHash(request, msg); return msg; } http://codereview.appspot.com/28075/diff/1/6 File ../trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth/OAuthAuthenticationHandler.java (right): http://codereview.appspot.com/28075/diff/1/6#newcode45 Line 45: public class OAuthAuthenticationHandler extends BaseOAuthRequestHandler { It might be nice to expose some configuration parameters to control signature verification policy, it'll make it a bit easier to experiment with deprecation. unsignedBodiesOk = true/false - if true, accept the OAuth core signature algorithm: non form-encoded bodies aren't integrity protected. legacySignedBodiesOk = true/false - if true, use verifySignatureLegacy http://codereview.appspot.com/28075/diff/1/6#newcode60 Line 60: contentType = FORM_URLENCODED; Given how much grief we've had about unspecified content-types lately, this gives me the willies. http://codereview.appspot.com/28075/diff/1/6#newcode84 Line 84: throw new InvalidAuthenticationException(ioe.getMessage(), ioe); instead of ioe.getMessage this should probably say something like "Error verifying OAuth signature". I don't know what ioe.getMessage will be, but probably not helpful. (Ditto for code below this.) http://codereview.appspot.com/28075/diff/1/12 File ../trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth/OAuthConsumerRequestAuthenticationHandler.java (right): http://codereview.appspot.com/28075/diff/1/12#newcode73 Line 73: if (!isValidOAuthRequest(requestMessage)) { Pretty sure the old code that was checking the token was a bad cut-and-paste job. http://codereview.appspot.com/28075/diff/1/12#newcode84 Line 84: throw new InvalidAuthenticationException(e.getMessage(), e); better message here would be "error verifying OAuth request" http://codereview.appspot.com/28075
