[firebase_auth] Add a constructor for token result object with a token

[mehmetf]

We have applications that implement FirebaseUser class. In order to support this use case, classes such as IdTokenResult need to be open to being constructed as opposed to having only an internal API.

Fixes #5

[collinjackson]

I'm wondering if it would make sense to cover this use case with a test somehow.

[mehmetf]

Yes, I will write a test for this.

[mehmetf]

Done, please take another look.

[collinjackson]

Thanks for adding a test. Now that I understand the use case better, I can give a more detailed review.

Some pre-existing issues I noticed.

• _app should be removed from IdTokenResult because it's not used anywhere in the class.
• I believe that the constructor for IdTokenResult should be package-private (IdTokenResult._) rather than @visibleForTesting because it's not needed for tests.

Assuming that those changes are made, it seems suboptimal to expose a constructor that can set the token field of the IdTokenResult but not any of the other fields. For example, it's impossible to set a timestamp if you're constructing the IdTokenResult using fromUser. I think we should either expose a full constructor (ensuring that all the fields are settable) or force developers who want a custom version of this class to use implements.

Since these classes are rather simple and extending them is an uncommon use case, I worry that the public constructor is creating a maintenance burden and cluttering up the public API of the class a bit. These full constructors will be tempting for developers to use, but really the only developers who should ever use them are those who are rolling their own FirebaseUser.

If we go with the implements approach, the developer could write a TestAuthResult class that implements AuthResult and a TestIdTokenResult class that implements IdTokenResult, overriding the public fields of AuthResult/IdTokenResult in the same way that it's being done for FirebaseUser. This requires the developer to explicitly choose whether to support each field of the class they're implementing (e.g. timestamps, additionalUserInfo). The advantage of this approach is that whenever a new field is added to AuthResult/IdTokenResult, the developer of the custom implementation would start getting analyzer warnings telling them that the interface has changed and they need to decide how to handle the new field. Whereas if we exposed a public constructor these additional fields would just be getting set to null.

Something like this:

class TestAuthResult implements AuthResult {
  TestAuthResult(this.user);

  @override
  final FirebaseUser user;

  @override
  AdditionalUserInfo get additionalUserInfo => null;

  @override
  String toString() {
    return '$runtimeType($user)';
  }
}

class TestIdTokenResult implements IdTokenResult {
  TestIdTokenResult(this.token);

  @override
  final String token;

  @override
  DateTime get expirationTime => null;

  @override
  DateTime get authTime => null;

  @override
  DateTime get issuedAtTime => null;

  @override
  String get signInProvider => null;

  @override
  Map<dynamic, dynamic> get claims => null;

  @override
  String toString() {
    return '$runtimeType($token)';
  }
}

[mehmetf]

Indeed, that's a better solution.

A test would not be required in that case, I would presume since you can always implement any extra classes that get introduced. Would you agree?

[collinjackson]

Indeed, that's a better solution.

A test would not be required in that case, I would presume since you can always implement any extra classes that get introduced. Would you agree?

Yes, I agree.

[mehmetf]

Closing this PR; nothing to see here :-)