fix(Core): support identityClaim "sub"

[wooj2]

In attempts to resolve:
#485

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[lawmicha]

i think these might be "cognito:username" https://github.com/aws-amplify/amplify-cli/blob/master/packages/amplify-codegen-appsync-model-plugin/src/__tests__/visitors/appsync-swift-visitor.test.ts#L1012

and "sub" looks correct: https://github.com/aws-amplify/amplify-cli/blob/master/packages/amplify-codegen-appsync-model-plugin/src/__tests__/visitors/appsync-swift-visitor.test.ts#L1120

[richardmcclellan]

This differs from the current Android logic, here. My understanding is that Android gets the username from the decrypted authToken instead of from getUsername, which is necessary to support email/phone sign in.

@TrekSoft can you confirm?

[wooj2]

Tested w/ schema of:

type ScenarioATest6Post @model @auth(rules: [{ allow: owner, identityClaim: "username" }]) {
  id: ID!
  title: String!
}

and

type ScenarioATest6Post @model @auth(rules: [{ allow: owner}]) {
  id: ID!
  title: String!
}

in both cases, in our model generated schema, we see:

    
    model.authRules = [
      rule(allow: .owner, ownerField: "owner", identityClaim: "cognito:username", operations: [.create, .update, .delete, .read])
    ]

[lawmicha]

change looks good as incremental improvement as it would support sub and username (iff user uses username for sign in). Considering Android's current implementation, we should probably keep #485 open to address the username as email and phone sign in to do a similar JWT parse to extract the username out

[lawmicha]

i wonder, if the auth plugin's get username behaves differently on Android and iOS such that on iOS we can rely on getUsername

[TrekSoft]

We shouldn't rely on getUsername - Cognito is comparing what is sent to what is in the token and it simplifies the code as well to just do it like Android is where we simply pull the right value from the token. With custom identity claims, we can pull whatever other value is specified.

[wooj2]

@TrekSoft & @richardmcclellan -- thanks for calling this out.. Looked into implementing the parsing of the JWT token, and it's a bit more work than expecting. For this reason, we are going to push this code in and view it as an incremental improvement, while keeping #485 open to ultimately fix it in the manner you have suggested. Thanks again for extra eyes on this.

[lawmicha]

approved if u want to merge this in as incremental support, but should consider Android's implementation differences and their requirement for parsing the JWT token