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.87k stars 265 forks source link

Typescript `.throwUnlessCan()`, won't accept fields array as third parameter #679

Open RemyMachado opened 2 years ago

RemyMachado commented 2 years ago

EDIT: I realised .can() of PureAbility (same as throwUnlessCan()) & .can() of AbilityBuilder don't have the same signature. This is confusing. Now I'm wondering: If I want to check several fields access I need to do them one by one?


The problem:

.throwUnlessCan(), won't accept field array as third parameter. It throws the following typescript error: TS2345: Argument of type 'string[]' is not assignable to parameter of type 'string'.

To reproduce:

import UserModel from 'myLib'
import {
    Ability,
    AbilityBuilder,
    ForbiddenError as CaslForbiddenError,
    InferSubjects,
} from '@casl/ability'

const user = {
  id: '0',
  email: 'email@email.email',
  roles: []
  // ...
}

type Actions = 'update' | 'manage'
type Subjects = InferSubjects<typeof UserModel | 'all'>
type AppAbility = Ability<[Actions, Subjects]>

const { can: allow, cannot: forbid, build } = new AbilityBuilder<AppAbility>(Ability)

if (user.roles.includes('ADMIN')) {
    allow('manage', 'all')
} else {
    allow('update', UserModel, { id: user.id })
    forbid('update', UserModel, ['email', 'roles'])
}

const ability = build()

CaslForbiddenError.from(ability).throwUnlessCan('update', user, ['field1', 'field2']) // TS error

TS2345: Argument of type 'string[]' is not assignable to parameter of type 'string'.

Expected behavior The third parameter ~should~ could accept string[] along string ~since the documentation states~:

~ThrowUnlessCan Accepts the same parameters as PureAbility's can method. Throws a ForbiddenError if user cannot do the provided action on provided subject. Here I realised there are two different signatures.~

I'm doing it wrong. Is this a possible improvements or is there a way to check several fields at once?

CASL Version "@casl/ability": "6.1.1"

Environment: Node v16.14.2 TS 4.6.4

RemyMachado commented 2 years ago

After understanding my mistake, I checked again the issues with a different approach and found this closed issue: https://github.com/stalniy/casl/issues/558

I think there is too much ambiguity between the two functions and using the same signatures (allowing for arrays) could be a great improvement.

Also, I spent so many hours looking for a good authorization library. Despite the fact that I reported this issue, I'm thankful I found this library. It's great, and handle most of what I could hope for.. Thank you!

stalniy commented 2 years ago

So, what are the expectations? Should it check that all fields can be accessed?

RemyMachado commented 2 years ago

So, what are the expectations? Should it check that all fields can be accessed?

Yes, I think it would be great. It would make the API less confusing. In my mind, since I could allow() several fields at a time, I was expecting the validation process to work the same way. Furthermore, despite having the same first two parameters signature, they almost share the same name (.can() & .throwUnlessCan()). I took it as a hint for the hidden implementation (composition of ErrorProcess + .can() method). It was so obvious for me that it would work the same way, that I first thought of a Typing error in the library. However, they just aren't working the same way.

We can use a loop, it's no big deal, it's less elegant, but it's more of a clarity problem than an elegance one. It took me some time to realise my mistake. I would say it's a good possible design improvement.