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
6.05k stars 270 forks source link

Unexpected behavior when using conditions #456

Closed danmana closed 3 years ago

danmana commented 3 years ago

Hi all, I'm just starting with using casl and nestjs, so maybe I am missing something...

Describe the bug I'm trying to define the following scenario (simplified):

The problem is that if I create a rule with conditions for Article, the check for "can read any Article" starts returning true, even if it shouldn't.
Is this the expected behavior? How can I implement correctly the scenario above?

I tracked this to this line, where if the subject is a subjectType it returns true ... why is that? https://github.com/stalniy/casl/blob/188218881231e17fc98b2465555ca92966035e91/packages/casl-ability/src/Rule.ts#L82

Note: detectSubjectType is implemented and works, so it's not that. The issue is the same if I use string types.

To Reproduce Steps to reproduce the behavior:

  1. Ability configuration (rules, detectSubjectType, etc)
    // user rule
    can('read', Article, { authorId: userId });
  2. How do you check abilities
    
    // this works as expected
    ability.can('read', myArticle); // expect true, returns true
    ability.can('read', otherArticle); // expect false, returns false

// why is this true? I read this as "user can read ANY Article" ability.can('read', Article); // expect false, returns true !!!



**Expected behavior**
The user should not have permission to read "any" Article.

**Interactive example** (optional, but highly desirable)
https://codesandbox.io/s/empty-architecture-412g8?file=/src/index.js

**CASL Version**

<!-- leave packages which you use -->

`@casl/ability` - v 5.2.2

**Environment**:
<!-- Nodejs version or Browser version or TypeScript version -->
danmana commented 3 years ago

Even if I try to disallow explicitly the "read any Article" for a user it does not work

cannot("read", Article);
can("read", Article, { authorId: userId });

ability.can('read', Article); // expect false, still returns true !!!
stalniy commented 3 years ago

Sorry for saying that but the issue is in mental model you use to think about permissions. The mental model which CASL uses for this is explained at the bottom of Checking logic section in Intro guide.

Regarding, your last comment. This is also explained in Intro guide.

When defining direct and inverted rules for the same pair of action and subject the order of rules matters: cannot declarations should follow after can, otherwise they will be overridden by can

danmana commented 3 years ago

@stalniy thanks for pointing out that piece of the docs

It seems I was making a wrong assumption, it's not "can I read ANY article", but "can I read SOME article".
Now it makes sense why it works the way it does.

After reading again the docs, I think I found what I was looking for

ability.can('read', new Article()); // user can read ANY article
ability.can('read', Article); // user can read SOME article
stalniy commented 3 years ago

Correct.

But to reflect the actual behavior I'd recommend to use at least one instead of SOME. So, when you check on subject type, you ask "can I read at least one article of that type?" and when you check on a particular instance you ask "can I read this article/object?".