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

DataStore sync fails for any type with multiple cognito @auth #6023

Closed
paddlefish opened this issue Dec 1, 2020 · 9 comments
Closed

DataStore sync fails for any type with multiple cognito @auth #6023

paddlefish opened this issue Dec 1, 2020 · 9 comments
Assignees
Labels
@auth Issues tied to @auth directive bug Something isn't working graphql-transformer-v1 Issue related to GraphQL Transformer v1 graphql-transformer-v2 Issue related to GraphQL Transformer v2

Comments

@paddlefish
Copy link

paddlefish commented Dec 1, 2020

Describe the bug
I'm not sure if this is a documentation bug, a cli bug, or an appsync bug. It probably could be handled in any of those ways, i'll leave it up to you.

If you declare a type in your schema.graphql and add two or more cognito auth types to it, then datastore will no longer be able to sync that type.

If this is the expected behavior then I suggest a warning be added to the documentation, e.g. here:

https://docs.amplify.aws/lib/datastore/setup-auth-rules/q/platform/js

where it describes various auth access patterns to warn that you must not mix these. For example this is a valid auth decoration for a type:

@auth(rules: [{ allow: owner }])

But the following declaration will result in a strange error ("errors":[{"message":"Validation error of type UnknownArgument: Unknown field argument owner @ 'onCreateXYZ'"}]}) when datastore tries to sync:

@auth(rules: [ 
{ allow: private, operations: [read] }
{ allow: private, provider: iam, operations: [create, read, update, delete] }
{allow:owner,ownerField: "owner" }
])

The documentation should caution against adding extra cognito auth types, and include the error message so that someone who encounters it will be lead to that warning when doing a search of the documentation.

Furthermore, I think the aws cli should detect when the schema auth params are invalid and issue a warning when you run amplify push. Because the sooner you can detect an error the better. As it is now, it's a runtime error when datastore tries to sync! It'd be much better to catch the error when running codegen or pushing.

Finally, I'm not convinced this isn't an outright bug in how the appsync schema and resolvers get generated. Why does it build resolvers that do NOT take an owner argument when additional cognito auth are found? Maybe it should generate uniquely named resolvers for each auth? or maybe it should take an optional owner and use one auth if it's provided and the other if not? or maybe appsync codegen should fail if it sees multiple cognito auth params since at best it is only guessing about which one to make a subscription for.

Background:

The reason this results in an error is because the generated schema in appsync loses the owner attribute to the Subscription type. Here are before/after screenshots from the AppSync console viewing the schema:

This shows the correct Subscription schema, where onCreateX, etc... have an owner attribute:

Screen Shot 2020-11-30 at 11 53 48 AM

This shows the invalid Subscription schema that gets created if additional cognito @auth attributes are included. Notice how onCreateX lacks the owner attribute:

Screen Shot 2020-11-30 at 11 49 13 AM

Amplify CLI Version
4.36.2

To Reproduce
Create an amplify project using datastore
add { allow: private, operations: [read] } to the auth for a type
amplify push
run the app

Expected behavior
amplify push failed with a message: You cannot use two kinds of cognito auth on a type with the path to the scema.graphql file and the line number this is on and the name of the type.

Desktop (please complete the following information):

  • OS: MacOS 10.15.7 (19H15)
  • Node Version. v14.9.0

Additional context
This problem was clarified with the help of Ivan (ObjectiveCat) on a chat

