Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
nickluger opened this issue Feb 22, 2018 · 11 comments
Labels

Comments

@nickluger
Copy link

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
Copy link
Contributor

schickling commented Feb 23, 2018

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
Copy link

@schickling, I am on it!

@daveols
Copy link

daveols commented Jul 25, 2018

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
Copy link

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
Copy link

daveols commented Aug 29, 2018

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
Copy link
Author

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
Copy link

daveols commented Aug 29, 2018

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:

  • You duplicate schema definitions possibly multiple times for different access levels
  • You need to rewrite custom resolvers for the common fields between the types you are duplicating
  • You are limited to exposing just one of the access levels per query/mutation, instead of showing relevant data depending on the user's access level
  • It's harder to scale when implementing more advanced, granular privacy control

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
Copy link
Author

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: dotansimha/graphql-binding#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
Copy link

voordev commented Oct 6, 2018

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
Copy link
Author

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

@abhiaiyer91
Copy link
Contributor

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants