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

Bug: TypeScript Compilation Error Due to Strict MergeUnion<T> Definition in @casl/ability #1015

Open
maltyxx opened this issue Jan 10, 2025 · 2 comments
Labels

Comments

@maltyxx
Copy link

maltyxx commented Jan 10, 2025

Bug Description

The MergeUnion<T> type in @casl/ability is overly restrictive, causing TypeScript errors when using dot-notation paths (e.g., 'tenant.id') in conditions. This limitation prevents referencing nested properties as string keys, even though CASL and MongoDB support such syntax.

Steps to Reproduce

  1. Configure an ability:
    import { AbilityBuilder, Ability } from '@casl/ability';
    
    const { can, build } = new AbilityBuilder(Ability);
    can('read', 'User', { 'tenant.id': '12345' }); // Dot-notation for nested property
    const ability = build();
  2. Check permissions:
    ability.can('read', 'User', userData);
  3. TypeScript raises an error:
    TS2345: Argument of type `{ 'tenant.id': string }` is not assignable...
    

Expected Behavior
TypeScript should allow dot-notation paths without requiring type casting.

Proposed Fix
Modify MergeUnion<T> in conditions.ts to support arbitrary string keys:

type MergeUnion<T, Keys extends keyof T = keyof T> = {
  [K in Keys]: T[K];
} & {
  [key: string]: any;
};

This change aligns TypeScript behavior with CASL’s runtime capabilities, improving developer experience while maintaining backward compatibility.

Environment

  • @casl/ability: v6.7.4
  • TypeScript: v5.1.3
  • Node.js: v20.18.1
@maltyxx maltyxx added the bug label Jan 10, 2025
@stalniy
Copy link
Owner

stalniy commented Jan 10, 2025

This change will basically allow any stringed property to be specified in conditions. Which is not good for the case when there are properly configured types.

I’ll double check this but I had the impression that TS will infer it by default from this mapping type

@maltyxx
Copy link
Author

maltyxx commented Jan 12, 2025

Thank you for your response! I understand your concern regarding type safety and the risks of allowing arbitrary string keys.

However, I find it unfortunate that CASL does not currently support conditions with nested objects like:

can('read', 'User', { tenant: { id: '12345' } });

This would align better with how many ORMs and libraries handle object relationships. For example, in an ORM, we often work with nested objects to represent relationships between entities, making this approach more natural and easier to use.

Supporting nested object conditions directly in CASL would improve both usability and consistency with ORM practices. It could also help avoid relying on dot-notation strings, which are less intuitive and more error-prone.

If implementing this behavior is not feasible for now, I'd appreciate any guidance on how to structure conditions more effectively within the current limitations.

Thanks again for your efforts on this library—it’s an amazing tool for access control. 😊

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