Graphql-shield: Adding rules to a type and its fields

[LK]

Question about GraphQL Shield

Let's say I have a type User in my GraphQL schema, and I want to limit all queries on that type to those that match some isAuthenticated rule. But within this User type, I also have a secret field, which I want to further restrict with an additional isAdministrator rule. Is there any way to list both type-wide and field-specific rules if the two overlap, or would I need to enumerate all of the fields in User to explicitly add the isAuthenticated rule for each?

• [x] I have checked other questions and found none that matches mine.

[maticzav]

Hey @LK 👋,

Unfortunately, yes. I believe, at least from my dwelling on the topic, that there's no clear solution which would allow such "smart" assigning of the rules or type-specific fallbacks. To my understanding, it boils down to making a very opinionated decision which is not the primary goal of this package.

Maybe you have a smart suggestion that we could discuss in the thread? 🙂

I believe this could also be connected to #211 (the messages at the bottom).

[LK]

Thanks for clarifying, I wasn't sure if I was just missing something in the documentation!

I actually think that the per-type fallbackRules implementation might not be so bad — the way I've been structuring my rule definitions is that each resolver file (one per type) exports an object defining the per-field rules for each field in that type. The resolvers/index.ts file then combines all of the individual resolver objects into one, and similarly combines all of the per-type rule definitions into one object that can be passed to shield(...). This is then exported out, and used to initialize the GraphQL server and middleware. This is the best way I could come up with to keep resolver logic and permissions in the same place (although I'm open to other suggestions if there is some other organization is better practice!).

