Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[firebase_auth] Enable passwordless sign in #1159

Merged
merged 24 commits into from
Apr 5, 2019

Conversation

bizz84
Copy link
Contributor

@bizz84 bizz84 commented Feb 5, 2019

This PR adds the missing code to interface with the native APIs to send and handle email links.

This consists of the following Dart methods:

  • Future<dynamic> sendLinkToEmail({ @required String email, @required String url, @required bool handleCodeInApp, @required String iOSBundleID, @required String androidPackageName, @required bool androidInstallIfNotAvailable, @required String androidMinimumVersion, String dynamicLinkDomain })
  • Future<bool> isSignInWithEmailLink({String link})
  • Future<FirebaseUser> signInWithEmailAndLink({String email, String link})

This is enough to tap into the underlying iOS and Android APIs from Firebase Auth.

Done as part of this PR

  • Add plugin code to call native iOS FirebaseAuth APIs
  • Add plugin code to call native Android FirebaseAuth APIs
  • Expose APIs to Flutter via platform channels

I propose that this PR is merged so that client apps have access to the required native APIs.

Future work

Additional native code is needed to handle the incoming dynamic links and pass these back to Flutter.

I have implemented a demo app that shows a full email link activation flow here:

https://github.com/bizz84/passwordless_sign_in_firebase_flutter

However, client apps should not need to write additional native code to get this sign-in flow working.

Hence, I propose either of the following:

  • A new PR is created to provide all required native code and integrate it into firebase_auth, or
  • A separate Flutter plugin is created. This would not need to be an official plugin.

@bizz84
Copy link
Contributor Author

bizz84 commented Feb 12, 2019

@mklim @cyanglaz anyone could please review and merge?

Many Thanks.

@mklim
Copy link
Contributor

mklim commented Feb 15, 2019

Thanks for the contribution! I don't think I have enough context in the plugin to really review this. Added @kroikie and @collinjackson.

@bparrishMines
Copy link
Contributor

Hi @bizz84

Thank you for the contribution!

Is receiving a dynamic link the only native code that a user would have to include? There is already an official firebase_dynamic_links plugin that can probably handle that portion for you.

Also,

As we currently don't have a harness for testing the platform side of plugins, current and new features are not properly covered by tests. In order to maintain a quality bar for flutter/plugins we are going to avoid adding new features that aren't critical until they can be properly tested.

The test harness work is tracked in flutter/flutter#10824 and flutter/flutter#26080, your feedback on the testing work will be really appreciated (will it properly support the testing needs of this PR?) some design details should be available on flutter/flutter#10824 soon(contributions to that effort are welcomed!).

@bizz84
Copy link
Contributor Author

bizz84 commented Feb 17, 2019

Hi @bparrishMines,

Thank you for pointing me at the firebase_dynamic_links plugin.

I have tried to integrate this plugin in my code, and I believe the functionality offered is not sufficient in my use case.

The plugin can be used to retrieve a deep link url if the app was started from a dynamic link.

However, I also need to detect if dynamic links that are opened while the app is running. This is the most common use case with passwordless sign-in as the user goes from the app to the email client and back to the (still running) app.

My own implementation supports this by with a stream-based API on the Flutter side, so that I can handle each dynamic link as an event.

The current API for firebase_dynamic_links is future-based only.

It would be great if firebase_dynamic_links could support this use case with a stream-based API.

Could you suggest what would be the best way forward on this? I assume a PR to the plugins repo would be the way to go?

The platform code for firebase_dynamic_links is not straightforward to me. Perhaps someone from the team could help out?


Regarding your comments about flutter/flutter#10824, I agree that some testing for this would be appropriate.

I feel that end-to-end testing would be very valuable as a way of assuring that the entire flow works. At the same time, passwordless sign-in is a use case that requires project-level configuration on Firebase, and I'm not sure how this could be brought into a test harness.

Perhaps this can be explored in more detail once we have a candidate PR for firebase_dynamic_links?

@bparrishMines bparrishMines changed the title Enable passwordless sign in [firebase_auth] Enable passwordless sign in Feb 22, 2019
@cbenhagen
Copy link
Contributor

@bizz84 The uni_links plugin exposes a stream. Guess this could be used here.

@creativecreatorormaybenot
Copy link
Contributor

Should probably be done with EmailAuthCredential instead.

@bparrishMines
Copy link
Contributor

@bizz84 I was able to receive dynamic links even while the app was already open. I did this by using the WidgetsBindingObserver. I added an example to this PR.

The reason we wouldn't go with a stream is because receiving links on android doesn't use one. You just call getDynamicLink(). We simulate the same functionality on iOS.

We try to have the dart API match the Android and iOS APIs despite plugins being asynchronous.

@bizz84
Copy link
Contributor Author

bizz84 commented Apr 2, 2019

@bparrishMines I agree with your implementation. I have tested this with dynamic links and widgets binding on another project and it works well.

As a bonus, with this approach a Flutter-only solution can be implemented without any host code.

