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.96k stars 270 forks source link

Type safety of fields in conditions #333

Closed dsebastien closed 4 years ago

dsebastien commented 4 years ago

Hello,

I've just started using casl, so I might be doing (multiple) things wrong, but it seems that "can*" rules are not type safe.

When I define rules such as the one below, the fields I'm using to express the MongoDB query are using any, meaning that if I refactor my code, I might break the rules.

I've of course tested this code to alleviate this issue, but it would be awesome if casl could recognize the types and enforce type safety.

Example:

import { Ability, defineAbility, InferSubjects } from "@casl/ability";
import { UserRoleInMeetingV1 } from "../user-role-in-meeting-v1.intf";
import { MeetingStateV1 } from "../../../entities/meetings/meeting-state-v1.intf";
import { assertUnreachable } from "@didowi/common-utils";

export enum MeetingBaseActions {
  VIEW = "VIEW",
  EDIT_BASIC_INFO = "EDIT_BASIC_INFO",
  EDIT_STATE = "EDIT_STATE",
  REMOVE = "REMOVE",
}

export class MeetingBaseSubject {
  /**
   * Helps CASL to detect the subject type
   * Reference: https://stalniy.github.io/casl/v4/en/guide/subject-type-detection
   */
  static readonly modelName = "MeetingBaseSubject";

  constructor(readonly roles: UserRoleInMeetingV1[]) {}
}

/**
 * Subjects can either be the type (when defining a rule) or an object of that type (when checking)
 */
export type MeetingBaseSubjects = InferSubjects<typeof MeetingBaseSubject>;

/**
 * The ability is the combination of possible actions and subjects
 */
export type MeetingBaseAbility = Ability<[MeetingBaseActions, MeetingBaseSubjects]>;

/**
 * Define the abilities for base
 * @param meetingState the meeting state to define abilities for
 */
export function defineAbilitiesForMeetingBase(meetingState: Readonly<MeetingStateV1>): MeetingBaseAbility {
  return defineAbility((can) => {
    switch (meetingState) {
      case MeetingStateV1.DRAFT:
        can(MeetingBaseActions.VIEW, MeetingBaseSubject, { roles: { $in: [UserRoleInMeetingV1.OWNER] } });
        can(MeetingBaseActions.EDIT_BASIC_INFO, MeetingBaseSubject, { roles: { $in: [UserRoleInMeetingV1.OWNER] } });
        can(MeetingBaseActions.EDIT_STATE, MeetingBaseSubject, { roles: { $in: [UserRoleInMeetingV1.OWNER] } });
        can(MeetingBaseActions.REMOVE, MeetingBaseSubject, { roles: { $in: [UserRoleInMeetingV1.OWNER] } });
        break;
      case MeetingStateV1.DELETED:
        // Noone can do anything
        break;
      case MeetingStateV1.CANCELLED:
        can(MeetingBaseActions.VIEW, MeetingBaseSubject, {
          roles: {
            $in: [
              UserRoleInMeetingV1.OWNER,
              UserRoleInMeetingV1.CATEGORY_MEMBER,
              UserRoleInMeetingV1.PARTICIPANT,
              UserRoleInMeetingV1.TIMEKEEPER,
              UserRoleInMeetingV1.SCRIBE,
            ],
          },
        });
        break;
      case MeetingStateV1.PLANNED:
        can(MeetingBaseActions.VIEW, MeetingBaseSubject, {
          roles: {
            $in: [
              UserRoleInMeetingV1.OWNER,
              UserRoleInMeetingV1.CATEGORY_MEMBER,
              UserRoleInMeetingV1.PARTICIPANT,
              UserRoleInMeetingV1.TIMEKEEPER,
              UserRoleInMeetingV1.SCRIBE,
            ],
          },
        });
        can(MeetingBaseActions.EDIT_BASIC_INFO, MeetingBaseSubject, {
          roles: { $in: [UserRoleInMeetingV1.OWNER, UserRoleInMeetingV1.SCRIBE] },
        });
        can(MeetingBaseActions.EDIT_STATE, MeetingBaseSubject, {
          roles: { $in: [UserRoleInMeetingV1.OWNER, UserRoleInMeetingV1.SCRIBE, UserRoleInMeetingV1.TIMEKEEPER] },
        });
        break;
      default:
        assertUnreachable(meetingState);
    }
  });
}

