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

Conversation

@lukaspili
Copy link
Contributor

Description

Firebase auth is missing the method getIdTokenResult() which is available on native SDKs. It allows to retrieve the current token + other useful info like the token custom claims.

The PR includes the changes:

  • Add new public API on dart side: Future<IdTokenResult> getIdTokenResult(bool refresh)
  • Add new object on dart side: IdTokenResult
  • Add new method call handler on iOS, which calls the native firebase getIdTokenResult(BOOL refresh)
  • Update the method call handler getIdToken on Android, in order to handle both getIdToken() and getIdTokenResult(). The reason is the firebase Android SDK only provides one method getIdToken, which returns the id token result. The method now returns only the token or the whole token result, depending on the dart call.

What is missing: tests, changelog, version bump.
Will add these later.

Related Issues

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.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@collinjackson collinjackson self-requested a review July 17, 2019 16:47
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.

Thanks for the PR! This is looking good, just a few requests around testing and make sure to reformat so that the bots are happy.

Could you please add unit tests for the added MethodChannel API surface area to test/firebase_auth_test.dart

Could you please also add integration tests to example/test_driver/firebase_auth.dart. Something like this:

    test('idTokenResult', () async {
      final FirebaseUser user = await auth.signInAnonymously();
      String token = await user.getIdToken();
      expect(token, isNotNull);
      IdTokenResult result = await user.getIdTokenResult();
      expect(result.token, token);
      // more assertions here
    });

return;
}

// Contrary to iOS, android has a single method to handle getIdToken() and getIdTokenResult()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Contrary to iOS, android has a single method to handle getIdToken() and getIdTokenResult()
// Contrary to iOS, Android has a single method to handle `getIdToken()` and `getIdTokenResult()`

@collinjackson
Copy link
Contributor

collinjackson commented Jul 17, 2019

After looking at this more carefully, I think that getIdToken() should probably return a result object that can be used to get the token. This is a breaking API change, so I'll get some internal feedback about naming.

@lukaspili
Copy link
Contributor Author

lukaspili commented Jul 19, 2019

I understand, in my opinion there should be only but 1 method. Javascript and iOS Firebase SDKs provide 2 methods, while Android SDK provides only 1.

What about deprecating getIdToken() (which currently returns the token string) and adding the new method getIdTokenResult() (which will return the result object)?

@collinjackson
Copy link
Contributor

Haven't forgotten about this one, I should have a naming decision soon and then we can move forward. Thanks for your patience.

@lukaspili
Copy link
Contributor Author

No problem. I'll wait on your decision before implementing the tests.

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.

The feedback that I got from naming committee was to change the signature of getIdToken to Future<IdTokenResult> getIdToken({bool refresh = false});. And at the same time make a breaking change to sign-in methods so that they return an AuthResult instead of a FirebaseUser. We are choosing to diverge from the React Native / web SDK's UserCredential naming and two getIdToken* methods that are there for historical reasons.

@lukaspili lukaspili requested a review from kroikie as a code owner July 26, 2019 15:54
@lukaspili
Copy link
Contributor Author

Implemented changes for renaming, updated the tests and the changelog.
PTAL.

@collinjackson
Copy link
Contributor

collinjackson commented Jul 27, 2019

I think we should call it authTime rather than authDate to be a bit more idiomatic: this matches the naming conventions of the web SDK, matches React Native Firebase, and reflects that the granularity of the data includes time info.

This lines up with what's happening in #1919 where I want to change Timestamp to Time in the Dart API.

@lukaspili
Copy link
Contributor Author

Makes sense. Let me know if there is anything else I can do.

@collinjackson collinjackson self-requested a review July 29, 2019 20:45
@collinjackson collinjackson merged commit 270170b into flutter:master Jul 29, 2019
@lookfirst
Copy link

@collinjackson lol, I was just looking for this and couldn't find it in 0.11! I guess I'll have to try your new version asap. =)

mithun-mondal pushed a commit to bKash-developer/archived_plugins that referenced this pull request Aug 6, 2019
* Implement IdResultToken type and update getIdTokenResult

* Add tests
julianscheel pushed a commit to jusst-engineering/plugins that referenced this pull request Mar 11, 2020
* Implement IdResultToken type and update getIdTokenResult

* Add tests
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.

5 participants