Skip to content
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.

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

v3 breaks consistent error handling #709

Closed
6 tasks done
rick-lannan-upgrowth opened this issue Aug 22, 2023 · 7 comments
Closed
6 tasks done

v3 breaks consistent error handling #709

rick-lannan-upgrowth opened this issue Aug 22, 2023 · 7 comments
Labels
bug This points to a verified bug in the code

Comments

@rick-lannan-upgrowth
Copy link

rick-lannan-upgrowth commented Aug 22, 2023

Checklist

Description

In the v2 versions of the module, errors returned from IOS and Android were consistent with the following format that could be normalised to get a cross platform error code and description from the name and message props (or the json.error and json.error_description props):

{
  name: string;
  message: string;
  status?: number;
  invalid_parameter?: string;
  json?: {
    error?: string;
    error_description?: string
    invalid_parameter?: string
  }
}

In the v3 implementation the errors are not the same for IOS and Android and there seems to be no way to get an API error code. Both IOS and Android returns errors with roughly the same structure but the code prop does not include the API error codes and the mappings used are not the same cross platform:

{
  code: string
  message: string
  ... platform specific props
}

Specific problem are:

  • The mappings of error code is not the same across device. For example, the cancel error is now USER_CANCELLED in IOS but it is still a0.session.user_cancelled in Android. It looks like the codes have been remapped in the new IOS implementation without realising this is a breaking change for any application that is listening for errors
  • All API errors are returned with an error code of either a0.response.invalid (Android) or OTHER (IOS) rather than passing through the error code returned from the API. This could be ok if another prop was provided for the api error code (or better yet the raw API error) but we seem to only have the message prop, which inconsistently either has the api error code or an error message, but not both. And the exact wording of the message is not consistent across IOS and Android so determining the actual error in code cross platform is problematic
  • There's no documentation on what the set of errors returned for each function are (e.g. authorize, getCredentials, clearSession, etc). This was also the case with v2 but at least the errors were somewhat consistent

Reproduction

Simplest way is to call authorize and cancel the overlay, you can see the error returned for the cancel event is inconsistent cross platform. The same issue applies to any error generated though

Additional context

No response

react-native-auth0 version

3.0.0, 3.0.1

React Native version

Any

Expo version

No response

Platform

Android and IOS

Platform version(s)

Any

@rick-lannan-upgrowth rick-lannan-upgrowth added the bug This points to a verified bug in the code label Aug 22, 2023
@rick-lannan-upgrowth
Copy link
Author

rick-lannan-upgrowth commented Aug 23, 2023

Apologies, an amendment to the bug report. Errors are not coming out meaningfully on Android at all. During testing I patched A0Auth0Module.java to return the underlying api error code and description instead of the generic a0.response.invalid - An error occurred when trying to authenticate with the server. error. Without this patch in place we always get the same generic error

Here is the patch that makes the error detectable on Android:

    private void handleError(AuthenticationException error, Promise promise) {
        if(error.isBrowserAppNotAvailable()) {
            promise.reject("a0.browser_not_available", "No Browser application is installed.");
            return;
        }
        if(error.isCanceled()) {
            promise.reject("a0.session.user_cancelled", "User cancelled the Auth");
            return;
        }
        // Patched to return the underlying API error instead of a generic error
        promise.reject(error.getCode(), error.getDescription());
    }

So as with IOS, we really need the error returned to include BOTH the API error code and error_description that the auth0 server returned, and not a mapping or abstraction, to be able to programatically figure out what caused the error

@stevehobbsdev
Copy link
Contributor

stevehobbsdev commented Sep 8, 2023

Thanks for the bug report @rick-lannan-upgrowth, and for your patience - we've had a support ticket raised internally as well and @poovamraj will be taking a look at this next week 👍🏻

@poovamraj
Copy link
Contributor

@rick-lannan-upgrowth thanks for raising this. Answering your specific problems

  1. I agree that user cancelled error can have similar error code but apart from that there is not much similarity to be found across platforms.

  2. This is a great catch, we can implement the patch in the next release. TY.

  3. Yes we can improve the documentation on this. Our error handling could be more standardised. We will improve this in our next release as well.

But to get more idea on the first point, can you point out to other error codes that have been similar in platforms that are causing issue in real world scenario.

@rick-lannan-upgrowth
Copy link
Author

rick-lannan-upgrowth commented Sep 13, 2023

Hi,

Glad to hear you are working on this issue. Do you have any time frames for when we could expect a release?

Here's the extra info you requested:

  1. This isn't a big issue as we can just allow for both cancel codes but ideally all the error codes and messages would be standardised across platform
  2. With this patch can you please return an additional object that has the api error code and message? For example an error response such as this would be perfect
{
  // mapped error code returned from the native IOS or Android library
  code: string
  // mapped error message returned from the native IOS or Android library
  message: string
  // the underlying api error (if applicable)
  cause?: {
    error: string
    error_description: string
    invalid_parameter?: string
  }
  ... platform specific props
}
  1. Great :-)

To answer your last question it's really just the CANCEL event and the API error response that is important for us as we have user friendly error messaging if a registration or login fails. We also have built some improvements to email verification process on Android that automatically completes a registration after the user verifies their email rather than requiring users to login again mid registration. For this we do a 'silent' call to auth0 with webAuth.authorize using screen_hint: 'login'; prompt: none to log the user in or show a message based on the error code returned

@poovamraj
Copy link
Contributor

Definitely. We will be working on a fix with the above mentioned feedbacks. You can expect a release by end of next week.

@poovamraj
Copy link
Contributor

@rick-lannan-upgrowth we have a PR that will propagate more error codes in Android. Do have a look at it and let us know. We will close this issue and continue the conversation in the PR.

@rick-lannan-upgrowth
Copy link
Author

Hi, I notice there is a PR for Android. Is there a PR coming that also fixes this for iOS?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This points to a verified bug in the code
Projects
None yet
Development

No branches or pull requests

3 participants