refactor: adds comments to rulesToQuery and Rule matching and adds more tests (#950)

Author: stalniy

packages/casl-ability/spec/rulesToQuery.spec.js

@@ -1,6 +1,5 @@
 import { defineAbility } from '../src'
 import { rulesToQuery } from '../src/extra'
-import './spec_helper'
 
 function toQuery(ability, action, subject) {
   const convert = rule => rule.inverted ? { $not: rule.conditions } : rule.conditions
@@ -12,14 +11,8 @@ describe('rulesToQuery', () => {
     const ability = defineAbility(can => can('read', 'Post'))
     const query = toQuery(ability, 'read', 'Post')
 
-    expect(Object.keys(query)).to.be.empty
-  })
-
-  it('returns `null` if empty `Ability` instance is passed', () => {
-    const ability = defineAbility(() => {})
-    const query = toQuery(ability, 'read', 'Post')
-
-    expect(query).to.be.null
+    expect(query).toBeInstanceOf(Object)
+    expect(Object.keys(query)).toHaveLength(0)
   })
 
   it('returns empty `$or` part if at least one regular rule does not have conditions', () => {
@@ -29,7 +22,8 @@ describe('rulesToQuery', () => {
     })
     const query = toQuery(ability, 'read', 'Post')
 
-    expect(Object.keys(query)).to.be.empty
+    expect(query).toBeInstanceOf(Object)
+    expect(Object.keys(query)).toHaveLength(0)
   })
 
   it('returns empty `$or` part if rule with conditions defined last', () => {
@@ -39,40 +33,48 @@ describe('rulesToQuery', () => {
     })
     const query = toQuery(ability, 'read', 'Post')
 
-    expect(Object.keys(query)).to.be.empty
+    expect(query).toBeInstanceOf(Object)
+    expect(Object.keys(query)).toHaveLength(0)
+  })
+
+  it('returns `null` if empty `Ability` instance is passed', () => {
+    const ability = defineAbility(() => {})
+    const query = toQuery(ability, 'read', 'Post')
+
+    expect(query).toBe(null)
   })
 
   it('returns `null` if specified only inverted rules', () => {
-    const ability = defineAbility((can, cannot) => {
+    const ability = defineAbility((_, cannot) => {
       cannot('read', 'Post', { private: true })
     })
     const query = toQuery(ability, 'read', 'Post')
 
-    expect(query).to.be.null
+    expect(query).toBe(null)
   })
 
-  it('returns `null` if at least one inverted rule does not have conditions', () => {
-    const ability = defineAbility((can, cannot) => {
+  it('returns `null` if inverted rule does not have conditions and there are no direct rules', () => {
+    const ability = defineAbility((_, cannot) => {
       cannot('read', 'Post', { author: 123 })
       cannot('read', 'Post')
     })
     const query = toQuery(ability, 'read', 'Post')
 
-    expect(query).to.be.null
+    expect(query).toBe(null)
   })
 
-  it('returns `null` if at least one inverted rule does not have conditions even if direct condition exists', () => {
+  it('returns `null` if at least one inverted rule does not have conditions even if direct rule exists', () => {
     const ability = defineAbility((can, cannot) => {
       can('read', 'Post', { public: true })
       cannot('read', 'Post', { author: 321 })
       cannot('read', 'Post')
     })
     const query = toQuery(ability, 'read', 'Post')
 
-    expect(query).to.be.null
+    expect(query).toBe(null)
   })
 
-  it('returns non-`null` if there is at least one regular rule after last inverted one without conditions', () => {
+  it('returns query if there is at least one regular rule after last inverted one without conditions', () => {
     const ability = defineAbility((can, cannot) => {
       can('read', 'Post', { public: true })
       cannot('read', 'Post', { author: 321 })
@@ -81,45 +83,45 @@ describe('rulesToQuery', () => {
     })
     const query = toQuery(ability, 'read', 'Post')
 
-    expect(query).to.deep.equal({
+    expect(query).toEqual({
       $or: [
         { author: 123 }
       ]
     })
   })
 
-  it('OR-es conditions for regular rules', () => {
+  it('OR-s conditions for regular rules', () => {
     const ability = defineAbility((can) => {
       can('read', 'Post', { status: 'draft', createdBy: 'someoneelse' })
       can('read', 'Post', { status: 'published', createdBy: 'me' })
     })
     const query = toQuery(ability, 'read', 'Post')
 
-    expect(query).to.deep.equal({
+    expect(query).toEqual({
       $or: [
         { status: 'published', createdBy: 'me' },
         { status: 'draft', createdBy: 'someoneelse' }
       ]
     })
   })
 
-  it('AND-es conditions for inverted rules', () => {
+  it('AND-s conditions for inverted rules', () => {
     const ability = defineAbility((can, cannot) => {
       can('read', 'Post')
       cannot('read', 'Post', { status: 'draft', createdBy: 'someoneelse' })
       cannot('read', 'Post', { status: 'published', createdBy: 'me' })
     })
     const query = toQuery(ability, 'read', 'Post')
 
-    expect(query).to.deep.equal({
+    expect(query).toEqual({
       $and: [
         { $not: { status: 'published', createdBy: 'me' } },
         { $not: { status: 'draft', createdBy: 'someoneelse' } }
       ]
     })
   })
 
-  it('OR-es conditions for regular rules and AND-es for inverted ones', () => {
+  it('OR-s conditions for regular rules and AND-es for inverted ones', () => {
     const ability = defineAbility((can, cannot) => {
       can('read', 'Post', { _id: 'mega' })
       can('read', 'Post', { state: 'draft' })
@@ -128,7 +130,7 @@ describe('rulesToQuery', () => {
     })
     const query = toQuery(ability, 'read', 'Post')
 
-    expect(query).to.deep.equal({
+    expect(query).toEqual({
       $or: [
         { state: 'draft' },
         { _id: 'mega' }
@@ -140,14 +142,39 @@ describe('rulesToQuery', () => {
     })
   })
 
-  it('returns empty `$and` part if inverted rule with conditions defined before regular rule without conditions', () => {
+  it('returns empty query if inverted rule with conditions defined before regular rule without conditions', () => {
     const ability = defineAbility((can, cannot) => {
       can('read', 'Post', { author: 123 })
       cannot('read', 'Post', { private: true })
       can('read', 'Post')
     })
     const query = toQuery(ability, 'read', 'Post')
 
-    expect(Object.keys(query)).to.be.empty
+    expect(query).toBeInstanceOf(Object)
+    expect(Object.keys(query)).toHaveLength(0)
+  })
+
+  it('should ignore inverted rules with fields and conditions', () => {
+    const ability = defineAbility((can, cannot) => {
+      can('read', 'Post', { author: 123 })
+      cannot('read', 'Post', 'description', { private: true })
+    })
+    const query = toQuery(ability, 'read', 'Post')
+
+    expect(query).toEqual({
+      $or: [{ author: 123 }]
+    })
+  })
+
+  it('should ignore inverted rules with fields and without conditions', () => {
+    const ability = defineAbility((can, cannot) => {
+      can('read', 'Post', { author: 123 })
+      cannot('read', 'Post', 'description')
+    })
+    const query = toQuery(ability, 'read', 'Post')
+
+    expect(query).toEqual({
+      $or: [{ author: 123 }]
+    })
   })
 })

packages/casl-ability/src/Rule.ts

@@ -95,13 +95,15 @@ export class Rule<A extends Abilities, C> {
     }
 
     if (!field) {
+      // if there is no field (i.e., checking whether user has access to at least one field on subject)
+      // we ignore inverted rules because they disallow to do an action on it, so we are continue looking for regular rule
       return !this.inverted;
     }
 
-    if (this.fields && !this._matchField) {
+    if (!this._matchField) {
       this._matchField = this._options.fieldMatcher!(this.fields);
     }
 
-    return this._matchField!(field);
+    return this._matchField(field);
   }
 }

packages/casl-ability/src/extra/rulesToQuery.ts

@@ -1,6 +1,6 @@
 import { CompoundCondition, Condition, buildAnd, buildOr } from '@ucast/mongo2js';
 import { AnyAbility } from '../PureAbility';
-import { RuleOf } from '../RuleIndex';
+import { Generics, RuleOf } from '../RuleIndex';
 import { ExtractSubjectType } from '../types';
 
 export type RuleToQueryConverter<T extends AnyAbility, R = object> = (rule: RuleOf<T>) => R;
@@ -15,27 +15,39 @@ export function rulesToQuery<T extends AnyAbility, R = object>(
   subjectType: ExtractSubjectType<Parameters<T['rulesFor']>[1]>,
   convert: RuleToQueryConverter<T, R>
 ): AbilityQuery<R> | null {
-  const query: AbilityQuery<R> = {};
+  const $and: Generics<T>['conditions'][] = [];
+  const $or: Generics<T>['conditions'][] = [];
   const rules = ability.rulesFor(action, subjectType);
 
   for (let i = 0; i < rules.length; i++) {
     const rule = rules[i];
-    const op = rule.inverted ? '$and' : '$or';
+    const list = rule.inverted ? $and : $or;
 
     if (!rule.conditions) {
       if (rule.inverted) {
+        // stop if inverted rule without fields and conditions
+        // Example:
+        // can('read', 'Post', { id: 2 })
+        // cannot('read', "Post")
+        // can('read', 'Post', { id: 5 })
         break;
       } else {
-        delete query[op];
-        return query;
+        // if it allows reading all types then remove previous conditions
+        // Example:
+        // can('read', 'Post', { id: 1 })
+        // can('read', 'Post')
+        // cannot('read', 'Post', { status: 'draft' })
+        return $and.length ? { $and } : {};
       }
     } else {
-      query[op] = query[op] || [];
-      query[op]!.push(convert(rule));
+      list.push(convert(rule));
     }
   }
 
-  return query.$or ? query : null;
+  // if there are no regular conditions and the where no rule without condition
+  // then user is not allowed to perform this action on this subject type
+  if (!$or.length) return null;
+  return $and.length ? { $or, $and } : { $or };
 }
 
 function ruleToAST(rule: RuleOf<AnyAbility>): Condition {