I think with the latest changes we should be able to merge this.

@bizz84
Copy link
Contributor Author

bizz84 commented Apr 2, 2019

@creativecreatorormaybenot the standard email & password auth call doesn't use EmailAuthCredential, so if we're not changing that I feel we shouldn't need to change this?

@bparrishMines
Copy link
Contributor

@bparrishMines publishable step failed. Plugin code doesn't require firebase_dynamic_links, but the example code does:

./script/check_publish.sh
Checking for changed packages from 5fba4906250082c0ec89ade148bdadcaf3144ca3
Detected changes in the following 1 package(s):
firebase_auth

Checking that firebase_auth can be published.
Suggestions:
* line 5, column 1 of example/lib/signin_page.dart: This package doesn't depend on firebase_dynamic_links.
    ╷
  5 │ import 'package:firebase_dynamic_links/firebase_dynamic_links.dart';
    │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    ╵

I forgot to add the dependency to the dev_dependencies: in the plugin level pubspec.yaml just added it with the last commit.

@bparrishMines
Copy link
Contributor

bparrishMines commented Apr 3, 2019

@bizz84 @creativecreatorormaybenot

From my understanding of the email and link flow, this PR deals with signing up a User, while EmailAuthProvider deals with Linking/re-authentication with email link.

I'm not an expert with using FirebaseAuth, but here is the documentation provided by the Firebase console when using email authentication; Android & iOS.

Both implementations seem to be needed.

In a separate PR we would probably just add another static method like the Android API.

class EmailAuthProvider {
  static const String providerId = 'password';

  static AuthCredential getCredential({
    String email,
    String password,
  }) {
    return AuthCredential._(providerId, <String, String>{
      'email': email,
      'password': password,
    });
  }

  static AuthCredential getCredentialWithLink({
    String email,
    String link,
  }) {
    return AuthCredential._(providerId, <String, String>{
      'email': email,
      'link': link,
    });
  }
}

Copy link
Contributor

@bparrishMines bparrishMines left a comment

Choose a reason for hiding this comment

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

LGTM since it works with firebase_dynamic_links.

We should also probably also get an LGTM from @kroikie since he is a code owner.

Copy link
Contributor

@collinjackson collinjackson left a comment

Choose a reason for hiding this comment

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

LGTM, I added some dartdocs and updated CHANGELOG/pubspec.yaml

I'd love to integration test this, because email is involved it may require out of process testing that can talk to other apps on the device. Support for that sort of test shouldn't hold back merging.

@collinjackson
Copy link
Contributor

We do need a unit test, though.

@collinjackson
Copy link
Contributor

After some debate I renamed the API sendSignInWithEmailLink to match Android. It's a bit more verbose but matches Android and anyone using it is going to be split onto multiple lines anyway. Feedback welcome.

@bparrishMines can you take a quick look at the tests I added and let me know if you agree that this change is ready to merge?

@collinjackson collinjackson added the submit queue The Flutter team is in the process of landing this PR. label Apr 4, 2019
@creativecreatorormaybenot
Copy link
Contributor

creativecreatorormaybenot commented Apr 4, 2019

@collinjackson Now, we are definitely missing the AuthCredential. It was updated to be the common way of signing in and linking and with the current way we only have an unneeded method signInWithEmailAndLink because that could be done using the credential and also no way to link.
We could obviously keep that method, however, I just think that the credentials are much nicer.
Anyway, we probably need a way to link. Either like this or using a credential. I could definitely help to quickly implement that.

@collinjackson
Copy link
Contributor

Thanks, @creativecreatorormaybenot.

Can you please update this PR to include an implementation of linking using a credential? I think we can keep the signInWithEmailAndLink but I don't feel strongly about it.

@creativecreatorormaybenot
Copy link
Contributor

creativecreatorormaybenot commented Apr 4, 2019

@collinjackson I am not exactly sure how to achieve it because it seems like the action code has to be extracted using Dynamic Links: https://github.com/firebase/firebase-js-sdk/blob/master/packages/auth/src/authcredential.js#L977

linkWithEmailAndLink would not be any problem to add though. However, I do not have access to update this pull request.
Parts of it are already in here. It is missing iOS and docs + exception handling.

It would be very helpful to know what the native libraries require, but as far as I can tell, there is no way to get access to that source code.

@collinjackson
Copy link
Contributor

@creativecreatorormaybenot if I go ahead and merge this PR and are you willing to add linkWithEmailAndLink in a follow-on PR?

@collinjackson
Copy link
Contributor

@creativecreatorormaybenot
Copy link
Contributor

@collinjackson Sure. However, I will just do it without the credential for now.
Started with something

@collinjackson collinjackson merged commit 121b9df into flutter:master Apr 5, 2019
julianscheel pushed a commit to jusst-engineering/plugins that referenced this pull request Mar 11, 2020
* Implement sendLinkToEmail, isSignInWithEmailLink and signInWithEmailAndLink in Flutter
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes feature flutterfire submit queue The Flutter team is in the process of landing this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants