aws-amplify / amplify-category-api

The AWS Amplify CLI is a toolchain for simplifying serverless web and mobile development. This plugin provides functionality for the API category, allowing for the creation and management of GraphQL and REST based backends for your amplify project.
https://docs.amplify.aws/
Apache License 2.0
89 stars 79 forks source link

RFC - @auth directive improvements #455

Open mikeparisstuff opened 5 years ago

mikeparisstuff commented 5 years ago

RFC - @auth directive improvements

This document will outline designs for a number of new features relating to authentication & authorization within the GraphQL Transform. The goal of these features is too fill holes and introduce new mechanisms that make protecting your valuable information easier.

Proposal 1: Replace 'queries' and 'mutations' arguments with 'operations'

Merged by aws-amplify/amplify-cli#1262

Currently an @auth directive like this:

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

causes these changes to the following resolvers:

  1. Query.getTask - Returns the post only if the logged in user is the post's owner.
  2. Query.listTasks - Filter items such that only owned posts are returned.
  3. Mutation.createTask - If an owner is provided via $ctx.args.input.owner and matches the identity of the logged in user, succeed. If no owner is provided, set logged in user as the owner, else fail.
  4. Mutation.updateTask - Append a conditional expression that will only update the record if the logged in user is its owner.
  5. Mutation.deleteTask - Append a conditional expression that will only delete the record if the logged in user is its owner.

In other words, the @auth directive currently protects the root level query & mutation fields that are generated for an @model type.

Problem: The 'queries' and 'mutations' arguments imply top level protection

GraphQL APIs are a graph and we need to be able to define access rules on any field, not just the top level fields.

Solution

I suggest replacing the queries and mutations arguments on the @auth directive with a single operations argument. This would be the new @auth directive definition.

directive @auth(rules: [AuthRule!]!) on OBJECT
input AuthRule {
    allow: AuthStrategy!
    ownerField: String # defaults to "owner"
    identityField: String # defaults to "cognito:username"
    groupsField: String
    groups: [String]

    # The new argument
    operations: [ModelOperation]

    # Old arguments
    queries: [ModelQuery] @deprecated(reason: "The 'queries' argument will be deprecated in the future. Please replace this argument with the 'operations' argument.")
    mutations: [ModelMutation] @deprecated(reason: "The 'mutations' argument will be deprecated in the future. Please replace this argument with the 'mutations' argument.")
}
enum AuthStrategy { owner groups }

# The new enum
enum ModelOperation { create update delete read }

# The old enums
enum ModelQuery { get list }
enum ModelMutation { create update delete }

This change generalizes the config such that it implies all read operations on that model will be protected. Not just the top level 'get' & 'list' queries. Auth rules that use the 'read' operation will be applied to top level query fields, @connection resolvers, top level fields that query custom indexes, and subscription fields. Auth rules that use the 'create', 'update', and 'delete' operation will be applied to createX, updateX, and deleteX mutations respectively. Those using queries & mutations will have the same behavior and those using operations will get the new behavior. The queries & mutations arguments will eventually be removed in a future major release.

Protect @connections by default

Merged by aws-amplify/amplify-cli#1262

Once the change from queries/mutations -> operations has been implemented, we will want to go back and implement any missing authorization logic in @connection fields by default.

For example, given this schema:

type Post @model @auth(rules: [{allow: owner}], operations: [create, update, delete, read]) {
    id: ID!
    title: String
    owner: String
}
type Blog @model {
    id: ID!
    title: String
    # This connection references type Post which has auth rules and thus should be authorized.
    posts: [Post] @connection
}

The new code would add authorization logic to the Blog.posts resolver such that only owner's of the post would be able to see the posts for a given blog. It is important to note that the new logic will restrict access such that you cannot see records that you are not supposed to see, but it will not change any index structures under the hood. You will be able to use @connection with the new custom index features to optimize the access pattern and then use @auth to protect access within that table or index.

Proposal 2: Implement @auth on @searchable search fields

Github Issues

Problem

Currently Query.searchX resolvers generated by @searchable are not protected by @auth rules.

Solution

The Elasticsearch DSL is very powerful and will allow us inject Elasticsearch query terms and implement authorization checks within Elasticsearch. This work will need to handle static & dynamic ownership and group based authorization rules. Any auth rule that includes the 'read' operation will protect the Query.searchX field.

Proposal 3: Make @auth protect subscription fields

Problem: @auth does not protect subscription fields.

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

Currently subscriptions are not protected automatically.

Solution

AppSync subscription queries are authorized at connect time. That means that we need to parameterize the subscription queries in such a way that any relevant authorization logic is included in the subscription query itself. In the case of ownership @auth, this means that the client must pass an owner as a query argument and the subscription resolver should verify that the logged in user and owner are the same.

For example, given this schema:

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

The following subscription fields would be output:

type Subscription {
    onCreatePost(owner: String): Post
    onUpdatePost(owner: String): Post
    onDeletePost(owner: String): Post
}

and when running a subscription query, the client must provide a value for the owner:

subscription OnUpdatePost($owner: String) {
    onUpdatePost(owner: $owner) {
        id
        title
    }
}

The proposed change would create a new subscription resolver for each subscription field generated by the @model. Each subscription resolver would verify the provided owner matches the logged-in identity and would fail the subscription otherwise.

There are a few limitation to this approach:

  1. There is a limit of 5 arguments per subscription field.
    • e.g. a field onCreatePost(owner: String, groups: String, otherOther: String, anotherOwner: String, anotherListOfGroups: String): Post has too many fields and is invalid. To handle this the CLI can emit a warning prompting you to customize your subscription field in the schema itself.
  2. Subscription fields are equality checked against published objects. This means that subscribing to objects with with multi-owner or multi-group auth might behave slightly differently than expected.
    • When you subscribe you will need to pass the full list of owners/groups on the item. Not just the calling identity.

As an example to point (2) above, imagine this auth rule:

type Post @model @auth(rules: [{allow: owner, ownerField: "members"}]) {
    id: ID!
    title: String
    members: [String]
}

Let's say that we want to subscribe to all new posts where I am a member.

subscription {
    onCreatePost(members: ["my-user-id"]) {
        id
        title
        members
    }
}

AppSync messages are published to subscriptions when the result of the mutation, to which the subscription field is subscribed, contains fields that equal the values provided by the subscription arguments. That means that if I were to publish a message via a mutation,

mutation {
    createPost(input: { title: "New Article", members: ["my-user-id", "my-friends-user-id"]}) {
        id
        title
        members
    }
}

the subscription started before would not be triggered because ["my-user-id", "my-friends-user-id"] is not the same as ["my-user-id"]. I bring this up for clarity but I still think this feature is useful. Single owner & group based authorization will behave as expected.

