Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong results testing abilities - bug correction suggested #59

Closed
jguillod opened this issue May 7, 2018 · 4 comments
Closed

Wrong results testing abilities - bug correction suggested #59

jguillod opened this issue May 7, 2018 · 4 comments

Comments

@jguillod
Copy link

jguillod commented May 7, 2018

Unexpected results in the code sample below are probably originated in line 110 of file casl/packages/casl-ability/src/ability.js which reads yet :

if (rules[i].matches(subject)) {

but when updated to :

if (rules[i].matches(field)) {

gives correct results with the following code example :

var casl = require('@casl/ability')
var ability = casl.AbilityBuilder.define(function(can, cannot) {
  can('read', 'all')
  can('manage', 'Post', { author: 'joe' })
  cannot('delete', 'Post', { 'comments.0': { $exists: true } })
})

console.log(ability.can('manage', 'Post')); // => false
console.log(ability.can('manage', 'Post', { author: 'al'})); // => false
console.log(ability.can('manage', 'Post', { author: 'joe'})); // => true

Without the suggested correction all results are unexpectedly true.

Hope this helps !
Joel

@stalniy
Copy link
Owner

stalniy commented May 8, 2018

Hi Joel,

Thanks for the issue! I see that you misunderstood how to use Ability instance.

There are 2 different can functions. 1 function which relates to AbilityBuilder DSL and creates a RawRule object. Another one relates to Ability instance and checks rules (i.e., permissions). If you look into Typescript definitions you will see they have different signature

So, in your example you cant pass object as a 3rd argument into Ability#can method. To correct the example you need to do this:

class Post {
  constructor(attrs) {
     Object.assign(this, attrs)
  }
}

console.log(ability.can('manage', 'Post')); // => true, correct because potentially you can manage at least 1 post whose `{ author: 'joe' }`
console.log(ability.can('manage', new Post({ author: 'al'}))); // => false, correct because you can manage only `{ author: 'joe' }`
console.log(ability.can('manage', new Post({ author: 'joe' }))); // => true, correct because you can manage only `{ author: 'joe' }`

Please read checking abilities page in documentation to understand the details :)

I'm going to close the issue as there is nothing to fix.
Hope this makes sense.

@wmwart
Copy link

wmwart commented Mar 19, 2019

Good idea, libraries!

Please explain why I need to create multiple instances of this class to check in the same Post context?

What's funny?

To work in a browser (especially with React) is a bad idea, don't you think?

@stalniy
Copy link
Owner

stalniy commented Mar 19, 2019

Hello @wmwart ,

The point is not in that you need to create a separate instance for every object in your app.

The point is that you need to understand the object type from object instance (otherwise you don't know with what you are working, what fields it has and so on). The easiest way to illustrate is to define class and its instance (what I did, and the same stuff is shown in docs). By default CASL, checks instance constructor.modelName and constructor.name to detect its type but this can be whatever you want actually. That's why it's possible to define custom subjectName function, which returns object type name.

This is explained in docs, in the section about Instance checks

Why I don't like suggeted API:

  1. I like when the code is clean and readable (obviously it's subjective). So, something like this
    ability.can('read', 'Post') // can read at least one object of type Post
    ability.can('read', post) // can read a particular instance of type Post
    ability.can('read', post, 'title') // can rea post's title field
    has more sense than
    ability.can('read', 'Post', object) // object instance has no metadata/knowledge about who he is
    ability.can('read', 'Post', object, 'title') // too much arguments
  2. I like when method has few arguments (easier to remember and know how to put stuff in correct order and code looks simpler)
  3. The library was initially created for backend app. On backend people use ORMs (in majority of cases), thus each object from database is mapped to some class instance.

What I do (suggest) on frontend:
if you use GraphQL, all your instance have __typename property which you can use to detect object's type
if you use REST API, then you have few options:

  • map all objects from response of REST API call into some class instances (if that make sense)
  • add custom property (like __typename in GraphQL), which allows to detect object type and provide
    custom subjectName implementation (by the way, it can be symbol property)
  • create helper function
    function a(type, instance) {
      instance[Symbol.for('instanceType')] = type
      return instance
    }
    
    function subjectName(subject) {
       if (!subject || typeof subject === 'string') {
          return subject
       }
    
       return subject[Symbol.for('instanceType')]
    }
    
    const ability = new Ability([], { subjectName })
    
    ability.can('read', a('Post', object))

If you don't like this, then you always can extend Ability class and provide whatever API you like in your app or encupsulate that logic in some component/function

@wmwart
Copy link

wmwart commented Mar 20, 2019

Thanks for the detailed answer! Your logic has become clearer.

Yes, indeed, I use 3 services: backend on GraphQL-yoga, frontend React, and CASL authorization service.

The same rules should be used in all services that require authorization.

I'm used to seeing what's on the box, what's in the box. Therefore, the syntax:

can ('manage', 'Post', { author: 'joe' }) 
ability.can ('manage',' Post', {author:' al'}); / / = > false

for me it is more evident.

In addition, in GraphQL in type Post there is no data about the value of the fields, so there is no sense in

new Post({ author: 'al'}).

More logical:

ability.can ('manage', Post, { author:'al'}).

In this case, you can really check the availability of the required fields and so on.

As you may have noticed GraphQL is gaining popularity, but CASL examples are not added. Do you have any information or usage examples?

Please help with the idea. I need to create a rule for example:
can ('list',' Order', {locations: $locatons}) // user can view orders when locations is user.locations.
To next:

const user = {
    locations: ['A', 'B']
}

caslToGraphql () => (
    query {
        listOrder(
            where: { locarions_in: ['A','B'] }
         ){...}
    }
)

how can this be implemented? Maybe there are already examples?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants