Owner @auth authorization limited to fields that are String type

hisham commented 3 years ago

Before opening, please confirm:

[X] I have installed the latest version of the Amplify CLI (see above), and confirmed that the issue still persists.
[X] I have searched for duplicate or closed issues.
[X] I have read the guide for submitting bug reports.
[X] I have done my best to include a minimal, self-contained set of instructions for consistently reproducing the issue.

How did you install the Amplify CLI?

No response

If applicable, what version of Node.js are you using?

No response

Amplify CLI Version

latest

What operating system are you using?

OSX

Amplify Categories

auth, api

Amplify Commands

codegen

Describe the bug

We have a custom attribute in cognito that is of "number" type.

We'd like to use this attribute as part of owner authorization via the identityClaim attribute.

The field is stored in dynamo as a "Float" type.

In the vtl resolver however, the identityClaim is returned as a string, and so the owner authorization comparison fails (string compared to float) and so owner authorization never works.

I'll send example code / screenshot to amplify-cli@amazon.com

Expected behavior

1) JWT Token decoding should respect cognito attribute type (something that is a number should be returned as number not string)

2) Owner authorization should work with both strings and numbers

Reproduction steps

I sent code that reproduces with annotations to amplify-cli@amazon.com

GraphQL schema(s)

Sent to amplify-cli@amazon.com

Log output

``` # Put your logs below this line ```

Additional information

At minimum this should be documented in amplify cli docs, that owner authorization only works with string fields.

attilah commented 3 years ago

@hisham beside not working could you give more details about this like:

if you decode the JWT token is your custom attribute value still in number format and not in quotes?
how is the custom attribute value gets into the identity as a claim? is the number type preserved?
if you dump $context.identity in appsync what value do you see string or number?

My suspicion is that the "number" type information is lost along the lines somewhere hence the comparison will fail in the resolver.

hisham commented 3 years ago

Hi @attilah -

If I decode the JWT token the custom attribute is in quotes (string) - I'll email an example jwt token to amplify-cli@

I had to use below code to add the id token as the authorization header. See https://github.com/aws-amplify/amplify-js/issues/4751#issuecomment-613717344.

Amplify.configure({
...awsmobile,
// Send id token instead of access token for authorization
// Needed for @auth graphql directives to work with cognito custom attributes
// Remove once https://github.com/aws-amplify/amplify-js/issues/4751 is resolved
graphql_headers: async () => ({
Authorization: (await Auth.currentSession()).getIdToken().getJwtToken(),
}),
});

3) Yes when I dump $context.identity the attribute is a string

You are correct in terms of that this issue is the number type information doesn't seem to be respected in the jwt token and when the jwt token is decoded it becomes a string. I don't know if this a JWT standard thing or cognito limitation. But I do see other JWT token attributes that are numbers (auth_time, exp, iat).

Thanks! Hisham

hisham commented 3 years ago

Even if the attribute is returned as number by jwt decoding, the $util.isString code in vtl will return false and so the numbers won't be compared. So there's two issues here - the "number" cognito custom attribute becomes a string at some point, and the owner authorization code in the vtl only does comparisons if the id is of type string.

hisham commented 3 years ago

Looks like cognito doesn't truly support non-string custom attributes, they are always turned into a string in requests. For example, in my pre token lambda, I check event.request.userAttributes, and my custom attribute there is a string even though it's classified as number type in cognito. I even tried to convert it back to number via claimsToAddOrOverride but that didn't work.

Looks like another developer ran into this as well - see https://github.com/aws-amplify/amplify-js/issues/2958#issuecomment-774022032 and https://bobbyhadz.com/blog/aws-cognito-amplify-bad-bugged#default-behavior-2

hisham commented 3 years ago

I ended up writing a custom graphql transformer that converts the id from number to string before the auth check then converts it back to number before the dynamodb save.

It works but I ran into trouble with the vtls - there's no official supported way to convert string to number. I did $myInt.toString() initially, which works with amplify mock and appsync testing console, but not the "real" appsync backend - it converted the number to a string with scientific notation, so then I used the $String.format("%.0f", $ctx.args.input.createdBy) notation below which works in the "real" appsync backend environment but not amplify mock (returns null) or the appsync aws testing console (throws 400 error).

So I might just succumb and not fight the amplify framework anymore and instead store the id I'm interested in as string...

So now there's 3 issues this whole thing uncovered:

1) Cognito does not support number custom attributes properly - it converts them to string in the jwt token 2) @auth transformer limits checks to string or list attributes (e.g. $util.isString) 3) The AppSync vtl environment does not have an officially supported way of converting a string to a number. And there are differences in implementation between the "real" appsync, amplify mock, and the testing console in the aws appsync console.

Example vtl code my custom transformer generates is below:

## [Start] Setting "createdBy" from number to string before auth check. **
#set( $createdByOriginalValue = $ctx.args.input.createdBy )
#if( $ctx.args.input.createdBy )
  #set( $String = '' )
  $util.qr($ctx.args.input.put("createdBy", $String.format("%.0f", $ctx.args.input.createdBy)))
#end
## [End] Setting "createdBy" from number to string before auth check. **

and then the following after the auth check:

## [Start] Setting "createdBy" back from string to number after auth check. **
#if( $ctx.args.input.createdBy )
  $util.qr($ctx.args.input.put("createdBy", $createdByOriginalValue))
#end
## [End] Setting "createdBy" back from string to number after auth check. **

So @attilah I'm not sure what to do here, whether I should keep my custom transformer workaround until you guys have a better solution or just store everything as string. My concern with my workaround is I lose out on using amplify mock, and that I am converting number to string in a potentially not supported way in AppSync.

hisham commented 3 years ago

I made the string -> number conversion in both mock and real appsync environments by checking the resulting string from toString and using String.format if it has non-digit characters:

  #if( $util.matches('.*\D.*', $ctx.args.input.createdBy.toString()) )
    $util.qr($ctx.args.input.put("createdBy", $String.format("%.0f", $ctx.args.input.createdBy)))
  #else
    $util.qr($ctx.args.input.put("createdBy", $ctx.args.input.createdBy.toString()))
  #end

So I have a solution that works now though hacky. Amplify team should decide whether to support Int/Float fields for @auth owner fields. Many systems still use numbers for ids - for example, relational db primary keys are typically bigint.

aws-amplify / amplify-category-api