> 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.
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
-----------------------------------------------------------
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
>
>
