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

Missing type-checking in classes with protected constructor #995

Open
yodosan opened this issue Dec 3, 2024 · 2 comments
Open

Missing type-checking in classes with protected constructor #995

yodosan opened this issue Dec 3, 2024 · 2 comments
Labels

Comments

@yodosan
Copy link

yodosan commented Dec 3, 2024

Describe the bug
Hello, I'm trying to set up CASL with NestJS and get full TypeScript support defining each ability (actions, subjects, fields, conditions) but something is missing.
My classes have the constructor protected, which CASL doesn't seem to like very much; however, I think I found an acceptable trade-off (if somewhat verbose).

This is what I've done so far:

class UserEntity {
  protected constructor(
    readonly id: string,
    readonly name: string,
    readonly createdBy: Date
  ) {}

  static create(id: string, name: string) {
    return new UserEntity(id, name, new Date())
  }
}

type Actions = 'create' | 'read' | 'update' | 'delete'

type User = UserEntity & TaggedInterface<'UserEntity'>
type Subjects = InferSubjects<User> | 'all'
type AppAbility = PureAbility<[Actions, Subjects]>

@Injectable()
export class CASLAbilityFactory {
  createForUser() {
    const { can, build } = new AbilityBuilder<AppAbility>(PureAbility)

    can<User>('create', 'UserEntity')

    return build()
  }
}

Now, the action, the subject, and the fields are correctly checked, but for some reason, it can be done for the properties; as we can see there is no type-checking:

Screenshot 2024-12-03 alle 17 29 16

Is CASL doing something wrong or is it me?

CASL Version

@casl/ability: 6.7.2

Environment:
TypeScript: 5.6.3
NodeJS: 22

@yodosan yodosan added the bug label Dec 3, 2024
@serhiistotskyi
Copy link
Contributor

This is actually how types work in TS. You can read this answer on SO -> https://stackoverflow.com/questions/66599065/typescript-abstract-static-factory-with-protected-constructor#answer-70118566

The issue is that there is no other way to check that Function is actually a class -> https://www.typescriptlang.org/play/?#code/MYGwhgzhAEAqCmEAu0DeBYAUNaAHATgPZLzAkAm0whAdsvgK5mH4AU+8Y5tIAntAEtyALmj0BNAOYBKNAF8sCzFiS9c8OGo0BeaDXgB3aKwB0ZsPkkRRYGrwDaAXVnaAfNFv8VWzeoAi8ABm0Lqm5pbWHnZOLu6e0ABkaHhESMRaNnbQSt7qvvAAkhAAcsQAggDC4FAh0KrqhMEIyNDwAB4kNOQwsD4A-HWMGqKBYCAQ8LkaAEIMSAVIABKQAAqp6eoA8jQLtfXwjXCIKO2d3ckExBvw

I obviously can check that object has prototype on it but then any object can actually be passed, even not a function. I also can check.

So, this is not something what I plan to address immediately. You can just remove protected from constructor and mark it with JSDoc as @deprecated to make it crossed in IDE

Screenshot 2024-12-03 at 19 23 14

@yodosan
Copy link
Author

yodosan commented Dec 10, 2024

Hi @serhiistotskyi, thanks for your reply.
I will not remove the protected attribute on constructors, this is an architectural choice made on the entire code base.

The issue is that there is no other way to check that Function is actually a class -> https://www.typescriptlang.org/play/?#code/MYGwhgzhAEAqCmEAu0DeBYAUNaAHATgPZLzAkAm0whAdsvgK5mH4AU+8Y5tIAntAEtyALmj0BNAOYBKNAF8sCzFiS9c8OGo0BeaDXgB3aKwB0ZsPkkRRYGrwDaAXVnaAfNFv8VWzeoAi8ABm0Lqm5pbWHnZOLu6e0ABkaHhESMRaNnbQSt7qvvAAkhAAcsQAggDC4FAh0KrqhMEIyNDwAB4kNOQwsD4A-HWMGqKBYCAQ8LkaAEIMSAVIABKQAAqp6eoA8jQLtfXwjXCIKO2d3ckExBvw

Unfortunately, the editor linked is empty.

However, I found a solution that can be handy if someone else has the same need.

Something like that:

class UserEntity {
  protected constructor(
    readonly id: string,
    readonly name: string,
    readonly createdBy: Date
  ) {}

  static create(id: string, name: string) {
    return new UserEntity(id, name, new Date())
  }
}

type Action = 'create' | 'read' | 'update' | 'delete'
type User = UserEntity & TaggedInterface<'UserEntity'>
type Subject = InferSubjects<User> | 'all'

@Injectable()
export class CASLAbilityFactory {
  createForUser() {
    const { can, build } = new AbilityBuilder<PureAbility<[Action, Subject], MatchConditions>>(
      PureAbility<[Action, Subject], MatchConditions>
    )

    can('create', 'UserEntity', ['name'], ({ id }) => id === '1')

    return build({
      conditionsMatcher: (matchConditions: MatchConditions) => matchConditions,
      fieldMatcher: fields => field => fields.includes(field),
    })
  }
}

In this way, it's possible to define conditions with the help of TypeScript:

image

What's your opinion? Do you see any drawbacks?

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

No branches or pull requests

2 participants