> 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?
>
> Henry Saputra wrote:
> 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.
>From Ryan, looks like the oauthpopup change is not in beta4. So the revert is
>safe and does not impact any release artifacts. Sorry for the false alarm.
- Henry
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7186/#review11730
-----------------------------------------------------------
On Sept. 20, 2012, 9:10 p.m., Dan Dumont wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7186/
> -----------------------------------------------------------
>
> (Updated Sept. 20, 2012, 9:10 p.m.)
>
>
> Review request for shindig, Paul Lindner, Henry Saputra, johnfargo, Matt
> Franklin, and Simon Hewett.
>
>
> Description
> -------
>
> NOTE:
> Original oauthpopup changes have been reverted in shindig trunk and attached
> as a single patched to the JIRA associated with this review.
> Using the changes in this review assumes you have first applied the patch in
> the JIRA.
> ---------------
>
> 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.
>
>
> This addresses bug SHINDIG-1864.
> https://issues.apache.org/jira/browse/SHINDIG-1864
>
>
> 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
>
>
