aws-amplify / amplify-js

A declarative JavaScript library for application development using cloud services.
https://docs.amplify.aws/lib/q/platform/js
Apache License 2.0
9.4k stars 2.11k forks source link

Access token passed to appsync resolver causes missing email claim #4751

Open atlowell-smpl opened 4 years ago

atlowell-smpl commented 4 years ago

This is half bug, half feature request, but I have decided that it fits better as a "bug" due to reasons listed below.

Describe the bug The issue is described here, but I will summarize it below.

When using Appsync with a cognito user pool with email enabled, $ctx.identity.claims.email is missing in the resolver for a mutation if that mutation is called using the amplify-js library. The issue does NOT exist when called using the query editor in Appsync itself.

This causes problems when using a cognito user pool where users sign up by email address rather than specifying a custom username. Since the email address is the human-readable identifying factor in an account, it is desired that it be stored on the profile level. However, in this use case, the $ctx.identity.username field is identical to the $ctx.identity.sub field (the account's UUID), so to obtain a user's email address without obtaining the entire profile, it must be passed via claims.

To Reproduce Steps to reproduce the behavior:

  1. Signin using cognito user pools via amplify-js
  2. Call the generated graphql mutation function
  3. In the resolver, check if $context.identity.claims.email is null (using $util.isNullOrEmpty())
  4. Find that $context.identity.claims.email is null because is does not exist

Expected behavior $context.identity.claims.email should be populated with the cognito user's email address

Cause of issue The issue is caused by the passing of the Access Token as authorization, while as claims are supposed to be obtained from the ID Token. See here (pointed out by cy6581 in the above stack overflow thread).

Using the Access token for claims is not in line with the cognito user pool documentation, which is why I classify this as a "bug". See here:

The ID Token contains claims about the identity of the authenticated user such as name, email, and phone_number. The Access Token grants access to authorized resources.

dantasfiles commented 4 years ago

I had a similar issue with the mock API: the mock API passes the id token but the real API passes the access token. I argued the opposite: both should use the access token by default. aws-amplify/amplify-category-api#352 Either way, you're right that all of the methods (mock API, Appsync console, real API) should be consistent in their use of either the id token or the access token.

manueliglesias commented 4 years ago

Hi @atlowell-smpl

What @dantasfiles points out is correct, the console uses the ID token and Amplify uses the acces token.

With code like this you can make Amplify use the ID token too:

const Amplify = require('aws-amplify').default;

Amplify.configure({
    API: {
        aws_appsync_graphqlEndpoint: 'https://xxxxxxxxxxxxxxxxxxxxxxxxxx.appsync-api.us-east-1.amazonaws.com/graphql',
        aws_appsync_region: 'us-east-1',
        aws_appsync_authenticationType: 'NONE',
        graphql_headers: async () => ({
            'Authorization': (await Auth.currentSession()).getIdToken().getJwtToken()
        })
    }
});

Amplify.API.graphql({
    query: 'query { someType { someField } }',
}).then(console.log, console.warn);

I hope this helps.

atlowell-smpl commented 4 years ago

@manueliglesias that is actually what we have been doing to get this to work, but it seems more like a workaround than a fix. Considering appsync has support for claims, and cognito claims expect the id token, I think the default token passed to appsync by amplify should be the id token. But as @dantasfiles also mentioned, there should at least be consistent use of tokens across services.

sbyaniv commented 4 years ago

@manueliglesias

Worked for us :)

scfox commented 4 years ago

+1 for at least making this consistent. Wasted many hours diagnosing why same exact user query would work in app sync console but fail as unauthorized in actual client. The reason is console uses id token and Amplify client uses access token, which does NOT include custom attributes like the id token does. Above fix resolved for me (thanks!), but feels like a hack. If it is supposed to be access token for better security, then the app sync console should also use it, and custom attributes should be included in the access token so that we can write authorization rules against custom attributes.

michaelbrewer commented 3 years ago

+1 for always using id token (or at least to support both)

dylan-westbury commented 3 years ago

Is there any security concerns with using ID token? Would this be trusting information that the client could alter the claims?

henryjarend commented 3 years ago

I ran into this as well when I was using users emails as custom claims (auth is setup to use email to sign-in so sub is a UUID) and the only way to get it working was to configure react to send the id token instead of the access token. Based on the threads here there is some controversy on whether that's viewed as secure but I'd really like to have it be consistent between the AppSynce console and the react web app regardless

ronaldocpontes commented 3 years ago

Having the same issue with lots of hours spent trying to debug and fix. Is there an 'official' way to do this? What would be the best practice?

halilduygulu commented 3 years ago

me too. spent a day on this reading all docs and google results. https://github.com/aws-amplify/amplify-cli/issues/7386

eettaa commented 2 years ago

+1 for making this consistent. I just spent many hours debugging the details of the Amplify Auth library (access token), the Amplify mock api GraphIQL auth mocking (id token afaict), and the cloud-based AppSync Query generator (id token). This behavior is inconsistent and totally undocumented.

