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.
http://codereview.appspot.com/28075
