Allow 3-Legged OAuth Requests when owner pages are secure

[beaton, 2009-11-06 01:06:43]

Needs a test case or two in OAuthRequestTest.java.  I'd suggest two test cases:

1) test approval process, then send data request with oauth state

2) test approval process, then send data request without oauth state.  (This
will hit the token store)

I think GadgetOAuthTokenStore.java needs a change similar to what you've made in
OAuthRequest.java.

http://codereview.appspot.com/149041/diff/1/2
File java/common/conf/shindig.properties (right):

http://codereview.appspot.com/149041/diff/1/2#newcode42
java/common/conf/shindig.properties:42: shindig.signing.ownerPageSecure=false
naming convention in this file suggest "shindig.signing.owner-page-secure".  

I think the documentation should be

"Set to true if you want to allow the use of 3-legged OAuth tokens when viewer
!= owner.  This setting is not recommeneded for pages that allow user-controlled
javascript, since that javascript could be used to make unauthorized requests on
behalf of the viewer of the page."

So maybe the parameter should be:

shindig.signing.enable-viewer-access-tokens=false

http://codereview.appspot.com/149041/diff/1/3
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthFetcherConfig.java
(right):

http://codereview.appspot.com/149041/diff/1/3#newcode39
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthFetcherConfig.java:39:
private static boolean ownerPageSecure = false;
I think you should go ahead and change the constructor interface rather than
using member injection.  It's OK to change this interface.

If people are using Guice injection, they'll get the right behavior without
changing code.

If people aren't using Guice injection, they'll need to make a minor change, but
I think that's OK.  Changing an interface to keep the code clean seems like a
good trade-off to me.  (Especially since this is not a wire-format change.)

http://codereview.appspot.com/149041/diff/1/4
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthRequest.java
(right):

