Graphql-shield: If fallbackRule is defined, it ignores the other rules

[THPubs]

Bug report

I just tried to set the fallbackRule to deny. The problem is, when this is set, even if the other rules pass, the server gives a Not authorized error.

1. This is my GraphQL Schema.

type Branch {
  id: ID!
  name: String
}

type Query {
  branch(where: BranchWhereUniqueInput!): Branch
  branches(
    where: BranchWhereInput
    orderBy: BranchOrderByInput
    skip: Int
    after: String
    before: String
    first: Int
    last: Int
  ): [Branch]!
}

1. This is the invoked query

query {
  restaurant(where: { id: "_" }) {
    id
  }
}

1. I use these permissions

const isAuthenticated = rule({ cache: 'no_cache' })(async (parent, args, ctx) => {
  console.log(ctx.user);
  return ctx.user !== null;
});

const permissions = shield(
  {
    Query: {
      branches: isAuthenticated,
      branch: isAuthenticated,
    },
  },
  {
    fallbackRule: allow,
    graphiql: true
  }
);

1. This is the error I see

Error: Not Authorised!

Expected behaviour

The user is coming to the isAuthenticated method. But if fallbackRule is defined, the isAuthenticated method runs and the user gets console logged. But it still returns Error: Not Authorised!. If I remove fallbackRule or set it to allow, the query runs without issue and also blocks if the user is actually not there.

Actual behaviour

The fallbackRule should be executed only if there are no rules set for the query.

[maticzav]

Hmm @THPubs, that's quite unusual! Would you mind creating a simple reproduction repository or CodeSandbox? This way I can help you with the issue a lot more quickly 🙂

[luisfrocha]

I'm currently having the same issue. I have the following:

module.exports = shield({
  Mutation: {
    login: allow
  },
}, { fallbackRule: isAuthenticated});

but I had to add a check at the beginning of the function to allow passing through on login.

const isAuthenticated = or(rule()(async (parent, args, context) => {
  if(context.request.body.operationName === 'login'){
    return true;
  }
  ...
  // some checks that could return false
  ...
  return true;
}), deny401);

After that I check for the auth token.
Using [email protected], [email protected]

[caioflores]

I'm having the same problem here, maybe it's a cache problem?

[THPubs]

Hmm @THPubs, that's quite unusual! Would you mind creating a simple reproduction repository or CodeSandbox? This way I can help you with the issue a lot more quickly 🙂

Really sorry in a tight schedule. Not sure whether I can create a sample repo soon 😢

[maticzav]

Hey @THPubs 👋,

I just noticed; the schema you provided in the issue report doesn't match the executed query. Could you sync that? I believe the issue stems from the schema definition, in particular, nullable and non-nullable fields. You can read more about it here;

https://github.com/maticzav/graphql-shield/issues/97#issuecomment-404867307

[ph55]

Make sure response type also allowed.

Example:

type Mutation {
  login(input: LoginInput!): LoginResponse
}

Not enough for login whitelist:

shield({ Mutation: { login: allow } }, { fallbackRule: deny })

Should be:

shield({ Mutation: { login: allow }, LoginResponse: allow }, { fallbackRule: deny })

Or just add custom fallbackRule and allow fields:

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.

[ellipticaldoor]

Can be possible to detect LoginResponse should be allowed by default?

Because login is allowed on the shield

[ph55]

Or maybe fallbackRule could be set per type.
For example:

// ...
{ 
  fallbackRules: {
    Query: deny,
    Mutation: deny,
    Resource: allow,
    Field: allow
  } 
}

[maticzav]

@ellipticaldoor I believe this is not a good practice because multiple fields could point to the same type. Trying to guess permissions would therefore usually resolve in a mess.

@ph55 I believe there's no real benefit of having such grained support for fallbackRule. I think this is an anti-pattern seemingly useful in small-scale projects, however, becomes rather tedious to maintain on large repositories with over dozen types because it posses a thread of split permission definitions which reduce code legibility.

[ph55]

@maticzav
It was just attempt to solve following behaviour https://github.com/maticzav/graphql-shield/issues/211#issuecomment-450636577 for { fallbackRule: deny }

If you allow Query or Mutation common sense tells that returned type should be allowed as well.
Unless you want to deny some of returned fields.

But I've already found the way to define such behaviour with custom fallbackRule.
So in some way - you already support it. https://github.com/maticzav/graphql-shield/issues/243#issuecomment-452771010

[maticzav]

@ph55 I love your suggestion on #243! What I tried to portray in my comment below is the following situation;

type Query {
  viewer: Viewer
  houses: [House!]!
}

type Viewer {
  houses: [House!]!
}

type House {
  id: ID!
  name: String!
  price: Float!
  ...
  inquiries: [String!]!
}

Now, imagine everything is public, except inquiries which only owners of the house can access. When having one unambiguous relation to a type, I indeed agree that such functionality could be beneficial, however, when multiple fields point to one, it's not that trivial anymore.

const permissions = shield({
  Query: {
    viewer: isAuthenticated,
    houses: allow,
  },
  House: {
    id: allow,
    name: allow,
    price: allow,
    ...
    inquiries: isHouseOwner,
})

I am guessing how we could magically yet intuitively predict permission type in the current system. Perhaps you have an idea? 🙂

[maticzav]

I will close the issue now because we've drifted far from the original question. I encourage you, however, to continue the discussion of the options in the thread, or even better open up a new one, summing our current findings, and let's keep the conversation going there 🙂

[lackovic]

I had the same problem and resolved allowing the response type as well, thanks @ph55

If I think about it now it makes sense, but it's not that straightforward. I am not sure I would have come to this conclusion all by myself.

I think it would be helpful if this would be clarified in the documentation.

[maticzav]

300