This model is then easily extendable by optionally exporting an additional type-wide rule in each resolver file. (I don't think it would be quite the same as a fallback rule, as I would expect it to apply to all fields in the type _in addition to_ any other field-specific rules that are defined — although I don't see why the same model couldn't apply to type-specific fallback rules as well.) If I'm understanding your concern in https://github.com/maticzav/graphql-shield/issues/211#issuecomment-452089630 correctly, I think this is actually the same problem that is raised by having to pass a single rule definition object to shield(...), and it can be solved in the same way.

[ph55]

It is easily achieved by defining custom fallbackRule.

In following example, deny queries and mutations which have no rules but allow types and fields.
So once you have rule that allows Query or Mutation you don't have to add rule for returned type and (or) it's fields.

const shieldFallback = async (parent, args, ctx, info) => {
    switch (info.parentType.name) {
      // queries
      case 'Query':
        return false;
      // mutations
      case 'Mutation':
        return false;
      // returned types and it's fields
      default:
        return true;
    }
};
const fallbackRule = rule({ cache: false })(shieldFallback);

const ruleTree = { Query: {}, Mutation: {} };
const permissions = shield(ruleTree, { fallbackRule });

You can play further with fallback for your needs.

[LK]

Thanks @ph55. My understanding of fallbackRule is that it is only applied if no other rules apply to that field. Going back to the example from my original message, if I set an isAdministrator rule on the field secret, then fallbackRule is not consulted when checking the permissions of that field. See https://github.com/maticzav/graphql-shield/blob/992e8b32223efda03b0097a73f97c8426edc749a/src/test/fallback.test.ts

The behavior that I need is to be able to specify a rule that applies to an entire type, and also list additional per-field rules on top of that.

[maticzav]

@LK let's think of the options we have. I think I understand why such functionality could be beneficial, however, I am struggling to find the right approach. My primary goal of this conversation is to understand the use case we are trying to solve and propose a meaningful solution to the problem which won't pose a holdback in the future.

A few concerns that I have:

1. We shouldn't split fallbacks with permissions because it doesn't scale on large systems.
2. We should apply a logic rule when extending default with a particular rule.

My proposal:

declare function extend(rule: ShieldRule, fieldMap: IRuleFieldMap): IRuleFieldMap

const houseType = extend(allow, {
  inquiries: isUserOwner,
})

which would reduce to

const houseType: IRuleFieldMap = {
  id: allow,
  name: allow,
  price: allow,
  inquiries: and(allow, isUserOwner),
}

A few open questions that I have:

1. What kind of logic rule should we use?
2. How does global fallbackRule work?
3. What should be the name of the function?
4. Should we omit fallbackRule altogether in favour of extend which could also accept IRules as an argument, or should we keep it narrowed down to types only?

cc @ph55

[ph55]

Doesn't seem we all on same page here 🙂

@LK I see what you mean. I don't know why I've written down https://github.com/maticzav/graphql-shield/issues/243#issuecomment-452771010 here.
It's actually relevant to related issue https://github.com/maticzav/graphql-shield/issues/211.

The behaviour that I need is to be able to specify a rule that applies to an entire type, and also list additional per-field rules on top of that.

So you want different rules for type and its fields. Not the fallbackRule regular rule that different for type and fields.

I had exact same thinking when built my ACL.

As I've seen it's not possible.
Since there is no such thing - resolving type, if I understand correctly.
You resolve not the type but list of its fields.
So basically when you set fallback for type - you set fallback for all of type's resolved fields.
I refer to original graphql-middleware repo logic (core of this package) - https://github.com/prisma/graphql-middleware

You can set custom rule for your type and console.log values you get in the rule. You'll se info about types' fields you selected - not the type itself.

Thats why if you deny whole type - you'll actually get same error for each field and not one error for type. Try it.

---

This leads us to what @maticzav propose I guess.
Not sure though how fallbackRule related here.

I personally not using rules per type for one reason I've already mentioned -
it returns same error for every field you trying to select.

{ User: rule()(async () => 'User type not allowed') }

{
  user(id: 1) {
    email
    phone
  }
}

will return

{
  "errors": [
    {
      "message": "User type not allowed",
      "locations": [
        {
          "line": 4,
          "column": 5
        }
      ],
      "path": [
        "user",
        "phone"
      ],
      "extensions": {
        "code": "INTERNAL_SERVER_ERROR"
      }
    },
    {
      "message": "User type not allowed",
      "locations": [
        {
          "line": 3,
          "column": 5
        }
      ],
      "path": [
        "user",
        "email"
      ],
      "extensions": {
        "code": "INTERNAL_SERVER_ERROR"
      }
    }
  ],
  "data": {
    "user": null
  }
}

[LK]

That looks like a reasonable proposal to me, @maticzav. extend would need to somehow receive what type to operate on, and be aware of what other fields are on that type, right? Any ideas on how to handle that? (Middleware generators?)

As for your questions:

1. and seems like the logic rule that would work for my intended use case. However it might make sense to include this as an optional additional parameter to extend, because I can imagine a case where a different rule might come in handy.
2. I think that fallbackRule should not apply to any fields in an extend-ed type.
3. I'll throw out a few more options: extendType, addRule, combine, defaultRule
4. Not sure I follow how fallbackRule can be implemented with this scheme, could you elaborate?

[maticzav]

@LK GraphQL Shield internally uses middleware generators. Therefore there should be no problem of handling extend-type.

1. I think it's best if we first release it as an and only extension, after seeing the response and having a more concrete basis for our decision, however, we can add an option for other logic rules as well.
2. 👍
3. extendType sound quite promising.
4. Let me try to clear that out;

// current approach
const permissions = shield({
  Query: {
     user: isAuthenticated
  }
}, { fallbackRule: deny })

// proposal one and its identity
const permissions = shield({
  Query: extendType(allow, {
     user: isAuthenticated
  })
}, { fallbackRule: deny })

const permissions = shield({
  Query: {
     book: allow,
     books: allow,
     user: isAuthenticated
  }
}, { fallbackRule: deny })

// proposal two and its identity
const permissions = shield({
  Query: {
     user: isAuthenticated
  }
}, { fallbackRule: deny })

const permissions = shield(
  extend(deny, {
    Query: {
      user: isAuthenticated
    }
  })
)

PS.: extendType sounds great, however, might be misleading or confusing due to its structure declare function extendType(rule: ShieldRule, fieldMap: IRuleFieldMap): IRuleFieldMap - we are not mentioning any types!?

[LK]

Regarding (4), does this produce the same behavior as the current fallbackRule implementation? My interpretation of how fallbackRule works now is like this:

// Assume `Query` has fields `user` and `books`

const permissions = shield({
  Query: {
     user: isAuthenticated
  }
}, { fallbackRule: deny })

// is equivalent to...

const permissions = shield({
  Query: {
     user: isAuthenticated,
     books: deny
  }
})

Meanwhile I am interpreting the last example of your message like this:

const permissions = shield(
  extend(deny, {
    Query: {
      user: isAuthenticated
    }
  })
)

// evaluates to...

const permissions = shield({
  Query: {
    user: and(deny, isAuthenticated),
    books: deny
  }
})