nraboy / ng-cordova-oauth

AngularJS oauth library for use with Apache Cordova projects
https://www.thepolyglotdeveloper.com
MIT License
456 stars 199 forks source link

Prevent cannot GET /callback screen from showing #38

Closed timcosta closed 9 years ago

timcosta commented 9 years ago

When returning from the Google OAuth flow, there is a screen that says "Cannot GET /callback" that disappears quickly, however I would like to know if there is a way to make it disappear quicker or if it is possible to display custom text such as "Signing in..." in it's place. That screen looks like an error and isnt anything I would like my end users to see.

nraboy commented 9 years ago

Its a known issue and there really isn't a great solution for it. At least not one that I've found.

The error flash happens because http://localhost/callback loads quicker than the event listeners can fire. In the near future I plan to let users override the callback where you can put your own URL, but in this scenario you'd need a hosted solution.

So we are faced with the following:

  1. Use localhost and show a brief error screen
  2. Redirect to a nice page for a brief second, but have to worry about hosted pages

You have any ideas that could beat this?

timcosta commented 9 years ago

None off of the top of my head. I was hoping there was a way for Ionic/Cordova to capture calls to that /callback url and just display a given template/text instead, similar to how it serves pages from localhost/index.html.

nraboy commented 9 years ago

You can try it but I'm skeptical.

Just crack open the library file and change the localhost part to reflect what you're after.

If you decide to pursue this, please report back anything you find. Feel free to fork the project too.

Thanks,

timcosta commented 9 years ago

Was looking at your module code this morning, and realized that (at least for google) you no longer need the browser as soon as the load event fires and the url matches, as the event parameter of the event contains the browser url where the auth information is.

What I did was took the browserRef.close() out of the timeout, and made it the first line after if((event.url).indexOf("http://localhost/callback") === 0). This hides the error page much faster as the url parsing happens after the window dismissal, instead of the window dismissal waiting the short time for the parameter parsing and then the timeout.

Are there any consequences you can think of to dismissing the browser immediately like I have done?

nraboy commented 9 years ago

I had it like that in earlier versions of the library, but I kept getting many complaints that it was not working correctly. The promises in each function were giving less than desirable results.

Nice thoughts though. And I do appreciate your input :-)

timcosta commented 9 years ago

Very interesting. I will keep testing, but from what I have seen the promises are working properly. Any idea where the earlier versions of the library would have promise issues so I can focus my testing?

nraboy commented 9 years ago

I first made this change in https://github.com/nraboy/ng-cordova-oauth/commit/9a029ace009f1c18c21162e859e68c6dbd546070 based on user feedback. You may want to traverse back one or two commits than this to see how I had it.

Here are few issue tickets on the subject:

https://github.com/driftyco/ng-cordova/issues/544 https://github.com/driftyco/ng-cordova/issues/496

If your thoughts are different, please explain further.

Thanks,

timcosta commented 9 years ago

Interesting. My solution is different than the solution you had, and should prevent the issues mentioned above. However, my solution likely will not work for all of the different providers.

https://gist.github.com/tjsail33/dbca5880b5572ec7d1f0

Check out line 16. I moved that out of a timeout after line 27, and up to the top. This way, the callback error only shows for a split second as the window is loading, but not for long enough to see what it says.

nraboy commented 9 years ago

The problem with that is that you'll always trigger a reject promise. As soon as the browser.close is called, the exit listener is triggered.

If the exit listener is triggered before any other promise, then it is the assumption that the flow was canceled, which is not true.

Does that make sense?

timcosta commented 9 years ago

It makes sense, however that is not how it is working. I am using the code in that gist in my development right now, and google authentication is working successfully. the error promise is not resolving on exit.

this is likely a race condition, i realize. what if we used the removeEventListener("exit") just before the exit call?

nraboy commented 9 years ago

I believe that would be because you're fortunate enough to use a first world device and internet connection.

I'm betting you get a success or error response before the exit event processes. Switch to a slower device or internet connection, and it might not be the case.

I'm on your side. I never experienced any of the problems reported, but the new logic placed made more sense.

Let me know your thoughts.

timcosta commented 9 years ago

Race condition is definitely possible. I updated the gist above with a new suggestion for lines 16/17. Should prevent the triggering of the exit rejection.

nraboy commented 9 years ago

In theory that would fix the problem for any provider that uses implicit grants (response_type=token). Nice work. Have you tested this? If yes, want to make me a pull request so your name ends up on the project as a contributor? Of course only if you feel this is our best solution over what already exists.

Not sure what to do about the providers like Twitter that don't offer implicit grants.

timcosta commented 9 years ago

I'll submit a PR shortly.

timcosta commented 9 years ago

Not sure what to do about the other providers that don't used implicit grants, and my current project does not use any of those providers so i will not be experimenting as much as I was in this case.

nraboy commented 9 years ago

Thanks for contributing!

I'm going to close this ticket, but if you one day have a brilliant idea for the other grant types please feel free to contribute again.

Thanks,