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

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

Merged
merged 6 commits into from
Sep 25, 2023

Conversation

Jordan-Nelson
Copy link
Member

@Jordan-Nelson Jordan-Nelson commented Aug 31, 2023

Issue #, if available:

Description of changes:

  • Update WithSigV4 to not sign requests that have optional auth
  • Update amplify auth to only use local credentials to determine if a user is logged in prior to calling sign in
  • Update the Authenticator to only use local credentials to determine if a user is logged in prior to calling sign in

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 marked this pull request as ready for review September 1, 2023 15:15
@Jordan-Nelson Jordan-Nelson requested a review from a team as a code owner September 1, 2023 15:15
@Jordan-Nelson Jordan-Nelson changed the title chore: do not sign optional requests fix: do not sign requests that have optional auth Sep 1, 2023
}
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.

Comment on lines +204 to +205
await Amplify.Auth.getCurrentUser();
return true;
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?

Equartey
Equartey previously approved these changes Sep 21, 2023
@Jordan-Nelson Jordan-Nelson dismissed stale reviews from Equartey and dnys1 via 8755015 September 21, 2023 16:03
Co-authored-by: Dillon Nys <24740863+dnys1@users.noreply.github.com>
@Jordan-Nelson Jordan-Nelson merged commit 4211e98 into main Sep 25, 2023
71 of 73 checks passed
@Jordan-Nelson Jordan-Nelson deleted the feat/optional-auth branch September 25, 2023 13:13
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.

3 participants