Getting rid of the window.opener access requirement would indeed make this
change much simpler.
On Thu, Sep 20, 2012 at 10:31 PM, Franklin, Matthew B.
<mfrank...@mitre.org>wrote:
> 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
> >>
> >>
> >
>
>