Proposal 4: Field level @auth

Merged by aws-amplify/amplify-cli#1262

Currently an @auth directive like this:

type Task @model @auth(rules: [{allow: owner}], queries: [get, list], mutations: [create, update, delete]) {
    id: ID!
    title: String
    owner: String
}

causes these changes to the following resolvers:

  1. Query.getTask - Returns the post only if the logged in user is the post's owner.
  2. Query.listTasks - Filter items such that only owned posts are returned.
  3. Mutation.createTask - If an owner is provided via $ctx.args.input.owner and matches the identity of the logged in user, succeed. If no owner is provided, set logged in user as the owner, else fail.
  4. Mutation.updateTask - Append a conditional expression that will only update the record if the logged in user is its owner.
  5. Mutation.deleteTask - Append a conditional expression that will only delete the record if the logged in user is its owner.

In other words, the @auth directive currently protects the root level query & mutation fields.

Github Issues

Problem: You cannot protect @connection resolvers

For example, look at this schema.

type Task @model {
    id: ID!
    title: String
    owner: String
    notes: [Task] @connection(name: "TaskNotes")
}
# We are trying to specify that notes should only be visible by the owner but
# we are unintentially opening access via *Task.notes*.
type Notes @model @auth(rules: [{allow: owner}]) {
    id: ID!
    title: String
    task: Task @connection(name: "TaskNotes")
    owner: String
}

Since only top level fields are protected and we do not have an @auth directive on the Task model, we are unintentionally opening access to posts via Task.notes.

Solution

We discussed having @auth rules on OBJECTs automatically protect connection fields in proposal 1, but I also suggest opening the @auth directive such that it can be placed on both FIELD_DEFINITION and OBJECT nodes. This will result in an updated definition for @auth:

directive @auth(rules: [AuthRule!]!) on OBJECT, FIELD_DEFINITION
# ...

You may then use the @auth directive on individual fields in addition to the object type definition. An @auth directive used on an @model OBJECT will augment top level queries & mutations while an @auth directive used on a FIELD_DEFINITION will protect that field's resolver by comparing the identity to the source object designated via $ctx.source.

For example, you might have:

type User @model {
    id: ID!
    username: String

    # Can be used to protect @connection fields.
    # This resolver will compare the $ctx.identity to the "username" attribute on the User object (via $ctx.source in the User.posts resolver).
    # In other words, we are authorizing access to posts based on information in the user object.
    posts: [Post] @connection(name: "UserPosts") @auth(rules: [{ allow: owner, ownerField: "username" }])

    # Can also be used to protect other fields
    ssn: String @auth(rules: [{ allow: owner, ownerField: "username" }])
}
# Users may create, update, delete, get, & list at the top level if they are the
# owner of this post itself.
type Post @model @auth(rules: [{ allow: owner }]) {
    id: ID!
    title: String
    author: User @connection(name: "UserPosts")
    owner: String
}

An important thing to notice is that the @auth directives compares the logged-in identity to the object exposed by $ctx.source in the resolver of that field. A side effect of (1) is that an @auth directive on a field in the top level query type doesn't have much meaning since $ctx.source will be an empty object. This is ok since @auth rules on OBJECT types handle protecting top level query/mutation fields.

Also note that the queries and mutations arguments on the @auth directive are invalid but the operations argument is allowed. The transform will validate this and fail at compile time with an error message pointing you to the mistake. E.G. this is invalid:

