maticzav / graphql-shield

šŸ›” A GraphQL tool to ease the creation of permission layer.
https://graphql-shield.com
MIT License
3.56k stars 172 forks source link

Incorrect Error Being Returned #770

Closed rtnolan closed 4 years ago

rtnolan commented 4 years ago

Hey guys, I'm currently trying to integrate graphql-shield into my project which is using type-graphql, inversifyJS and prisma and I'm running into an issue where it seems that no matter what I throw, I get a "Not Authorised!" returned and I'm really not sure why. Relevant Code:

//permissions.ts
const authService = serviceContainer.get(AuthService);

const rules = {
  isAuthenticatedUser: rule()( async (parent, args, context) => {
    const userId = await authService.getUserId(context).catch(err => err);
    return Boolean(userId);
  }),
};

export const permissions = shield({
  Query: {
    me: rules.isAuthenticatedUser
  }
});
// auth-resolver.ts
@Mutation(() => UserResponse)
    async registerUser(@Arg("data") data: RegisterInput): Promise<UserResponse> {
        const { email, password, firstname, lastname } = data;
        const user = await this.authService.register(email, password, firstname, lastname);
        return user;
    }
//auth-service.ts
async register(email: string, password: string, firstname: string, lastname: string): Promise<UserResponse> {
        return new Promise(async (resolve, reject) => {
            const hashedPassword = await hash(password, 10);
            const user = await this.prisma.user.create({
                data: {
                    email: email,
                    password: hashedPassword,
                    firstname: firstname,
                    lastname: lastname,
                },
            // reject error if user can't be created
            }).catch(err => reject(err));
            if (user) {
                resolve(user);
            }
            reject(new Error("Invalid"));
        });
    }
//server.ts
const schema = await buildSchema({
        resolvers: [ AuthResolver, UserResolver ],
        container: serviceContainer,
        emitSchemaFile: path.resolve(__dirname, "./generated-schema.graphql"),
        validate: false,
        dateScalarMode: "timestamp",
    });

    const server = new ApolloServer({
        schema: applyMiddleware(schema, permissions),
        playground: true,
        context: ({ req, res }): Context => ({ 
            req,
            res,
            prisma: serviceContainer.get(PrismaClient),
            container: serviceContainer
         }),
    });

In the above code, I'm only protecting one endpoint me(). When I called the mutation registerUser() with an email that's already been registered (there's a unique constraint on email) it's responding with Not Authorized, instead of the error that should be resolved from Prisma, any ideas on why this might be happening when registerUser() isn't behind the shield? (Including a link to sample repository incase that helps: https://github.com/tnolan8/typegraphql-api)

rtnolan commented 4 years ago

Believe I've found my issue. When inspecting the output of generateMiddlewareFromSchemaAndRuleTree() I can see that it's applying Middleware to all of my types:

MIDDLEWARE:  {
  Query: { me: [AsyncFunction: middleware] },
  UserResponse: {
    email: { fragments: [], resolve: [AsyncFunction: middleware] },
    firstname: { fragments: [], resolve: [AsyncFunction: middleware] },
    lastname: { fragments: [], resolve: [AsyncFunction: middleware] },
    createdAt: { fragments: [], resolve: [AsyncFunction: middleware] }
  },
  Mutation: {
    registerUser: { fragments: [], resolve: [AsyncFunction: middleware] },
    loginUser: { fragments: [], resolve: [AsyncFunction: middleware] }
  },
  AuthPayload: {
    token: { fragments: [], resolve: [AsyncFunction: middleware] },
    user: { fragments: [], resolve: [AsyncFunction: middleware] }
  }
}

When the middleware is executing for registerUser() it gets to: return await resolve(parent, args, ctx, info) Ln 70 in generator.ts. The parent fails to resolve because of the failure on the unique constraint and is then caught in the catch() block. Looks like all I needed was to set { allowExternalErrors: true } when creating the shield, seems to do the trick.

darrylyoung commented 4 years ago

Thanks for following up with your solution, @tnolan8. I'm also using Prisma and was wondering why I was seeing the "Not Authorised!" message, despite having a custom error in my resolver. Adding allowExternalErrors: true fixed it for me, too. Thanks.

maticzav commented 4 years ago

Hey @darrylyoung šŸ‘‹ ,

Are docs unclear about handling errors? What I'd advise is that you return the error inside your resolver - that's a mechanism shield uses to distinguish server errors from "predictable" patterns so that you don't leak internal structure of the app.

darrylyoung commented 4 years ago

Hi, @maticzav. šŸ‘‹

Thanks for your message. No, the docs aren't unclear; I just clearly didn't read everything thoroughly enough. Taking another look now, the docs cover all of this well.

Related to the issue I thought I had, I did have an error being thrown in my resolver but all I ever got was "Not Authorised!". I thought this was an issue that I could resolve with allowExternalErrors: true but I see now, looking at the docs again, that this is by design. I'll go back through my error handling and, where necessary, return the errors as opposed to throwing them.

This tool is great, by the way. Thank you very much for your hard work on it. I think one of the reasons I rushed through the docs is because I started a project (with Prisma 2) using an example that was using an old version of this package so I didn't pay much attention to it at first until I realised I needed to update it and add some more rules. šŸ˜„

maticzav commented 4 years ago

šŸ˜‚ I see! Awesome! I am glad that you've fixed your issue and that you enjoy the library.