Prisma1: Automatically filter inaccessible nodes in the response

[marktani]

This has an interesting interaction with pagination.
This could also be a package that filters nodes after query execution.

Maybe we can enforce a better defined structure for custom permission rules, which would make this filtering easier in combination with pagination.

[kbrandwijk]

Apparently, this is also the case for subscriptions. If I create a subscription without a filter, and I only have READ permission on a subset of nodes, any node that is created that I'm not allowed to read throws an 'Insufficient permissions' error

[serverlesspolska]

I would very much to see this functionality implemented. To be honest, I'm really surprised by the way currently query results are displayed to the client when permission queries are involved.

First of all, in relay we see a count of all nodes, even those to which we don't have access - that forces additional handling on the client side. And is not secure: see https://github.com/graphcool/graphcool/issues/711

Secondly, have all those null's instead node elements, again additional handling needed.

And last, but not least, why are you sending so much irrelevant data back to the client? Error node for each node to which I don't have access. So much wasted traffic! And again I need to parse errors object to see if there were REAL errors or just Permission Queries are working FINE.

Don't get me wrong but for me, it's really really strange.

Could you please enable the option to automatically remove all those things? Basically, if a client can read two nodes out of million I want to see: count: 2 and two nodes, nothing less nothing more (that means NO error nodes).

[kbrandwijk]

Also related: https://github.com/graphcool/graphcool/issues/274

[enginoid]

As someone who just stumbled upon this via graphcool#711, I'd like to just pitch that this issue gets a bit of a push in the way suggested by @kbrandwijk and @jonathanheron in that issue.

I think if I was running Graphcool in production and relying on the permissions system, I would find this behavior very surprising. If the aim is that Graphcool projects can graduate to production for any moderately serious usage, I'd definitely prioritize a fix for this.

@jonathanheron has given lucid examples of the security impact in #711. From reading, I can say for my own purposes that if I were to release a product that needed the permissions system (eg. if it stored user data or anything sensitive), this issue would be a blocker for using Graphcool.

[kbrandwijk]

@enginoid Priority is, according to https://github.com/graphcool/framework#roadmap, for a big part influenced by people giving a 👍 on Github issues. And while I completely agree with what you're writing, this issue has two 👍, and #711 has only one 👍. I would like to advise you, and anyone else affected by these issues, to vote it up, so it will get the attention it deserves.

[enginoid]

Thanks for the tip! I think that's understandable in general and a worthy product practice, but I would suggest Graphcool adopt an expedited approach for security issues that may affect customers without them being aware.

It's easy to see how a customer could rely on the permission system for sensitive data and be unaware that their data is readily exposed through progressively building a wildcard string with increasing specificity.

[serverlesspolska]

@enginoid I totally agree with you! While the community can vote on feature requests company should be responsible for taking care of security above other things.

Also, I would like to describe another problem with returning full count and null nodes.
Unfortunately, I have to repeat myself as this issue is not only about data security.

Let's consider an example where we have a type MyType and two users A and B. Let's say A is admin and has access to all nodes of MyType, while B has access to only 4 of them. We have total 12 nodes in db.

query {
    viewer {
        allMyTypes {
            count
            edges {
                node {
                    id
                }
            }
        }
    }
}

Above query will return for A all nodes without any errors, count = 12. However for B we get something else, for example:

{
  "data": {
    "viewer": {
      "allMyTypes": {
        "count": 12,
        "edges": [
          node(id: 1,  other props... - B has access to that node),
          null,
          null,
          node(id: 4,  B has access to that node),
          node(id: 5, B has access to that node),
          null,
          null,
          null,
          null,
          null,
          node(id: 11 B has access to that node),
          null
        ]
      }
    }
  },
  "errors": [
    {
      "locations": [
        {
          "line": 7,
          "column": 21
        }
      ],
      "path": [
        "viewer",
        "allMyTypes",
        "edges",
        0,
        "node",
        "id"
      ],
      "code": 3008,
      "message": "Insufficient Permissions","requestId" .....
...

Again count = 12 which IMHO is totally stupid. It also creates HUGE problem with _Relay Modern_ PaginationContainer. PaginationContainer uses count value to determin how many nodes there are to display. Moreover, this container expectes that all nodes are not null.

Imagine, I want to display 3 nodes of MyType per page on my website for user B.

1. In the first step when the page is loaded the only node with id: 1 is displayed (second and third nodes are null but Relay is not aware of that!)
2. On the second page, It would display nodes with id:4 and id:5 ( node id:6 is null for B)
3. On the third page, it would not display anything at all (as nodes from id=7 to 9 are null for user B)

Is this really the desired solution by Graphcool Team? I mean Graphcool advertises that supports Relay. @marktani could please respond?

Honestly, I'm surprised I'm the first one spotting that problem. Or am I not?

[serverlesspolska]

@marktani Could please add bug label to that issue?

[serverlesspolska]

@marktani @schickling Any chance for a comment from Graphcool team?

[marktani]

Thanks for your feedback, @panzupa! 🙂 We are currently looking into this topic, I will keep this thread posted with news 👍

[serverlesspolska]

@marktani any news on that?

[marktani]

This issue has been moved to graphcool/graphcool-framework.