Skip to content

[firebase_auth] implement missing auth functionality for web #1864

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jan 29, 2020
Merged

[firebase_auth] implement missing auth functionality for web #1864

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8f477b9
implement missing auth functionality for web
Jan 24, 2020
b055c5e
add changelog
Jan 24, 2020
fe31a76
bump pubspec version
Jan 24, 2020
4cea174
add missing method to changelog
Jan 24, 2020
109936b
fix interface
Jan 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/firebase_auth/firebase_auth_web/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 0.1.2

* Implement `fetchSignInMethodsForEmail`, `isSignInWithEmailLink`, `signInWithEmailAndLink`, and `sendLinkToEmail`.

## 0.1.1+4

* Prevent `null` users (unauthenticated) from breaking the `onAuthStateChanged` Stream.
Expand Down
38 changes: 22 additions & 16 deletions packages/firebase_auth/firebase_auth_web/lib/firebase_auth_web.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,10 +147,8 @@ class FirebaseAuthWeb extends FirebaseAuthPlatform {

@override
Future<List<String>> fetchSignInMethodsForEmail(String app, String email) {
// TODO(hterkelsen): Use `fetchSignInMethodsForEmail` once
// https://github.com/FirebaseExtended/firebase-dart/issues/272
// is resolved.
throw UnimplementedError('fetchSignInMethodsForEmail');
final firebase.Auth auth = _getAuth(app);
return auth.fetchSignInMethodsForEmail(email);
}

@override
Expand All @@ -162,8 +160,6 @@ class FirebaseAuthWeb extends FirebaseAuthPlatform {

@override
Future<PlatformIdTokenResult> getIdToken(String app, bool refresh) async {
// TODO(hterkelsen): `package:firebase` added `getIdTokenResult` in
// version 7.0.0. Use it here once that is published.
final firebase.Auth auth = _getAuth(app);
final firebase.User currentUser = auth.currentUser;
final firebase.IdTokenResult idTokenResult =
Expand All @@ -173,10 +169,8 @@ class FirebaseAuthWeb extends FirebaseAuthPlatform {

@override
Future<bool> isSignInWithEmailLink(String app, String link) {
// TODO(hterkelsen): Implement this once
// https://github.com/FirebaseExtended/firebase-dart/issues/273
// is resolved.
throw UnimplementedError('isSignInWithEmailLink');
final firebase.Auth auth = _getAuth(app);
return Future.value(auth.isSignInWithEmailLink(link));
}

@override
Expand Down Expand Up @@ -232,9 +226,20 @@ class FirebaseAuthWeb extends FirebaseAuthPlatform {
String androidPackageName,
bool androidInstallIfNotAvailable,
String androidMinimumVersion}) {
// TODO(hterkelsen): File issue with `package:firebase` to show
// `sendSignInLinkToEmail`.
throw UnimplementedError('sendLinkToEmail');
final firebase.Auth auth = _getAuth(app);
final actionCodeSettings = firebase.ActionCodeSettings(
url: url,
handleCodeInApp: handleCodeInApp,
iOS: firebase.IosSettings(
bundleId: iOSBundleID,
),
android: firebase.AndroidSettings(
packageName: androidPackageName,
installApp: androidInstallIfNotAvailable,
minimumVersion: androidMinimumVersion,
),
Comment on lines +233 to +240
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

);
return auth.sendSignInLinkToEmail(email, actionCodeSettings);
}

@override
Expand Down Expand Up @@ -280,9 +285,10 @@ class FirebaseAuthWeb extends FirebaseAuthPlatform {
@override
Future<PlatformAuthResult> signInWithEmailAndLink(
String app, String email, String link) async {
// TODO(hterkelsen): Use signInWithEmailLink once 7.0.0 of package:firebase
// is released.
throw UnimplementedError('signInWithEmailAndLink');
final firebase.Auth auth = _getAuth(app);
final firebase.UserCredential userCredential =
await auth.signInWithEmailLink(email, link);
Comment on lines +289 to +290
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes, that 1698 is going to be handy. Thanks for pointing it out!

return _fromJsUserCredential(userCredential);
}

@override
Expand Down
2 changes: 1 addition & 1 deletion packages/firebase_auth/firebase_auth_web/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
name: firebase_auth_web
description: The web implementation of firebase_auth
homepage: https://github.com/FirebaseExtended/flutterfire/tree/master/packages/firebase_auth/firebase_auth_web
version: 0.1.1+4
version: 0.1.2

flutter:
plugin:
Expand Down