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

Shield seems to only work for the first request, but allows subsequent requests #1341

Closed
1 task done
michaelrevans opened this issue Oct 7, 2021 · 5 comments · Fixed by #1459
Closed
1 task done

Comments

@michaelrevans
Copy link

Question about GraphQL Shield

I haven't managed to set up graphql-shield to correctly block requests to certain queries/mutations. I'm using express and express-graphql. Here are the relevant (I hope) parts of my setup:

import { GraphQLObjectType, GraphQLSchema } from "graphql";
import { graphqlHTTP } from "express-graphql";
import { applyMiddleware } from 'graphql-middleware';

const app = express();

const Query = new GraphQLObjectType({
  name: "Query",
  fields: {
    institution: getInstitution(),
    ...
    login: login(),
  },
});

const Mutation = new GraphQLObjectType({
  name: "Mutation",
  fields: () => ({
    ...
  }),
});

export default new GraphQLSchema({
  query: Query,
  mutation: Mutation,
});

const isAuthenticated = rule()(async (_parent, _args, _context) => {
  return false;
});

export const permissions = shield(
  {
    Query: {
      "*": isAuthenticated,
      login: allow,
    },
    Mutation: {
      "*": isAuthenticated,
    }
  }
);

app.use(
  "/graphql",
  graphqlHTTP((_, res) => ({
    schema: applyMiddleware(schema, permissions),
  }))
);

Unfortunately the wildcard doesn't seem to work for me in this case - whenever I make a query other than login it gets a Not Authorized the first time, but succeeds the following times. My only workaround now is to add every query and mutation to the permissions list, which I'd prefer not to do, because they'll always be the same.

I haven't found anything in the documentation to suggest a solution.

  • I have checked other questions and found none that matches mine.
@open-collective-bot
Copy link

Hey @michaelrevans 👋,

Thank you for opening an issue. We will get back to you as soon as we can. Have you seen our Open Collective page? Please consider contributing financially to our project. This will help us involve more contributors and get to issues like yours faster.

https://opencollective.com/graphql-shield

We offer priority support for all financial contributors. Don't forget to add priority label once you become one! 😄

@maticzav
Copy link
Owner

@michaelrevans are you also allowing the types you are returning or only fields? This is a common misconception that it's enough to allow the field, but forget to allow the type.

@cainlevy
Copy link

I am also observing this. It appears to be related to wildcard rules.

Given a simple schema:

type Query {
  ping: String!
}

The following will allow the first query { ping } request and deny the second:

export const permissions = shield(
  {
    Query: {
      "*": allow
    }
  },
  { fallbackRule: deny }
);

The following will deny the first query { ping } request and allow the second:

export const permissions = shield(
  {
    Query: {
      "*": deny
    }
  },
  { fallbackRule: allow }
);

The following will allow all query { ping } requests:

export const permissions = shield(
  {
    Query: {
      "*": deny,
      ping: allow
    }
  },
  { fallbackRule: allow }
);

@cainlevy
Copy link

I narrowed my problem down to my Jest testing setup pattern. The problem appears to be surprising behavior with applyMiddleware and shield. Is there some kind of side effect that prevents applying the same permissions object to multiple schemas?

import { loadSchema } from '@graphql-tools/load';
import { ApolloServer } from 'apollo-server';
import { GraphQLSchema } from 'graphql';
import { applyMiddleware } from 'graphql-middleware';
import { allow, deny, shield } from 'graphql-shield';

const permissions = shield({ Query: { '*': allow } }, { fallbackRule: deny });

let schema: GraphQLSchema;
// NOTE: `beforeAll` works
beforeEach(async () => {
  schema = applyMiddleware(
    await loadSchema(`type Query { ping: String! }`, {
      loaders: [],
      resolvers: {
        Query: {
          ping: () => 'pong',
        },
      },
    }),
    // NOTE: inlining (re-evaluating) works
    permissions,
  );
});

describe('bug', () => {
  test('sequential queries work', async () => {
    const server = new ApolloServer({ schema });
    const result1 = await server.executeOperation({
      query: `query { ping }`,
    });
    expect(result1.errors).toBe(undefined);
    const result2 = await server.executeOperation({
      query: `query { ping }`,
    });
    expect(result2.errors).toBe(undefined);
  });

  test('new servers do not work', async () => {
    const server = new ApolloServer({ schema });
    const result = await server.executeOperation({
      query: `query { ping }`,
    });
    expect(result.errors).toBe(undefined);
  });
});

@stale
Copy link

stale bot commented Apr 16, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

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

Successfully merging a pull request may close this issue.

3 participants