On 9/20/12 3:42 PM, "Henry Saputra" <hsapu...@apache.org> wrote:
>
>
>> On Sept. 20, 2012, 4:47 a.m., Henry Saputra wrote:
>> > Dan, could you tell a bit more details what the issue looks like with
>>3-legged OAuth 1.0a dance? It will help review the change.
>> >
>> > This needs to be fixed ASAP bc the oauthpopup change is part of beta4
>>release and if its broken we need to come up with beta5 quickly.
>>
>> Dan Dumont wrote:
>> Hey, thanks for helping with the review :)
>>
>> Let me start by describing what I saw before any of these changes
>>or the oauthpopup changes with locked domains in a deployment.
>> 1. Oauth request made by a gadget on locked.gadget.com through the
>>proxy, no oauth creds...
>> 2. The remote server sends back the appropriate response
>> 3. The proxy ads info to the response like the oauth authorize uri
>>with the authorize callback.
>>
>> 3's callback is generated by the shindig property
>>shindig.signing.global-callback-url which points to the unlocked server
>>domain. In the (now deleted) GadgetOAuthCallbackGenerator.java we would
>>take the callback endpoint generated from container.js
>>gadgets.uri.oauth.callbackTemplate and %host% is replaced with the
>>authority of the gadget on the locked domain and add it to a token.
>> (I will be updating this patch to delete the
>>gadgets.uri.oauth.callbackTemplate param in the container config, it's
>>not used anymore)
>>
>> 4. Encrypt the locked domain callback endpoint in a token and add
>>it to the callback url in 3 as a query param.
>> 5. Gadget gets request failure and uses the oauthpopup class to
>>open the authorize url (with callback
>>url?state={encrypted-callback-url-query-param}) window.open on locked
>>domain.
>> 6. Log in on remote service, press authorize, yadda yadda
>> 7. Redirected back to UNLCOKED domain (the one from
>>shindig.properties).
>> 8. The (now deleted) code in the servlet would see you have a
>>token, decrypt it, move all current query params to the decrypted
>>callback and send a redirect.
>> 9. Back to the same servlet on the LOCKED domain. Servlet serves
>>up js code that sets window.opener.someoauthproperty for some
>>makerequest oauth completion.
>>
>> Note here that window.opener is on the gadget's locked domain, so
>>this redirection madness was necessary to make sure that the
>>window.opener would be able to be written to.
>>
>>
>> After my changes to oauthpopup, everything was the same except for
>> 5. The container page on an unlocked domain sends the window.open
>>to the authorize url (with callback
>>url?state={encrypted-callback-url-query-param})
>> So during 9, the js code had access violation accessing a property
>>in window.opener (from locked domain to container domain)
>>
>> After these changes, we change
>> 4. do nothing
>> 5. changed as above
>> 8. do nothing
>> 9. Now on the unlocked domain, window.opener would now match
>>domains.
>>
>> Dan Dumont wrote:
>> After talking with Stanton, we're kind of weary of how large this
>>change is getting because we found a slight wrinkle in the redirect in
>>the case where the container isn't on the same domain as the shindig
>>server.
>>
>> Typically one would have a proxy set up so that rpc stuff would
>>work... so we were thinking we could take the container host in the
>>security token and use that to generate a url to the oauth endpoint via
>>the container's proxy (you would have to edit the shindig.properties
>>value with any proxy you are using on that url).
>>
>> I could add the comments describing this setup and create
>>warnings/errors/exceptions for values for the container in the security
>>token that cannot be handled so that admins can easily understand the
>>failures and correct them. And also write all this up in UPGRADING...
>> Or I can just rip all this out now and revert my oauthpopup changes
>>and save this for post 2.5. I had hoped that this would be a simpler
>>change and a stepping stone to better, more fluid handling of the oauth
>>experience... but the change has become larger than I had planned....
>>Though removing all of these classes does cut down on complexity a bit
>>and remove 1 redirect from the equation. What do you think?
>
>Dan, the flow seems need a lot of testing and configuration change =(
>The problem is that now the total fixes for the oauthpopup changes are
>spread into 3 different change requests (the original change is already
>released in beta4)
>
>Since we are trying to close all the missing implementations and critical
>bug for 2.5.0 release, I have to Vote for -1 for these changes and ask to
>revert all the oauthpopup changes back.
>We need to ask Ryan to spin 2.5.0-beta5 immediately since its breaking
>existing release.
>
>Lets proposed all the required changes for this enhancement in one CR
>with all the required changes, and have enough time to test the flow in
>actual implementations.
>Otherwise it could become hard to understand code like the ones
>contributed by Googlers to just work for their container.
I am not an OAuth expert (though I am familiar) and I am even less of one
in Shindig's implementation, but it seems to me that we might be making
this more complicated than it needs to be. After looking through the
patch and some of the code, I think we can make this strategy work in all
cases, across all combinations of container & shindig domains with a
change in how we handle the callbacks. The whole purpose of setting the
value on the window.opener is that the oath verifier needs to be added to
the access token exchange request. If we just persisted this value with
the Request token in the OAuthStore when the OAuthCallbackServlet is
invoked, then we could have it readily available when the gadget goes to
make its next request for Oauth. From what I can tell from the Oauth 1.0a
spec, the verifier is valid for the life of the authorization, so I don't
see any issue in persisting it. This is a slightly bigger change, but
will make the whole process simpler.
I would be +0 for Dan continuing to pursue this for 2.5 and making changes
along the lines of what I suggested (or other better suggested path).
Since there is a lot of discussion about changing the whole Authorization
model in 3.0, I think these might be a series of short lived changes
anyway...
>
>
>- Henry
>
>
>-----------------------------------------------------------
>This is an automatically generated e-mail. To reply, visit:
>https://reviews.apache.org/r/7186/#review11730
>-----------------------------------------------------------
>
>
>On Sept. 20, 2012, 12:02 a.m., Dan Dumont wrote:
>>
>> -----------------------------------------------------------
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/7186/
>> -----------------------------------------------------------
>>
>> (Updated Sept. 20, 2012, 12:02 a.m.)
>>
>>
>> Review request for shindig, Paul Lindner, Henry Saputra, johnfargo,
>>Matt Franklin, and Simon Hewett.
>>
>>
>> Description
>> -------
>>
>> So I found this in our deployments for 1.0a where we have locked
>>domains turned on. From what I can tell, we would set the callback url
>>to the base one defined in shindig.properties and then encode a security
>>token with the locked domain callback url. When we got the callback on
>>the unlocked domain, we would redirect to the locked domain in the
>>encoded token.
>>
>> This appears to have been done because of some window.opener completion
>>code for makerequest which used to be set by the gadget (the gadget used
>>to open the window). Now the container opens the window and we don't
>>need all this redirection magic. So I've removed all the classes that
>>had anything to do with that (there were a few, but mostly well
>>contained).
>>
>> After digging through this code, I now fear for my sanity. Please,
>>please, PLEASE! code review this carefully. I am not an oauth expert.
>>
>>
>> Diffs
>> -----
>>
>>
>>http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/
>>org/apache/shindig/gadgets/oauth/GadgetOAuthCallbackGenerator.java
>>1387677
>>
>>http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/
>>org/apache/shindig/gadgets/oauth/OAuthCallbackGenerator.java 1387677
>>
>>http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/
>>org/apache/shindig/gadgets/oauth/OAuthCallbackState.java 1387677
>>
>>http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/
>>org/apache/shindig/gadgets/oauth/OAuthCallbackStateToken.java 1387677
>>
>>http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/
>>org/apache/shindig/gadgets/oauth/OAuthFetcherConfig.java 1387677
>>
>>http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/
>>org/apache/shindig/gadgets/oauth/OAuthRequest.java 1387677
>>
>>http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/
>>org/apache/shindig/gadgets/servlet/OAuthCallbackServlet.java 1387677
>>
>>http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/
>>org/apache/shindig/gadgets/uri/DefaultOAuthUriManager.java 1387677
>>
>>http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/
>>org/apache/shindig/gadgets/uri/OAuthUriManager.java 1387677
>>
>>http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/
>>org/apache/shindig/gadgets/uri/UriModule.java 1387677
>>
>>http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/
>>org/apache/shindig/gadgets/oauth/GadgetOAuthCallbackGeneratorTest.java
>>1387677
>>
>>http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/
>>org/apache/shindig/gadgets/oauth/GadgetTokenStoreTest.java 1387677
>>
>>http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/
>>org/apache/shindig/gadgets/oauth/OAuthFetcherConfigTest.java 1387677
>>
>>http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/
>>org/apache/shindig/gadgets/oauth/OAuthRequestTest.java 1387677
>>
>>http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/
>>org/apache/shindig/gadgets/servlet/OAuthCallbackServletTest.java 1387677
>>
>>http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/
>>org/apache/shindig/gadgets/uri/DefaultOAuthUriManagerTest.java 1387677
>>
>> Diff: https://reviews.apache.org/r/7186/diff/
>>
>>
>> Testing
>> -------
>>
>> Updated tests.
>> Removed obsolete tests.
>>
>>
>> Thanks,
>>
>> Dan Dumont
>>
>>
>