I want to add access to a GraphQL object via a lambda. It's so that when a new user signs up, the DynamoTable gets a user object created. This is to workaround an issue where if the client creates the user object, there is a possibility of two of them getting created.  However, when I change @auth(rules: [{ allow: owner }]) on my type User to   @auth(rules: [ { allow: private, operations: [read] }      { allow: private, provider: iam, operations: [create, read, update, delete] }{allow:owner,ownerField: "owner" }]) the generated schema in appsync breaks: onCreateUser(owner: String!): User @aws_subscribe(mutations: ["createUser"]) onUpdateUser(owner: String!): User @aws_subscribe(mutations: ["updateUser"]) onDeleteUser(owner: String!): User @aws_subscribe(mutations: ["deleteUser"])  loses the (owner: String!) parameter
paddlefish replied to paddlefishYesterday at 12:00 PM
I've seen other people complaining about the resulting error here: "errors":[{"message":"Validation error of type UnknownArgument: Unknown field argument owner @ 'onCreateXYZ'"}]}
I've tried reordering the arguments to @auth but right now it looks like if you add anything other than owner auth, datastore subscriptions break.
IvanYesterday at 1:17 PM
@paddlefish DataStore doesn't currently support using multiple authorization types, e.g. cognito + iam
paddlefish replied to IvanYesterday at 1:18 PM
ugh. that's too bad.  So there's no way to talk to the database that's syncing to DataStore via a lambda?
paddlefish replied to paddlefishYesterday at 1:18 PM
Or .. maybe I could talk to it via the dynamo APIs but not via AppSync?
IvanYesterday at 1:20 PM
DataStore will use whatever is the default auth type (aws_appsync_authenticationType property in aws-exports).  So you can have other rules in there for your lambda to use but DataStore itself won't be able to utilize them
paddlefish replied to IvanYesterday at 1:21 PM
"aws_appsync_authenticationType": "AMAZON_COGNITO_USER_POOLS",
IvanYesterday at 1:23 PM
Which version of Amplify are you using?
paddlefishYesterday at 1:23 PM
i try to be up to date...4.36.2 of the clie
IvanYesterday at 1:23 PM
What about the library?
paddlefishYesterday at 1:24 PM
which part ... how do i tell?
IvanYesterday at 1:24 PM
In your package.json
paddlefish replied to IvanYesterday at 1:24 PM
➜ grep amplify package.json
    "@aws-amplify/analytics": "^3.3.10",
    "@aws-amplify/core": "^3.8.2",
    "@aws-amplify/datastore": "^2.8.1",
    "@aws-amplify/storage": "^3.3.10",
    "aws-amplify": "^3.3.7",
    "aws-amplify-react-native": "^4.2.0",
IvanYesterday at 1:26 PM
You shouldn't need to install those @aws-amplify/* packages if you have aws-amplify installed. That can cause conflicts although I don't think that's the issue here
paddlefishYesterday at 1:26 PM
good to know, i can yank those out
as to my original observation that the appsync schema loses (owner: String) when i add additional auth types ... this happens when I do amplify push so it's more a matter of the cli, than the node_module, isn't it? Like maybe it's due to graphql-auth-transformer ?
IvanYesterday at 1:29 PM
For your owner auth rule, can you try explicitly specifying operations - {allow:owner, operations: [create, read, delete, update] } You don't need the ownerField if you're just using owner.
paddlefishYesterday at 1:30 PM
yeah, i added ownerField b/c i was trying to fix this issue.  DataStore works if I remove the private auth lines.
after I push and go to AWS AppSync, look at the Schema for my Env, then scroll down to type Subscription I see the onCreateUser has owner again.
So this then leaves me with happy datastore.  but an unhappy lambda that cannot access appsync for that type.
i'm assuming that trying to run datastore from within a lambda would be a fools errand.
IvanYesterday at 1:33 PM
onCreateUser subscriptions will need to have the owner subscriptions argument if you want to use owner auth from DataStore. 

