deg / re-frame-firebase

Re-frame wrapper around Google's Firebase database
95 stars 32 forks source link

Link multiple auth providers #3

Open deg opened 7 years ago

deg commented 7 years ago

We support authorization with Google, Facebook, Twitter, or Github, but don't support some key functions that will make this feature more useful for real apps. These include:

pbzdyl commented 7 years ago

Hello @deg,

I have a proof of concept implemenation in my fork branch.

You can see a demo at https://proto-app.bzdyl.net/ (please check Outstanding issues below before testing).

The flow is quite simple:

Linking accounts works without issues in popup mode as the app stores pending credential in app-db. For redirect flow, the application would have to store it e.g. in session storage, as suggested by Firebase docs:

Redirect mode

This error is handled in a similar way in the redirect mode, with the difference that the pending credential has to be cached between page redirects (for example, using session storage).

This hasn't been implemented in my demo app.

What do you think of this approach?


Outstanding issues:

  1. firebase.auth.AuthError doesn't include credential field in externs causing it to be mangled in advanced compiler optimizations (thus my demo app uses only simple optimizations). We would either find a way to hint that this field should not be mangled by the compiler or open a ticket to fix it in externs.

  2. It looks that creating an account (signing for the first time) with Google overwrites existing account with the same email created with another provider. For example:

    • sign in with Facebook
    • check in Firebase console that a new account has been created with Facebook provider
    • sign out
    • sign in with Google with the same email address as Facebook account
    • check in Firebase console that the existing account now has only Google provider (you need to refresh whole page as refresh button on user list doesn't refresh provider column correctly)
    • sign out
    • try to sign in with Facebook again
    • an error is returned that an account with the email address already exists (unexpected as the account was initially created with Facebook)
    • sign in again with Google (flow to link account)
    • check Firebase console - the account now has two providers listed

    I think this is a Firebase issue. It doesn't occur if you repeat this scenario for example with Twitter and Facebook providers (Twitter provider doesn't overwrite account created by Facebook provider and vice versa). I will report a bug in Firebase.

deg commented 7 years ago

It will probably be a few days before I can look at this closely. I’ll try to review by early next week.

The Google issue you found seems strange. I don’t know enough about it to comment intelligently now.

Did you tackle just account merging half of this issue? Or also the unified login flow?

The merging part is definitely useful on its own. The unified login part would be wonderful to have, but it is harder because it requires the library to handle GUI issues that normally belong to the app. I played with Stormpath a bit last year, and they handled this by supplying default html templates that the app could override.

deg commented 7 years ago

I just tested by logging in via Twitter, logging out, then via Google, log out, and again via twitter.

The app got stuck with a spin wheel and the word "loading..." The console showed

core.cljs:3867 re-frame: no  :event  handler registered for:  
...

I think your approach is exactly right, but this needs a bit more work before it is ready.

pbzdyl commented 7 years ago

My fix supports only handling the linking - there is no GUI. I am not sure it is a good idea to force users of the library to depend on library's author choice of the GUI design and GUI components.

The error you have seen is probably because of a bug in my updated app - I have made some changes recently and the issue you've seen is caused in my application code, not in re-frame-firebase. I have already reverted it to the previous version.

deg commented 7 years ago

Ok.

When you are ready, please submit a PR. Even if there are still problems, I think it is worth having a partial solution.

Btw, re your point (1) above: It would be good to have a solution to this in place. externs breakages in advanced builds are definitely annoying to users! It's probably enough to just add firebase.auth.GoogleAuthProvider.credential to an externs.js. The one complication is that I originally based this library on mies, rather than lein-cljsbuild. I'm not 100% certain where to pass in the externs file. Perhaps in scripts/build.clj. Or, worst case, the project may need to be re-based onto more conventional tooling.

deg commented 7 years ago

Thanks! I've committed your PR.

I'm leaving this issue open because some points are still open.

pbzdyl commented 7 years ago

Thank you!