maoosi / prisma-appsync

⚡ Turns your ◭ Prisma Schema into a fully-featured GraphQL API, tailored for AWS AppSync.
https://prisma-appsync.vercel.app
BSD 2-Clause "Simplified" License
225 stars 18 forks source link

Authorization rules conditions cannot be applied to list #15

Closed mikerudge closed 3 years ago

mikerudge commented 3 years ago

Hey

first off, thank you so much for making this project. It has been awesome to be integrate it with our CDK setup and although I am just getting started with it, its been awesome so far.

I am hoping this is more user error than a bug, but wanted to report it anyway.

In the handler.ts I have one rule, which is

    app.deny({
        action: AuthActions.all,
        subject: 'Task',
        condition: { isAdmin: true },
        reason: 'Only admins can access admin tasks',
    });

however when running the listTasks query, I now get the error

Authorization rules conditions cannot be applied to list (no where clause available)

I have tried different conditions, like

condition: { isAdmin: {$eq: true} }

But still no luck

Anything obvious I am missing here?

Again thanks for the great work!

EDIT: Sorry should have put in that we are using the mongodb preview from prisma, so it could be that?


generator client {
  provider        = "prisma-client-js"
  previewFeatures = ["mongodb"]
  binaryTargets   = ["native", "rhel-openssl-1.0.x"]

}

generator appsync {
  provider                  = "prisma-appsync"
  directiveAlias_adminsOnly = "@aws_iam @aws_cognito_user_pools(cognito_groups: [\"Admin\"])"
  debug                     = true
  directiveAlias_default    = "@aws_iam @aws_cognito_user_pools(cognito_groups: [\"Admin\", \"User\"])"
  customSchema              = "./custom-schema.gql"
}
``
maoosi commented 3 years ago

Hey @mikerudge! Thanks for your words, that is great to hear!

With the current implementation of Prisma-AppSync, conditions in rules can only apply to single record queries (get, update, delete). The reason is that when querying multiple records, you could potentially get back hundreds of results (some authorized, some unauthorized). So Prisma-AppSync would have to verify the condition for each individual record. Not impossible, but more complex to implement (especially if the condition includes a related model).

Access-control is something that will be re-engineered in the future - to cover more use cases like yours, but also to solve a known limitation on nested fields (see: https://github.com/maoosi/prisma-appsync/issues/8).

Assuming I have interpreted your use case correctly, one way to solve it could be writing your own access control logic, such as:

/**
Code to add in your handler function.
Just after `app.parseEvent(event)` and before `app.resolve()`.
**/

const customAccessControl = async (
    { authIdentity, args, operation, subject }: BeforeResolveProps
) => {
    let isAuthorized = true

    // custom function that returns true if the user is an admin
    const isUserAdmin = await isUserAdmin(authIdentity)

    // if trying to read tasks (get or list) as non-admin
    if (!isUserAdmin && 
        ['get', 'list'].includes(operation) &&
        ['task'].includes(subject.toLowerCase())
    ) {
        // find all tasks matching query
        const requestedTasks = await app.prisma.task.findMany({
            select: { isAdmin: true },
            ...(args.where && { where: args.where }),
            ...(args.skip && { skip: args.skip }),
            ...(args.take && { take: args.take }),
        })

        // amongst requested tasks, only authorize if task.isAdmin === false
        isAuthorized = requestedTasks.filter((task: any) => task.isAdmin === true).length === 0
    }

    return isAuthorized
}

app.beforeResolve(async (props: BeforeResolveProps) => {
    return await customAccessControl(props)
})

I haven't tried it myself, but you should also be able to combine both built-in and custom access control:

app.beforeResolve(async (props: BeforeResolveProps) => {
    // first :: custom access control
    const isAuthorized = await customAccessControl(props)
    if (!isAuthorized) return false;

    // second :: built-in access control
    app.deny({
        action: [AuthActions.delete, AuthActions.deleteMany],
        subject: 'Task',
        reason: 'No one can delete tasks',
    })
})
mikerudge commented 3 years ago

@maoosi thank you for the answer and code examples. That really helped. Totally understand that this is a big challenge to tackle, and it feels frustrating that this isn't just a solved problem, especially for AppSync.

In the example you have given (which is spot on by the way), the behaviour is,

if any of these tasks have isAdmin: true then fail then entire request

I think in this example, the behaviour we would want is,

If non-admin, filter the request so that it does not contain tasks that have isAdmin: true

Also does the above approach mean that it's calling the database twice? Once to check the list, then running the same query again to return the results if isAuthorised = true

From my understanding we could solve it two ways,

1) Create a new custom query called listAdminTasks and then protect that query with @aws_cognito_user_pools(cognito_groups: ["Admin"])

2) Create a customer resolver for listTasks that filters out admin tasks if the user is not in the Admin group.

Anything obvious I am missing there?

I will have a go at making custom resolvers tomorrow and see if this makes sense.

Again thanks for all the advice!

maoosi commented 3 years ago

I think in this example, the behaviour we would want is, If non-admin, filter the request so that it does not contain tasks that have isAdmin: true

The current implementation of hooks doesn't allow to modify the data, only to return true or false to reject or approve a given operation.

But this is interesting! Maybe in the future we could have 2 different modes for built-in access control, allowing developers to select whether they prefect the access-control to be strict (aka. reject the entire request) VS. cherry-pick (aka. filter out non-authorized fields, but return the rest). Similarly, we could have ways to modify the data from within the hooks.

Also does the above approach mean that it's calling the database twice?

Correct! One way to reduce this to only one API call only could be to dynamically intercept and modify the query (to make sure isAdmin is part of the requested fields) and the results (to filter out unauthorized fields based on the user). This can probably be done using Prisma middleware:

https://prisma-appsync.vercel.app/reference/client-api.html#prisma https://www.prisma.io/docs/concepts/components/prisma-client/middleware

From my understanding we could solve it two ways...

If you are happy to create a custom resolver, then yes this is another perfectly valid solution to your use case - and probably the easiest to implement as well.

By following this guide, you should be able to add a new query, that you can then protect using AppSync authz directives in your Schema.

maoosi commented 3 years ago

Closing this issue for now. Let me know if more questions about this.