OAuth body signing fixes

Add support for the oauth_body_hash signing proposal to OAuth

Add support for legacy body signing to remain compatible with opensocial client libraries

Fix consistency issues in OAuth handlers and add tests!

[beaton, 2009-03-20 23:13:04]

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"

[louiscryan, 2009-03-23 23:16:59]

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 {
On 2009/03/20 23:13:05, beaton wrote:
> Why is this package private?  Seems like it should be either private or
public.

Made private

http://codereview.appspot.com/28075/diff/1/7#newcode122
Line 122: HttpServletRequest wrapped;
On 2009/03/20 23:13:05, beaton wrote:
> private and final for some of these?

Class is now private. Made wrapper, rawStream final

http://codereview.appspot.com/28075/diff/1/7#newcode135
Line 135: public ServletInputStream getInputStream() throws IOException {
On 2009/03/20 23:13:05, beaton wrote:
> Might be nice to have the precondition check right at the top of the function:
> 
> if (reader != null) {
>    throw new IllegalStateException(...);
> }
> 
> Same for getReader

Done.

http://codereview.appspot.com/28075/diff/1/7#newcode140
Line 140: return rawStream.read();
On 2009/03/20 23:13:05, beaton wrote:
> Is the efficiency of this code acceptable?

Given that its reading out of a byte array, yes.

http://codereview.appspot.com/28075/diff/1/7#newcode157
Line 157: encoding = "UTF8";
On 2009/03/20 23:13:05, beaton wrote:
> prefer CharsetUtil.UTF8 to String charsets

Done.

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";
On 2009/03/20 23:13:05, beaton wrote:
> One of these days we need to stick some of these constants in common, or merge
> them into the oauth.net libraries.

No kidding.

http://codereview.appspot.com/28075/diff/1/5#newcode39
Line 39: public static final String FORM_URLENCODED =
"application/x-www-form-urlencoded";
On 2009/03/20 23:13:05, beaton wrote:
> This one already is in oauth.net, can use OAuth.FORM_ENCODED

Done.

http://codereview.appspot.com/28075/diff/1/5#newcode57
Line 57: }
On 2009/03/20 23:13:05, beaton wrote:
> 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;
> }
> 
> 
> 

Ok. I've just merged the two handlers into one, much cleaner. See
OAuthAuthenticationHandler

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
{
On 2009/03/20 23:13:05, beaton wrote:
> 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
> 

Done.

http://codereview.appspot.com/28075/diff/1/6#newcode60
Line 60: contentType = FORM_URLENCODED;
On 2009/03/20 23:13:05, beaton wrote:
> Given how much grief we've had about unspecified content-types lately, this
> gives me the willies.

Removed

http://codereview.appspot.com/28075/diff/1/6#newcode84
Line 84: throw new InvalidAuthenticationException(ioe.getMessage(), ioe);
On 2009/03/20 23:13:05, beaton wrote:
> 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.)

Generally cleaned up

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#newcode84
Line 84: throw new InvalidAuthenticationException(e.getMessage(), e);
On 2009/03/20 23:13:05, beaton wrote:
> better message here would be "error verifying OAuth request"

Done.

[louiscryan, 2009-03-23 23:17:20]

Updated patch

[beaton, 2009-03-24 15:12:33]

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;
don't need this variable

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
add |none

http://codereview.appspot.com/28075/diff/23/1010#newcode79
Line 79: Boolean.valueOf(params.get("--legacyBodySigning"));
Dead code.

http://codereview.appspot.com/28075/diff/23/1010#newcode107
Line 107: bodySigningEnum = BodySigning.valueOf(paramLocation);
paramLocation?  should be bodySigning.

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;
these can be final

http://codereview.appspot.com/28075/diff/23/1014#newcode88
Line 88: if (allowLegacyBodySigning &&
!request.getContentType().contains(OAuth.FORM_ENCODED)) {
Comment about meaning of allowLegacyBodySigning would be good here, or
somewhere.

http://codereview.appspot.com/28075/diff/23/1014#newcode101
Line 101: OAuthEntry entry = null;
This function is kind of long, could it be broken up a bit?

http://codereview.appspot.com/28075/diff/23/1014#newcode106
Line 106: throw new InvalidAuthenticationException("No oauth entry for token",
null);
It's weird this isn't a checked exception.

http://codereview.appspot.com/28075/diff/23/1014#newcode115
Line 115: OAuthConsumer authConsumer = store.getConsumer(consumerKey);
Should die a horrible and verbose death if authConsumer is null.

http://codereview.appspot.com/28075/diff/23/1014#newcode134
Line 134: 
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);

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;
private static final

http://codereview.appspot.com/28075/diff/23/1018#newcode154
Line 154: static String getAuthorizationHeader(List<Map.Entry<String, String>>
oauthParams) {
these methods can be private.

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
These tests look great.  Should change comment to mention that it handles both
2-legged and 3-legged.

[louiscryan, 2009-03-24 18:52:59]

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.

[louiscryan, 2009-03-24 18:53:29]

Updated patch

[beaton, 2009-03-24 20:26:23]

Looks good to me.  Thanks.

http://codereview.appspot.com/28075/diff/5001/5008
File
../trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth/OAuthAuthenticationHandler.java
(right):

http://codereview.appspot.com/28075/diff/5001/5008#newcode144
Line 144: throw new InvalidAuthenticationException("No oauth entry for token",
null);
Should include token in exception somewhere, probably in message.