type User @model {
    # @auth on FIELD_DEFINITION is always protecting a field that reads data.
    # Fails with error "@auth directive used on field User.ssn cannot specify arguments 'mutations' and 'queries'"
    ssn: String @auth(rules: [{allow: owner}, mutations: [create], queries: [get]]) 

    # No one but the owner may update/delete/read their own email.
    email: String @auth(rules: [{allow: owner}, operations: [update, delete, read]) 
}

The implementation for allowing operations in field level @auth directives is a little different.

  1. create - When placed on a @model type, will verify that only the owner may pass the field in the input arg. When used on a non @model type, this does nothing.
  2. update - When placed on a @model type, will verify that only the owner may pass the field in the input arg. When used on a non @model type, this does nothing.
  3. delete - When placed on a @model type, will verify that only the owner may set the value to null. Only object level @auth directives impact delete operations so this will actually augment the update mutation and prevent passing null if you are not the owner.
  4. read - Places a resolver on the field (or updates an existing resolver in the case of @connection) that restricts access to the field. When used on a non-model type, this still protects access to the resolver.

Proposal 5: And/Or in @auth rules

Github Issues

Problem

Currently all @auth rules are joined via a top level OR operation. For example, the schema below results in rules where you can access Post objects if you are the owner OR if you are member of the "Admin" group.

type Post @model @auth(rules: [{ allow: owner }, { allow: groups, groups: ["Admin"] }]) {
    id: ID!
    title: String
    author: User @connection(name: "UserPosts")
    owner: String
}

It would be useful if you could organize these auth rules using more complex rules combined with AND and OR.

Solution

We can accomplish this by adding to the the @auth definition.

directive @auth(rules: [TopLevelAuthRule!]!) on OBJECT, FIELD_DEFINITION
input TopLevelAuthRule {
    # For backwards compat, any rule specified at the same level as an "and"/"or" will be joined via an OR.
    allow: AuthStrategy!
    ownerField: String # defaults to "owner"
    identityField: String # defaults to "cognito:username" for UserPools, "username" for IAM, "sub" for OIDC
    groupsField: String
    groups: [String]

    # This only exists in top level rules and specifies operations for all the rules even when combined with and/or.
    # Neseted "operations" tags are not allowed because it would confuse evaluation logic.
    operations: [ModelOperation]

    # New recursive fields on AuthRule
    and: [AuthRule]
    or: [AuthRule]   
}
input AuthRule {
    allow: AuthStrategy!
    ownerField: String # defaults to "owner"
    identityField: String # defaults to "cognito:username" for UserPools, "username" for IAM, "sub" for OIDC
    groupsField: String
    groups: [String]

    # New recursive fields on AuthRule
    and: [AuthRule]
    or: [AuthRule]
}
enum AuthStrategy { owner groups }
# Reduces get/list to read. See explanation below.
enum ModelOperation { create update delete read }

This would allow users to define advanced auth configurations like:

type User
  @model 
  @auth(rules: [{
    and: [
      { allow: owner },
      { or: [
        { allow: groups, groups: ["Admin"] },
        { allow: owner, ownerField: ["admins"] },
      }
    ],
    operations: [read]
  }]) {
  id: ID!
  admins: [String]
  owner: String
}
# Logically: ( isOwner && ( isInAdminGroup || isMemberOfAdminsField ) )

The generated resolver logic will need to be updated to evaluate the expression tree.

Proposal 6: Deny by default mode.

Github Issues

Problem: There is currently no way to specify deny access by default for Amplify APIs.

If you create an API using a schema:

type Post @model {
    id: ID!
    title: String!
}

then the generated create, update, delete, get, and list resolvers allow access to any request that includes a valid user pool token (for USER_POOL auth). This proposal will introduce a flag that specifies that all operations should be denied by default and thus all fields that do not contain an explicit auth rule will be denied. This will also change the behavior of create mutations such that the logged in user identity is never added automatically when creating objects with ownership auth.

Solution: Provide a flag that enables deny by default

By adding a DenyByDefault flag to parameters.json or transform.conf.json will allow users to specify whether fields without an @auth directive will allow access or not. When deny by default is enabled the following changes will be made.

  1. Mutation.createX resolvers will no longer auto-inject the ownership credential when the ownership credential is not provided when creating objects. Users will have to supply the ownership credential from the client and it will be validated in the mutation resolver (this happens already when you provide the ownership credential in the input).
  2. All resolvers created by a @model without an @auth directive will be denied by default.
  3. All resolvers created by @searchable on a @model without an @auth will be denied by default.
  4. All resolvers created by @connection that return a @model with an @auth AND that do not have their own field level @auth will be denied by default.

For example, with deny by default enabled

type Post @model {
    id: ID!
    title: String
    author: User @connection(name: "UserPosts")
    comments: [Comment] @connection(name: "PostComments")
}
type User @model @auth(rules: [{allow: owner}]) {
    id: ID!
    username: String!
    posts: [Post] @connection(name: "UserPosts")
}
type Comment @model {
    id: ID!
    content: String
    post: [Comment] @connection(name: "PostComments")
}

This mutation would fail:

mutation CreatePost {
    createPost(...) {
        id
        title
    }
}

This mutation would succeed:

mutation CreateUser {
    createUser(input: { username: "me@email.co" }) { # Assuming the logged in user identity is the same
        id
        title
    }
}

This top level query would succeed but Post.comments would fail.

query GetUserAndPostsAndComments {
    getUser(id: 1) { # succeeds assuming this is my user.
        posts { # succeeds because the @auth on User authorizes the child fields
            items {
                title
                comments { # fails because there is no auth rule on Post, Comment, or the Post.comments field.
                    items {
                        content
                    }
                }
            }
        }
    }
}

More details coming soon

  1. Write custom auth logic w/ pipeline functions
  2. Enable IAM auth within the API category

Request for comments

This document details a road map for authorization improvements in the Amplify CLI's API category. If there are use cases that are not covered or you have a suggestion for one of the proposals above please comment below.

jkeys-ecg-nmsu commented 5 years ago

You will be able to use @connection with the new custom index features to optimize the access pattern and then use @auth to protect access within that table or index.

Loving it.

The proposed change would create a new subscription resolver for each subscription field generated by the @model. Each subscription resolver would verify the provided owner matches the logged-in identity and would fail the subscription otherwise.

Fantastic!

You may then use the @auth directive on individual fields in addition to the object type definition. An @auth directive used on an @model OBJECT will augment top level queries & mutations while an @auth directive used on a FIELD_DEFINITION will protect that field's resolver by comparing the identity to the source object designated via $ctx.source

I'm drooling a bit at this point...

This proposal will introduce a flag that specifies that all operations should be denied by default and thus all fields that do not contain an explicit auth rule will be denied. This will also change the behavior of create mutations such that the logged in user identity is never added automatically when creating objects with ownership auth.

Thank you please.

Fantastic proposals! I think the level of granular control this will enable will make it easier to maintain regulatory compliance (e.g. HIPAA, PCI) with the graphql-transformer and generally make schema design simpler. Looking forward to seeing it in production.

chrisco512 commented 5 years ago

Great write up and love the proposals. One big area is the idea of multi-tenancy and not relying on Cognito Groups but "tenantId" or some other arbitrary field to determine tenant ownership. This might be covered by enabling custom Auth logic with pipeline functions. This is how I'm solving it currently, but it's a manual process and a directive to cover this case would be nice!

blazinaj commented 5 years ago

The "Deny by default" mode would be perfect. For a secure multi-tenant app I would rather have all of my queries/mutations protected by default and need to be explicitly opened up for a user/group, so that a schema/resolver mistake on my end doesn't automatically mean compromised user data.

amirmishani commented 5 years ago

These are all fantastic and I can't wait for them to be implemented. I think proposal 1 needs to get implemented asap so we don't have to refactor too much. I also think proposal 5 is very important feature that's lacking from Amplify. Having to write pipeline resolvers + CFN for every operation because you need to verify the tenant sucks the joy out of my life!

ajhool commented 5 years ago

Please consider the aws-amplify/amplify-cli#805 issue. To summarize: it would be nice if the GraphQL endpoint created by Amplify could have cognito-level auth (eg. owner, groups/admin, etc.), but also be accessible by other AWS resources like Lambda.

Perhaps the auth directive could include a flag that would generate an IAM policy or IAM role that would allow other resources (eg. Lambda) to perform mutations and queries? The other role (eg. Lambda) would need to be responsible for setting the owner field correctly.

On one hand, I recognize that Amplify prefers to keep everything within its own ecosystem, however, it would be nice if there were simple hooks to allow other resources to interact with the Amplify stack and leverage the graphQL endpoint.

It seems like a common use-case that the user might create a DynamoDB object and then a backend system would update it or delete it.

vparpoil commented 5 years ago

Proposal 3 to secure subscriptions is a must have for us (and I’m really curious to know if I could as of today us the console to attach a particular resolver to a subscription that would secure it). Regarding proposal 4, I would like being able to define a field that is read only for certain users and writable for others. I am not sure that this proposal take this use case into account.

Finally, regarding @ajhool remarks right above, I would like to add that making the auth directive compatible with a setup with AWS_IAM would be great ! (for apps that have an auth/unauth setup)

Thank you @mikeparisstuff and the team, please keep going like this, we love amplify :) !

anarerdene commented 5 years ago

Allow Owner not working On IAM ...

mikeparisstuff commented 5 years ago

Proposals 1 & 5 implemented in aws-amplify/amplify-cli#1262

jkeys-ecg-nmsu commented 5 years ago

Proposal 3 would help companies working with Amplify/AppSync subscriptions maintain compliance (PCI, HIPAA, privacy, etc) more easily. Please consider prioritizing it for your next iteration!

et304383 commented 5 years ago

@mikeparisstuff If it's not too late, you REALLY need to change auth owner validation to be based on the cognito sub and not the username.

Usernames can be reassigned, and if that happens, you could have a potential situation where person can access a record owned by the person who used to own that record.

The sub value does not change, ever, and should be the unique identifier for validation of ownership.

AWS response explains this pretty clearly: https://forums.aws.amazon.com/thread.jspa?threadID=243796

zfarrell commented 5 years ago

@mikeparisstuff - I'm wondering if Proposal 5 could be expanded to support conditions more complex than just and/or?

A particular use case I'd love to see @auth support, is arbitrary field value comparison.

For example, i have a model (let's call it "node") which has various states: ["start","middle","end"]. I want user's to only have access to certain nodes, based on the node's state and the group they belong to.

Rather than a static and/or field, something like condition could be used to future proof, which could encapsulate any possible operator (which could be rolled out over time): and, or, eq, lt, gt, in, etc.

type Node
    @model
    @auth(rules: [
      // allow Admins full access
      { allow: groups, groups: [ "Admin" ] },

      // allow employees in group "pre-process" to access Node's in state "start"
      // Example 1 - static comparison. explicitly set group name, and expected value of `state` field
      { allow: groups, groups: ["pre-process"],
        // -- here's an expansion on the and/or logic proposed in "Proposal 5"
        // note the use of `condition`, `eq`, `field`, `val`
        condition: {
          eq: [
            // value of `state` field must equal 'pre-process'
            { field: 'state' },
            { val: 'pre-process'}
          ]
        },

      // Example 2 - dynamic comparison.
      {
        allow: groups,
        condition: {
          eq: [
            // value of `state` field, must equal value of groupField concatenated to 'state-processors-'
            // e.g. someone in group 'state-processors-start' could access nodes with state `start`
            { field: 'state' },
            { join: [
                // static value
                { val: 'state-processors-'},
                // variable reference, in this case it's the value of the groupInput
                { ref: 'group' }
              ]
            }
          ]
        }
      }
    ]) {

    id: ID!
    state: State!
}

enum State {
    Start,
    Middle,
    End
}

This is pretty rough - but just sketching out a possible solution. I also can see the case for this creating too much complexity, but the only alternative right now (that I see) seems to be writing a custom resolver for every model, which is complex in it's own way.

ajhool commented 5 years ago

A common use case that is currently a little bit awkward is a DynamoDB object where each user should have only one (eg. UserSettings). I usually implement that setting the id field to the owner field. It would be really nice if the Get resolver recognized that the ownerField was also the ID field and returned the object. I believe that similar logic is used in the list query when { allow: owner, operations: [get, list ] }, where the resolver queries on the ownerField.

For instance:

type UserSettings @model
@auth(rules: [
    { allow: owner, operations: [get, update] },
    { allow: groups, groups: ["Admin"] }
]) {
  owner: ID
  favoriteColor: String!
}

I believe the 2 current ways to achieve this query are: a.

// must add "list" to the auth model operations
const result = await API.graphql(graphqlOperation(queries.listUserSettings));
const { items } = result.listUserSettings;
const userSettings = (items.length === 1) ? items[0] || null;
If( null === userSettings) {
   throw 'No user settings found'
}

b.

 // I'm not sure if the id field is the field that amplify chooses for "owner"
 const { id } = await Auth.currentAuthenticatedUser();
 await API.graphql(graphqlOperation(queries.getUserSettings, { owner: id }));

Neither of those are bad at all and this definitely isn't high priority, but it would be nice to have in the backend and it is common enough that it would make sense to support natively IMHO

ajhool commented 5 years ago

Two comments related to rule 6. Both comments advocate for an explicit AUTHENTICATED or PUBLIC group that can be used to explicitly state auth strategies.

First:

The allow by default with an optional deny by default FLAG is the design pattern that caused so many data leaks with public S3 buckets. Just make everything explicit and stop adding implicit public auth strategies. Any tiny amount of time/effort saved by using allow by default is negated by the inevitable data leaks caused by accidentally granting public write access to every object.

For instance:

// BAD: Throw error
type SomePublicItem @model
  {
    id: ID!
    name: String
  }

-> ERROR: Please provide an auth strategy for SomePublicItem
// GOOD: Do not throw error. Although, frankly, I'm not sure what API EVER has a totally open public CRUD item like this.
type SomePublicItem @model
  @auth(rules: [ { allow: groups, groups: [ _PUBLIC_ ] } ] )
  {
    id: ID!
    name: String
  }

Second:

Another common use case that I don't think is currently possible with the @auth directive (please correct me if I'm wrong, I'd really like to use it!) is to explicitly provide public/unauthenticated/authenticated read permissions. I would consider this to be a better implementation of Rule 6 by simply making everything explicit.

A persisted object that an Admin creates/updates, but that everybody should be able to get/list. It would be nice if there were default groups named EVERYBODY or UNAUTHENTICATED or AUTHENTICATED or ALL.

type Product @model
  @auth(rules: [ 
    { allow: groups, groups: ["Admin"] },
    { allow: groups, groups: [ _PUBLIC_, __UNAUTHENTICATED__ ], operations: [ get, list ] }
  ])
  {
    id: ID!
    name: String
    description: String
  }

It seems unnecessarily complex that we would create a Cognito group called "everybody" or "customers" and automatically add all users to that group when they sign up, but maybe that is the better design pattern?

jmorecroft commented 5 years ago

Hi folks, might be the wrong thread or place for this but somewhat related - can anyone explain to me the rationale for choosing Cognito:username as the implicit "owner" if none is provided, as implemented in the auto gen'ed resolvers, as opposed to the identity's "sub"? My guess is one's as good as the other and the former is more readable, but I do wonder about the impact of username changing/reassignment etc.

Googling a little on this pulled up this thread for example, which suggests that "sub" would be a much more resilient identifier for auth? For example, if a user is deleted and another created with the same username, the current auth means they'll get access to all the first user's entities..?

https://stackoverflow.com/questions/39223347/should-i-use-aws-cognito-username-or-sub-uid-for-storing-in-database

et304383 commented 5 years ago

@jmorecroft it's purely a mistake on the Amplify team's part. The sub should always be used to uniquely identify a user. I already posted about this here in this same thread.

ajhool commented 5 years ago

@jmorecroft @et304383 I had posted a similar bug/bad design decision (in the amplify js repo), where the Storage object is connected to a transient id: https://github.com/aws-amplify/amplify-js/issues/1787 , not sure if that is still the case.

BeaveArony commented 5 years ago

Protecting a @connection field by queries is done by using this pattern:

type User @model {
    id: ID!
    username: String

    posts: [Post] 
      @connection(name: "UserPosts") 
      @auth(rules: [{ allow: owner, ownerField: "username" }])
}
type Post @model(queries: null) { ... }

How can we protect, that someone else adds a new Post to any other User?

Say I'm logged in as 'Cersei' and do the following mutation:

mutation addPostToOtherUser {
  createPost(input: { title:"My evil Post", userPostId:"Daenerys"} ) {...}
}

I would like to protect against this.

Edit:

Never mind it works quite well when I do something like this:

type User @model @auth(rules: [{ allow: owner, ownerField: "id" }]) {
    id: ID!
    posts: [Post] 
      @connection(name: "UserPosts") 
      @auth(rules: [{ allow: owner, ownerField: "id" }])
}

type Post @model(queries: null) { 
  title: String!
  user: User!
      @connection(name: "UserPosts") 
      @auth(rules: [{ allow: owner, ownerField: "userPostId" }])
}

If I save the username as id in the type User, it will be used as the userPostId in the @connection of the user field in type Post and block the creation of the Post when the username does not match the identity.

leetmachine commented 5 years ago

I'm using the new @auth 'operations' argument to achieve:

  1. allow read (get/list) access to any unauthenticated or authenticated user.
  2. allow only an authenticated user to create, update, delete.

my setup: {allow: owner, operations: [create, update, delete]}

Outcome:

  1. allow read (get/list) access to any unauthenticated or authenticated user. -- it does work for authenticated user. --it does NOT work for unauthenticated user. error: "no current user".

  2. allow only an authenticated user to create, update, delete. -- it does work for authenticated user, allow to perform operations. -- it does work for unauthenticated user, does not allow to create an object.

Since `{allow: owner} will impose OwnerStrategy for ALL operations, I would expect my setup to exclude the read operation from the OwnerStrategy. Because it is excluded I would expect it to function just as it would without the OwnerStrategy. Regardless of a user or not, it should produce the read.

markau commented 5 years ago

I am interested in Proposal 5 (and/or auth rules), as I would like to have a group membership override the owner priveleges. For example, a banned user (still owner of a post, but not currently allowed to edit/delete due to being in the "Banned Users" group).

hisham commented 5 years ago

Can @auth support be extended to S3Objects (storage) category? Last time I checked the storage category (https://github.com/aws-amplify/amplify-cli/blob/master/packages/amplify-category-storage/Readme.md) creates 3 folders - private, protected, and public. Objects in private are only accessible to the user who uploaded them. Protected is accessible to any authenticated user, and public is public access.

The authentication for this is also through identity management I believe, not cognito sub.

It seems kind of divorced from the @auth rules set in the GraphQL schema, though access to the S3Object stored in dynamodb is controlled by the @auth rules. The actual objects stored in S3 are not. This is a point of confusion and perhaps can be considered as part of this RFC or aws-amplify/amplify-cli#766.

From my high level point of view, data in S3 should be controlled under the same access rules set by this @auth directive.

blazinaj commented 5 years ago

Can @auth support be extended to S3Objects (storage) category? Last time I checked the storage category (https://github.com/aws-amplify/amplify-cli/blob/master/packages/amplify-category-storage/Readme.md) creates 3 folders - private, protected, and public. Objects in private are only accessible to the user who uploaded them. Protected is accessible to any authenticated user, and public is public access.

The authentication for this is also through identity management I believe, not cognito sub.

It seems kind of divorced from the @auth rules set in the GraphQL schema, though access to the S3Object stored in dynamodb is controlled by the @auth rules. The actual objects stored in S3 are not. This is a point of confusion and perhaps can be considered as part of this RFC or aws-amplify/amplify-cli#766.

From my high level point of view, data in S3 should be controlled under the same access rules set by this @auth directive.

+1 for this. I have a use case that I need to control S3 access based on the same Cognito/Auth Groups that govern my API access. I need Admin level access, group level access, or individual level access based on logged in user. I'm trying to set up S3 Bucket policies based on Cognito User Pool groups but I'm having to do it manually at the moment.

ajhool commented 5 years ago

Wait a second, per item number 3, where "Currently subscriptions are not protected automatically." does this mean that protected data can leak to anybody who subscribes to a data type? (issue also referenced, here: aws-amplify/amplify-cli#1766 )

Let's say the owner of the website defines their auth module as follows:

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

And then I go to their website and run this code from their site (that developer has to be running the code from the client because amplify-js doesn't support server side execution).

import Amplify, { API, graphqlOperation } from 'aws-amplify';
import * as subscriptions from './graphql/subscriptions';

// Subscribe to creation of Todo
const subscription = API.graphql(
    graphqlOperation(subscriptions.onCreateTodo)
).subscribe({
    next: (todoData) => console.log(todoData)
});

// Stop receiving data updates from the subscription
subscription.unsubscribe();

Do I now have access to every single Todo object that gets created on their site? Even though the developer added the @auth rule for "owner" only with the clear goal of keeping that user's Todo (or, say, healthcare records) information private?

Please tell me that I'm misunderstanding something here because this makes a mockery of the efforts I've taken to secure my website if the Amplify team is aware of such a terrible vulnerability in the default configuration of the framework and doesn't make any effort to document it (or demonstrate any urgency to make it the non-default configuration), aside from a proposal in an RFC. If amplify were actually used by any serious companies in production, this would effectively be a zero-day vulnerability -- this isn't the "absence of a feature", it's an undocumented vulnerability in the default configuration

Why is there even an @auth directive provided if it doesn't currently prevent subscriptions from providing a firehose of data to anybody who wants it when used in the default configuration? And why is there no documentation suggesting that a non-default configuration is required? Proper documentation would have given me the opportunity to not use this framework before realizing these shortcomings so late in the game; I will be far more hesitant to depend on future AWS products if this is the amount of thought given to such a clear and ridiculous vulnerability.

Would anybody who uses amplify in production and protects valuable information please send me their website url?

rawadrifai commented 5 years ago

@mikeparisstuff here's some feedback on our use case regarding auth and subscriptions. I hope it helps. It's a pretty straight forward scenario.

Consider a slack clone. We have a message table or type. Every message is inserted with the groups and individuals that can see it (according to the auth rules). The identity claims are custom and injected pre token generation, via Cognito triggers.

When a new message is created, we hope that all the users that are authorized to read it, will get notified through an onCreateMessage subscription. The case right now is that every client that is subscribed gets the notification. Clearly that cannot scale and compromises security.

I understand there is custom work that can be done through vtl. Our suggestion is that all the groups and individuals that are authorized to read the record, would receive an event. It is what I expected the case would be before diving in.

Hope this helps.

ajhool commented 5 years ago

To anybody who is currently using Amplify in production, you are probably exposed to the data leak vulnerability discussed in aws-amplify/amplify-cli#1766 and I highly recommend that you add this fix to your graphql schema. Amplify automatically creates Subscription resolvers that do not respect @auth, so an attacker can simply Subscribe to your data model's endpoint and they will receive all/site-wide OnCreate / OnUpdate events for that data model. Those events include the data, even if the attacker is not authorized to see that data. The fix is to disable Subscription autogeneration by adding the below line to your code:

@model(subscriptions: null)

This fix appears to be undocumented (the vulnerability is also undocumented) but it comes from the Amplify team. If your app uses Subscriptions, then you will currently need to create a custom Subscription resolver with custom Authentication logic -- or use some other form of security in depth.

mikeparisstuff commented 5 years ago

Hey @ajhool as discussed in the other thread aws-amplify/amplify-cli#1766 we have added documentation covering this. Please see Authorizing Subscriptions. That being said, we do still want to support these features so please reply with details on use cases and scenarios that you would like to support so we can work it into the roadmap and schedule.

undefobj commented 5 years ago

@mikeparisstuff here's some feedback on our use case regarding auth and subscriptions. I hope it helps. It's a pretty straight forward scenario.

Consider a slack clone. We have a message table or type. Every message is inserted with the groups and individuals that can see it (according to the auth rules). The identity claims are custom and injected pre token generation, via Cognito triggers.

When a new message is created, we hope that all the users that are authorized to read it, will get notified through an onCreateMessage subscription. The case right now is that every client that is subscribed gets the notification. Clearly that cannot scale and compromises security.

I understand there is custom work that can be done through vtl. Our suggestion is that all the groups and individuals that are authorized to read the record, would receive an event. It is what I expected the case would be before diving in.

Hope this helps.

@rawadrifai With this model, how would you see yourself doing the rules? Essentially you have public & private chat rooms in this model. Would you just blanket apply the query permissions to subscriptions? Do the mutation permissions get ignored? Or is it something that you add onto the @auth directive?

For example one hypothetical method could be that you specify named groups that might be allowed to receive messages:

type Message @auth({ subscribers: ["GroupA", "GroupB"]}) {
  id: ID!
  content: String
}

But realistically this wouldn't be that flexible so each room would have an id with arguments (per @mikeparisstuff comments in the original proposal) so maybe you do something like:

type Message @auth({ subscribers: [{RoomX: GroupA, RoomY: GroupB}]}) {
  id: ID!
  content: String
}

This would mean that anyone from GroupA who did a onCreateMessage(id:"RoomX") would be granted access. Another option might be saying only owners to objects they created can receive updates to them:

type Message @auth({ subscribers: owner}) {
  id: ID!
  content: String
}

These are just some thoughts to spark a little more dialogue on your use cases and requirements. Getting more specifics on what exact controls you'd like to use for granting or restricting access will help us know the MVP for such a feature.

rawadrifai commented 5 years ago

@undefobj thank you for this. Here's a subset of my model below. The idea is that every user when they sign up, they do after an invite from someone in the "Community" (similar to slack).

A community has multiple users. A user can belong to a single community. A Channel can be private or public. Public channels can be viewed by everyone. Private channels only by their members. Conversations have a users field that tell you who are the users in a given conversation. A conversation is a DM as opposed to a channel.

They way a user knows they received a message, is by subscribing to onCreateMessage. My expectation was that they would receive the messages that they can read. Check out the permissions on the Message type.

# Users that belong to the community (group name in id) have read access

type Community @model @auth(rules: [
  { allow: owner },
  { allow: groups, groupsField: "id", operations: [read] }
]) {
  id: String!
  owner: String
  description: String
  enabled: Boolean
  users: [User] @connection(name: "communityUsers")
}

# The owner of the user has full control
# Users that belong to the same COMMUNITY have read access
# The groups field should be the name of the COMMUNITY

type User @model @auth(rules: [
  { allow: owner },
  { allow: groups, groupsField: "group", operations: [read] }
]) {
  id: String! 
  group: String
  firstName: String 
  lastName: String
  dobMonth: String
  dobDay: Int
  dobYear: Int
  address1: String
  address2: String
  country: String
  region: String
  city: String
  postalCode: String
  email: String
  enabled: Boolean
  community: Community @connection(name: "communityUsers")
  memberships: [Membership] @connection(name: "userMemberships")
}

# The owner of the conversation has full control
# Users of the conversation have read access (2 or more people conversation)

type Conversation @model @auth(rules: [
  { allow: owner },
  { allow: owner, ownerField: "users", operations: [read] }
]) {
  id: ID!
  users: [String]
  archived: String
  messages: [Message] @connection(name: "ConversationMessages", sortField: "createdAt")
}

# The owner of the message has full control
# The users field should be an array of users of a conversation (if this message is for a conversation)
# The group field should be the name of the CHANNEL (if this message is for a channel)

type Message @model(queries:null) @auth(rules: [
  { allow: owner },
  { allow: groups, groupsField: "group", operations: [read] },
  { allow: owner, ownerField: "users", operations: [read] }
]) {
  id: ID!
  owner: String
  createdAt: String
  content: String
  group: String
  users: [String]
  archived: String
  conversation: Conversation @connection(name: "ConversationMessages")
  channel: Channel @connection(name: "ChannelMessages")
  comments: [Comment] @connection(name: "MessageComments", sortField: "createdAt")
}

# The owner of a Channel has full control
# The Channel created should have a group field that controls access. 
# If the channel is public the group should be the name of the COMMUNITY.
# If the channel is private the group should be the name of the CHANNEL itself.

type Channel @model @auth(rules: [
  { allow: owner },
  { allow: groups, groupsField: "group", operations: [read] }
]) {
  id: String! 
  description: String
  archived: String
  group: String
  type: ChannelType
  messages: [Message] @connection(name: "ChannelMessages", sortField: "createdAt")
  memberships: [Membership] @connection(name: "channelMemberships")
}

# Maintains the channel users
# Also we query this table to know what groups a user belongs to

type Membership @model @auth(rules: [
  { allow: owner },
  { allow: groups, groupsField: "group", operations: [read] }
]) 
{
  id: String!
  group: String
  user: User @connection(name: "userMemberships")
  channel: Channel @connection(name: "channelMemberships")
}
rawadrifai commented 5 years ago

One caveat for inserting new messages, that has to do with auth too, is that I am not sure how to prevent users who do not belong to a channel from creating a message with channelId that they shouldn't use.

So basically, pre insert, I would like to check if the "channelId" they're using is one of the values of the cognito groups they're in.

rawadrifai commented 5 years ago

I have another suggestion that if not implemented now, might be to keep in mind for the future. At the risk of sounding too wishful, from a development perspective, it would be very convenient to have s3 objects adopt the auth of a reference field in the schema.

The most common use case is someone storing a reference for an object in a database, Dynamo in my case. When accessing the s3 object the desired outcome is to apply the same rules as the @auth.

ajhool commented 5 years ago

@mikeparisstuff The documentation additions look good, although the main definition is still incorrect:

@auth Object types that are annotated with @auth are protected by a set of authorization rules. Currently, @auth only supports APIs with Amazon Cognito User Pools enabled. You may use the @auth directive on object type definitions and field definitions in your project’s schema.

When using the @auth directive on object type definitions that are also annotated with @model, all resolvers that return objects of that type will be protected.

The bolded part says that all resolvers are protected, but actually Subscription resolvers aren't protected.

I still don't think that documentation is enough and that the current unprotected Subscriptions should be opt-in, not opt-out. I also think that this basic security strategy of requiring developers to explicitly opt-in to public resources should be used in all future Amplify feature releases.

Anyways, I'll stop pestering about the public subscription resolvers, but I do think there is a lesson there that should be applied to the @auth directive...

It's the same lesson that AWS learned from the PUBLIC-by-default S3 buckets, which caused so many data leaks. If the default protection for a resource is Public, then the developer should need to explicitly add a PUBLIC flag in code.

For instance, if you don't specify an @auth flag on an /@/model, that model is a public resource (I think). I can't think of any application with a publicly readable/writeable/updatable/deletable data object, so the "convenience" of this default is dubious.

IMO, the @auth flag should always be mandatory, perhaps there could be an @auth(PUBLIC) flag that would replicate the current behavior without an @auth directive.

TLDR; The @auth flag should be mandatory on all /@/models & functions. Use @auth(PUBLIC) to opt-in to the current default behavior. Use the same concept of explicit auth on all resources, including Storage

ajhool commented 5 years ago

Oh boy, so the @searchable directive is also not protected by @auth and this is also not documented anywhere except for this RFC despite the first issue (#460) being posted in November 2018?

Documentation: "When using the @auth directive on object type definitions that are also annotated with @model, all resolvers that return objects of that type will be protected."

Searchable resolver: Provides unprotected search of all private data to anybody who wants it even when using @auth. (No documentation, no CLI warnings)

Subscription resolvers: Provides an unprotected streaming data pipeline of private data updates/creates/deletes to anybody who wants it even when using @auth. (Insufficient documentation, no CLI warnings, opt-out behavior).

AWS' willingness to push out zero-security features with little to no documentation or warning to developers is incredibly troubling, particularly as we depend on this framework in production and we value our customers' data security.

Even worse, as seen in the discussion on the aws-amplify/amplify-cli#460 issue, the Amplify team actually tried to close that issue as being resolved... and it is marked as a Feature Request instead of as a Bug... Again, the issue is that when you use the @searchable directive, it exposes a search resolver of private data to anybody who wants it... Despite the behavior not being intuitive, contrary to the documentation, and it being a clearly outrageous security vulnerability, it's considered a feature request and not a bug?

"Well, you can just make a custom resolver, yourself" -- maybe so, but AWS provides zero indication that you would need to while presenting all of these features as working seamlessly with each other and as being protected by @auth.

AWS is presenting security as a feature while actually treating security as a Feature Request.

Amplify is a very useful framework that has grown tremendously, but it is clear that the team needs to take immediate steps to fix current vulnerabilities / documentation, implement better security audits when pushing out new features, and shift to a "secure-by-default" design strategy for all future features.

rawadrifai commented 5 years ago

Hey @mikeparisstuff - do you have any update on the likely direction with @auth and subscriptions?

undefobj commented 5 years ago

They way a user knows they received a message, is by subscribing to onCreateMessage. My expectation was that they would receive the messages that they can read. Check out the permissions on the Message type.

@rawadrifai We've been reading through the feedback on the design here, and one thing that might be a misunderstanding is the way that authorization works for subscriptions. It is at connection time, and a resolver will run granting or denying the subscription request. So to take your use case where the client is subscribing to onCreateMessage we would actually need to require arguments in that subscription which would give the resolvers enough context for them to perform authorization logic.

For example in might be onCreateMessage(userId: ID!) or onCreateMessage(roomId:ID!) which basically says all messages for a specific user or room. Channels/conversations are just generalizations of this. Here's an example in the AppSync docs of how this might look: https://docs.aws.amazon.com/appsync/latest/devguide/security-authorization-use-cases.html#real-time-data

With this in mind, my current thinking is that by adding an explicit ownership or channel protection this could make the input fields on a subscription required as outlined above. I've got to think a bit more on field level settings.

One caveat for inserting new messages, that has to do with auth too, is that I am not sure how to prevent users who do not belong to a channel from creating a message with channelId that they shouldn't use. So basically, pre insert, I would like to check if the "channelId" they're using is one of the values of the cognito groups they're in.

This is a more straightforward to to, since unlike subscriptions mutations are not long held connections and we can do conditional checks on membership before the write occurs. That's what @auth does today.

undefobj commented 5 years ago

Hello everyone - We have a version of this functionality which has been published to the following tag:

npm install -g @aws-amplify/cli@authSubRelease

In this implementation, when a model is protected using the owner auth strategy, each subscription request will require that the user is passed as an argument to the subscription request. If the user field is not passed, the subscription connection will fail. In the case where it is passed, the user will only get notified of updates to records for which they are the owner.

Alternatively, when the model is protected using the static group auth strategy, the subscription request will only succeed if the user is in an allowed group. Further, the user will only get notifications of updates to records if they are in an allowed group. Note: You don't need to pass the user as an argument in the subscription request, since the resolver will instead check the contents of your JWT token.

For example suppose you have the following schema:

type Post @model 
@auth(rules: [{allow: owner}])
{
  id: ID!
  username: String
  postname: String
  content: String
}

This means that the subscription must look like the following or it will fail:

subscription onCreatePost(username: “Bob”){
  postname
  content
}

In the case of groups if you define the following:

type Post @model 
@model @auth(rules: [{allow: groups, groups: ["Admin"]}]) {
{
  id: ID!
  username: String
  postname: String
  content: String
}

Then you don’t need to pass an argument, as the resolver will check the contents of your JWT token at subscription time and ensure you are in the “Admin” group.

Finally, if you use both owner and group authorization then the username argument becomes optional. This means the following:

Dynamic groups have no impact to subscriptions, so you will not get notified of any updates to them.

Next week we will add capability that if your type doesn’t already have username the Transformer will automatically add this for you. There are also still a few edge cases with field level auth we need to validate for subscriptions, however the behavior will be that the field will return null if the field auth is not a subset of the object type auth. In the meantime please have a look at the early version published to the tag above. Our aim is to do final validations over the next few days and publish to the latest build next week.

ajhool commented 5 years ago

It is great to see that subscriptions are now covered by auth.

Will future features follow this same trajectory of releasing the feature and then adding security a year later? The @connection directive was also released before it was protected by auth as was/is @searchable and those were also not documented.

Security is a weakest-link problem so the design strategy is more important than the individual features and I really hope that the Amplify team would acknowledge that leaving undocumented public autogenerated subscriptions open for a year (they're still open until this branch is merged) was not a good strategy and indicate that better security audits will be used for future features.

I'm sorry for continuing to make these posts even though they aren't receiving official responses but I think basic security is critically important and official responses would be appreciated.

rawadrifai commented 5 years ago

@undefobj This is a great start. Please keep in mind the use case where Cognito groups do not suffice.

For example, I interject a login request to append the groups a user belongs to. I get those from Dynamo. So to check whether a user belongs in a group or not, please do so with the token not a call to Cognito. I assume this is the case with this release.

undefobj commented 5 years ago

@rawadrifai the Static Group checks are done against claims in the JWT which Cognito gives after a user logs in, so it's not a call to Cognito during connection time. The reason we cannot do Dynamic Group auth is unlike a query or mutation there isn't an item to check against for a subscription (e.g. no authorization metadata exists). I'm still thinking about how to expand that in the future, you would need some sort of a user-to-tag mapping in a DynamoDB table. In the meantime we're trying to pull in this request aws-amplify/amplify-category-api#403 as part of this so you can use alternative claims for the subscription authorization.

rawadrifai commented 5 years ago

@undefobj I am a bit confused. I use the pre token generation trigger in Cognito to inject the groups after pulling them from Dynamo. So they're in the token, and they work fine for @auth models. Will this update to subscriptions be able to read the groups from the jwt if done as described?

undefobj commented 5 years ago

@rawadrifai that's the idea, we're trying to see if you can pass in a groupsField (or something like that) into the auth rule for subscriptions which would correspond to your claim.

undefobj commented 5 years ago

@rawadrifai we have published to the tag again:

sudo npm install -g @aws-amplify/cli@authSubRelease --unsafe-perm=true

I've also just submitted a PR for the new functionality: https://github.com/aws-amplify/docs/pull/868

You should be able to use this for documentation before we merge this to latest build. We're still running final stability tests but are trying to get this out ASAP. Your testing would assist us in expediting this. Note that the custom claims for either the user identity (identityClaim) or the groups (groupClaim) are also part of this PR. While the default behavior is to protect subscriptions, customers have an explicit control now to make them public if they wish to keep previous behavior. Also we've added some automatic inspection to flag a warning if you're not using @auth on any @model, listing the ones that are unprotected, with information on how to learn how to add more controls. This happens before an $amplify push sends updates to the cloud allowing you to abort if you made a mistake. Finally there are protections in place if you're using per-field level @auth to protect sensitive data inside a model from being sent over subscriptions.

ilyador commented 5 years ago

Is there a way ATM to give public permissions to a query, without any Cognito authentication? Something like:

type Post @model @auth( rules: [{allow: public}], operations: [read] ) {
  id: ID!
  title: String!
}
undefobj commented 5 years ago

Is there a way ATM to give public permissions to a query, without any Cognito authentication? Something like:

type Post @model @auth( rules: [{allow: public}], operations: [read] ) {
  id: ID!
  title: String!
}

This is coming very soon as part of https://github.com/aws-amplify/amplify-cli/pull/1916

rawadrifai commented 5 years ago

@undefobj Thank you for this. I gave it a try. The owner-based auth works great! For the group based auth, and in my case many of the models have mixed auth (owner & groups), it is failing to properly set up the subscription.

You mentioned that in case of group auth, we don't need to pass a parameter to the subscription. That causes the app to crash on load (in case of RN, didn't try React).

image

Strack trace: image

What changes should take place in the code below for group auth to work for subscriptions?

This is my model:

type Message @model(queries:null) @auth(rules: [
  { allow: owner },
  { allow: groups, groupsField: "group", operations: [read] },
  { allow: owner, ownerField: "users", operations: [read] }
]) {
  id: ID!
  owner: String
  group: String
  users: [String]
}

This is my subscription:

export const onCreateMessage = `subscription OnCreateMessage($owner: String!) {
  onCreateMessage(owner: $owner) {
    id
    owner
  }
}
`;

This is how I'm setting up the subscription:

API.graphql(
      graphqlOperation(subscriptions.onCreateMessage, { owner: 'abc123' })).subscribe({
        next: async (eventData) => {
          console.log('eventData ==========> ', eventData)
        }
})
RossWilliams commented 5 years ago

@rawadrifai Can't remember if this is mentioned in issue comments or code directly, but the PR does not support dynamic group authorisation (yet), only static group auth.

undefobj commented 5 years ago

@rawadrifai comment from @RossWilliams is correct, only Static Group auth is supported for subscriptions. You need something like:

@auth(rules: [
    {allow: owner},
    {allow: groups, groups: ["Moderator"]}
])
amirmishani commented 5 years ago

@mikeparisstuff is proposal 5 being worked on? Any updates?

rawadrifai commented 5 years ago

@undefobj In that case it passes my testing. Owner based auth works for subscriptions. I did not try static groups. Support for dynamic groups is the desired feature.

undefobj commented 5 years ago

@mikeparisstuff is proposal 5 being worked on? Any updates?

@amirmishani we're going to try and look at this in the coming couple of months. It's high on our radar but is a bit complex so needs design.

undefobj commented 5 years ago

@undefobj In that case it passes my testing. Owner based auth works for subscriptions. I did not try static groups. Support for dynamic groups is the desired feature.

@rawadrifai conceptually, dynamic groups might not actually be possible for subscriptions. Authorization metadata needs to exist somewhere, and in a request/response pattern of a Query it makes sense as the object exists already. For a subscription which is a push pattern, the action is ephemeral and doesn't exist yet. We've thought about this quite a bit and it doesn't really match up. For instance the resolver could read from a DB somewhere but what would it read? The object which doesn't yet exist? This is why we added the Group Claim information which you mentioned you were populating in your pre-token trigger, as this will read from your DB. BTW we also added that template in today to better support your use case: https://github.com/aws-amplify/amplify-cli/pull/2126

RossWilliams commented 5 years ago

@undeobj I’ve implemented an alternative design without the recursive nesting. I’ve added a property and=“name” to each auth rule that should be combined. Then when an auth rule matches I increment a counter with the name as the key. After all rules are run I check which counters are set and compare their value to how many rules have that key in total. If there is a mismatch I set it as unauthorised.