prisma-labs / graphql-prisma-typescript

🏡 GraphQL server reference implementation (Airbnb clone) in Typescript using Prisma & graphql-yoga
MIT License
749 stars 108 forks source link

Examples leak data - i can see what other users booked - or is this on purpose? #109

Open nickluger opened 6 years ago

nickluger commented 6 years ago

For example, if i have a booking, i can see who else booked the same place and when. I can fetch their email, personal data etc.

{
  viewer {
    me {
      lastName
    }
    bookings {
      place {
        name
        bookings {
            bookee {
            firstName
            lastName
            email
          }
        }
      }
    }
  }
}

Response

{
  "data": {
    "viewer": {
      "me": {
        "lastName": "Smith"
      },
      "bookings": [
        {
          "place": {
            "name": "Mushroom Dome Cabin: #1 on airbnb in the world",
            "bookings": [
              {
                "bookee": {
                  "firstName": "Karl",
                  "lastName": "Marx",
                  "email": "karl.marx@yandex.ru"
                }
              },
              {
                "bookee": {
                  "firstName": "Adam",
                  "lastName": "Smith",
                  "email": "adam.smith@gmail.com"
                }
              }
            ]
          }
        }
      ]
    }
  }
}

Hmm interesting, Karl will also be at Mushroom Dome!

This is a problem i ran into, not only in this server example, but also in my own and other peoples server code. As soon as we have cyclic model references, hiding data becomes very complex.

schickling commented 6 years ago

Hi @nickluger. Thanks for bringing this up. You're right, the permission setup is not yet fully implemented.

Would be great if someone would like to take a stab at this - maybe based on graphql-shield? /cc @maticzav

maticzav commented 6 years ago

@schickling, I am on it!

daveols commented 6 years ago

Hey @schickling, @maticzav,

Has there been any progress with this one? I've encountered a similar issue with a project I'm working on.

It'd be super useful for us GraphQLNoobs to see how to mask certain fields at arbitrary query depths given some permissions. In this example it would involve removing the "bookings" field from place or perhaps limiting the selection on the relation to specified public fields.

I'm using graphql-shield but don't see an obvious way to filter the incoming selection or verify it given the incoming user's permissions without hard coding the selection passed to prisma-binding. I imagine one possible solution might be to create a masking type but again not sure how to implement this.

Any help appreciated!

maticzav commented 6 years ago

Hey 👋,

I started playing with this a bit but came to a strange conclusion. GraphQL Shield serves as means to prevent unwanted requests. Data leaks don't happen because of bad queries but rather because of wrong data manipulation.

I think it could be beneficial to add GraphQL Shield to the example but hardly believe it will solve the data leaking problem.

daveols commented 6 years ago

I found a solution to this, there are a couple of options. The first is more flexible, allowing you to get dynamic or filtered results for your resolvers. The second just returns null for the resolver when certain conditions are not met.

  1. Write a Place resolver which, for the bookings resolver, returns only bookings for the current user or all for the host:
export const Place = {
  bookings: {
    description: "The current user's bookings at this place, or all if the current user owns it",
    resolve: async ({ id }, args, ctx: Context, info) => {
      const place = await ctx.db.query.place({ where: { id } }, `{ host { id }, bookings { id } }`)
      if (place.host.id === ctx.user.id) {
        return place.bookings
      }
      return ctx.db.query.bookings({ where: { AND: [{ place: { id } }, { hostee: { id: ctx.user.id } }] } })
    }
  }
}

The resolver could probably be written better to improve performance but you get the idea.

  1. Use graphql-shield and write a rule which blocks access to the bookings key on the Place type if the current user is not an owner of that place:
const rules = {
  Query: {
    // ...
  },
  Mutation: {
    // ...
  },
  Place: {
    bookings: and(isAuthenticated, isPlaceOwner)
  },
}

I hope this helps someone!

nickluger commented 6 years ago

One can also introduce two different data models PublicBooking and Booking, where the first one hides certain fields, among those, bokee. So i can use PublicBookings to e.g. determine availability of a place and use Booking for example in my own profile or an admin view. Then you use graphql-shield to restrict access to those queries and mutation that use the Booking type.

We are using a similar approach currently with PrivateProfile and PublicProfile for our user data model, which both map to the same underlying User model but expose different fields.

You just need to take care, that, if you use Booking (or PrivateBooking) as a reference field type on another model, queries and mutations on this model also need to be protected by graphql-shield.

daveols commented 6 years ago

I considered that approach too @nickluger. It is beneficial in terms of being a more accurate representation to the client via introspection. My thoughts on the potential downsides:

There is no 'right' approach here I think and it really depends on the use case and how you expect that resolver tree to behave throughout the schema lifespan.

nickluger commented 6 years ago

I strongly support the 2nd approach suggested by @daveols, using graphql-shield. The multi-model approach has another disadvantage, if you use Prisma - you cannot use fragments on the client any longer!

See here: https://github.com/graphql-binding/graphql-binding/issues/67

The 1st approach has the disadvantage of mingling resolvers with imperative authorization code, as described here https://www.prisma.io/blog/graphql-directive-permissions-authorization-made-easy-54c076b5368e/.

voordev commented 5 years ago

Just a very blant remark, this does mean that you cannot currently implement prisma for a production application... or is there a concrete solution soon to be released. I ask this question since im evaluating this project vs other solutions like graphile.

nickluger commented 5 years ago

We're using the solution suggested by @daveols now, succesfully for a production app. You could try that, too.

abhiaiyer91 commented 5 years ago

@voordev this by no means that prisma is not ready for a production application. Just means we need to add this to this implementation. We’ll add some shield soon