but an unhappy lambda that cannot access appsync for that type
Are you talking about subscriptions? Or which operation(s) are you attempting to perform from your Lambda?
paddlefishYesterday at 1:33 PM
I want to create a User object
The way I read to give lambda access to create was to add this second auth param, via iam.  but datastore doesn't work if i add additional auth to that type, per your first reply
IvanYesterday at 1:35 PM
What I meant is that you can't have DataStore choose between 2 different authorization types, e.g., between a cognito type (owner or group) and IAM. You should still be able to have an IAM type defined on your API
Any type other than the default will just be ignored by DS
paddlefishYesterday at 1:37 PM
ok.... but why is my AppSync schema's Subscription type changing if I add additional auth types to my API?  Even if DS is savvy and can ignore those other types, it's still going to try and subscribe to notifications using onCreateUser(owner: String!): User, right?
...except as soon as I decorate my type in schema.graphql, that subscription changes to onCreateUser: User @aws_subscribe(mutations: ["createUser"]
That's why apollo throws the cryptic error Validation error of type UnknownArgument: Unknown field argument owner @ 'onCreateUser'.  Data store is passing in {"owner": "some uuid"} but the graphql no longer accepts an owner argument
IvanYesterday at 1:42 PM
Let me make sure I understand correctly:
When you just have the {allow: owner} rule, the subscriptions get generated with the owner arguments. 
When you add IAM as an additional auth type (Cognito remains default) the subscriptions no longer get generated with the owner argument. Is that correct?
paddlefishYesterday at 1:43 PM
yes, i believe so!.  Except I don't know how to ensure Cognito remains default.
Cognito is still there, for sure.
but it's not decoratated with @default or something...
IvanYesterday at 1:45 PM
Just the value in aws-exports or in the AppSync Console in settings -> default authorization mode
paddlefishYesterday at 1:45 PM
yes, i Default authorxation mode in AppSync console is Amazon Cognito User Pool
IvanYesterday at 1:51 PM
I think that's because you also have the { allow: private, operations: [read] } which uses Cognito
So if you get rid of that one and just keep your iam and owner rules, it should still generate the subscriptions with the owner sub arg
paddlefishYesterday at 1:53 PM
ah! ok.  Brilliant, I'm pushing that now.
I'm not sure why i had that there.  I think I was trying to also do some other relationships in graphql
IvanYesterday at 1:54 PM
Gotcha (: Let me know if that doesn't fix it
paddlefishYesterday at 1:55 PM
ok, thanks so much Ivan!
IvanYesterday at 1:55 PM
Any time!
paddlefishYesterday at 1:59 PM
Confirmed: My type Subscription is fixed after removing that extra @auth.

@SwaySway
Copy link
Contributor

SwaySway commented Dec 2, 2020

Hello @paddlefish
The library is adding the owner arg here is a part of how the library determines on how to create the subscription request.

When adding another auth mode to protect the subscription the owner argument should be optional in this scenario. Labeling this as a bug.

@SwaySway SwaySway added @auth Issues tied to @auth directive bug Something isn't working DataStore graphql-transformer-v1 Issue related to GraphQL Transformer v1 labels Dec 2, 2020
@paddlefish
Copy link
Author

Thanks @SwaySway let me know if i should revise teh description since I wasn't sure which way to phrase it.

@Jupdi
Copy link

Jupdi commented Feb 6, 2021

We are experiencing the same error when using multiple ownerfields on one model. On all subscriptions, all ownerfields are marked as mandatory...

@Jupdi
Copy link

Jupdi commented Jun 7, 2021

Are there any news when to expect a fix?

@InnovateWithEric InnovateWithEric added graphql-transformer-v1 and removed graphql-transformer-v1 Issue related to GraphQL Transformer v1 labels Feb 8, 2022
@josefaidt josefaidt added graphql-transformer-v1 Issue related to GraphQL Transformer v1 and removed graphql-transformer-v1 labels Feb 8, 2022
@lazpavel lazpavel self-assigned this Feb 16, 2022
@lazpavel lazpavel added the graphql-transformer-v2 Issue related to GraphQL Transformer v2 label Feb 16, 2022
@lazpavel
Copy link
Contributor

lazpavel commented Feb 16, 2022

reproducible with GraphQL Transformer V2 as well, used below schema

type Test
  @model
  @auth(
    rules: [
      { allow: private, operations: [create, read, update, delete] }
      {
        allow: private
        provider: iam
        operations: [create, read, update, delete]
      }
      { allow: owner, ownerField: "owner" }
    ]
  ) {
  id: ID!
}

@iartemiev
Copy link
Member

iartemiev commented Feb 21, 2022

@lazpavel for v2, I think the last auth rule would need to explicitly list the allowed operations since those now follow deny-by-default and the first rule should only allow read.

(This probably doesn't affect the output of the transformer re: subscriptions, but just in case)

type Test
  @model
  @auth(
    rules: [
      { allow: private, operations: [read] }
      {
        allow: private
        provider: iam
        operations: [create, read, update, delete]
      }
      { allow: owner, ownerField: "owner", operations: [create, read, update, delete] }
    ]
  ) {
  id: ID!
}

@lazpavel
Copy link
Contributor

@iartemiev, from V2 docs:

authorized operations (operations): which operations are allowed for the given strategy and provider. If not specified, create, read, update, and delete operations are allowed.

Authorization rules operate on the deny-by-default principle. Meaning that if an authorization rule is not specifically configured, it is denied.

@iartemiev
Copy link
Member

@lazpavel - you're right. I had forgotten that implicitly-defined operations have the same effect as all operations explicitly allow-listed

@lazpavel
Copy link
Contributor

lazpavel commented Mar 8, 2022

This looks to be solved by this PR: #7765, please reopen if issue still persists

@lazpavel lazpavel closed this as completed Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@auth Issues tied to @auth directive bug Something isn't working graphql-transformer-v1 Issue related to GraphQL Transformer v1 graphql-transformer-v2 Issue related to GraphQL Transformer v2
Projects
None yet
Development

No branches or pull requests

7 participants