From 917dd017bd95627f2550fc8f34b4ccf03fea94c5 Mon Sep 17 00:00:00 2001 From: Sergii Date: Mon, 11 Jan 2021 19:52:39 +0200 Subject: [PATCH] feat(mongoose): throws `ForbiddenError` instead of returning a hard-coded value when user has not permissions to do some action Fixes #404 BREAKING CHANGE: `accessibleBy` eventually throws `ForbiddenError` instead of returning a hard-coded value **Before**: ```ts // ability doesn't allow to read Post const ability = defineAbility(can => can('manage', 'Comment')); try { const items = await Post.accessibleBy(ability, 'read'); console.log(items); // [] } catch (error) { console.error(error); // no error thrown } ``` **After**: ```ts // ability doesn't allow to read Post const ability = defineAbility(can => can('manage', 'Comment')); try { const items = await Post.accessibleBy(ability, 'read'); console.log(items); // not reached, because query fails with error } catch (error) { console.error(error); // ForbiddenError thrown } ``` --- .eslintrc | 3 +- .../spec/accessible_records.spec.js | 143 +++++++++++------- .../casl-mongoose/src/accessible_records.ts | 54 +++---- 3 files changed, 117 insertions(+), 83 deletions(-) diff --git a/.eslintrc b/.eslintrc index e551f6538..6fba2b2e7 100644 --- a/.eslintrc +++ b/.eslintrc @@ -58,7 +58,8 @@ }, "globals": { "expect": true, - "spy": true + "spy": true, + "fail": true }, "rules": { "@typescript-eslint/semi": ["error", "never"], diff --git a/packages/casl-mongoose/spec/accessible_records.spec.js b/packages/casl-mongoose/spec/accessible_records.spec.js index e1b918cba..e57f2245f 100644 --- a/packages/casl-mongoose/spec/accessible_records.spec.js +++ b/packages/casl-mongoose/spec/accessible_records.spec.js @@ -1,4 +1,4 @@ -import { defineAbility } from '@casl/ability' +import { defineAbility, ForbiddenError } from '@casl/ability' import mongoose from 'mongoose' import { accessibleRecordsPlugin, toMongoQuery } from '../src' @@ -84,57 +84,96 @@ describe('Accessible Records Plugin', () => { query = Post.find().accessibleBy(ability, 'notAllowedAction') }) - it('adds non-existing property check to conditions for other callback based cases', async () => { - expect(query.getQuery()).to.have.property('__forbiddenByCasl__').that.equal(1) - }) - - it('returns empty array for collection request', async () => { - const items = await query.exec() - - expect(items).to.be.an('array').that.is.empty - }) - - it('returns empty array when `find` operation is passed to `exec`', async () => { - const items = await query.exec('find') - - expect(items).to.be.an('array').that.is.empty - }) - - it('returns empty array for collection request when callback is passed to `exec`', async () => { - const items = await wrapInPromise(cb => query.exec(cb)) - - expect(items).to.be.an('array').that.is.empty - }) - - it('returns `null` for item request', async () => { - const item = await query.findOne().exec() - - expect(item).to.be.null - }) - - it('returns `null` when `findOne` operation is passed to `exec`', async () => { - const item = await query.exec('findOne') - - expect(item).to.be.null - }) - - it('returns `null` for item request when callback is passed to `exec`', async () => { - const item = await wrapInPromise(cb => query.findOne().exec(cb)) - - expect(item).to.be.null - }) - - it('returns `0` for count request', async () => { - const count = await query.count() - - expect(count).to.equal(0) - }) - - it('calls original `exec` for other cases', async () => { - Post.Query.prototype.exec = spy(() => Promise.resolve('original `exec` method call')) - await query.update({ $set: { state: 'draft' } }) - - expect(Post.Query.prototype.exec).to.have.been.called() + describe('when exist private `_pre` method', () => { + it('throws `ForbiddenError` for collection request', async () => { + await query.exec() + .then(() => fail('should not execute')) + .catch((error) => { + expect(error).to.be.instanceOf(ForbiddenError) + expect(error.message).to.match(/cannot execute/i) + }) + }) + + it('throws `ForbiddenError` when `find` operation is passed to `exec`', async () => { + await query.exec('find') + .then(() => fail('should not execute')) + .catch((error) => { + expect(error).to.be.instanceOf(ForbiddenError) + expect(error.message).to.match(/cannot execute/i) + }) + }) + + it('throws `ForbiddenError` when callback is passed to `exec`', async () => { + await wrapInPromise(cb => query.exec(cb)) + .then(() => fail('should not execute')) + .catch((error) => { + expect(error).to.be.instanceOf(ForbiddenError) + expect(error.message).to.match(/cannot execute/i) + }) + }) + + it('throws `ForbiddenError` for item request', async () => { + await query.findOne().exec() + .then(() => fail('should not execute')) + .catch((error) => { + expect(error).to.be.instanceOf(ForbiddenError) + expect(error.message).to.match(/cannot execute/i) + }) + }) + + it('throws `ForbiddenError` when `findOne` operation is passed to `exec`', async () => { + await query.exec('findOne') + .then(() => fail('should not execute')) + .catch((error) => { + expect(error).to.be.instanceOf(ForbiddenError) + expect(error.message).to.match(/cannot execute/i) + }) + }) + + it('throws `ForbiddenError` for item request when callback is passed to `exec`', async () => { + await wrapInPromise(cb => query.findOne().exec(cb)) + .then(() => fail('should not execute')) + .catch((error) => { + expect(error).to.be.instanceOf(ForbiddenError) + expect(error.message).to.match(/cannot execute/i) + }) + }) + + it('throws `ForbiddenError` for count request', async () => { + await query.count() + .then(() => fail('should not execute')) + .catch((error) => { + expect(error).to.be.instanceOf(ForbiddenError) + expect(error.message).to.match(/cannot execute/i) + }) + }) + + it('throws `ForbiddenError` for count request', async () => { + await query.count() + .then(() => fail('should not execute')) + .catch((error) => { + expect(error).to.be.instanceOf(ForbiddenError) + expect(error.message).to.match(/cannot execute/i) + }) + }) + + it('throws `ForbiddenError` for `countDocuments` request', async () => { + await query.countDocuments() + .then(() => fail('should not execute')) + .catch((error) => { + expect(error).to.be.instanceOf(ForbiddenError) + expect(error.message).to.match(/cannot execute/i) + }) + }) + + it('throws `ForbiddenError` for `countDocuments` request', async () => { + await query.estimatedDocumentCount() + .then(() => fail('should not execute')) + .catch((error) => { + expect(error).to.be.instanceOf(ForbiddenError) + expect(error.message).to.match(/cannot execute/i) + }) + }) }) }) }) diff --git a/packages/casl-mongoose/src/accessible_records.ts b/packages/casl-mongoose/src/accessible_records.ts index 242ecb12a..9b795383e 100644 --- a/packages/casl-mongoose/src/accessible_records.ts +++ b/packages/casl-mongoose/src/accessible_records.ts @@ -1,33 +1,28 @@ -import { Normalize, AnyMongoAbility, Generics } from '@casl/ability'; +import { Normalize, AnyMongoAbility, Generics, ForbiddenError, getDefaultErrorMessage } from '@casl/ability'; import { Schema, DocumentQuery, Query, Model, Document } from 'mongoose'; import { toMongoQuery } from './mongo'; -const DENY_CONDITION_NAME = '__forbiddenByCasl__'; // eslint-disable-line - -function returnQueryResult(this: any, methodName: string, returnValue: any, ...args: any[]) { - const [conditions, , callback] = args; - - if (conditions[DENY_CONDITION_NAME]) { - return typeof callback === 'function' - ? callback(null, returnValue) - : Promise.resolve(returnValue); - } - - if (conditions.hasOwnProperty(DENY_CONDITION_NAME)) { - delete conditions[DENY_CONDITION_NAME]; - } - - return this[methodName].apply(this, args); -} - -function emptifyQuery(query: DocumentQuery) { - query.where({ [DENY_CONDITION_NAME]: 1 }); - const privateQuery: any = query; - const collection = Object.create(privateQuery._collection); // eslint-disable-line - privateQuery._collection = collection; // eslint-disable-line - collection.find = returnQueryResult.bind(collection, 'find', []); - collection.findOne = returnQueryResult.bind(collection, 'findOne', null); - collection.count = returnQueryResult.bind(collection, 'count', 0); +function failedQuery( + ability: AnyMongoAbility, + action: string, + modelName: string, + query: DocumentQuery +) { + const error = ForbiddenError.from(ability); + error.action = action; + error.subjectType = modelName; + error.setMessage(getDefaultErrorMessage(error)); + + query.where({ __forbiddenByCasl__: 1 }); // eslint-disable-line + query.exec = function patchedExecByCasl(...args: any[]) { + const cb = typeof args[0] === 'function' ? args[0] : args[1]; + if (typeof cb === 'function') { + process.nextTick(() => cb(error)); + return; + } + // eslint-disable-next-line consistent-return + return Promise.reject(error); + } as typeof query['exec']; return query; } @@ -52,11 +47,10 @@ function accessibleBy( throw new TypeError('Cannot detect model name to return accessible records'); } - const toQuery = toMongoQuery as (...args: any[]) => ReturnType; - const query = toQuery(ability, modelName, action); + const query = toMongoQuery(ability, modelName, action); if (query === null) { - return emptifyQuery(this.where()); + return failedQuery(ability, action || 'read', modelName, this.where()); } return this instanceof Query ? this.and([query]) : this.where({ $and: [query] });