Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Link multiple auth providers #3

Open
deg opened this issue Sep 25, 2017 · 7 comments
Open

Link multiple auth providers #3

deg opened this issue Sep 25, 2017 · 7 comments

Comments

@deg
Copy link
Owner

deg commented Sep 25, 2017

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:

  • Account merging. A user (as identified by his email address) may login via multiple auth providers (e.g. Google and Facebook). Our current behavior when this happens is to report an error "An account already exists with the same email address". Instead, we need to implement account linking, as described here: https://firebase.google.com/docs/auth/web/account-linking.

  • Unified login flow. (This is trickier, because it involves introducing GUI decisions into this library and/or figuring out the right hook points). Currently, applications using this library need to handle login via each auth provider. We should offer a single call that, e.g., pops up a set of buttons to let the user choose a desired auth provider.

@pbzdyl
Copy link
Contributor

pbzdyl commented Sep 27, 2017

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
Copy link
Owner Author

deg commented Sep 27, 2017 via email

@deg
Copy link
Owner Author

deg commented Oct 2, 2017

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
Copy link
Contributor

pbzdyl commented Oct 2, 2017

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
Copy link
Owner Author

deg commented Oct 2, 2017

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.

pbzdyl added a commit to pbzdyl/re-frame-firebase that referenced this issue Oct 2, 2017
@deg
Copy link
Owner Author

deg commented Oct 2, 2017

Thanks! I've committed your PR.

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

@pbzdyl
Copy link
Contributor

pbzdyl commented Oct 2, 2017

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants