Skip to content

fix(auth)!: Fetch Auth Session offline behavior #2585

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

Merged
merged 29 commits into from
Jan 26, 2023

Conversation

Jordan-Nelson
Copy link
Member

@Jordan-Nelson Jordan-Nelson commented Jan 18, 2023

Issue #, if available: #2398, #760

Description of changes:

  • Remove getAWSCredentials from Fetch Auth Session API
  • Add new types, AWSResult and AuthResult, to hold the result of an operation that may have thrown.
  • Update exception handling in fetch auth session state machine
  • Update CognitoAuthSession members to type AuthResult
  • Add Fetch Auth Session test cases
  • Update Authenticator to catch NetworkException.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@Jordan-Nelson Jordan-Nelson requested a review from a team as a code owner January 18, 2023 21:47
@codecov-commenter
Copy link

codecov-commenter commented Jan 18, 2023

Codecov Report

Merging #2585 (dc791a5) into next (e842619) will increase coverage by 0.00%.
The diff coverage is 66.66%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##             next    #2585   +/-   ##
=======================================
  Coverage   50.89%   50.90%           
=======================================
  Files          86       86           
  Lines        5812     5813    +1     
=======================================
+ Hits         2958     2959    +1     
  Misses       2854     2854           
Flag Coverage Δ
android-unit-tests ∅ <ø> (∅)
flutter-unit-tests 38.30% <66.66%> (+0.01%) ⬆️
ios-unit-tests 95.32% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ticator/lib/src/services/amplify_auth_service.dart 57.14% <66.66%> (+0.69%) ⬆️

Copy link
Contributor

@dnys1 dnys1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Thanks for refactoring the fetch auth session tests!

