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