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