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

Authorization using wrapResolve #219

Open
Scriptkid2001 opened this issue May 12, 2020 · 11 comments
Open

Authorization using wrapResolve #219

Scriptkid2001 opened this issue May 12, 2020 · 11 comments

Comments

@Scriptkid2001
Copy link

Scriptkid2001 commented May 12, 2020

Hi,

first of all thanks to Pavel for implementing the issue I sent him via Twitter Direct.

What I want to acchieve is the ability to check for permissions before a query or mutation is executed. The user info is transmitted via JWT.

What I have done so far:
I've created the method requirePermissions which wraps the resolvers and to which the required permission(s) can be passed as string(s). For now it looks like this:

function requirePermissions(resolvers: Record<string, Resolver>, ...permissions: Array<String>): Record<string, Resolver> {
    Object.keys(resolvers).forEach((k) => {
        resolvers[k].wrapResolve((next) => async (rp) => {
            // to be done
            return next(rp);
        });
    });

    return resolvers;
};

It is meant to be used in the following way:

schemaComposer.Query.addFields({
    ...requirePermissions({
        userById: UserTC.getResolver('findById'),
        userByIds: UserTC.getResolver('findByIds'),
        // ...
    }, 'users:read'),
    // ...

In this case users:read is the required permission string.

The Express request object is passed to the context field in the GraphQLHTTP Middleware:

this.app.use(this.route + '/api', graphqlHTTP((req) => ({
    schema: graphql,
    graphiql: this.debug,
    context: { req }
})));

Now, the following is left to do:

  • Find the user document in MongoDB using username in req.body.username. (Authencity of the JWT is verified via separate middleware, so no need to worry about that.)
  • Check if the user doc includes the required permission string.
  • If so, allow the request.
  • If not, block the request and return an error (e.g. Unauthorized) instead.

Hope someone can point me in the right direction :)

@amarflybot
Copy link

I think Authorization could be handled in different way.
Try having a look at: https://github.com/maticzav/graphql-shield

nodkz added a commit that referenced this issue May 13, 2020
@nodkz
Copy link
Member

nodkz commented May 13, 2020

Try to use rp.beforeQuery here is a small test case:
https://github.com/graphql-compose/graphql-compose-mongoose/blob/a880f3eaa9f33f337539d80879157e8b89e92864/src/__tests__/github_issues/219-test.js

Feel free to extend it as you wish.

@Scriptkid2001
Copy link
Author

I think Authorization could be handled in different way.
Try having a look at: https://github.com/maticzav/graphql-shield

This looks like an extensive library...great to know that it exists, but for now, I'll try to keep things as simple as possible :)

@Scriptkid2001
Copy link
Author

Scriptkid2001 commented May 15, 2020

Try to use rp.beforeQuery here is a small test case:
https://github.com/graphql-compose/graphql-compose-mongoose/blob/a880f3eaa9f33f337539d80879157e8b89e92864/src/__tests__/github_issues/219-test.js

Feel free to extend it as you wish.

Thanks for the test case! Didn't know that rp.beforeQuery(...) exists.

The reason why the function above didn't do anything was because I just called resolvers[k].wrapResolve without assigning it (resolvers[k] = resolvers[k].wrapResolve).

Now everything works so far (the code is executed), but if I throw an error, the query/mutation is executed anyway. The error is not caught and causes the application to halt.
How do I prevent the query/mutation from being executed and return an error instead?

This is the code I have so far:

function requirePermissions(resolvers: Record<string, Resolver>, ...permissions: Array<string>): Record<string, Resolver> {
    Object.keys(resolvers).forEach((k) => {
        resolvers[k] = resolvers[k].wrapResolve((next) => (rp) => {
            if (!('username' in rp.context.req.user)) {
                global.app.logger.warning('Unauthenticated user attempted actions ' + permissions.join(', '));
                throw new Error('Unauthenticated');
            }
            const username: string = rp.context.req.user.username.toLowerCase();

            User.findOne({ username: username },
            (err, User) => {
                if (err) {
                    global.app.logger.warning('Database error while retrieving user document for ' + username);
                    throw new Error('Internal');
                }
                if (!User) {
                    global.app.logger.warning('Document for user ' + username + ' not found');
                    throw new Error('Internal');
                }

                if (!User.checkPermissions(permissions)) {
                    global.app.logger.warning('User ' + username + ' attempted actions ' + permissions.join(', ') +  ' with missing permissions');
                    throw new Error('Unauthorized');
                }
            });
            return next(rp);
        });
    });

    return resolvers;
};

@Scriptkid2001
Copy link
Author

Ok, so I found the mistake which prevented the error from being forwarded:
Errors thrown inside the Mongoose callback scope will not be caught by GraphQL.

The solution:
Create an outer error variable and assign the error from inside the callback if necessary. Not that you need to await the Mongoose query function.
If an error has been set, throw it from outside the callback.

function requirePermissions(resolvers: Record<string, Resolver>, ...permissions: Array<string>): Record<string, Resolver> {
    Object.keys(resolvers).forEach((k) => {
        resolvers[k] = resolvers[k].wrapResolve(next => async rp => {
            const username: string = rp.context.req.user.username.toLowerCase();
            let error: Error = null;

            await User.findOne({ username: username },
            (err, User) => {
                if (err) {
                    global.app.logger.warning('Database error while retrieving user document for ' + username);
                    error = new Error('Internal');
                }
                if (!User) {
                    global.app.logger.warning('Document for user ' + username + ' not found');
                    error = new Error('Internal');
                }

                if (!User.checkPermissions(permissions)) {
                    global.app.logger.warning('User ' + username + ' attempted actions ' + permissions.join(', ') +  ' with missing permissions');
                    error = new Error('Unauthorized');
                }
            });

            if (error) throw error;
            return next(rp);
        });
    });

    return resolvers;
};

@nodkz
Copy link
Member

nodkz commented May 18, 2020

@Mastercoder3000 you should not use async & callback simultaneously with mongoose. If you provide a callback, then promise doesn't work.

Try to rewrite your code in such way:

function requirePermissions(resolvers: Record<string, Resolver>, ...permissions: Array<string>): Record<string, Resolver> {
    Object.keys(resolvers).forEach((k) => {
        resolvers[k] = resolvers[k].wrapResolve(next => async rp => {
            const username: string = rp.context.req.user.username.toLowerCase();

            const User = await User.findOne({ username: username }).exec();

            if (!User) {
                global.app.logger.warning('Document for user ' + username + ' not found');
                throw new Error('Internal');
            }

            if (!User.checkPermissions(permissions)) {
                global.app.logger.warning('User ' + username + ' attempted actions ' + permissions.join(', ') +  ' with missing permissions');
                error = new Error('Unauthorized');
            }

            return next(rp);
        });
    });

    return resolvers;
};

@Scriptkid2001
Copy link
Author

Scriptkid2001 commented May 19, 2020

@nodkz Is there a way of accessing the Mongoose document(s) of a query (and skip it = dont return some or throw an error in specific cases) before returning it?

@Scriptkid2001
Copy link
Author

The reason why I'm asking is because I've found a (potential) way of handling permissions with more flexibility. Permissions to a specific document are defined by its property values.

For every Mongoose model, a permissions callback is created, which returns the permissions strings (of which the user needs at least one) that allow access to a specific document.

// IClientModel extends Mongoose.Document
export const ClientPermissions = (doc: IClientModel): Array<String> => {
	return [
		doc._id,
		doc.name,
		'author:' + doc.author._id,
		'author:' + doc.author.username,
		'group:' + doc.group.id,
		'group:' + doc.group.name
	];
}

For example, a user will need the permission clients:read:clientname OR clients:read:author:authorname in order to query a document with 'clientname' as name field OR 'authorname' as username of the author.

In order to check the permissions by property values, I need to access the Mongoose document before the query is executed, just like in beforeRecordMutate(doc, rp) {...}

@nodkz
Copy link
Member

nodkz commented May 21, 2020

Yep it possible and a solution is very obvious:

function requirePermissions(resolvers: Record<string, Resolver>, ...permissions: Array<string>): Record<string, Resolver> {
    Object.keys(resolvers).forEach((k) => {
        resolvers[k] = resolvers[k].wrapResolve(next => async rp => {
            const username: string = rp.context.req.user.username.toLowerCase();

            const User = await User.findOne({ username: username }).exec();

            if (!User) {
                global.app.logger.warning('Document for user ' + username + ' not found');
                throw new Error('Internal');
            }

            if (!User.checkPermissions(permissions)) {
                global.app.logger.warning('User ' + username + ' attempted actions ' + permissions.join(', ') +  ' with missing permissions');
                error = new Error('Unauthorized');
            }

-             return next(rp);
+             let result = await next(rp);
+             result = changeResultAsYouWishOrThrowError(result);
+             return result;
        });
    });

    return resolvers;
};

@Oluwatemilorun
Copy link
Contributor

You can create a middleware for this and use the Resolver.withMiddlware() method.

// ./permissions.middleware

export const CheckPermission: (permission: string) => ResolverMiddleware<any, any> = (
	p
) => async (next, s, a, c: { auth: true; user: IUser }, i) => {
	if (!c.user.permissions.includes(p))
		throw new ForbiddenError('You do not have permission.');

	const res = await next(s, a, c, i);

	return res;
};

Then, you can use it in your with your resolver like so

schemaComposer.Query.addFields({
    userById: UserTC.getResolver('findById', [CheckPermission('users:read')]), // Method 1
    userByIds: UserTC.getResolver('findByIds').withMiddlewares([CheckPermission('users:read')]), // Method 2. Use if you will be using other methods like `.wrap()`
    // ...

@oklas
Copy link
Contributor

oklas commented Feb 12, 2021

Current approach for middlewares is declared here #158 (comment):

schemaComposer.Query.addFields({
    userById: UserTC.mongooseResolvers.findById().withMiddlewares([authMiddleware]),
   userByIds: UserTC.mongooseResolvers.findByIds().withMiddlewares([authMiddleware]),
});

for future track #275

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

No branches or pull requests

5 participants