[firebase_auth] implement missing auth functionality for web #1864
Conversation
There was a problem hiding this comment.
@hterkelsen is going to love this. Thanks for the contribution @CDDelta!!
packages/firebase_auth/firebase_auth_web/lib/firebase_auth_web.dart
Outdated
Show resolved
Hide resolved
| iOS: firebase.IosSettings( | ||
| bundleId: iOSBundleID, | ||
| ), | ||
| android: firebase.AndroidSettings( | ||
| packageName: androidPackageName, | ||
| installApp: androidInstallIfNotAvailable, | ||
| minimumVersion: androidMinimumVersion, | ||
| ), |
There was a problem hiding this comment.
I was reading the documentation and... do we want to kick the user to the Android/iOS app from the web? This change kind of adds a requirement of setting up an iOSBundleID and an androidPackageName on a potentially web-only app.
Can these attributes be passed to the ActionCodeSettings conditionally? Does this handle null values gracefully, or does it cause an error?
There was a problem hiding this comment.
You're right, we shouldn't force users to set up those attributes.
I can change to make this optional but it would require modifying the plugin to not assert that these attributes are non-null and update the native code to gracefully handle non-null values too to keep the api behavior consistent on all platforms.
Is that the route we want to go down or is there an alternative?
There was a problem hiding this comment.
I'm deferring this one to @hterkelsen, he wrote the original implementation. IMO all these should be optional. I'm guessing in a web-only flow, those are never expected to be used, right?
There was a problem hiding this comment.
Since these are already required parameters in package:firebase_auth I think it makes sense to use them like this. All this is saying is if the Android or iOS app is installed then the sign in link will attempt to open the app,
There was a problem hiding this comment.
Maybe this is out of the scope of this PR but there is currently no way for users to not include an Android packageName or iOS bundleId.
Passing a null will cause the method to assert and passing an empty string will cause Firebase to throw an argument error. A possible hack might be to pass in a random string but that seems fragile and possibly dangerous.
I think it would be a good idea to allow users to opt out of certain platforms by passing null.
| final firebase.UserCredential userCredential = | ||
| await auth.signInWithEmailLink(email, link); |
There was a problem hiding this comment.
This might "Fail with an error if the email address is invalid or OTP in email link expires.". Docs.
Does that case need to be handled, or would that exception (?) propagate to the client app? (I've seen other places in the web plugin with a similar pattern... How do the Android/iOS versions behave in that case?)
There was a problem hiding this comment.
Right now it looks like the function will throw a FirebaseError instance to the client which we will want to map to a PlatformException but that would require #1698 to land first.
There was a problem hiding this comment.
Ah, yes, that 1698 is going to be handy. Thanks for pointing it out!
| @@ -1,3 +1,7 @@ | |||
| ## 0.1.2 | |||
|
|
|||
| * Implement `fetchSignInMethodsForEmail`, `isSignInWithEmailLink`, and `sendLinkToEmail`. | |||
There was a problem hiding this comment.
Can you add some unit tests? At least, it'd be interesting to have a test of the sad path of signInWithEmailLink, that codifies whatever is decided in this PR (whether it throws or catch + return null... whatever)
There was a problem hiding this comment.
Do we need to do that? I believe the firebase-dart package will properly throw the expected FirebaseError instances which #1698 will map to our expected PlatformExceptions.
|
@hterkelsen what are the things I need to do to get this change merged? :) |
|
Never mind. The failure is unrelated and is being fixed in a separate PR. |
Description
Implements the missing auth behavior for web except for
verifyPhoneNumber.Related Issues
#1682
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?