By the way, I wanted to use the abilityBuilder to define those rules, but for some reason that eludes me, it never worked and I'm not too sure why. It was as if the subjects were not recoginized although I had the same base setup. I'm not sure I understand the docs about it..

stalniy commented 4 years ago

Hello,

Both AbilityBuilder and defineAbility are generic types. So, you need to use:

return defineAbility<MeetingBaseAbility>((can) => {})

// or

const { can, build } = new AbilityBuilder<MeetingBaseAbility>()

can(...)

return build()

If you don't pass generic type, they use a default one. You can find more about this in API docs

P.S.: if you are doing Role based access control then please check the cookbook article. Also it looks like you are trying to use CASL wrong by defining special subject class. You need to identify your subjects first, usually these business entities, if you have mongo collection meetings then very likely you have Meeting model and this model is your subject. You need to define permissions based on the subjects' attributes.

dsebastien commented 4 years ago

Thanks for the feedback. Indeed I did not set the generic type, but even now that I have, the MongoDB query remains weakly typed:

export function defineAbilitiesForMeetingBase(meetingState: Readonly<MeetingStateV1>): MeetingBaseAbility {
  return defineAbility<MeetingBaseAbility>(can => {
    switch (meetingState) {
      case MeetingStateV1.DRAFT:
        can(MeetingBaseActions.VIEW, MeetingBaseSubject, { roles: { $in: [UserRoleInMeetingV1.OWNER] } });

Above, the roles property which is defined in MeetingBaseSubject could be mistyped. Is there a way to allow this to be strongly typed?

I have tried again also the AbilityBuilder, but these two definitions are not equivalent:

This one works fine (but is not strongly typed enough):

export function defineAbilitiesForMeetingBase(meetingState: Readonly<MeetingStateV1>): MeetingBaseAbility {
  return defineAbility<MeetingBaseAbility>((can) => {
    switch (meetingState) {
      case MeetingStateV1.DRAFT:
        can(MeetingBaseActions.VIEW, MeetingBaseSubject, { roles: { $in: [UserRoleInMeetingV1.OWNER] } });
        can(MeetingBaseActions.EDIT_BASIC_INFO, MeetingBaseSubject, { roles: { $in: [UserRoleInMeetingV1.OWNER] } });
        can(MeetingBaseActions.EDIT_STATE, MeetingBaseSubject, { roles: { $in: [UserRoleInMeetingV1.OWNER] } });
        can(MeetingBaseActions.REMOVE, MeetingBaseSubject, { roles: { $in: [UserRoleInMeetingV1.OWNER] } });
        break;
      case MeetingStateV1.DELETED:
        // Noone can do anything
        break;
      case MeetingStateV1.CANCELLED:
        can(MeetingBaseActions.VIEW, MeetingBaseSubject, {
          roles: {
            $in: [
              UserRoleInMeetingV1.OWNER,
              UserRoleInMeetingV1.CATEGORY_MEMBER,
              UserRoleInMeetingV1.PARTICIPANT,
              UserRoleInMeetingV1.TIMEKEEPER,
              UserRoleInMeetingV1.SCRIBE,
            ],
          },
        });
        break;
      case MeetingStateV1.PLANNED:
        can(MeetingBaseActions.VIEW, MeetingBaseSubject, {
          roles: {
            $in: [
              UserRoleInMeetingV1.OWNER,
              UserRoleInMeetingV1.CATEGORY_MEMBER,
              UserRoleInMeetingV1.PARTICIPANT,
              UserRoleInMeetingV1.TIMEKEEPER,
              UserRoleInMeetingV1.SCRIBE,
            ],
          },
        });
        can(MeetingBaseActions.EDIT_BASIC_INFO, MeetingBaseSubject, {
          roles: { $in: [UserRoleInMeetingV1.OWNER, UserRoleInMeetingV1.SCRIBE] },
        });
        can(MeetingBaseActions.EDIT_STATE, MeetingBaseSubject, {
          roles: { $in: [UserRoleInMeetingV1.OWNER, UserRoleInMeetingV1.SCRIBE, UserRoleInMeetingV1.TIMEKEEPER] },
        });
        break;
      case MeetingStateV1.STARTED: // Transient state
        can(MeetingBaseActions.VIEW, MeetingBaseSubject, {
          roles: {
            $in: [
              UserRoleInMeetingV1.OWNER,
              UserRoleInMeetingV1.CATEGORY_MEMBER,
              UserRoleInMeetingV1.PARTICIPANT,
              UserRoleInMeetingV1.TIMEKEEPER,
              UserRoleInMeetingV1.SCRIBE,
            ],
          },
        });
        can(MeetingBaseActions.EDIT_BASIC_INFO, MeetingBaseSubject, {
          roles: { $in: [UserRoleInMeetingV1.OWNER, UserRoleInMeetingV1.SCRIBE] },
        });
        can(MeetingBaseActions.EDIT_STATE, MeetingBaseSubject, {
          roles: { $in: [UserRoleInMeetingV1.OWNER, UserRoleInMeetingV1.SCRIBE, UserRoleInMeetingV1.TIMEKEEPER] },
        });
        break;
      case MeetingStateV1.ONGOING:
        can(MeetingBaseActions.VIEW, MeetingBaseSubject, {
          roles: {
            $in: [
              UserRoleInMeetingV1.OWNER,
              UserRoleInMeetingV1.CATEGORY_MEMBER,
              UserRoleInMeetingV1.PARTICIPANT,
              UserRoleInMeetingV1.TIMEKEEPER,
              UserRoleInMeetingV1.SCRIBE,
            ],
          },
        });
        can(MeetingBaseActions.EDIT_BASIC_INFO, MeetingBaseSubject, {
          roles: { $in: [UserRoleInMeetingV1.OWNER, UserRoleInMeetingV1.SCRIBE] },
        });
        can(MeetingBaseActions.EDIT_STATE, MeetingBaseSubject, {
          roles: { $in: [UserRoleInMeetingV1.OWNER, UserRoleInMeetingV1.SCRIBE, UserRoleInMeetingV1.TIMEKEEPER] },
        });
        break;
      case MeetingStateV1.PAUSED:
        can(MeetingBaseActions.VIEW, MeetingBaseSubject, {
          roles: {
            $in: [
              UserRoleInMeetingV1.OWNER,
              UserRoleInMeetingV1.CATEGORY_MEMBER,
              UserRoleInMeetingV1.PARTICIPANT,
              UserRoleInMeetingV1.TIMEKEEPER,
              UserRoleInMeetingV1.SCRIBE,
            ],
          },
        });
        can(MeetingBaseActions.EDIT_BASIC_INFO, MeetingBaseSubject, {
          roles: { $in: [UserRoleInMeetingV1.OWNER, UserRoleInMeetingV1.SCRIBE] },
        });
        can(MeetingBaseActions.EDIT_STATE, MeetingBaseSubject, {
          roles: { $in: [UserRoleInMeetingV1.OWNER, UserRoleInMeetingV1.SCRIBE, UserRoleInMeetingV1.TIMEKEEPER] },
        });
        break;
      case MeetingStateV1.FINISHED:
        can(MeetingBaseActions.VIEW, MeetingBaseSubject, {
          roles: {
            $in: [
              UserRoleInMeetingV1.OWNER,
              UserRoleInMeetingV1.CATEGORY_MEMBER,
              UserRoleInMeetingV1.PARTICIPANT,
              UserRoleInMeetingV1.TIMEKEEPER,
              UserRoleInMeetingV1.SCRIBE,
            ],
          },
        });
        can(MeetingBaseActions.EDIT_BASIC_INFO, MeetingBaseSubject, {
          roles: { $in: [UserRoleInMeetingV1.OWNER, UserRoleInMeetingV1.SCRIBE] },
        });
        can(MeetingBaseActions.EDIT_STATE, MeetingBaseSubject, {
          roles: { $in: [UserRoleInMeetingV1.OWNER, UserRoleInMeetingV1.SCRIBE, UserRoleInMeetingV1.TIMEKEEPER] },
        });
        break;
      case MeetingStateV1.FROZEN:
        can(MeetingBaseActions.VIEW, MeetingBaseSubject, {
          roles: {
            $in: [
              UserRoleInMeetingV1.OWNER,
              UserRoleInMeetingV1.CATEGORY_MEMBER,
              UserRoleInMeetingV1.PARTICIPANT,
              UserRoleInMeetingV1.TIMEKEEPER,
              UserRoleInMeetingV1.SCRIBE,
            ],
          },
        });
        break;
      default:
        assertUnreachable(meetingState);
    }
  });
}

But the following one does not give me the expected results (breaks my tests):

export function defineAbilitiesForMeetingBase(meetingState: Readonly<MeetingStateV1>): MeetingBaseAbility {
  const { can, build, } = new AbilityBuilder<MeetingBaseAbility>(); // cannot, rules

  switch (meetingState) {
    case MeetingStateV1.DRAFT:
      can(MeetingBaseActions.VIEW, MeetingBaseSubject, { roles: { $in: [UserRoleInMeetingV1.OWNER] } });
      can(MeetingBaseActions.EDIT_BASIC_INFO, MeetingBaseSubject, { roles: { $in: [UserRoleInMeetingV1.OWNER] } });
      can(MeetingBaseActions.EDIT_STATE, MeetingBaseSubject, { roles: { $in: [UserRoleInMeetingV1.OWNER] } });
      can(MeetingBaseActions.REMOVE, MeetingBaseSubject, { roles: { $in: [UserRoleInMeetingV1.OWNER] } });
      break;
    case MeetingStateV1.DELETED:
      // Noone can do anything
      break;
    case MeetingStateV1.CANCELLED:
      can(MeetingBaseActions.VIEW, MeetingBaseSubject, {
        roles: {
          $in: [
            UserRoleInMeetingV1.OWNER,
            UserRoleInMeetingV1.CATEGORY_MEMBER,
            UserRoleInMeetingV1.PARTICIPANT,
            UserRoleInMeetingV1.TIMEKEEPER,
            UserRoleInMeetingV1.SCRIBE,
          ],
        },
      });
      break;
    case MeetingStateV1.PLANNED:
      can(MeetingBaseActions.VIEW, MeetingBaseSubject, {
        roles: {
          $in: [
            UserRoleInMeetingV1.OWNER,
            UserRoleInMeetingV1.CATEGORY_MEMBER,
            UserRoleInMeetingV1.PARTICIPANT,
            UserRoleInMeetingV1.TIMEKEEPER,
            UserRoleInMeetingV1.SCRIBE,
          ],
        },
      });
      can(MeetingBaseActions.EDIT_BASIC_INFO, MeetingBaseSubject, {
        roles: { $in: [UserRoleInMeetingV1.OWNER, UserRoleInMeetingV1.SCRIBE] },
      });
      can(MeetingBaseActions.EDIT_STATE, MeetingBaseSubject, {
        roles: { $in: [UserRoleInMeetingV1.OWNER, UserRoleInMeetingV1.SCRIBE, UserRoleInMeetingV1.TIMEKEEPER] },
      });
      break;
    case MeetingStateV1.STARTED: // Transient state
      can(MeetingBaseActions.VIEW, MeetingBaseSubject, {
        roles: {
          $in: [
            UserRoleInMeetingV1.OWNER,
            UserRoleInMeetingV1.CATEGORY_MEMBER,
            UserRoleInMeetingV1.PARTICIPANT,
            UserRoleInMeetingV1.TIMEKEEPER,
            UserRoleInMeetingV1.SCRIBE,
          ],
        },
      });
      can(MeetingBaseActions.EDIT_BASIC_INFO, MeetingBaseSubject, {
        roles: { $in: [UserRoleInMeetingV1.OWNER, UserRoleInMeetingV1.SCRIBE] },
      });
      can(MeetingBaseActions.EDIT_STATE, MeetingBaseSubject, {
        roles: { $in: [UserRoleInMeetingV1.OWNER, UserRoleInMeetingV1.SCRIBE, UserRoleInMeetingV1.TIMEKEEPER] },
      });
      break;
    case MeetingStateV1.ONGOING:
      can(MeetingBaseActions.VIEW, MeetingBaseSubject, {
        roles: {
          $in: [
            UserRoleInMeetingV1.OWNER,
            UserRoleInMeetingV1.CATEGORY_MEMBER,
            UserRoleInMeetingV1.PARTICIPANT,
            UserRoleInMeetingV1.TIMEKEEPER,
            UserRoleInMeetingV1.SCRIBE,
          ],
        },
      });
      can(MeetingBaseActions.EDIT_BASIC_INFO, MeetingBaseSubject, {
        roles: { $in: [UserRoleInMeetingV1.OWNER, UserRoleInMeetingV1.SCRIBE] },
      });
      can(MeetingBaseActions.EDIT_STATE, MeetingBaseSubject, {
        roles: { $in: [UserRoleInMeetingV1.OWNER, UserRoleInMeetingV1.SCRIBE, UserRoleInMeetingV1.TIMEKEEPER] },
      });
      break;
    case MeetingStateV1.PAUSED:
      can(MeetingBaseActions.VIEW, MeetingBaseSubject, {
        roles: {
          $in: [
            UserRoleInMeetingV1.OWNER,
            UserRoleInMeetingV1.CATEGORY_MEMBER,
            UserRoleInMeetingV1.PARTICIPANT,
            UserRoleInMeetingV1.TIMEKEEPER,
            UserRoleInMeetingV1.SCRIBE,
          ],
        },
      });
      can(MeetingBaseActions.EDIT_BASIC_INFO, MeetingBaseSubject, {
        roles: { $in: [UserRoleInMeetingV1.OWNER, UserRoleInMeetingV1.SCRIBE] },
      });
      can(MeetingBaseActions.EDIT_STATE, MeetingBaseSubject, {
        roles: { $in: [UserRoleInMeetingV1.OWNER, UserRoleInMeetingV1.SCRIBE, UserRoleInMeetingV1.TIMEKEEPER] },
      });
      break;
    case MeetingStateV1.FINISHED:
      can(MeetingBaseActions.VIEW, MeetingBaseSubject, {
        roles: {
          $in: [
            UserRoleInMeetingV1.OWNER,
            UserRoleInMeetingV1.CATEGORY_MEMBER,
            UserRoleInMeetingV1.PARTICIPANT,
            UserRoleInMeetingV1.TIMEKEEPER,
            UserRoleInMeetingV1.SCRIBE,
          ],
        },
      });
      can(MeetingBaseActions.EDIT_BASIC_INFO, MeetingBaseSubject, {
        roles: { $in: [UserRoleInMeetingV1.OWNER, UserRoleInMeetingV1.SCRIBE] },
      });
      can(MeetingBaseActions.EDIT_STATE, MeetingBaseSubject, {
        roles: { $in: [UserRoleInMeetingV1.OWNER, UserRoleInMeetingV1.SCRIBE, UserRoleInMeetingV1.TIMEKEEPER] },
      });
      break;
    case MeetingStateV1.FROZEN:
      can(MeetingBaseActions.VIEW, MeetingBaseSubject, {
        roles: {
          $in: [
            UserRoleInMeetingV1.OWNER,
            UserRoleInMeetingV1.CATEGORY_MEMBER,
            UserRoleInMeetingV1.PARTICIPANT,
            UserRoleInMeetingV1.TIMEKEEPER,
            UserRoleInMeetingV1.SCRIBE,
          ],
        },
      });
      break;
    default:
      assertUnreachable(meetingState);
  }

  return build();
}

I suppose it's because I don't understand the specifics of the abilityBuilder.

PS: I have created special-purpose classes because I aim to reuse the very same ability definitions in various contexts, both on back-end and front-end, where I have different expressions of the data models. On the back-end I have a MeetingEntity for instance which only holds references to other documents, while on the front-end I have various other types which are composite types that replace/add/remove properties depending on the purpose at hand (e.g., I have a MeetingDetailPageViewModel, which is specific to a page).

My subjects here is the simplest expression of what can be used to determine whether access is granted or not (per object); in this case: the state of the meeting object and the roles determined for that user concerning that specific meeting object. For instance if the user is the owner of the object (one property of that object, simple case) then he gets the "OWNER" role for it. If he's instead only a participant in the meeting, then he gets the "PARTICIPANT" role for that meeting. There it's more complex because the property is not always available on the document itself. In the back-end, that information is stored in a different document (DB wise), while it is indeed on the object for specific scenarios on the front-end.

stalniy commented 4 years ago

I don’t understand how MongoDB is connected to shown examples. Could you please show what exactly is weakly typed and were types are incorrectly checked?

stalniy commented 4 years ago

AbilityBuilder constructor accepts single argument is an Ability class which you want to produce. So, if you want to use build function do this new AbilityBuilder<AppAbility>(Ability)

dsebastien commented 4 years ago

Sorry @stalniy, I meant the query language expression is not type safe:

can(MeetingBaseActions.VIEW, MeetingBaseSubject, { roles: { $in: [UserRoleInMeetingV1.OWNER] } });

In this line, the "roles" field name is not strongly typed, although it refers to the name of a field on the MeetingBaseSubject class. So if that class is refactored and that field is renamed/removed, then the code won't break at compile time here, even if it should.

PS: I've tried your proposal for AbilityBuilder and could indeed make it work, thanks for the tip about the constructor. That API isn't super clear to me though ;-)

I could make that work with this:

export type MeetingBaseAbility = Ability<[MeetingBaseActions, MeetingBaseSubjects]>;

// I was missing this
export const MeetingBaseAbility = Ability as AbilityClass<MeetingBaseAbility>;

...

export function defineAbilitiesForMeetingBase(meetingState: Readonly<MeetingStateV1>): MeetingBaseAbility {
  const { can, build } = new AbilityBuilder<MeetingBaseAbility>(MeetingBaseAbility);
  ...
  return build();

With the above, I do indeed end up with the same results using the AbilityBuilder (there's still the type safety problem though :p)

stalniy commented 4 years ago

Yes, properties are not type safe because you can use dot notation to nested property.

Check this issue for more details: https://github.com/stalniy/casl/issues/308

stalniy commented 4 years ago

Also we can use strings instead of classes for subject names what also makes it impossible to infer property names.

dsebastien commented 4 years ago

Ah yes, indeed; it makes more sense to me now. I'm pretty sure we could make that work with the help of TypeScript, but it would cost a few sleepless nights of work I guess ;-)

stalniy commented 4 years ago

Yes, actually for conditions it should be possible to implement.

andreialecu commented 4 years ago

I'm also evaluating whether to use CASL, and I was initially drawn to it by the mention that it is written in TypeScript and type safe. However, the lack of strong typing in queries seems like a major limitation, since they're the building blocks of rules.

Note that the types for mongoose (@types/mongoose on DefinitelyTyped) do have some ability to at least show intellisense for field names when used in queries while also allowing dot notation

I believe they can be used for inspiration to implement something similar here.

Perhaps there could be a MongoQuery<T, AdditionalQueries = never> type that enforces that properties exist, so that dot notation is not allowed by default. If someone needs dot notation they can explicitly request a MongoQuery<T, any> type.

Or, there could be an ExactMongoQuery<T> and keep MongoQuery as MongoQuery<T, AdditionalQueries = any>

This way you could override it like MongoQuery<Subject, { "prop.dot.notated": string }>, which seems similar to how other types are implemented in CASL.

andreialecu commented 4 years ago

Interestingly, there seems to be an update to sift.js which added this recently: https://github.com/crcn/sift.js/issues/197

stalniy commented 4 years ago

@andreialecu it will take some time to implement. The priority for now is SQL integration.

I’m waiting for the update in https://github.com/crcn/sift.js/issues/202, and in parallel work on alternative library that will fulfill my request to sift.

stalniy commented 4 years ago

About implementation details: I plan to create a helper function that infers nested properties from subjects

And eventually the code will look like this:

can(“read”, User, { [p<User>(“address”, “country”)]: “UA” })

Any feedback is welcome

stalniy commented 4 years ago

If @crcn will agree to go AST way, we will be able speed up CASL by utilizing AST builder instead of Mongo query:

can(“read”, User, where => where(“address”, “country”).eq(“UA”));
stalniy commented 4 years ago

Anyway if you store conditions in the database, static typing of fields is not so helpful

andreialecu commented 4 years ago

There is a Query<T> type introduced by the PR I linked in sift. Should be easy to just reuse that type directly for the query parameters expected by the methods in CASL.

I can work on a small PR for it if you have something else prioritized.

stalniy commented 4 years ago

If you have some time, please check whether sift supports strict typings for dot notation.

andreialecu commented 4 years ago

I think it does via the same mechanism I suggested in a previous post. See the test here: https://github.com/crcn/sift.js/pull/201/files#diff-697deed4ecf7aa58bf3ab81a6035c3ebR43-R47

stalniy commented 4 years ago

But does this approach really helps? if I change address to be addresses, it won't affect address.zip, will it?

andreialecu commented 4 years ago

No, but there will be at least only one location where it needs to be changed, and it can be kept near to the original schema. There's no other way really to handle dot notation.

But dot notation can be most times avoided by using $elemMatch. Example at: https://github.com/crcn/sift.js/pull/201/files#diff-697deed4ecf7aa58bf3ab81a6035c3ebR34

stalniy commented 4 years ago

From the mongo docs:

The $elemMatch operator matches documents that contain an array field with at least one element that matches all the specified query criteria.

So, we can’t use $elemMatch on not an array.

stalniy commented 4 years ago

So, I think the best alternative is not to use dot notation at all because it's almost impossible to make it type safe. Eventually, it's a tradeoff between value and property type safety.

I tried the approach with typed path<Person> function, works good for path but not for value :)

interface Person {
  address: {
    street: string
  }
}

const query: MongoQuery<Person> = {
  [path<Person>('address', 'street')]: 1 // typescript accepts this, so we can't ensure type safety of value
}

So, I need to do something like this:

const query: MongoQuery<Person> = {
  [path<Person>('address', 'street')]: valueAt<Person>(['address', 'street'], 'test') // the last element is the value at specified path 
}

Or we can do this:

const query: MongoQuery<Person> = {
  ...objectPath<Person>(['address', 'street'], 'test') // creates { 'address.street': 'test' }
}

Or this:

const query: MongoQuery<Person> = {
  ...flattenObject<Person>({
    address: {
      street: 'test'
    }
  }) // creates { 'address.street': 'test' }
}

But all that looks overcomplicated for such a simple thing as property definition in object :)

So, I believe the way to go is a mix what @andreialecu suggested and path solution:

interface Person {
  address: {
    street: string
  }
}

type PersonQuery = MongoQuery<Person & {
  'address.street': ValueAt<Person, ['address', 'street']> // if you rename `address` to `addresses` it at least will warn at `ValueAt` side
}>;

const query: PersonQuery = {
  'address.street': 'test'
}
andreialecu commented 4 years ago

Seems good to me. Another alternative could be to add a custom operation to sift.js to allow something like $elemMatch to work on objects. Maybe it could be named $objMatch. That way dot notation could possibly be prevented entirely.

stalniy commented 4 years ago

After some investigation, I understood that this cannot be implemented without specialized AbilityBuilder class. The issue is that Conditions is a generic parameter of Ability class. But restriction on specific type are defined at AbilityBuilder method generic parameter. So, the code looks like this:

type AppAbility = Ability<['read' | 'update', Post], MongoQuery>

class AbilityBuilder<T extends AnyAbility> {
  can<S extends Generics<T>['abilities'][1]>(action: Generics<T>['abilities'][0], subject: S, conditions: Generics<T>['conditions']) {}
}

AbilityBuilder infers all parameters from passed in Ability type and I don't see a way how I can filter out Generics<T>['conditions'] (which is MongoQuery in this case) to get a type MongoQuery<S>.

Probably the only way to do this is to move Conditions generic parameter under tuple type as the 3rd element, so then we can write this:

type AppAbility = Ability<
  ['read' | 'update', Post, MongoQuery<Post>]
>

Any thoughts?

andreialecu commented 4 years ago
class AbilityBuilder<T extends AnyAbility> {
  can<S extends Generics<T>['abilities'][1]>(action: Generics<T>['abilities'][0], subject: S, conditions: MongoQuery<S>) {}
}

Would this not work? I've been out of the loop on this for a bit, hopefully the above code isn't silly. Held off from implementing casl until type safety is available for conditions.

You could then just do type AppAbility = Ability<['read' | 'update', Post]>.

I've tested in a playground here and I see code completion.

stalniy commented 4 years ago

As I said it requires specialized AbilityBuilder instance. Currently AbilityBuilder knows nothing about restriction on Conditions shape, it infers it from Ability, in that case from T, so you can't write MongoQuery<S>

andreialecu commented 4 years ago

Like I said, I'm not familiar with AbilityBuilder at the moment.

I was thinking about allowing to use a simpler type: Ability<['read' | 'update', Post]> vs Ability<['read' | 'update', Post, MongoQuery<Post>]> since it seems to be redundant in this case.

But it may be necessary to override it with a custom type to allow dot notation, as per previous messages here. So probably both syntaxes should be allowed.

stalniy commented 4 years ago

Ok, now I know how it's called :) The issue is that typescript doesn't support Higher Kinded Types(HKT). There is an issue from 2014 - https://github.com/microsoft/TypeScript/issues/1213 which probably will never be fixed.

I'll try to use https://github.com/strax/tshkt to emulate HKT. If this doesn't work well for casl, then the only way is to create one type of AbilityBuilder per condition type (e.g., MongoAbilityBuilder, FunctionAbilityBuilder, JsonSchemaAbilityBuilder, etc) and use it as:


import { AbilityBuilder, AbilityBuilderClass } from '@casl/ability';

interface JsonSchemAbilityBuilder {
   can<S extends ...>(action: string, subject: S, conditions: JsonSchema<S>): RuleBuilder
   cannot(...)...
}

const JsonSchemAbilityBuilder = AbilityBuilder as AbilityBuilderClass<JsonSchemAbilityBuilder>;
andreialecu commented 4 years ago

Also possibly useful: https://github.com/Morglod/ts-pathof

stalniy commented 4 years ago
class AbilityBuilder<T extends AnyAbility> {
  can<S extends Generics<T>['abilities'][1]>(action: Generics<T>['abilities'][0], subject: S, conditions: MongoQuery<S>) {}
}

Would this not work? I've been out of the loop on this for a bit, hopefully the above code isn't silly. Held off from implementing casl until type safety is available for conditions.

You could then just do type AppAbility = Ability<['read' | 'update', Post]>.

I've tested in a playground here and I see code completion.

The provided sample contains a wrong assumptions:

const x = new AbilityBuilder<AppAbility>();

x.can("read", {} as Post, { $eq: { id: "1" } });

The 2nd argument of AbilityBuilder is a type not instance, so it should be:

const x = new AbilityBuilder<AppAbility>();

x.can("read", 'Post', { $eq: { id: "1" } });
x.can("read", Post, { $eq: { id: "1" } }); // Post here is a class

And then I need to understand the instance out of passed in type. It's easy for class but a bit more complicated for string. Probably the best way is to rely on tagged union.

Just wanted to say that it's not easy to implement a general solution :)

stalniy commented 4 years ago

Implemented in next branch and will be available in v5, including support for typesafe fields