search

DeadAlready / easy-rbac

RBAC implementation for Node.js
MIT License
169 stars 32 forks source link

Errors are swallowed inside easyRBAC #13

Open menocomp opened 5 years ago

menocomp commented 5 years ago

Good day,

I have been using your nice library for Node app and React app both are running in production at the moment. Thanks for your great job.

One thing I noticed that if a when function throw an error. The error is being swallowed and easyRBAC return false instead! With that being siad, it is impossible to know if easyRBAC returns false because when function return false or because it throws an error.

After debugging this library I found the issue in these lines: https://github.com/DeadAlready/easy-rbac/blob/master/lib/rbac.js#L159 https://github.com/DeadAlready/easy-rbac/blob/master/lib/rbac.js#L176 https://github.com/DeadAlready/easy-rbac/blob/master/lib/utils.js#L19 https://github.com/DeadAlready/easy-rbac/blob/master/lib/utils.js#L29

If you agree with me that easyRBAC should not swallow errors and it should just re-throw errors back to the caller. I would be happy to provide PR if you are busy.

Thanks

DeadAlready commented 5 years ago

First off thank you for the feedback.

I thought I already addressed this, but I guess it was in my dreams.

I agree with the idea that an error should be thrown out of the rbac check if the underlying conditional functions throw an error - otherwise it is confusing. In that regard I agree with your assessment about the lines involved in this process.

A PR would be appreciated, but just to make sure we don't end up breaking any existing implementations: The aim is to return true or false based on the RBAC check and pass through errors that are thrown so that we could see/diagnose the RBAC checking from the outside.

Hopefully it is evident from the code, but just in case I want to point out that this means handling errors based on their type or message, because of the way parallel processing is done. I want to keep the library light and not use a 3rd party promises library to handle the parallel promises of the rbac check. As such currently I use the Promise.all system in reverse. Promise.all waits for all the promises in an array to finish before moving forward, except when one of the promises throws, then it will instantly throw and discard the rest of the promises - this is the opposite of what is needed - we want to run checks in parallel and if one returns a positive result then pass it through or wait until all return a negative result and then pass that through. A simple implementation is to run all the checks in parallel with Promise.all and then OR the results (if at least one is true then the result is true). This will however need to run through all the checks before returning a result. Instead I used the error throwing mechanism to shortcut to the result -

  1. Run all checks in parallel
  2. If one returns true (check passes) then throw a specific error, which in turn will break the Promise.all and allow us to move directly into the handling
  3. If the Promise.all returns an error and the error is of a specific type then we know that the check actually succeeded

Hopefully this helps in understanding the current code and implementing changes accordingly.

Thanks