Wonderful library, but with major issue.

[bin-cc-aryd-cc-igits]

Describe the bug Creating "can" ability with FIELD, must not grant "can" ability for the ENTIRE SUBJECT!!!

Consider standard use case in any midsize corporation: For parent corporation administrator, create ability to "manage" any filed of the corporation including any child office information in the 'offices' array. However, for the Office Hiring Manager, we want to create ability to ONLY ADD employees to his/her office and NOTHING ELSE. When we check permissions for above use case, we first check if user has permission on the parent object such as corporation, and if yes, they can modify anything including child office(s). If user does not have permission on parent object, then we check user's permission on "child" object property such as "employees" array of the office that belongs to corporation.

To Reproduce Steps to reproduce the behavior:

1. Create SINGLE rule for 'Office Hiring Manager' who can ONLY ADD EMPLOYEES to his/her office using 'employees' array: can('add', 'Office', ['employees']);
2. Checking above rule returns true - which is a BUG, and it should be false, because we never granted add permission to the whole office subject: ability.can('add', subject('Office', office)); // returns true; <== BUG. ability.can('add', subject('Office', office), 'employees'); //returns true as expected ability.can('add', subject('Office', office), 'managers'); //returns false as expected

Expected behavior Below ability check must return false: ability.can(action, subject('Office', office)); // must return false As per general security principles, as well as the information on official CASL JS website, we should have "everything restricted" and "grant access" to bare minimum resources/features/etc.

Interactive example (optional, but highly desirable) provide a link to the example from http://repl.it/, https://codesandbox.io/ or similar, so we can quickly test and provide feedback. Otherwise

CASL Version

@casl/ability - v 6.7.1

Environment:

Nodejs v18.16.1 TypeScript 4.5.4

[bin-cc-aryd-cc-igits]

After reading this: https://github.com/stalniy/casl/issues/236 I start to have a feeling that it was by design for some reason... unless I am missing something very obvious. Fingers crossed :)

[stalniy]

Yes, it’s by design.

It’s explained in documentation) but people are lazy :)). It has logic underneath. Check this:

• https://casl.js.org/v6/en/guide/restricting-fields
• https://casl.js.org/v6/en/guide/intro#checking-logic

I’m not going to change this.

[bin-cc-aryd-cc-igits]

@stalniy : I read all docs multiple times prior using CASL, and the case above is not mentioned in documentation. I have not looked at the source of CASL yet, but do you think it might be doable to supply some sort of configuration object/flag before I define abilities which will result in the following:

when ability is created with filed(s), ex: can('add', 'Office', ['employees']);

the check for the 'add' action on Office will result in 'false', ex: ability.can('add', subject('Office', office)); // will return false, instead of true

Note: I understand that the plugin is very good and popular, and such change will likely break functionality of quite a few users to say the leas... therefore I am trying to see if there is an option to modify the library with config object. It is the only thing that is awkward by design, all other features are simply beautiful from design perspective including extensions for other operators, etc.

[stalniy]

It’s just a matter of how you think about your task.

Let me reiterate why it’s done this way. Casl allows to ask 2 different types of questions:

1. Can I do something on at least one entity of this type
2. Can I do something on this particular entity

In code this means:

can(“create”, “BlogPost”, { id:1})

ability.can(“create”, “BlogPost”) // true, because in general I can create Blogposts. Their structure is restricted but I can! So here I ask a question: can I create blogposts in general?
ability.can(“create”, new BlogPost({ id: 1 })) // yes, I can create this particular blogpost
 ability.can(“create”, new BlogPost({ id:2}) // no, this blogpost you can’t create!

and the same about fields:

1. Can I do smth on at least 1 field on at least one entity of this type
2. Can I do smth on this particular field of this particular entity

can(“update”, “BlogPost”, [“comments”])

ability.can(“update”, “BlogPost”) // true, because in general I can update Blogposts. Only 1 field but I can! This question is NOT “can I update all fields of blogpost” but instead “can I update at least 1 field of at least 1 blogpost in the system?”
ability.can(“update”, “BlohlgPost”, “comments”) // yes, I can update this particular field of at least 1 blogpost

ability.can(“update”, “BlohlgPost”, “title”) // no, I canNOT update this particular field of at least 1 blogpost

[stalniy]

there are several ways to achieve what you want in current design:

1. When you want to check whether all fields can be updated - iterate over them and check them all one by one
2. Introduce virtual field * which means all fields. If there is star field then all fields can be updated. can(“update”, “BlogPost”, [“*”]) => ability.can(update, BlogPost, “*”) vs ability.can(update, BlogPost, “comments”)
3. Get rid of field based permissions and do it on entity level. Every field can be perceived as nested entity. So ability.can(update, BlogPost, “comments”) will be converted to ability.can(create, BlogPostComment)

[bin-cc-aryd-cc-igits]

@stalniy thank you for your time to reply. your comment next to below line of code hit the "home run in my brain": ability.can(“update”, “BlogPost”) // true,... but instead “can I update at least 1 field of at least 1 blogpost in the system?” You should probably add the bold comment somewhere in the docs, I am sure someone will have same question...

I considered suggestions 1 and 3 few days ago and dropped them: 1 is too messy in terms of additional code, and 3 is a bit unintuitive when you think of tree structures/subdocuments/etc.

Suggestion number 2 is well documented and usage of wild cards is superb for ability definitions. On the documentation page () I have not seen check for wildcard at the root level, and was stupid enough not to check for all fields using '*'... will experiment with it tomorrow.

LOL, I already wiped out CASL from my project and did my own security with roles/permissions/objects... however, your suggestion number 2 gives me hope, so tomorrow I will roll-back the branch and will try to update definitions with wild card for all fields and check for all fields. I really hope it's going to work - your library is VERY well designed... except this one bloody use case that I described initially.