auth0 / auth0-flutter

Auth0 SDK for Flutter
https://pub.dev/documentation/auth0_flutter/latest/
Apache License 2.0
61 stars 41 forks source link

[SDK-4046] WebException wrapper #227

Closed stevehobbsdev closed 1 year ago

stevehobbsdev commented 1 year ago

πŸ“‹ Changes

This PR wraps incoming exceptions from the underlying SPA client to a WebException type that surfaces the error and error_description properties.

stevehobbsdev commented 1 year ago

It should be possible, I saw your earlier comment and have not yet finished it. The way you write it sounds like it refers to a pre-existing list of valid codes somewhere but I'm not finding MFA_REQUIRED, can you help me out?

Widcket commented 1 year ago

There's not pre-existing valid list of codes for web errors, as mentioned in the original comment it comes form the SPA SDK error.

have not yet finished it.

My bad, I apologize.

stevehobbsdev commented 1 year ago

No worries, I got confused by this:

it should be MFA_REQUIRED

I mistakenly took it for something that already existed, but you saying it should be that according to what the original type name is?

Widcket commented 1 year ago

I mistakenly took it for something that already existed, but you saying it should be that according to what the original type name is?

I'm saying that the WebException code should come from the type name of the original SPA SDK. SPA SDK consumers can use that to check what type of error they're getting (using instanceof IIRC), but auth0_flutter_web consumers can't –unless we add that to a code they can check.

Widcket commented 1 year ago

We did something similar in the Next.js SDK v2: https://github.com/auth0/nextjs-auth0/blob/main/src/utils/errors.ts#L192

Widcket commented 1 year ago

Note that in the case of Next.js it's useful because an error type can have many codes: https://github.com/auth0/nextjs-auth0/blob/main/src/utils/errors.ts#L79

stevehobbsdev commented 1 year ago

I've rolled out a pattern for mfa_required in ac5f71b, can you have a look at it before I roll it out to other exceptions?

Widcket commented 1 year ago

Thats's great πŸš€

stevehobbsdev commented 1 year ago

How do you want to handle AuthenticationErrror? According to the requirements above I should return this with a code of AUTHENTICATION_ERROR with the description, but this error could surface thanks to one of these errors, which has its own descriptive code and would be surfaced as error - should this instead be in the details map?

Widcket commented 1 year ago

Yes, let's put that info in the details map.

stevehobbsdev commented 1 year ago

@Widcket I've added the remaining exception types, with the exception of PopupTimeout (see internal discussion), so is ready for a re-review.