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

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

Open
RemyMachado opened this issue Sep 8, 2022 · 3 comments

Comments

@RemyMachado
Copy link

RemyMachado commented Sep 8, 2022

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 RemyMachado added the bug label Sep 8, 2022
@RemyMachado
Copy link
Author

RemyMachado commented Sep 8, 2022

After understanding my mistake, I checked again the issues with a different approach and found this closed issue:
#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
Copy link
Owner

stalniy commented Sep 8, 2022

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

@RemyMachado
Copy link
Author

RemyMachado commented Sep 9, 2022

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.

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

2 participants