sergey-telpuk / nestjs-rbac

Awesome RBAC for NestJs
Other
421 stars 36 forks source link

Async filters support #65

Closed vshjxyz closed 3 years ago

vshjxyz commented 3 years ago

refs #62

Hello, I've introduced async filters with this attempt by ensuring to wrap any filter with a Promise (via Promise.resolve) so we can return boolean or Promise<boolean>. This introduces a breaking change so perhaps we can discuss if it's best to have a canAsync rather than modifying the existing can - thoughts?

codecov[bot] commented 3 years ago

Codecov Report

Merging #65 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #65   +/-   ##
=======================================
  Coverage   97.05%   97.05%           
=======================================
  Files           8        8           
  Lines          68       68           
  Branches        2        2           
=======================================
  Hits           66       66           
  Misses          2        2           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7db79bc...ff3370c. Read the comment docs.

sergey-telpuk commented 3 years ago

refs #62

Hello, I've introduced async filters with this attempt by ensuring to wrap any filter with a Promise (via Promise.resolve) so we can return boolean or Promise<boolean>. This introduces a breaking change so perhaps we can discuss if it's best to have a canAsync rather than modifying the existing can - thoughts?

Hi @vshjxyz , looks good but it's incompatible with previous version. How do you think that we'll add asyncFilteror specific filter prefix for example if you want to use async filter you can do the following:

  1. prefix
    export interface IStorageRbac {
    roles: string[];
    permissions: object;
    grants: object;
    filters: { [key: string]:  "ASYNC@FILTER!" }; - add prefix ASYNC
    }
  2. propetry
    export interface IStorageRbac {
    roles: string[];
    permissions: object;
    grants: object;
    asyncfilters: { [key: string]: any | IAsyncFilterPermission };
    }

    Let me know your thoughts.

vshjxyz commented 3 years ago

@sergey-telpuk sorry if this was delayed but I didn't got time to get back on this yet! Actually I would prefer if we add a canAsync instead of relying on strings as it's pretty straightforward to figure out what it does by name (thinking also about Typescript intellisense). Would you be ok with that? if so I can update the PR and readme

sergey-telpuk commented 3 years ago

@sergey-telpuk sorry if this was delayed but I didn't got time to get back on this yet! Actually I would prefer if we add a canAsync instead of relying on strings as it's pretty straightforward to figure out what it does by name (thinking also about Typescript intellisense). Would you be ok with that? if so I can update the PR and readme

@vshjxyz I think it'll work for me, please update PR

vshjxyz commented 3 years ago

@sergey-telpuk I've just updated the PR with the canAsync support. Let me know if I need to do any change or not. I also had to update devDependencies to be able to use Typescript ?. and ?? syntax (if you prefer to keep the old version, I can refactor to accommodate)

sergey-telpuk commented 3 years ago

@sergey-telpuk I've just updated the PR with the canAsync support. Let me know if I need to do any change or not. I also had to update devDependencies to be able to use Typescript ?. and ?? syntax (if you prefer to keep the old version, I can refactor to accommodate)

Hi @vshjxyz, looks good, will be merged