firebase / flutterfire

šŸ”„ A collection of Firebase plugins for Flutter apps.
https://firebase.google.com/docs/flutter/setup
BSD 3-Clause "New" or "Revised" License
8.72k stars 3.97k forks source link

[firebase_auth_web] handle exception as documented in platform interface. #1559

Closed GregorySech closed 4 years ago

GregorySech commented 4 years ago

Describe the bug Exceptions in the web implementation are not catchable using the platform interface.

To Reproduce Steps to reproduce the behavior:

  1. Create an empty flutter project using master
  2. Setup firebase_auth and firebase_auth_web following the web package README. (I didn't activate email auth provider to make sure of getting an API error).
  3. Place a try catch block to catch a possible error using AuthException
  4. Try to cause the expected error. I tried to authenticate with email and password without activating the email provider to cause ERROR_OPERATION_NOT_ALLOWED.

Expected behavior I expect the try catch to trigger but it gets ignored. The error is instead reported in the development console as "uncaught in Promise".

Additional context I'm working on a fix. I will create an "exception mapper" from package:firebase's FirebaseError to AuthException using this reference.

I'm working on the mapping here https://docs.google.com/spreadsheets/d/1VrmcLK3-KF5_DK732XDA0G64wxgiP4W3KvwxeP6Y_DA/edit?usp=sharing

collinjackson commented 4 years ago

/cc @hterkelsen

GregorySech commented 4 years ago

I'm kind of stuck, I need someone familiar with firebasejs to understand if some errors are missing from the mapping but not documented in the reference docs. Also I'm not sure what exactly is auth/unauthorized-domain about.

collinjackson commented 4 years ago

@GregorySech If you have specific questions feel free to post them here, or if you want to put up a PR with your initial attempt we can review it.

GregorySech commented 4 years ago

I've not written code so far, I'm working on getting a decent mapping first on the linked sheet. Once that's done I'll start the PR.

My first question would be to @slightfoot about ERROR_UNAUTHORIZED_DOMAIN. In his sheet that error is linked to the same description of auth/unauthorized-continue-uri per js reference. The problem is that auth/unauthorized-domain exists but has a different meaning per js reference. Is that an error or in the mobile SDKs unauthorized domain has a different meaning?

After that I have a lot of errors that seem to not have correspondence to the js API. Such as: ERROR_USER_MISMATCH ERROR_INVALID_MESSAGE_PAYLOAD ERROR_INVALID_SENDER ERROR_INVALID_RECIPIENT_EMAIL ERROR_MISSING_EMAIL ERROR_MISSING_PASSWORD MISSING_APP_CREDENTIAL INVALID_APP_CREDENTIAL ERROR_SESSION_EXPIRED* ERROR_MISSING_APP_TOKEN ERROR_NOTIFICATION_NOT_FORWARDED ERROR_APP_NOT_VERIFIED ERROR_WEB_CONTEXT_ALREADY_PRESENTED ERROR_WEB_CONTEXT_CANCELLED ERROR_APP_VERIFICATION_FAILED ERROR_INVALID_CLIENT_ID ERROR_WEB_NETWORK_REQUEST_FAILED ERROR_WEB_INTERNAL_ERROR ERROR_KEYCHAIN_ERROR ERROR_INTERNAL_ERROR ERROR_MALFORMED_JWT

To find if those are actually missing I should either find someone that can confirm them as missing or go through the js SDK source. If someone already encountered them I would be thankful otherwise I'll start going through the source this evening.

EDIT: I've found the error sources and I'm starting to adding the missing mappings. In the evening I will remove what I've mapped.

slightfoot commented 4 years ago

@GregorySech I built the original spreadsheet from the source code of both SDKs and the docs for Firebase Auth for both of them too. It could be a mistake in the docs?

GregorySech commented 4 years ago

Yeah, it's probably a mistake in the docs. I guess i'll just make a new error value for unauthorized-continue-uri. How do you guys feel about having a list of the possible error values in one of the packages? If you like the idea should I put it in the "master" package (firebase_auth) or in the platform interface?

edit: I'm thinking that those errors have actually been collapsed in the mobile sdks as unauthorized-domain is a generalization that contains unauthorized-continue-uri.

GregorySech commented 4 years ago

I'm done with implementing the mapper here. I need to add tests and update documentation. I need some direction on both. About tests: should I just make the mapping function visible inside firebase_auth_web and exercise it or try something more integrated with the actual calls? On documentation I will look through the reference for missing errors in the firebase_auth doc-blocks.

harryterkelsen commented 4 years ago

Thanks for doing this! Your approach LGTM for the most part. You can make the mapping function @visibleForTesting so it won't be part of the public API.

yumin-chen commented 4 years ago

Thank you for implementing this exception mapping to handle FirebaseError from the web.

I saw that you are mapping all FirebaseError to AuthException in your implementation. However, FirebaseAuth seems to actually throw a PlatformException rather than AuthException. See previous dicussion on #725 and https://github.com/flutter/plugins/pull/775, both threads are referencing to PlatformException. Shouldn't this be mapped to PlatformException instead?

In my own experience, if you try and catch AuthException, it won't be caught. Even though the error code is part of the auth module.

Unhandled Exception: PlatformException(ERROR_INVALID_CREDENTIAL, Unable to verify the ID Token signature.

Above example is when signInWithCredential throws a PlatformException (code: auth/invalid-credential)

PlatformException is from the flutter/services.dart package:

import 'package:flutter/services.dart' show PlatformException;
GregorySech commented 4 years ago

Thanks @chen-yumin totally missed that. Mapping everything to PlatformException is easy this afternoon I'll change the mapping, realign my fork and open the PR. Wondering what AuthException is for tho.

illia-romanenko commented 4 years ago

Hi, @GregorySech

any updates on this? Do you need any help with finishing this PR?

GregorySech commented 4 years ago

Hi, @GregorySech

any updates on this? Do you need any help with finishing this PR?

On my end the merge conflicts are solved. I'll ask to get the PR reviewed again.

wer-mathurin commented 4 years ago

Sonner than later will be great. Working on a customer project and this can simplify a lot the errors handling since we need to support both web and native.

harisijazwarraich commented 4 years ago

Hello, the exceptions can be handled using, FirebaseAuthException class.

void loginUser(String email, String password) async { try { await _auth.signInWithEmailAndPassword(email: email, password: password); } on FirebaseAuthException catch (e) { Get.snackbar('Error logging in!', e.message, snackPosition: SnackPosition.BOTTOM); } catch (e) { // For all other exceptions! }

I am using GetX for state management here, Get.snackbar() is used to show a snackbar showing the error, you can use your own logic to handle the error, e.g show a alert dialog e.t.c Cheers <3

Salakar commented 4 years ago

Hey šŸ‘‹

Our rework of the firebase_auth plugin as part of the FlutterFire roadmap was published over a week ago with a ton of fixes and new features. Please could you try the new version and see if this is still an issue for you? If it is then please submit a new up to date GitHub issue. There was lot of work done to improve error handling: https://firebase.flutter.dev/docs/auth/error-handling

For help migrating to the new plugins please see the new migration guide: https://firebase.flutter.dev/docs/migration