http://codereview.appspot.com/149041/diff/1/4#newcode323
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthRequest.java:323:
if (!fetcherConfig.isOwnerPageSecure() && !pageOwner.equals(pageViewer)) {
I love that this config is per fetcherConfig; that means we can make it a
per-container option really easily!

[beaton, 2009-11-09 00:58:10]

http://codereview.appspot.com/149041/diff/3003/3004
File java/common/conf/shindig.properties (right):

http://codereview.appspot.com/149041/diff/3003/3004#newcode45
java/common/conf/shindig.properties:45: shindig.signing.ownerPageSecure=false
naming convention in this file suggests "shindig.signing.owner-page-secure". 
But I think a better name might be "shindig.signing.enable-viewer-access-tokens"

http://codereview.appspot.com/149041/diff/3003/3005
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/GadgetOAuthTokenStore.java
(right):

http://codereview.appspot.com/149041/diff/3003/3005#newcode62
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/GadgetOAuthTokenStore.java:62:
public GadgetOAuthTokenStore(OAuthStore store, GadgetSpecFactory specFactory,
OAuthFetcherConfig fetcherConfig) {
hmm.  my initial instinct was to say fetcherConfig should be a parameter on
getOAuthAccessToken, so that fetcher configs can vary per request...  Does that
seem reasonable?

http://codereview.appspot.com/149041/diff/3003/3005#newcode116
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/GadgetOAuthTokenStore.java:116:
securityToken.getOwnerId().equals(securityToken.getViewerId())) {
my goodness.  maybe time to create a method for this instead?  That way we can
collapse these lines to "if (shouldUseToken(...)) {"

http://codereview.appspot.com/149041/diff/3003/3007
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthRequest.java
(right):

http://codereview.appspot.com/149041/diff/3003/3007#newcode330
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthRequest.java:330:
"(state owner=" + stateOwner + ", pageOwner=" + pageViewer + ')');
text should be pageViewer here

http://codereview.appspot.com/149041/diff/3003/3011
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth/OAuthRequestTest.java
(right):

http://codereview.appspot.com/149041/diff/3003/3011#newcode422
java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth/OAuthRequestTest.java:422:
assertEquals(OAuthError.UNAUTHENTICATED.toString(),
response.getMetadata().get("oauthError"));
hrm.  I hadn't expected this change.  It seems like the right thing to do, but
it is a change to the external makeRequest interface.

I think it's OK.  I can't imagine a real application depending on the old
behavior.

[Paul Lindner, 2009-11-09 05:54:51]

next patch coming up implementing all reccomendations.

http://codereview.appspot.com/149041/diff/3003/3004
File java/common/conf/shindig.properties (right):

http://codereview.appspot.com/149041/diff/3003/3004#newcode45
java/common/conf/shindig.properties:45: shindig.signing.ownerPageSecure=false
okay, changes to 

viewer-access-tokens-enabled

and isViewerAccessTokensEnabled


On 2009/11/09 00:58:11, beaton wrote:
> naming convention in this file suggests "shindig.signing.owner-page-secure". 
> But I think a better name might be
"shindig.signing.enable-viewer-access-tokens"

http://codereview.appspot.com/149041/diff/3003/3005
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/GadgetOAuthTokenStore.java
(right):

http://codereview.appspot.com/149041/diff/3003/3005#newcode62
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/GadgetOAuthTokenStore.java:62:
public GadgetOAuthTokenStore(OAuthStore store, GadgetSpecFactory specFactory,
OAuthFetcherConfig fetcherConfig) {
sounds good, changed..

On 2009/11/09 00:58:11, beaton wrote:
> hmm.  my initial instinct was to say fetcherConfig should be a parameter on
> getOAuthAccessToken, so that fetcher configs can vary per request...  Does
that
> seem reasonable?

http://codereview.appspot.com/149041/diff/3003/3005#newcode116
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/GadgetOAuthTokenStore.java:116:
securityToken.getOwnerId().equals(securityToken.getViewerId())) {
not sure where this would live?  fetcherConfig?  Willing to add this later..


On 2009/11/09 00:58:11, beaton wrote:
> my goodness.  maybe time to create a method for this instead?  That way we can
> collapse these lines to "if (shouldUseToken(...)) {"

http://codereview.appspot.com/149041/diff/3003/3007
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthRequest.java
(right):

http://codereview.appspot.com/149041/diff/3003/3007#newcode330
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthRequest.java:330:
"(state owner=" + stateOwner + ", pageOwner=" + pageViewer + ')');
fixed

On 2009/11/09 00:58:11, beaton wrote:
> text should be pageViewer here

[beaton, 2009-11-09 19:49:15]

http://codereview.appspot.com/149041/diff/3003/3005
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/GadgetOAuthTokenStore.java
(right):

http://codereview.appspot.com/149041/diff/3003/3005#newcode62
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/GadgetOAuthTokenStore.java:62:
public GadgetOAuthTokenStore(OAuthStore store, GadgetSpecFactory specFactory,
OAuthFetcherConfig fetcherConfig) {
Yeah, just double-checked our pipeline implementation.  GadgetOAuthTokenStore is
created per-container and is constant for the life of the server after that. 
OAuthFetcherConfig, OTOH, is created per-request.  So it would be a royal PITA
for us to pass in OAuthFetcherConfig at token store creation time, the scopes of
the two objects are too different.

But fetcher config passed to getOAuthAccessor is easy.

http://codereview.appspot.com/149041/diff/3021/2009#newcode75
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/GadgetOAuthTokenStore.java:75:
* - what HTTP method to use for request token and access token requests
OAUth -> OAuth

http://codereview.appspot.com/149041/diff/3021/2009#newcode116
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/GadgetOAuthTokenStore.java:116:
securityToken.getOwnerId().equals(securityToken.getViewerId())) {
we lost a check for securityToken.getOwnerId() != null.  Maybe swap the order
here to avoid any potential null pointer exception?

http://codereview.appspot.com/149041/diff/3021/2010
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthFetcherConfig.java
(right):

http://codereview.appspot.com/149041/diff/3021/2010#newcode38
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthFetcherConfig.java:38:
private static boolean viewerAccessTokensEnabled = false;
shouldn't be static, not thread safe.  Make it private final member variable
instead.

[Paul Lindner, 2009-11-09 20:29:05]

okay, fixed all noted issues.  I think we're good to go..

http://codereview.appspot.com/149041/diff/3021/2009
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/GadgetOAuthTokenStore.java
(right):

http://codereview.appspot.com/149041/diff/3021/2009#newcode75
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/GadgetOAuthTokenStore.java:75:
* - Information from the OAUth Fetcher config to determine if owner pages are
secure
fixed

On 2009/11/09 19:49:15, beaton wrote:
> OAUth -> OAuth

http://codereview.appspot.com/149041/diff/3021/2009#newcode116
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/GadgetOAuthTokenStore.java:116:
securityToken.getOwnerId().equals(securityToken.getViewerId())) {
fixed

On 2009/11/09 19:49:15, beaton wrote:
> we lost a check for securityToken.getOwnerId() != null.  Maybe swap the order
> here to avoid any potential null pointer exception?

http://codereview.appspot.com/149041/diff/3021/2010
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthFetcherConfig.java
(right):

http://codereview.appspot.com/149041/diff/3021/2010#newcode38
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthFetcherConfig.java:38:
private static boolean viewerAccessTokensEnabled = false;
true, true.  fixed.

On 2009/11/09 19:49:15, beaton wrote:
> shouldn't be static, not thread safe.  Make it private final member variable
> instead.

[beaton, 2009-11-09 21:03:59]

Looks great.  Thanks.