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

feat(authentication): support sign-in with redirect on Web #238

Merged
merged 8 commits into from Nov 30, 2022

Conversation

robingenz
Copy link
Member

@robingenz robingenz commented Nov 4, 2022

Pull request checklist

Please check if your PR fulfills the following requirements:

  • The changes have been tested successfully.
  • A changeset has been created (npm run changeset).

Close #224

@robingenz robingenz added this to the v1.3.0 milestone Nov 4, 2022
@robingenz robingenz self-assigned this Nov 4, 2022
@robingenz
Copy link
Member Author

@lincolnthree You can give it a try if you want:

npm i @capacitor-firebase/authentication@1.2.0-dev.57e621c.1667594791

Would be glad if you give feedback.

@lincolnthree
Copy link

Thanks @robingenz, I will try this!

@robingenz
Copy link
Member Author

@lincolnthree Have you already had the chance to test it?

@lincolnthree
Copy link

@robingenz Apologies. I have had a crazy few weeks. I haven't had a chance to test this yet. Is there a nightly build I can try?

@lincolnthree
Copy link

Out of curiosity, have you tried this yourself? Given the fact that (as I understand it)linkCurrentUserWithPopupOrRedirect() should never return for redirect authentication, does this work?

Unless the firebase API has changed or I am not understanding it, it should block forever and never return (and the page will redirect, cancelling the script).

Or... are you already handling this scenario and passing the authenticated user via FirebaseAuthentication.addListener('authStateChange') ? If so, I may be blind, I just couldn't find it in the source code.

This is the relevant example from the Firebase docs:

import { getAuth, getRedirectResult, GoogleAuthProvider } from "firebase/auth";

const auth = getAuth();
getRedirectResult(auth)
  .then((result) => {
    // This gives you a Google Access Token. You can use it to access Google APIs.
    const credential = GoogleAuthProvider.credentialFromResult(result);
    const token = credential.accessToken;

    // The signed-in user info.
    const user = result.user;
  }).catch((error) => {
    // Handle Errors here.
    const errorCode = error.code;
    const errorMessage = error.message;
    // The email of the user's account used.
    const email = error.customData.email;
    // The AuthCredential type that was used.
    const credential = GoogleAuthProvider.credentialFromError(error);
    // ...
  });

@robingenz
Copy link
Member Author

I don't know what I did there, half of the code is missing. 😆
Sorry for that, i will fix it and test it.

@robingenz
Copy link
Member Author

@lincolnthree I fixed and tested it and published a new version:

npm i @capacitor-firebase/authentication@1.2.0-dev.e7b5a2a.1668711492

The authStateChange listener is invoked after the redirect.

@lincolnthree
Copy link

lincolnthree commented Nov 18, 2022

@robingenz Haha yeah, it happens, and awesome! I will try this today or tomorrow. Sorry for the wait. Crazy week.

@lincolnthree
Copy link

One thing I'm curious about. How are error codes reported with this? It looks like it would just be an unhandled exception, or possibly be swallowed entirely, right?

Also, perhaps users should have some control over when the redirect result is checked, so it doesn't always get called if unnecessary? Maybe FirebaseAuthentication.checkRedirectResult() should be a more manual method call (but would still just trigger the auth event)?

Just thinking out loud.

https://firebase.google.com/docs/reference/js/v8/firebase.auth.Auth#getredirectresult

@robingenz
Copy link
Member Author

You are right. I improve that.

@robingenz
Copy link
Member Author

@lincolnthree I have implemented and tested your suggestions and am much happier with the new solution. Thanks for your ideas! Here's a dev version in case you want to try it out as well:

npm i @capacitor-firebase/authentication@1.2.0-dev.21cdd25.1669210347

I merge this PR in a timely manner.

@robingenz robingenz merged commit 9024eef into main Nov 30, 2022
@robingenz robingenz deleted the feat/issue-224 branch November 30, 2022 20:32
@github-actions github-actions bot mentioned this pull request Nov 30, 2022
Paso pushed a commit to ice-cream-club/capacitor-firebase that referenced this pull request Apr 19, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: support for loginWithRedirect()
2 participants