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.93k stars 269 forks source link

Inference of subjects in documentation not working? #473

Closed anlauren closed 3 years ago

anlauren commented 3 years ago

Describe the bug When i try to run the following example in the docs:

import { Ability } from '@casl/ability';

interface Article {
  id: number;
  title: string;
  content: string;
  authorId: number;
}

interface User {
  id: number;
  name: string;
}

interface Comment {
  id: number;
  content: string;
  authorId: number;
}

type Action = 'create' | 'read' | 'update' | 'delete';
type Subject = Article | Comment | User | 'Article' | 'User' | 'Comment';

const ability = new Ability<[Action, Subject]>();

It is raising a typescript error explaining that Type 'Article' is not assignable to type 'Subject' or Type 'Article' is not assignable to type 'AnyRecord' or Index signature is missing in type 'Article'.

I've tried to google around without luck 🤔 I'm guessing it has something to deal with how strict are the class rules but i'm using a fairly basic ts config

To Reproduce Steps to reproduce the behavior:

  1. just copy paste the above and try to build it

Expected behavior It should build

CASL Version

"@casl/ability@^5.2.2"

Environment: node: v10.22.0 "typescript": "^4.0.5"

tsconfig:

{
  "compilerOptions": {
    /* Basic Options */
    "target": "es2018",
    "module": "commonjs",
    "lib": [
      "esnext.asynciterable"
    ],
    "declaration": true /* Generates corresponding '.d.ts' file. */,
    "sourceMap": true /* Generates corresponding '.map' file. */,
    "outDir": "./build" /* Redirect output structure to the directory. */,
<img width="1031" alt="Screenshot 2021-03-04 at 18 37 04" src="https://user-images.githubusercontent.com/7627816/110016701-403a0580-7d1d-11eb-95c3-c64bfbcf1be3.png">

    "removeComments": false /* Do not emit comments to output. */,

    "importHelpers": true /* Import emit helpers from 'tslib'. */,
    "strict": false /* Enable all strict type-checking options. */,
    "noImplicitAny": true /* Raise error on expressions and declarations with an implied 'any' type. */,
    "strictNullChecks": true /* Enable strict null checks. */,
    "strictFunctionTypes": true /* Enable strict checking of function types. */,
    "strictPropertyInitialization": false /* Enable strict checking of property initialization in classes. */,
    "noImplicitThis": true /* Raise error on 'this' expressions with an implied 'any' type. */,
    "alwaysStrict": true /* Parse in strict mode and emit "use strict" for each source file. */,

    /* Additional Checks */
    "noUnusedLocals": false /* Report errors on unused locals. */,
    "noUnusedParameters": false /* Report errors on unused parameters. */,
    "noImplicitReturns": true /* Report error when not all code paths in function return a value. */,
    "noFallthroughCasesInSwitch": true /* Report errors for fallthrough cases in switch statement. */,

    /* Module Resolution Options */
    "moduleResolution": "node" /* Specify module resolution strategy: 'node' (Node.js) or 'classic' (TypeScript pre-1.6). */,

    "allowSyntheticDefaultImports": true /* Allow default imports from modules with no default export. This does not affect code emit, just typechecking. */,
    "esModuleInterop": true /* Enables emit interoperability between CommonJS and ES Modules via creation of namespace objects for all imports. Implies 'allowSyntheticDefaultImports'. */,

    "forceConsistentCasingInFileNames": true,
    "skipLibCheck": true /* Don't check libs typings - due to https://github.com/prisma/graphql-request/issues/26 */,

    /* Experimental Options */
    "experimentalDecorators": true /* Enables experimental support for ES7 decorators. */,
    "emitDecoratorMetadata": true /* Enab*/
  }
}
Screenshot 2021-03-04 at 18 37 04
stalniy commented 3 years ago

Hey, very strange. I’ve noticed there is no kind property on your interfaces.

Except of that the rest should be fine. Please specify ts version

update: ah I see. Will check it tomorrow

anlauren commented 3 years ago

these are my different packages installed:

    "ts-jest": "^26.4.3",
    "ts-loader": "^8.0.8",
    "ts-node": "^9.0.0",
    "tsconfig-paths": "^3.9.0",
    "typescript": "^4.0.5"

Adding the "kind" doesn't change anything, i've tried with every combination i could but i went back to the example in the docs to report the issue and be sure i was not doing something stupid

Screenshot 2021-03-04 at 20 49 13
anlauren commented 3 years ago

Okay i'm reading the implementation and I don't understand how it's supposed to work 🤔

export declare type Subject = AnyRecord | SubjectType;

-> Either it's a record or a SubjectType. A record can accept any key as value accessor, so something of type Article cannot be of type AnyRecord

So it has to be of type SubjectType.

export declare type SubjectType = string | SubjectClass;

-> well it's not a string so let's move on

export declare type SubjectClass<N extends string = string> = AnyClass & {
    modelName?: N;
};

-> so first it has to be AnyClass

export declare type AnyClass<ReturnType = any> = new (...args: any[]) => ReturnType;

That is a classic definition of class typechecking, which would work for typeof Article, but is not true for Article since it's an instance.

Hence, i have no idea how it's supposed to work 🤔 I don't know the library enough to understand it all though. I don't see the point of & {modelName?: N;} we're adding an optional constraint?

anlauren commented 3 years ago

the only way i found to make that work is quite hacky, it's to turn either the class or the interface into a record. But that allows for unsafe practices (adding random things to the class/object is permitted and reading them as well)

import { Ability } from '@casl/ability';

interface IArticle {
  [k: string]: any;
  id: string;
}
class Article implements IArticle {
  [k: string]: any;
  constructor(public id: string) {}
}

type Action = 'create' | 'read' | 'update' | 'delete';
type Subject = Article;

const ability = new Ability<[Action, Subject]>();

const a: IArticle = {
  hello: 'nope',
  id: 'fd',
};

const article = new Article('test');
article.nope = 'works i know';
console.log('That is really unsafe', article.something.else);
ability.can('create', article);
stalniy commented 3 years ago

There are several examples in casl-examples repo that do the same or similar. For example for react: https://github.com/stalniy/casl-examples/blob/master/packages/react-todo/src/config/ability.ts#L8

stalniy commented 3 years ago

Please check them until I have time to test it by myself

stalniy commented 3 years ago

I've just create a sample repo: https://github.com/stalniy/casl-ts-inference

And cannot reproduce your issue

update: use npm run build or npm start in that repo

anlauren commented 3 years ago

I am baffled. I cloned the repo and it does work when I run but my VSCode keeps complaining about it so i guess it's on my VS code config somewhere somehow, never seen that happening before.

There was build errors as well in my project but it might have come from my experiments to try to make it work.

I'm still a bit confused about why it works and build, reading the code i mentioned above, but I guess it's on me to get better at typescript ;p

(My Vscode error in that repo if somebody sees that in the future:)

Screenshot 2021-03-05 at 11 42 35
stalniy commented 3 years ago

I also use vscode. Check which ts version vscode uses

stalniy commented 3 years ago

There are some exceptions when interface is assignable to record. I don’t know all of them as well but looks like ts uses one of such exceptions in case of casl

stalniy commented 3 years ago

OK. I close this. Feel free to comment if you find more information about this.

jamesmeneghello commented 3 years ago

This happened to me also, with the root cause being that VS Code was opening using node v12 and my CLI was using node v15.

e; the implication probably being that the wrong version of typescript was installed or similar.

stalniy commented 3 years ago

yes, node version cannot affect this only ts version. You need TS ^3.8.3 as far as I remember