aws-amplify / amplify-cli

The AWS Amplify CLI is a toolchain for simplifying serverless web and mobile development.
Apache License 2.0
2.81k stars 821 forks source link

Connect a cognito user with an AppSync type #1313

Closed punksta closed 5 years ago

punksta commented 5 years ago

Which Category is your question related to? how to? What AWS Services are you utilizing? amplify, react-native, AppSync, Auth Provide additional details e.g. code snippets I am trying to build an app where a user can be added to friends by other user. Users should have name, avatar(image on s3), email and list of friends. Also users should be able to fetch its list of friends and see avatars and names.

type User @model {
  id: ID!
  name: String!
  avatar: String
  friends: [User] @connection(name: "friends")
}

So my questions:

I've found this in samples, https://github.com/aws-samples/aws-appsync-chat/blob/f1bc066fe28e95995db0492ecc3441de61561e2d/src/mobx/UserStore.js#L20-L24 I think that's not a secure way. In this approach any cognito user can create appsync user record with any "username" multiple times.

jkeys-ecg-nmsu commented 5 years ago

@punksta you can use the token's sub field as the ID for your User field to enforce a 1-1 constraint.

+1 because I would like to see this type of functionality built-in (what application with authenticated users won't have a User model in its schema)?

evelant commented 5 years ago

@jkeys-ecg-nmsu Can you provide a quick example of how to do that? I'm just getting started with amplify and am also curious about how to manage user model data. Would I just create a user object immediately after signup? Can you point me to documentation about the token and sub field? Thanks!

jkeys-ecg-nmsu commented 5 years ago

@AndrewMorsillo this documentation shows how you can get the payload (including the sub and username claims) from the ID token. https://docs.aws.amazon.com/cognito/latest/developerguide/amazon-cognito-user-pools-using-tokens-with-identity-providers.html#amazon-cognito-user-pools-using-the-id-token

You would probably use an admin Cognito user to create your user in a Lambda trigger, which would require setting the owner field manually (otherwise your admin user will be the owner of the record). You can do that in the migrate users trigger or pre-token generation triggers, I believe.

https://docs.aws.amazon.com/cognito/latest/developerguide/user-pool-lambda-migrate-user.html

jkeys-ecg-nmsu commented 5 years ago

@haverchuck maybe there is space for an @user directive to specify that a model represents a user, then automatically configure resolvers to create the model with the ID! field as the cognito:username or sub?

evelant commented 5 years ago

Is it even necessary to do that? Would it be sufficient to do the following:

type User @model @auth(rules: [{ allow: owner }]) {
    id: ID!
    avatarUrl: String
   ....etc
}

If I understand correctly the auth directive will restrict that document to its Cognito owner, no custom triggers necessary.

jkeys-ecg-nmsu commented 5 years ago

Yeah but it won't enforce the constraint that the id field must be their cognito:username or cognito:sub, so you'll need to make sure you enforce that constraint manually. Also subscriptions are not yet protected by @auth directive, so if you need to subscribe to user events you may need to write your own resolvers.

https://github.com/aws-amplify/amplify-cli/issues/1043

jkeys-ecg-nmsu commented 5 years ago

@AndrewMorsillo please close this issue if your OP was answered though, I don't think my proposal deserves its own issue and it sounds like your issue is resolved.

kwhitejr commented 5 years ago

@AndrewMorsillo If you figure that out, please post back here. I've seen a lot of docs suggest the Cognito --> Lambda --> AppSync --> DynamoDb work flow to build out a Users db from Cognito events, but I haven't found any complete examples, i.e. with full CloudFormation / SAM reference architecture. Indeed, to the best of my understanding, SAM does not support Cognito as a triggering event (docs).

jkeys-ecg-nmsu commented 5 years ago

@kwhitejr it helps if your primary key can be stored in the token, which is why this issue is actually a good solution IMO (since username/sub is not useful to an attacker and is available wherever the token is).

I am unfamiliar with SAML and can't comment on that, might be worth its own issue though, if only to request documentation. Not sure about creating a CF/SAM template either, lol. @haverchuck

https://docs.aws.amazon.com/cognito/latest/developerguide/cognito-user-identity-pools-working-with-aws-lambda-triggers.html

kwhitejr commented 5 years ago

+1 because I would like to see this type of functionality built-in (what application with authenticated users won't have a User model in its schema)?

@jkeys-ecg-nmsu Your main point here is on the button. I'm still learning Cognito, but I also concur that the sub field seems best suited to map to an AppSync schema id.

For example

type User @model { // createUser mutation handled by Lambda in response to Cognito events
  userId: ID! # cognito.identity.sub
  username: String
  posts: [Post] @connection(name: "UserPosts")
}

type Post @model { // createPost mutation comes from authorized client
  authorId: ID! # User.userId
  slug: String # unrelated issue, but authorId+slug is the preferred Primary Key
  author: User! @connection(name: "UserPosts")
  text: String
}
evelant commented 5 years ago

Yikes! Subscriptions, connections, and search aren't protected by the auth directive? That really limits its usefulness. It seems everywhere I look there are undocumented landmines (batch invoke limit of 5 I found out about most recently) in amplify waiting to bite us if we choose to use it for our project...

mikeparisstuff commented 5 years ago

@AndrewMorsillo As of two weeks ago, @connections are now protected by the @auth directive. Support for protecting subscriptions are coming soon and is detailed in #1043.

In general, to connect a cognito user to a @model user, you can add a custom or lambda resolver to a field on user that resolves the object from Cognito. You can see the answer to #1352 which addresses a similar topic.

et304383 commented 5 years ago

https://github.com/aws-amplify/amplify-cli/issues/1313#issuecomment-483383520

This suggestion is exactly what we need. I shouldn't have to write custom resolvers to support a one to one mapping between cognito user and dynamodb user that has been practice for years when using federated identities with IAM restrictions to allow a user to only access their own record.

Hell, you even do this in Amplify for S3 buckets - you use IAM restrictions on the federated auth role to allow a user to only access private content based on their sub. Why wouldn't you follow the same model for DynamoDB records managed via AppSync?

And for the love of god, use the sub, not the username. The username field can technically be reused and is a potential security hole (let's ignore the fact that the auth directive uses username for now).

mikeparisstuff commented 5 years ago

@et304383

An @user directive is a matter of prioritization as I agree that it is a useful construct. When we have support for loading your own transformers then you will be able to implement this on your own and/or share directives via npm.

You can specify the identity token attribute that you want to use for authorization checks (try @auth(rules: [{ allow: owner, identityField: "sub" }])).

mikeparisstuff commented 5 years ago

I have added this as an example in the Amplify docs. As soon as these PRs from the docs (aws-amplify/docs#640) and cli (#1346) are merged you will be able to add this without much trouble.

jan-wilhelm commented 5 years ago

Any updates on this? Having to manually write a Lambda triggered by Cognito events to create an AppSync User is really painful compared to the easy development Amplify promises. Isn't connecting a Cognito User to his AppSync model one of the most basic questions of every Amplify application?

jan-wilhelm commented 5 years ago

For example, how would you, at the moment, let a User create another User? From my perspective, there are two options that are both not very clean:

  1. You could offer a CreateUser GraphQL mutation. This would take all the required attributes of a new User, including all relevant Cognito authentication information (email, password, username etc.). You could then add a custom request resolver to the User model that creates a Cognito user with the supplied information (email, password...). The request resolver could then use the sub of the newly created Cognito user as the id of the User model.

  2. You could also just hide / remove the CreateUser GraphQL mutation and expose a Lambda function to the client. This would work similar to a REST Api so that a user could call into the function and supply all relevant information (both for Cognito data and AppSync User model). The Lambda function could then build out a Cognito user from the email, password etc. and use the user's sub as the ID on a new AppSync User. The AppSync User would be created with the remaining attributes supplied to the Lambda function, depending on your use case.

This option does not really seam viable since it would expose a second, different type of API (REST) next to the AppSync GraphQL API.

What would you recommend? Is option #1 the right one? Is there a better alternative that I've missed?

babus commented 5 years ago

May be something worth sharing. Had a talk with the AWS support regarding the same requirement. I was suggested to use AppSync + DynamoDB to keep all the user data collected from the user himself and use Cognito just for provisioning tokens. The docs says the same but it is not very clear.

Note

If you're new to Amazon Cognito Sync, use AWS AppSync. Like Amazon Cognito Sync, AWS AppSync is a service for synchronizing application data across devices.

It enables user data like app preferences or game state to be synchronized. It also extends these capabilities by allowing multiple users to synchronize and collaborate in real time on shared data.

Likely, the Cognito Streams might be deprecated soon which is what I heard but I may be wrong. The suggestion to keep the user data collected from the User seems fitting to me as I had to provision ownership of the record only to the matching user sub which created it. Though it might differ for other's use cases.

ataibarkai commented 4 years ago

@mikeparisstuff are there updates on this by any chance? The suggestion of using a lambda resolver to essentially forward User queries to the underlying Cognito table is problematic because:

morgler commented 3 years ago

No progress on this for over a year? It would be awesome to have this @user built in – it's such a basic thing virtually every application using AppSync with Cognito needs. Having to write our own sync lambda to add/update user attributes between Cognito and AppSync tedious and error-prone.

pedroapfilho commented 3 years ago

And yet, today, we don't have a clear link here, that points to a place in the docs that responds to this issue. Even if we have to use the @function, where can we see any docs about it?

racer161 commented 3 years ago

Not sure who needs to see this, but after looking over the repo in the OP, I noticed a really important distinction about the user object that solves (probably?) the duplicated username problem.

You can see it in the schema definition for user:

type User 
  @model 
  @auth(rules: [{ allow: owner, ownerField: "id", queries: null }]) {
  id: ID!
  username: String!
  conversations: [ConvoLink] @connection(name: "UserLinks")
  messages: [Message] @connection(name: "UserMessages")
    createdAt: String
    updatedAt: String
}

Notice the @auth rule assigns the owner field to be the ID of the item in DynamoDB. This has the effect that when the resolvers call putItem() on the actual underlying DynamoDB table it will prevent duplication and simply replace the item with updated information. At a cursory glance, this seems to be the only downfall of this strategy. That any (unchecked) call from the user on to create a new user will result in an overwrite if a user already exists. I'm sure this could be abused somehow but at the very least it would only affect one user in the even of password theft or similar.

At the same time, the @auth rule prevents anyone creating a record in the table with an ID other than their username. Since the putItem call will always go through the resolvers.

This essentially enables the User to manage their own data. You could easily add a User creation screen on first login.

It feels like this strategy has many pitfalls I can't think of right away. Namely the reliance on two sources of truth, DynamoDB and Cognito. But practically it seems like a less error prone alternative to lambda since it has lower level duplication prevention at the DynamoDB level.

github-actions[bot] commented 3 years ago

This issue has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels for those types of questions.