stalniy / casl

CASL is an isomorphic authorization JavaScript library which restricts what resources a given user is allowed to access
https://casl.js.org/
MIT License
5.93k stars 269 forks source link

`modelName` not used when using class to check ability #464

Closed jfahrenkrug closed 3 years ago

jfahrenkrug commented 3 years ago

Describe the bug When defining a rule and later checking the ability using a class as the subject, the check returns false although modelName is set on the class.

Example:

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

class Thing {
  static readonly modelName = "Thing";
}

const ability = new PureAbility([
  {
    action: "read",
    subject: "Thing"
  }
]);

const thingInstance = new Thing();

const output = [
  `can read Thing (class): ${ability.can("read", Thing)}`,
  `can read Thing (instance): ${ability.can("read", thingInstance)}`,
  `can read Thing (string): ${ability.can("read", "Thing")}`
];

document.body.innerHTML = output.join("<br/>");

The output is:

can read UserEntity (class): false
can read UserEntity (instance): true
can read User (string): true

To Reproduce The above sample code demonstrates it, but to make it very easy, here's a code sandbox: https://codesandbox.io/s/typescript-examples-forked-5up2p?file=/src/index.ts

Expected behavior I'd expect the output to be:

can read UserEntity (class): true
can read UserEntity (instance): true
can read User (string): true

Interactive example (optional, but highly desirable) https://codesandbox.io/s/typescript-examples-forked-5up2p?file=/src/index.ts

CASL Version

@casl/ability - v5.2.2

Environment: Node v14.15.4 TS 3.9.5

stalniy commented 3 years ago

Works as expected. This is one of the breaking changes in v5:

Also it's important to note that it's no longer possible to use classes and strings as subject types interchangeably as it was before. Now, if you want to use classes, you should use them everywhere and define custom detectSubjecType:

So, if you want to use JSON serialized rules, you need to remap subjects to classes. Otherwise it will not work.

Also check the section about using classes: https://casl.js.org/v5/en/guide/subject-type-detection#use-classes-as-subject-types

jfahrenkrug commented 3 years ago

@stalniy Thank you for the quick reply.

Is there documentation I can look up to find a solution for how to remap subjects to classes?

This is not a critique, just an observation: This new behavior in v5 is not very intuitive. Was there a technical reason why the modelName would not be used anymore in v5 when using classes?

stalniy commented 3 years ago

It's simpler than you think:

class Thing {
}

const ALL_SUBJECTS = { Thing };

function remap(rules) {
  return rules.map(rule => ({ ...rule, subject: ALL_SUBJECTS[rule.subject] })); // + failover
}

const ability = new PureAbility(remap([
  {
    action: "read",
    subject: "Thing"
  }
]));
jfahrenkrug commented 3 years ago

@stalniy Thanks so much for your quick reply. When I implement the remapping, I now get false when checking the ability of an instance. The class works, and the string doesn't (which is expected). See https://codesandbox.io/s/typescript-examples-forked-ygu7g?file=/src/index.ts

With the remapping in place, I now get these results:

User:
can read UserEntity (class): true
can read UserEntity (instance): false
can read User (string): false

Thing:
can read Thing (class): true
can read Thing (instance): false
can read Thing (string): false

Is that expected? I would expect can read UserEntity (instance) and can read Thing (instance) to both return true.

stalniy commented 3 years ago

Check this link - https://casl.js.org/v5/en/guide/subject-type-detection#use-classes-as-subject-types

at the bottom of the page pay attention at custom detectSubjectType. Classes are not supported by default.

This greatly simplifies logic and allows v8 to do its optimizations. I plan to publish an article sooooooooon about pros of this

jfahrenkrug commented 3 years ago

Thank you, but that doesn't seem to work for me either :( I've created a new issue here: https://github.com/stalniy/casl/issues/465 Thank you very much for your help.