Field-specific abilities act differently based on whether conditions are given as null or empty object.

[tmikoss]

Describe the bug Field-specific abilities act differently based on whether conditions are given as null or empty object. If conditions is an empty object, field-specific inverted rule can not override previously given general rule. With conditions as null, it can. When generating rules to use by frontend code from backend definitions (Ruby gem "cancancan" in my case), it can lead to unexpected behaviour.

To Reproduce

import { Ability } from "@casl/ability";

const nullConditionRules = [
  {
    subject: "User",
    action: "manage",
    conditions: null,
    fields: null,
    inverted: false
  },
  {
    subject: "User",
    action: "update",
    conditions: null,
    fields: ["admin"],
    inverted: true
  }
];

const canUpdateWithNullCondition = new Ability(nullConditionRules).can(
  "update",
  "User",
  "admin"
);

const emptyConditionRules = [
  {
    subject: "User",
    action: "manage",
    conditions: {},
    fields: null,
    inverted: false
  },
  {
    subject: "User",
    action: "update",
    conditions: {},
    fields: ["admin"],
    inverted: true
  }
];

const canUpdateWithEmptyCondition = new Ability(emptyConditionRules).can(
  "update",
  "User",
  "admin"
);

console.log({ canUpdateWithNullCondition, canUpdateWithEmptyCondition });

Expected behavior I expect both cases to work identically, because they represent identical rule definitions.

Interactive example (optional, but highly desirable) https://codesandbox.io/s/jovial-rhodes-kyj9v7?file=/src/index.js

CASL Version

@casl/ability - 6.2.0

Environment: Latest chrome

[stalniy]

Hmm... this is very nice edge case :)

Good catch. So, currently it works the way it works because there are particular expectations, in other words if inverted rule has conditions and we check based on subject type then this rule cannot match because subjectType has no properties.

For example:

ability = defineAbility((can, cannot) => {
  can('read', 'Post')
  cannot('read', 'Post', { private: true })
})

expect(ability).to.allow('read', 'Post')

If I change the logic then this test fails and user cannot read Post but technically there is likelihood there are posts between that are not private and can be read.

In your cause, situation like this:

ability = defineAbility((can, cannot) => {
  can('read', 'Post')
  cannot('read', 'Post', {})
})

I know there are conditions but I can't do any assumptions based on their evaluation (because it depends on conditionsMatcher) and I cannot evaluate check on subject type. Actually, in this case can but I don't know how this will behave in cases conditions are not empty (for different conditions matchers).

Let's make assumption that we have custom conditions matcher that executes function on object:

ability = defineAbility((can, cannot) => {
  can('read', 'Post')
  cannot('read', 'Post', (post) => true)
})

How can we detect that conditions always return true in this case? I think there is no good way to do it unless you see a good solution.

One possible way to solve this is to force conditions matcher to provide some additional information and tell casl whether conditions can be perceived as always returning true. But this is hard to do even on conditions matcher side. So, any value except of null and undefined cannot be treated in anyway on casl side.

You can comment out fields in rules and in check, and you will see that results are different. By coincidence, you get true for both checks if you remove field from check (without touching rules) but this is another story.

[tmikoss]

Hmmmm, my first assumption would have been that if a cannot condition exists, you automatically have to prove your subject does not match any of the cannot rules. But that would immediately break every subject-only check the minute a cannot rule comes into play, and as you said, those are useful, for example a can('read', 'Post') check before including a post display route in the router.

Another option would be to treat subject and instance checks differently, but that seems like a great source of confusion on its own, and a big breaking change to boot.

Is there a case where empty conditions object is meaningfully different from a null? Unless there's another edge case somewhere else, conditions === {} ? null : conditions in the ability constructor could just discard these empty objects. Or spit out a "you probably don't want to do this, use a null for 'no conditions'" warning, if you don't want to modify what the user passes.

In the project where I encountered this issue I solved that by replacing empty objects with nulls on the rule generation side, and so far the test suite seems to pass. But then again, that project does not use particularly advanced ability logic.

Overall, it seems to me that even a "fix your parameters" warning on encountering an empty object would go most of the way for this particular issue.

[stalniy]

warnings makes sense. I think this can be implemented on conditions matcher level. It can provide warnings and casl can forward this to some function passed through options, so you can forward that to error handling system or just to console.log.

For cases, when @ucas/core AST is used I could even check for some predefined ConditionNode to make this functionality work as you requested