search

casbin / node-casbin

An authorization library that supports access control models like ACL, RBAC, ABAC in Node.js and Browser
https://casbin.org
Apache License 2.0
2.58k stars 215 forks source link

Extract file handling out (as a plugin?) to enable casbin to be used on React Native #257

Closed harveyappleton closed 3 years ago

harveyappleton commented 3 years ago

Hey,

firstly thanks for this awesome library!

I noticed that the frontend casbin library uses this under-the-hood, so I'm using this library on both microservice and Frontend.

However, I'd love to be able to use this library on React Native - which would work, but because the model loading from file uses the fs library, I can't.

How likely/possible would it be to extract out model loading from a file/file handling to be a plugin/adapter to remove the need for fs? (I load the model and policy dynamically from the microservice).

Thanks!

hsluoyz commented 3 years ago

@harveyappleton I think node-casbin can be used at frontend, see: https://github.com/casbin/casbin-editor . And it also works for React: https://github.com/casbin-js/react-authz

Can you explain why it doesn't work as the same way in React Native?

Zxilly commented 3 years ago

@harveyappleton Have you try API casbin.newModelFromString()? You can later init Enforcer with the model. I think this can help.

harveyappleton commented 3 years ago

@hsluoyz - fs is a node library that allows access to file system, which doesn't exist on React Native. I also don't need access to filesystem when I'm loading the model and the policy dynamically from an API call, so in that way I can't use casbin on React Native :(

@Zxilly - yeh I was using that. But still doesn't work because casbin source code imports fs (src/util/util.ts) - which isn't supported on React Native :(

harveyappleton commented 3 years ago

The ideal would be if the loading the model (or anything) from a file could be abstracted out to an adapter or plugin - or something, which means that the dependency on Node's fs can be taken out of the main library, that would mean that a) the package is smaller and b) can be used on mobile (React Native)

Zxilly commented 3 years ago

What you ask for is the original design of casbin.js. Refactoring of casbin.js has been planned.

willpercey-gb commented 3 years ago

Refactoring of casbin.js has been planned.

Could a branch be opened that would allow us to contribute towards refactoring intended to allow the use of this in React Native?

Zxilly commented 3 years ago

@willpercey-gb Thank you for your interest in this. This task has not started yet. We will let you know when the project start.

hsluoyz commented 3 years ago

@hsluoyz - fs is a node library that allows access to file system, which doesn't exist on React Native. I also don't need access to filesystem when I'm loading the model and the policy dynamically from an API call, so in that way I can't use casbin on React Native :(

@Zxilly - yeh I was using that. But still doesn't work because casbin source code imports fs (src/util/util.ts) - which isn't supported on React Native :(

@harveyappleton I mean you can see how Casbin-editor works. It uses Node-Casbin and it doesn't use file and it's frontend (web).

harveyappleton commented 3 years ago

Hey @hsluoyz - yeh as mentioned in first post, I'm actually using Casbin on a front end web app and it works great! It's just that it won't work on React Native (because of the mentioned fs dependency).

Would be cool if we could make Casbin fully isomorphic across Server (node), Frontend (web) and Mobile (react native) - rather than having a separate frontend Casbin.Js ? Would that be feasible? As @willpercey-gb - I think I'd be interested in helping make that a reality. :)

hsluoyz commented 3 years ago

@harveyappleton I think the idea about separating fs related code from Node-Casbin is good. Your contribution will be welcome. But I'm not familiar with React-Native. Can you elaborate more on the next steps to do this?

I also want opinions from @nodece @Sefriol @Zxilly

harveyappleton commented 3 years ago

@hsluoyz sure - I mean, if we extract out the fs package from Node-Casbin, it'll just really be Casbin.js by that point - usable by server, frontend and mobile - so almost no need for 2 different packages anymore. Let people handle their own way of storing the policy/model (eg cookies, local storage, whatever). 1 js package = easier to maintain.

To make it React Native friendly, we'd certainly need to remove the fs package. Tbh - it'd be better anyway if you could use casbin without having to depend on a file - if you could pass a model and a policy into it however you want - loaded from a file (using a casbin adapter) or dynamically (loaded from an API call) - that'd be better and make the package more flexible?

Sefriol commented 3 years ago

Is there any specific reason why loading a model / policy from a file is necessary? I personally removed the file versions from our backend and moved the model definition into javascript.

I think creating model adapters might be a good idea. That way model could be offloaded to database for example. And having the model in a file would still be a possibility.

At the same time though, this might generate more complexity for the library than necessary without really providing any benefits. Maybe we could just change the fs import to be conditional based on platform.

hsluoyz commented 3 years ago

@harveyappleton @Sefriol what about making a small change at first:

Move Node-Casbin's built-in file adapter into a separated repo and package, just like how other DBs adapter do. In other languages like Go, Java, we usually make file adapter built-in but this is not mandatory.

Then the new Node-Casbin without file adapter code can be used in React-Native I think. Does it work for you?

A difficult part is the tests of Node-Casbin. We may put the new file adapter as the dev dependencies of the new Node-Casbin, so latter can still use the model and policy files as unit tests.

Zxilly commented 3 years ago

@hsluoyz plz make a new repo for file-adapter, I will work on this.

hsluoyz commented 3 years ago

@Zxilly https://github.com/node-casbin/file-adapter

Sefriol commented 3 years ago

A difficult part is the tests of Node-Casbin. We may put the new file adapter as the dev dependencies of the new Node-Casbin, so latter can still use the model and policy files as unit tests.

These files can quite easily be converted to plain javascript. However, I think these files should be a submodule repo that is common for all casbin projects. That way we have similar test cases (models and policies) for all casbin language versions and keeping them up to date is much easier.

hsluoyz commented 3 years ago

@Sefriol using git-submodules is clever. But I'm afraid that because different languages have different progress on features. A feature update on a language may result in the submodule repo change. But I think it's worth a try. And it's a big problem to resolve.

Another difficulty is the requests and expected are not in the examples folder. So the test code for each language still needs to maintain a lot of info. Better the new testset can contain all info:

  1. Model (conf)
  2. Policy (csv)
  3. Requests (how to handle ABAC element?)
  4. Expected enforcement result (also csv is OK)
  5. Matcher functions (how to do this?)

But like my quotes, there are challenges.

Sefriol commented 3 years ago

@Sefriol using git-submodules is clever. But I'm afraid that because different languages have different progress on features. A feature update on a language may result in the submodule repo change. But I think it's worth a try. And it's a big problem to resolve.

Another difficulty is the requests and expected are not in the examples folder. So the test code for each language still needs to maintain a lot of info. Better the new testset can contain all info:

  1. Model (conf)
  2. Policy (csv)
  3. Requests (how to handle ABAC element?)
  4. Expected enforcement result (also csv is OK)
  5. Matcher functions (how to do this?)

But like my quotes, there are challenges.

First step could be just to include 1. and 2. in the repo. Submodules can be assigned with commit ID, so different progress in features could still be solved. However, the test cases for models and policies should not change often unless there are breaking changes on how models and policies are dealt with. However, this can still be solved with submodule commit/branch UID.

Everything that transfers between different casbin language versions should be shared from common source. Everything else that is language specific should be included in their respective repositories.

nodece commented 3 years ago

node-casbin includes os and fs module from Node. I tried to fix fs module by load fs and react-native-fs dynamically, but still failed, so I suggest to use the rn-nodeify to fix this error.

I think the process is huge if we split a common package from node-casbin, and make RN version of casbin base on common package. This is a bit like: https://github.com/ReactTraining/react-router

If covert the node-casbin to plain javascript, it looks good, but I'll worry about losing some user, because node-casbin only accepts string model or policy, the user must to install plugin to load the file model or policy.

Sefriol commented 3 years ago

node-casbin includes os and fs module from Node. I tried to fix fs module by load fs and react-native-fs dynamically, but still failed, so I suggest to use the rn-nodeify to fix this error.

rn-nodeify looks really scary. I would rather keep fs as is than use it.

If covert the node-casbin to plain javascript, it looks good, but I'll worry about losing some user, because node-casbin only accepts string model or policy, the user must to install plugin to load the file model or policy.

Same could be said for not doing the change: users won't use Casbin since it does not work on their platform. Adapters are already in use and I do not see file adapter any more special than any other adapter.

In my opinion, since string / in memory adapter is usable between all platforms, it should be the default one and file adapter should be a plugin like the rest of them.

So pretty much two options:

  1. File-adapter will be deprecated in node-casbin and add it shall be a plugin. Plugin is added into devDependancies for the tests.
  2. We dynimically install/load a library based on the platform.
harveyappleton commented 3 years ago

Yeh I completely echo @Sefriol on all of the above. The bare node-cabin package should just implement model and policy loading from a string, and if you want to load it from a file, or a database, or any other source, that should be done with an adapter.

By removing os and fs from the package:

I know this means adding an adapter to load the model from a file, but hopefully(?) that won't be too much work? Would value everyones thoughts.

nodece commented 3 years ago

@hsluoyz I agreed with @Sefriol and @harveyappleton idea.

The following is my integration steps:

hsluoyz commented 3 years ago

@harveyappleton @Zxilly any opinions?

Zxilly commented 3 years ago

@nodece Place fs adapter/model in other repos will be helpful for reduce build bundle size, which is important for frontend usage.

nodece commented 3 years ago

@Zxilly If we are a built-in fs adapter/model, it is easier for users to use.

hsluoyz commented 3 years ago

@nodece we can add it via NPM dependency and expose the API if needed.

harveyappleton commented 3 years ago

@hsluoyz I understand that @nodece PR is a big breaking change and get that brings challenges, but if we don't do it, how else are we able to extract out all of the Node functionality to enable this to be a truly cross-platform library?

hsluoyz commented 3 years ago

@harveyappleton we have a solution, see: https://github.com/casbin/node-casbin/issues/270

nodece commented 3 years ago

closed by https://github.com/casbin/node-casbin/issues/270#issuecomment-903232062.