@@ -186,13 +182,13 @@ void main() {
final clearedSession = await cognitoPlugin.fetchAuthSession();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
final clearedSession = await cognitoPlugin.fetchAuthSession();
final unauthSession = await cognitoPlugin.fetchAuthSession();

Comment on lines 29 to 31
final CognitoAuthSessionResult<AWSCredentials>? _credentialsResult;
final CognitoAuthSessionResult<CognitoUserPoolTokens>? _userPoolTokensResult;
final CognitoAuthSessionResult<String>? _identityIdResult;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would these be null?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They won't. Not sure why I made them nullable. Updated.

/// The value which was successfully retrieved.
///
/// [exception] will be thrown if there was an issue fetching the value.
T? get value {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API is a bit awkward since we're trying to model three states - successfully retrieved T, successfully retrieved null, unsuccessful with error.

But thinking about the use cases, I don't think we want to squash all these into a single API. Having to try/catch and null check is kind of a pain. And a nullable getter which throws is not intuitive since an error state can be modeled with null already.

I think something more like this could make sense:

final T? value;

T get valueOrThrow => value ?? (throw _exception ?? const UnknownException('Value could not be retrieved'));

That being said, I wonder if the null value is ever even interesting... would this be a better API? Besides testing, when would people call session.identityId and want anything besides a value?

final T? _value;

T get value => _value ?? (throw _exception ?? const UnknownException('Value could not be retrieved'));

This way the session values could be made non-null.

Yet another option is to completely break the API and make the release of the result value explicit - so change the session values to be of type CognitoAuthSessionResult and customers have to unwrap them with value or valueOrThrow. It might be better to be explicit since a getter which throws is also kind of an anti-pattern.

} on Object {
// If there was an exception fetching userPoolTokens, return the cached
// value
return _userPoolTokensResult?.previousValue?.userId;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this logic be handled in the fetch auth session machine instead of in this API and the result API? previousValue seems a bit hacky and makes this inconsistent with the other calls since this will never throw even when, for example, the device is offline from the start and there never was a value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like you could have a userIdResult and do what you did for identityId in the state machine

Copy link
Member Author

@Jordan-Nelson Jordan-Nelson Jan 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to match identityId 👍

I also updated this behavior since it was incorrect. userSubResult will always throw if userPoolTokenResult throws based on discussion on behavior alignment.

Comment on lines 251 to 253
accessToken: accessToken.raw,
refreshToken: refreshToken,
idToken: newIdToken.raw,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, I think access and ID token would both be new here. Only refresh stays the same

authenticationResult: AuthenticationResultType(
accessToken: newAccessToken.raw,
refreshToken: refreshToken,
idToken: idToken.raw,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment

/// The result may contain an error even though the method
/// fetching it had a success callback.
class CognitoAuthSessionResult<T>
with AWSEquatable<CognitoAuthSessionResult<T>> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be AWSDebuggable, too - only values which are also AWSDebuggable will print, so it's safe

} on Object catch (e) {
credentialsResult = CognitoAuthSessionResult.error(e);
// return existing identityId if refreshing credentials fails
if (identityId != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be accomplishing the same as previousValue

result.data.identityId,
);
} else {
credentialsResult = const CognitoAuthSessionResult<AWSCredentials>.error(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we exit early with this before the try/catch?

Copy link
Contributor

@dnys1 dnys1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@@ -15,6 +15,14 @@ class SignedOutException extends AuthException {
super.underlyingException,
});

/// Thrown when no user is currently signed in.
const SignedOutException.noUserSignedIn({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be merged into the default constructor? I don't see why this case is special.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, probably. I think a few exceptions should have a default message. I'm not sure how to do this without making it a breaking change, which would expand this PR a bit.

I reverted this change. If we want to add default messages it probably makes sense to do it all at once in a separate PR.

final nativeAuthSession = NativeAuthSession(
isSignedIn: authSession.isSignedIn,
userSub: authSession.userSub,
identityId: authSession.identityId,
userSub: _valueOrNull(() => authSession.userSubResult.value),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add T? get valueOrNull to AWSResult. This seems like something most people would want to do at one point or another.

/// The User Pool tokens.
final CognitoUserPoolTokens? userPoolTokens;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we leave these, and mark them as deprecated to a make this relatively non-breaking?

@Deprecated('Use `userPoolTokensResult.value` instead')
CognitoUserPoolTokens? get userPoolTokens => userPoolTokensResult.valueOrNull;

];

@override
Map<String, Object?> toJson() => _$CognitoAuthSessionToJson(this);
Map<String, Object?> toJson() => <String, dynamic>{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toJson should include all the results' presence IMO.

{
  'isSignedIn': false,
  'userPoolTokens': null,
  'awsCredentials': 'Instance of AWSCredentials',
  // etc.
}

Comment on lines 283 to 293
} on AuthException catch (e) {
credentialsResult = AuthResult.error(e);
identityIdResult = AuthResult.error(e);
} on Object catch (e) {
final authException = UnknownException(
'An unknown exception occurred.',
underlyingException: e,
);
credentialsResult = AuthResult.error(authException);
identityIdResult = AuthResult.error(authException);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Catching Object is risky in general since we may swallow errors that are not expected, like incorrect code. At the state machine layer and above, I'm not sure it's every really needed.

Suggested change
} on AuthException catch (e) {
credentialsResult = AuthResult.error(e);
identityIdResult = AuthResult.error(e);
} on Object catch (e) {
final authException = UnknownException(
'An unknown exception occurred.',
underlyingException: e,
);
credentialsResult = AuthResult.error(authException);
identityIdResult = AuthResult.error(authException);
}
} on Exception catch (e) {
final authException = AuthException.fromException(e);
credentialsResult = AuthResult.error(authException);
identityIdResult = AuthResult.error(authException);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

switch (type) {
case AWSResultType.success:
if (_value == null) {
// this should not happen since AWSResultType.success requires a non
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it literally cannot happen - safe to unwrap here since it's guaranteed by the compiler.

class AWSResult<T, E extends Object>
with AWSEquatable<AWSResult<T, E>>, AWSDebuggable {
/// Creates a failed result.
const AWSResult.error(E this.exception)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const AWSResult.error(E this.exception)
const AWSResult.error(E this.exception, [this.stackTrace])

Comment on lines 31 to 33
/// This object will contains exceptions that occur while trying to obtain or
/// refresh this value, such as an [AWSHttpException] if the service cannot be
/// reached due to a network error.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think this belongs here - maybe move to AuthResult's doc comments if you think it adds value.

Suggested change
/// This object will contains exceptions that occur while trying to obtain or
/// refresh this value, such as an [AWSHttpException] if the service cannot be
/// reached due to a network error.

Comment on lines 283 to 293
} on AuthException catch (e) {
credentialsResult = AuthResult.error(e);
identityIdResult = AuthResult.error(e);
} on Object catch (e) {
final authException = UnknownException(
'An unknown exception occurred.',
underlyingException: e,
);
credentialsResult = AuthResult.error(authException);
identityIdResult = AuthResult.error(authException);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@Jordan-Nelson Jordan-Nelson merged commit dc2a3f4 into next Jan 26, 2023
@Jordan-Nelson Jordan-Nelson deleted the feat/fetchAuthSession-offline-behavior branch January 26, 2023 14:55
fjnoyp pushed a commit that referenced this pull request Jan 27, 2023
* feat(auth): update fetch auth session behavior

* fix(auth): remove getAWSCredentials arg from native plugin

* chore: update federateToIdentityPool test

* chore: CognitoAuthSessionResult doc comments

* chore: add/update tests for fetchAuthSession

* chore: add tests for unauth access, fix failing tests

* chore: update fetchAuthSession integ tests

* fix(authenticator)!: Update authenticator offline behavior

* chore: add comment to CognitoAuthSession

* chore: update failing android test

* chore: expose results from CognitoAuthSession, throw AuthExceptions

* chore: make tests more realistic

* chore: make AWSResult AWSDebuggable

* chore: early exit for no identity pool

* chore: update CognitoAuthSession.toJson

* chore: add doc comment to AWSResultType

* chore: update fetchAuthSession stub

* chore: undo sample app changes

* chore: simplify AuthProvider

* chore: revert changes to SignedOutException

* chore: add valueOrNull to AWSResult

* chore: add back members to CognitoAuthSession, update toJson

* chore: add getAWSCredentials back as deprecated

* chore: remove empty CognitoSessionOptions

* chore: refactor exception handling

* chore: force unwrap awsCredentials & identityId

* chore: update authenticator to catch Exception not object

* chore: add stacktrace to AWSResult

* fix: amplify_test
dnys1 pushed a commit to Jordan-Nelson/amplify-flutter that referenced this pull request Jan 30, 2023
- fix(auth)!: Fetch Auth Session offline behavior ([aws-amplify#2585](aws-amplify#2585))

- fix(api): do not include null values in ModelMutations.create ([aws-amplify#2504](aws-amplify#2504))
- fix(api): model helpers use targetNames in schemas with CPK enabled ([aws-amplify#2559](aws-amplify#2559))
- fix(auth): Clear credentials before redirect on Web ([aws-amplify#2603](aws-amplify#2603))
- fix(auth): Refresh token in non-state machine calls ([aws-amplify#2572](aws-amplify#2572))
- fix(authenticator): ARB syntax ([aws-amplify#2560](aws-amplify#2560))
- fix(aws_common): AWSFile contentType getter should not throw exception
- fix(datastore): prevent unhandled exception crashing App rebuilding sync expression
- fix(storage): incorrect transferred bytes emitted from upload task

- feat(analytics): Legacy data migration of Pinpoint Endpoint ID ([aws-amplify#2489](aws-amplify#2489))
- feat(smithy_aws): add copyWith to S3ClientConfig
- feat(storage): allow configuring transfer acceleration

Updated-Components: Amplify Flutter, Amplify Dart, Amplify UI, DB Common, Secure Storage, AWS Common, Smithy, Worker Bee
dnys1 pushed a commit to Jordan-Nelson/amplify-flutter that referenced this pull request Jan 30, 2023
- fix(auth)!: Fetch Auth Session offline behavior ([aws-amplify#2585](aws-amplify#2585))

- fix(api): do not include null values in ModelMutations.create ([aws-amplify#2504](aws-amplify#2504))
- fix(api): model helpers use targetNames in schemas with CPK enabled ([aws-amplify#2559](aws-amplify#2559))
- fix(auth): Clear credentials before redirect on Web ([aws-amplify#2603](aws-amplify#2603))
- fix(auth): Refresh token in non-state machine calls ([aws-amplify#2572](aws-amplify#2572))
- fix(authenticator): ARB syntax ([aws-amplify#2560](aws-amplify#2560))
- fix(aws_common): AWSFile contentType getter should not throw exception
- fix(datastore): prevent unhandled exception crashing App rebuilding sync expression
- fix(storage): incorrect transferred bytes emitted from upload task

- feat(analytics): Legacy data migration of Pinpoint Endpoint ID ([aws-amplify#2489](aws-amplify#2489))
- feat(smithy_aws): add copyWith to S3ClientConfig
- feat(storage): allow configuring transfer acceleration

Updated-Components: Amplify Flutter, Amplify Dart, Amplify UI, DB Common, Secure Storage, AWS Common, Smithy, Worker Bee
Jordan-Nelson added a commit that referenced this pull request Jan 30, 2023
* chore(repo): Update component definition

Adds `amplify_api_dart` to the mix and creates components for DB Common, Secure Storage, and AWS Common

* fix(aft): Update changelog logic

For the version commit message changelog, only include publishable packages. Also updates base ref logic to ensure packages can be moved in and out of components.

* chore(version): Bump version

- fix(auth)!: Fetch Auth Session offline behavior ([#2585](#2585))

- fix(api): do not include null values in ModelMutations.create ([#2504](#2504))
- fix(api): model helpers use targetNames in schemas with CPK enabled ([#2559](#2559))
- fix(auth): Clear credentials before redirect on Web ([#2603](#2603))
- fix(auth): Refresh token in non-state machine calls ([#2572](#2572))
- fix(authenticator): ARB syntax ([#2560](#2560))
- fix(aws_common): AWSFile contentType getter should not throw exception
- fix(datastore): prevent unhandled exception crashing App rebuilding sync expression
- fix(storage): incorrect transferred bytes emitted from upload task

- feat(analytics): Legacy data migration of Pinpoint Endpoint ID ([#2489](#2489))
- feat(smithy_aws): add copyWith to S3ClientConfig
- feat(storage): allow configuring transfer acceleration

Updated-Components: Amplify Flutter, Amplify Dart, Amplify UI, DB Common, Secure Storage, AWS Common, Smithy, Worker Bee

---------

Co-authored-by: Dillon Nys <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants