Closed SamCosta1 closed 3 months ago
I would love to see something like this merged. More granular handling of those errors is required in many applications. 👍
So if I would want to handle the raw OAuth 2.0 standard errors now like "invalid_grant", "expired_token", etc. that are now provided by both the iOS and Android plugin in FlutterAppAuthPlatformErrorDetails.error
, I can just do something like this, right?
try {
await appAuth.token(...);
} on FlutterAppAuthPlatformException catch (e) {
final String oauthError = e.details.error;
if (oauthError != null && (oauthError == "expired_token" || oauthError == "invalid_grant" || oauthError == "invalid_token")) {
logout();
}
}
Neat! Two thumbs up!
Yes exactly, the logout use case was the driver for creating this PR 😎😎.
@MaikuB would it be possible to merge this? I'm happy to make changes if needed
I'll review and take a look when I have the time to do so. This is something I maintain in my spare time and isn't the only plugin I maintain either. I suggest others point to your fork in the meantime so they can provide feedback and raise and issues should they find any.
From the quick look I did take though, I agree that this would replace #395. Whilst I would personally like to see errors map to something that is cross platform, I understand that could be a difficult/time consuming task. In the absence of that, it looks like this PR provides a better approach than what was in #395. What I'm curious about as well though is suppose this plugin migrated to use pigeon, how would it affect this PR? Not saying I've got definite plans to do so but was something that crossed my mind. Would you happen to know?
Other thing I forgot to add is I noticed is the exception for cancelling is documented to only be for authorizeAndExchange()
. If this is true then there's a gap there as getting the authorisation code is really the only part that requires a user to sign in. Exchange the code for a token is a web request to a server's token
endpoint.
I did think about creating a cross platform enum but as you say it quickly looked like a big task that would also take a while to test. I'm not familiar with Objective C and my Java in rusty (is there appetite for swapping to Swift?).
I'm afraid I have absolutely no idea about Pigeon though, I've not used it before.
You're absolutely correct about the docs, I'll update those :) Thanks for your response, appreciate you're busy!
Whilst I would personally like to see errors map to something that is cross platform, I understand that could be a difficult/time consuming task. In the absence of that, ...
I mean, the OAuth 2 error codes are "cross platform" in a way. They are not "polluted" by client code and they are already quite useful.
We could have a sealed class for them and provide them along the error details instead of the raw error data. Something like this:
sealed class OAuth2Error {
final String error;
final String? errorDescription;
final String? errorUri;
const OAuth2Error({
required this.error,
this.errorDescription,
this.errorUri,
});
static OAuth2Error fromErrorCode(String error, {String? errorDescription, String? errorUri}) {
switch (error) {
case 'invalid_request':
return InvalidRequest(errorDescription: errorDescription, errorUri: errorUri);
case 'unauthorized_client':
return UnauthorizedClient(errorDescription: errorDescription, errorUri: errorUri);
case 'access_denied':
return AccessDenied(errorDescription: errorDescription, errorUri: errorUri);
case 'unsupported_response_type':
return UnsupportedResponseType(errorDescription: errorDescription, errorUri: errorUri);
case 'invalid_scope':
return InvalidScope(errorDescription: errorDescription, errorUri: errorUri);
case 'server_error':
return ServerError(errorDescription: errorDescription, errorUri: errorUri);
case 'temporarily_unavailable':
return TemporarilyUnavailable(errorDescription: errorDescription, errorUri: errorUri);
case 'invalid_client':
return InvalidClient(errorDescription: errorDescription, errorUri: errorUri);
case 'invalid_grant':
return InvalidGrant(errorDescription: errorDescription, errorUri: errorUri);
case 'unsupported_grant_type':
return UnsupportedGrantType(errorDescription: errorDescription, errorUri: errorUri);
case 'invalid_token':
return InvalidToken(errorDescription: errorDescription, errorUri: errorUri);
case 'insufficient_scope':
return InsufficientScope(errorDescription: errorDescription, errorUri: errorUri);
default:
return UnknownError(error: error, errorDescription: errorDescription, errorUri: errorUri);
}
}
}
class InvalidRequest extends OAuth2Error {
const InvalidRequest({super.errorDescription, super.errorUri})
: super(error: 'invalid_request');
}
class UnauthorizedClient extends OAuth2Error {
const UnauthorizedClient({super.errorDescription, super.errorUri})
: super(error: 'unauthorized_client');
}
class AccessDenied extends OAuth2Error {
const AccessDenied({super.errorDescription, super.errorUri})
: super(error: 'access_denied');
}
class UnsupportedResponseType extends OAuth2Error {
const UnsupportedResponseType({super.errorDescription, super.errorUri})
: super(error: 'unsupported_response_type');
}
class InvalidScope extends OAuth2Error {
const InvalidScope({super.errorDescription, super.errorUri})
: super(error: 'invalid_scope');
}
class ServerError extends OAuth2Error {
const ServerError({super.errorDescription, super.errorUri})
: super(error: 'server_error');
}
class TemporarilyUnavailable extends OAuth2Error {
const TemporarilyUnavailable({super.errorDescription, super.errorUri})
: super(error: 'temporarily_unavailable');
}
class InvalidClient extends OAuth2Error {
const InvalidClient({super.errorDescription, super.errorUri})
: super(error: 'invalid_client');
}
class InvalidGrant extends OAuth2Error {
const InvalidGrant({super.errorDescription, super.errorUri})
: super(error: 'invalid_grant');
}
class UnsupportedGrantType extends OAuth2Error {
const UnsupportedGrantType({super.errorDescription, super.errorUri})
: super(error: 'unsupported_grant_type');
}
class InvalidToken extends OAuth2Error {
const InvalidToken({super.errorDescription, super.errorUri})
: super(error: 'invalid_token');
}
class InsufficientScope extends OAuth2Error {
const InsufficientScope({super.errorDescription, super.errorUri})
: super(error: 'insufficient_scope');
}
class UnknownError extends OAuth2Error {
const UnknownError({required super.error, super.errorDescription, super.errorUri});
}
// Usage example (in plugin)
details.oauthError = OAuth2Error.fromErrorCode(
errorCode,
errorDescription: errorDescription,
errorUri: errorUri
);
// Usage example (in app code)
try {
await appAuth.token(...);
} on FlutterAppAuthPlatformException catch (e) {
switch (e.details.oauthError) {
case InvalidRequest():
case UnauthorizedClient():
case AccessDenied():
case UnsupportedResponseType():
case InvalidScope():
case ServerError():
case TemporarilyUnavailable():
case InvalidClient():
print('Something went wrong');
case InvalidGrant():
case UnsupportedGrantType():
case InvalidToken():
case InsufficientScope():
case UnknownError():
logout();
}
}
That looks pretty good to me! I'll probably also include the raw error too though alongside this since there are other errors the different platforms spit out which aren't mappable.
I'll add this in as soon as I get time
I mean, the OAuth 2 error codes are "cross platform" in a way. They are not "polluted" by client code and they are already quite useful.
To clarify, I was referring to the error codes within the AppAuth iOS and Android SDKs. Each SDK has different error codes for same scenarios. As the plugin is a cross-platform library, ideally developers should not have to concern themself with platform-specific error codes where possible. This is for Android whilst this is for iOS. One example that can spotted there is is an invalid discovery doc uses 0
on Android and -2
on iOS
Ah right yeah, so not just the 400 error types int he RFC but all the others.
Personally though I'd say we're at risk of making the perfect the enemy of the good here. In that FlutterAppAuthPlatformErrorDetails
could easily be added to in future PRs with @retendo 's suggestions for OAuth errors from the RFC, and with what you're suggesting @MaikuB ?
Given how little time we all seem to have I'd suggest we merge this and add these suggestions as new properties on the details object in subsequent PRs? Your call of course!
@SamCosta1 the response was specifically to @retendo as I suspect my point/preference from an ideal standpoint wasn't understood. It's not blocker so that's why I mentioned to you I understand it's a time consuming task. The main issue with #395 was the lack of follow-up to address the feedback I did raise and from what I've seen so far, your PR addresses it.
Agreed that what @retendo could be used to address the ones tied to RFC though I've not been to see if there's a better way that has been recommended and followed by the Flutter team. I once had a link in one of comments in #395 but it appears the page doesn't exist anymore or got relocated
@MaikuB Thanks for clarifying, I suspected that you meant the platform specific library error, but I wanted to point out that IMO they are not that important to make a huge improvement for most cases.
I think apart from the RFC cases, the most important one is the cancellation failure case which @SamCosta1 already implemented. At least in my iOS app, which uses AppAuth-iOS, this is the only one apart from the RFC errors that I handle. That seems to be enough for a lot of use cases, so this PR is already a big improvement that also leaves room for future iteration.
Yes exactly, the logout use case was the driver for creating this PR
@SamCosta1 you about to elaborate on the use case? Curious what it is so I can try to reproduce it
Yes exactly, the logout use case was the driver for creating this PR
@SamCosta1 you about to elaborate on the use case? Curious what it is so I can try to reproduce it
Of course! :)
On the apps I've been working on the way we determine if a user is authenticated is by checking if we have tokens cached to disk. No tokens = not logged in.
When we use a refresh token to refresh an access token, this can obviously fail. One reason for this is because the refresh token has expired. In this case we must clear the locally cached tokens and log the user out (otherwise the user is in an irrecoverable state).
Without this PR it's impossible to know why a token refresh failed so we're forced to perform this logout on any failure. (This isn't quite true, we can detect a failure due to network issues but still). We only want to log the user out in certain cases e.g invalid_grant
when the state is irrecoverable.
Without these granular checks, downtime in the authorization server could cause mass logouts. For example if at peak time the authorization server starts returning 500 errors, many users could be logged out at once which adds system load as they try to get back in, and more importantly is terrible user experience.
This PR will hopefully replace https://github.com/MaikuB/flutter_appauth/pull/395.
This change will expose errors raised by the underlying AppAuth SDKs such that they can be consumed by Dart code. The changes are designed such that they should be entirely backwards compatible and any existing error handling should be unaffected.
As documented in the readme errors can now be processed as follows
Where the
FlutterAppAuthPlatformErrorDetails
class contains all the specific information exposed by the platform. TheFlutterAppAuthUserCancelledException
has been added since it's a common use case to need to handle this differently to other errors.