ritxweb commented 1 year ago

+1. Spent so many hours till find this workaround. This is another failure (among many others) that is not documented and makes really dificult to work with DataStore. Is the workaround pointed out by @manueliglesias the only possible solution or we can spect this to be solved in the near future?

halilduygulu commented 1 year ago

@ritxweb does not look like it will be fixed anytime soon, considering my message was in May 23, 2021, original request is in Jan 2020.

mattiLeBlanc commented 11 months ago

I work around this like this: #set($userId = $util.defaultIfNullOrBlank($ctx.identity.claims.username, $ctx.identity.claims["cognito:username"])). So that resolver will work both in Appsync Query console and when calling the endpoint via your app.

@chrisbonifacio What I really find a missing functionality is that the AccessToken is send, which doesn't include any of the custom attributes I added to my tokens via the PreToken Lambda.

I need to do this for security reasons, and speed. I am enriching the idtoken with the facilityId of the Cognito user, so that when I run a resolver to update a record that belongs to a facility, I can fetch the facilityId from the claims directly in the resolver and I don't have do an extra query to fetch the facilityId of that Cognito user. It would mean I have to turn many resolvers in pipeline resolvers and that is just a lot of extra templates and overhead.

OR, I do everything in a lambda resolver, which is what I mostly do now because VTL sucks and it a shame I can't get all claims data I need directly, unless I pass in extra resolver inputs (but thoses can be manipulated).

Is this something that is on the radar for Appsync/Cognito?

I considered for one moment to add Cognito Groups with the facilityID and add cognito users to facility Id groups, but I am maxed at 10K groups and I think it will become very messy.

Another solution would be to separate Groups and Roles in Cognito, so you can make logical groups/folders to group your users in and use Roles to assign permissions which you can use in Appsync. I hate to mix folders and permission groups into Groups.

Just some thoughts.

*EDIT: I did see the solution to send idToken to Appsync. Is there any security concern doing this?

mattiLeBlanc commented 11 months ago

Hi @atlowell-smpl

What @dantasfiles points out is correct, the console uses the ID token and Amplify uses the acces token.

With code like this you can make Amplify use the ID token too:

const Amplify = require('aws-amplify').default;

Amplify.configure({
    API: {
        aws_appsync_graphqlEndpoint: 'https://xxxxxxxxxxxxxxxxxxxxxxxxxx.appsync-api.us-east-1.amazonaws.com/graphql',
        aws_appsync_region: 'us-east-1',
        aws_appsync_authenticationType: 'NONE',
        graphql_headers: async () => ({
            'Authorization': (await Auth.currentSession()).getIdToken().getJwtToken()
        })
    }
});

Amplify.API.graphql({
    query: 'query { someType { someField } }',
}).then(console.log, console.warn);

I hope this helps.

This solution actually causes issues when you want to use Auth mode AWS_IAM. This works:

 graphql_headers: async () => ((await Auth.currentUserInfo()) ? {
    'Authorization': (await Auth.currentSession()).getIdToken().getJwtToken()
  } : '')
uclaeamsavino commented 8 months ago
const Amplify = require('aws-amplify').default;

Amplify.configure({
    API: {
        aws_appsync_graphqlEndpoint: 'https://xxxxxxxxxxxxxxxxxxxxxxxxxx.appsync-api.us-east-1.amazonaws.com/graphql',
        aws_appsync_region: 'us-east-1',
        aws_appsync_authenticationType: 'NONE',
        graphql_headers: async () => ({
            'Authorization': (await Auth.currentSession()).getIdToken().getJwtToken()
        })
    }
});

This is great. But FYI - Amplify ignores the custom graphql_headers configuration when initiating subscriptions. It still sends the Access Token instead of the ID Token. This is causing a lot of pain for our app.

Here's the code in Amplify for subscriptions that hardcodes using the Access Token for auth type = userPool: https://github.com/aws-amplify/amplify-js/blob/main/packages/api-graphql/src/Providers/AWSAppSyncRealTimeProvider/index.ts#L985

private async _awsAuthTokenHeader({ host }: AWSAppSyncRealTimeAuthInput) {
    const session = await fetchAuthSession();

    return {
        Authorization: session?.tokens?.accessToken?.toString(),
        host,
    };
}
private async _awsRealTimeHeaderBasedAuth({

...

    userPool: this._awsAuthTokenHeader.bind(this),
ggorge-etiqa commented 2 months ago

We are working with NextJs and amplify v5. We tried the workaround explained here to configure the IdToken at application level but it makes SSR graphQL queries(withSSRContext) no longer working.

For example this snippet gives "no current user" error.

      const SSR = withSSRContext({ req });
      const result = await SSR.API.graphql({
        query: myQuery,
        variables: {
          foo: bar
        },
      });
  1. Is it something that we can fix in VTLs ?
  2. Using AccessToken for some API calls and IdToken for others can lead to problems?