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.

Sign up for GitHub

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

fix: do not sign requests that have optional auth #3671

Merged
merged 6 commits into from
Sep 25, 2023
Merged

fix: do not sign requests that have optional auth #3671

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0f6d4a9
fix(smithy_aws)!: do not sign optional requests
Jordan-Nelson Aug 31, 2023
c9634fb
chore: remove mock state machine from test
Jordan-Nelson Aug 31, 2023
2aeded7
fix(auth): use loadCredentials to check login state
Jordan-Nelson Aug 31, 2023
e3399bb
fix(authenticator): use getCurrentUser to check sign in state
Jordan-Nelson Jun 10, 2023
a3294c0
chore: update comment
Jordan-Nelson Sep 20, 2023
8755015
chore: update test
Jordan-Nelson Sep 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/auth/amplify_auth_cognito_dart/lib/src/state/machines/sign_in_state_machine.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -770,8 +770,8 @@ final class SignInStateMachine
Future<void> _assertSignedOut() async {
bool isSignedIn;
try {
await manager.getUserPoolTokens();
isSignedIn = true;
final credentials = await manager.loadCredentials();
isSignedIn = credentials.userPoolTokens != null;
} on Exception {
isSignedIn = false;
}
Expand Down
23 changes: 15 additions & 8 deletions packages/auth/amplify_auth_cognito_test/test/plugin/fetch_user_attributes_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,11 @@ void main() {
late AmplifyAuthCognitoDart plugin;

group('fetchUserAttributes', () {
setUp(() {
tearDown(() async {
await plugin.close();
});

test('converts user attributes correctly', () async {
stateMachine = MockCognitoAuthStateMachine()
..addInstance<CognitoIdentityProviderClient>(
MockCognitoIdentityProviderClient(
Expand All @@ -107,13 +111,6 @@ void main() {
),
);
plugin = AmplifyAuthCognitoDart()..stateMachine = stateMachine;
});

tearDown(() async {
await plugin.close();
});

test('converts user attributes correctly', () async {
final res = await plugin.fetchUserAttributes();
final expected = [
AuthUserAttribute(
Expand Down Expand Up @@ -202,6 +199,16 @@ void main() {
});

test('refreshes token before calling Cognito', () async {
stateMachine = CognitoAuthStateMachine()
..addInstance<CognitoIdentityProviderClient>(
MockCognitoIdentityProviderClient(
getUser: () async => GetUserResponse(
userAttributes: [],
username: username,
),
),
);

final secureStorage = MockSecureStorage();
SecureStorageInterface storageFactory(scope) => secureStorage;
seedStorage(
Expand Down
9 changes: 9 additions & 0 deletions packages/auth/amplify_auth_cognito_test/test/state/sign_in_state_machine_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,15 @@ void main() {
).ignore();

final signInStateMachine = stateMachine.expect(SignInStateMachine.type);

final fetchAuthSessionStateMachine = stateMachine.getOrCreate(
FetchAuthSessionStateMachine.type,
);

fetchAuthSessionStateMachine.stream.listen(
(_) => fail('.signIn() should not fetch auth session.'),
);

expect(
signInStateMachine.stream,
emitsInOrder([
Expand Down
15 changes: 12 additions & 3 deletions packages/authenticator/amplify_authenticator/lib/src/services/amplify_auth_service.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,18 @@ abstract class AuthService {

Future<AuthUser?> get currentUser;

/// Checks to see if a user has a valid session.
///
/// A valid session is a session in which the tokens are not expired, OR
/// the access/id tokens have expired but the state of the refresh token is
/// unknown due to network unavailability.
Future<bool> isValidSession();

/// Checks if a user is logged in based on whether or not there are
/// tokens on the device.
///
/// This will not check whether or not those tokens are valid. To check
/// if tokens are valid, see [isValidSession].
Future<bool> get isLoggedIn;

Future<ResetPasswordResult> resetPassword(String username);
Expand Down Expand Up @@ -191,9 +201,8 @@ class AmplifyAuthService
Future<bool> get isLoggedIn async {
return _withUserAgent(() async {
try {
final result = await Amplify.Auth.fetchAuthSession();

return result.isSignedIn;
await Amplify.Auth.getCurrentUser();
return true;
Comment on lines +204 to +205
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this throws if it can't get the user?

} on SignedOutException {
return false;
}
Expand Down
17 changes: 7 additions & 10 deletions packages/smithy/smithy_aws/lib/src/http/interceptors/with_sig_v4.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,16 +31,13 @@ class WithSigV4 extends HttpRequestInterceptor {

@override
Future<AWSBaseHttpRequest> intercept(AWSBaseHttpRequest request) async {
// Try to retrieve credentials. If it fails, continue without authentication
// for optional auth requests only.
try {
await credentialsProvider.retrieve();
} on Exception {
if (isOptional) {
return request;
}
rethrow;
}
// Do not attempt to sign requests where auth is optional.
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
// Do not attempt to sign requests where auth is optional.
// Do not attempt to sign requests where auth is optional.
//
// This is only set in Cognito and SSO services where the trait indicates
// that signing is strictly unnecessary and that signing the request does
// not impact the behavior of the APIs.

Wondering if we should put a check in place in smithy_codegen which throws if we encounter an @optionalAuth trait which is not in one of these services 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

For example, we could add this to packages/smithy_codegen/lib/src/generate.dart:

/// Asserts that the `@optionalAuth` trait is only present in Cognito and SSO
/// services, where we know the behavior is that signing is strictly unnecessary.
///
/// Any other services which onboard this trait must be validated for the same behavior
/// before we can generate for them.
class _AssertOptionalAuthVisitor extends CategoryShapeVisitor<Shape> {
  const _AssertOptionalAuthVisitor();

  /// The list of known services where `@optionalAuth` is present and its behavior has
  /// been verified.
  static const _knownServices = [
    'com.amazonaws.cognitoidentity',
    'com.amazonaws.cognitoidentityprovider',
    'com.amazonaws.sso',
    'com.amazonaws.ssooidc',
  ];

  @override
  EnumShape enumShape(EnumShape shape, [Shape? parent]) => shape;

  @override
  ListShape listShape(ListShape shape, [Shape? parent]) => shape;

  @override
  MapShape mapShape(MapShape shape, [Shape? parent]) => shape;

  @override
  MemberShape memberShape(MemberShape shape, [Shape? parent]) => shape;

  @override
  OperationShape operationShape(OperationShape shape, [Shape? parent]) {
    if (shape.hasTrait<OptionalAuthTrait>() &&
        !_knownServices.contains(shape.shapeId.namespace)) {
      throw StateError(
        'Shape has @optionalAuth but it is not from a validated service: '
        '${shape.shapeId}',
      );
    }
    return shape;
  }

  @override
  ResourceShape resourceShape(ResourceShape shape, [Shape? parent]) => shape;

  @override
  ServiceShape serviceShape(ServiceShape shape, [Shape? parent]) => shape;

  @override
  SetShape setShape(SetShape shape, [Shape? parent]) => shape;

  @override
  SimpleShape simpleShape(SimpleShape shape, [Shape? parent]) => shape;

  @override
  StructureShape structureShape(StructureShape shape, [Shape? parent]) => shape;

  @override
  UnionShape unionShape(UnionShape shape, [Shape? parent]) => shape;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the comment.

As for the check, I think we have to be able to assume traits mean the same thing across services. In this case "optionalAuth" means that signing the request is not required, has no impact on the behavior.

//
// This is only set in Cognito and SSO services where the trait indicates
// that signing is strictly unnecessary and that signing the request does
// not impact the behavior of the APIs.
if (isOptional) return request;

final signer = AWSSigV4Signer(
credentialsProvider: credentialsProvider,
algorithm: algorithm,
Expand Down
Loading