Database transactions similar api addPolicies, removePolicies

[GopherJ]

Hello, recently I'm having another issue, while creating a new user/role, I would like to assign some default rights for it.

We can definitly call multiple times addPolicy but it cannot guarantee that all the default policies have been added, I'm risking having an invalid user in that case.

So I'm thinking if we could have some wrappers like addPolicies, removePolicies.

addPolicies returns Promise<true> only if all the policies have been added, if one of them fails then it cancels this operation (remove the ones have been added)

removePolicies is similar.

[hsluoyz]

I think this idea is awesome. I have been expecting such a feature for a long time. The function naming is also good. Go ahead.

BTW it should be optional for an adapter.

[GopherJ]

I'll implement it first in casbin-rs

[Sefriol]

I think this should be added adapter level, not node-casbin level.

Not all storage options fully support ACID operations, and those should be the only way to do it (i.e. what in databases are called "transactions").

I had some ideas for this in Mongoose

i.e. you use normal casbin operations and create your addPolicies function to your application using "synced adapter". If one of your additions / removals fails, just abort transaction.

[GopherJ]

@Sefriol We need it in both sides I think. In casbin side it's pretty simple because everything happens in memory. In adapter side we need to implement these methods according to databases.

If the database supports Transaction then we use transaction.
If not then we check the result of every operation, if one operation fails, we remove all the inserted/removed policies.

But definitly if the database doesn't support transaction, in some cases we will have unexpected errors. Like:

We insert 1000000 policies in one time but before finishing we got an unexpected network error.

[Sefriol]

There is a lot of things that you have to take into account if you do not do this on adapter level. Let's say that you added 10000000 policies and one of them failed. Now when you are removing these policies, it fails again. What do you do?

On the other hand, if you just do it on adapter level, you can just abort and refresh your enforcer and you are done. If transaction is not saved, nothing happens.

AddPolicies would also be quite custom for each storage solution, so finding a common set of functionality does not seem feasible to me. That's why I thought that adapter level solution is the best course of action at the moment.

[GopherJ]

@Sefriol I'm aware of this problem. It's tough but for me I can accept that part of adapters will have this type of unexpected errors.

However I don't like the solution that every time after executing addPolicies I need to reload all policies to casbin enforcer.

[GopherJ]

We can actually give users the information.

If the database doesn't support transaction, then you may get unexpected errors. (addPolicies failed but still inserted part of policies)

Let the users choose the correct databases for their application. If they only use addPolicy/removePolicy. It's ok for all databases but if they will use addPolicies then they should choose a database with transactions

[Sefriol]

However I don't like the solution that every time after executing addPolicies I need to reload all policies to casbin enforcer.

If you are using any of the current request based authz libraries (atleast based on the examples), it's doing that already every time there is a new request to the backend (as far as I know. i.e. a new enforcer for a new request).

Reload would also only happen during abort. In addition, you would probably need to do that anyway if your backend is more complicated than just a single server (i.e. backend is a cluster or a microservice).

In general, I'm not against the idea. I just think it's quite a lot of work for a feature that is more simpler to do case by case basis in the adapter itself. And because all of the implementations for adapters are different, it might be hard to find common ground for this to have any use.

[GopherJ]

Any one wants to take this?

[nodece]

Sorry, I miss this issue. I will follow up on this.

[divy9881]

Any one wants to take this?

If it is available, I will take up this issue.

[GopherJ]

@divypatel9881 Thanks

[divy9881]

Just for confirmation, I just have to add the implementation for batch_adapter interface and addPolicies and removePolicies methods as was implemented under core-API, right?
I have started fixing this issue.

[Sefriol]

IMHO, this really should try to create a standard which adapters could also use. Similar to LoadPolicy we should create internal functions that are used by addPolicies/removePolicies to trigger (for example) a transaction in MongoDB and also committing or aborting them when appropriate.

Just having an array of stored policies and removing them on failure won't cut it. Since I think this requires quite a lot of design work and discussion I did not move forward with this.

But the general idea of what I have had in mind can be looked from the adapter that's still work in progress:
https://github.com/Sefriol/casbin-mongoose-adapter/tree/feature-synced-adapter

[divy9881]

@Sefriol, this feature is already being implemented in casbin-core, so I just have to port this feature to TypeScript.

[GopherJ]

@Sefriol In casbin-rs, we force every adapter to implement batch operations, users have to choose the correct adapters to use to avoid unexpected behaviour

[nodece]

Hi @divypatel9881, @GopherJ, @Sefriol We must break the current Adapter interface then add addPolicies and removePolicies to Adapter interface.

[divy9881]

Hi @divypatel9881, @GopherJ, @Sefriol We must break the current Adapter interface then add addPolicies and removePolicies to Adapter interface.

Yeah, thanks @nodece, you are right, I have followed the casbin-core implementation structure.

[Sefriol]

I think that current idea of addPolicies and removePolicies is too limiting. Some policy additions and removals are tied together.

As an example, you might want to add same policies to all users, but some users might have some of these policies already, so update would fail if you tried to add a policy that already exists. Because of this, you would delete all of those permissions from the users before adding them again.

However, now you cannot do these operations in same safe mechanism. You need to remove and then add policies in different actions. But what if the addition fails for some other reason? Now you have deleted permissions from users, but not add new ones.

This is a reason why we should go with an adapter which has a startPolicyBatch,abortPolicyBatch,commitPolicyBatch (or whatever naming we see fit) functions and then all actions within the batch are either saved or not done at all. This gives us much more customizability and then we are not limited to simple one dimensional functions and it also does not require other adapters to generate these new addPolicies -functions either.

This requires much more from the adapters which want to implement this (especially those which do not have transactions or anything similar at their disposal), but I think that's the correct way to move forward.

[nodece]

As an example, you might want to add same policies to all users, but some users might have some of these policies already, so update would fail if you tried to add a policy that already exists. Because of this, you would delete all of those permissions from the users before adding them again.

Hi @Sefriol User data must be deleted by the user. Users need to check themselves.

We will add transactions to savePolicy, addPolicies and removePolicies.

[nodece]

Why update fails? Casbin should provide reason.

[GopherJ]

@Sefriol

Because of this, you would delete all of those permissions from the users before adding them again.

This wont happen, also maybe just forgot the idea that I wrote at the beginning of this issue, it's not true. For example, in casbin-rs's implementation, add_policies must all succeed for every policy, otherwise we cancel the operation. casbin golang won't cancel the operation but I also don't think it will cause any critical bug.

It's safe to cancel the operation in memory. in adapter's level we should use transaction, it's also safe. So for me, it's all safe.

Why I think we should cancel the whole operation when there is already one policy exists? because that may help on the implementation of watcher and adapter, we will always be able to know the exact policies which have been added/removed.

I consider it's safe, at least in casbin-rs, I cannot see what kind of bug it can rise. If database doesn't have transaction, we may have unexpected behaviour, but users need to choose the correct database from the beginning.

[GopherJ]

startPolicyBatch,abortPolicyBatch,commitPolicyBatch makes it more complex but the complexity is not necessary. we then need to reload policies from db adapter to model, it's not ideal for me.

If we can already solve things in just one operation for both model and adapter, I don't see why will we need similar apis.

[nodece]

addPolicies and removePolicies must be atomic operations. It must all succeed for every policy, otherwise we cancel the operation.

[divy9881]

casbin golang won't cancel the operation but I also don't think it will cause any critical bug.

Actually, in casbin-core the operation is atomic, every policy rule has to succeed in order to update the policy, otherwise, it reverts back to initial state and cancels the operation. @nodece, @hsluoyz and I made sure of this.
Otherwise, everything you said was correct, and I agree with you.

[GopherJ]

In casbin-rs, while calling add_policies in model:

https://github.com/casbin/casbin-rs/blob/cdd1aa380fe64126bbefd8a5f1b3a08b46e78513/src/model/default_model.rs#L161

we use HashSet to avoid duplicated policies, if one policy already exist, then we cancel the whole operation.

In adapter, we use transactions:

https://github.com/casbin-rs/diesel-adapter/blob/ac1d2fdc1c0e37c60dcb86297ef65006ace96114/src/actions.rs#L245

https://github.com/casbin-rs/sqlx-adapter/blob/5bc464fc2581cdcfc7c4457326130544403a20a3/src/actions.rs#L551

If one insertion fails, we cancel all.

At last:

https://github.com/casbin/casbin-rs/blob/cdd1aa380fe64126bbefd8a5f1b3a08b46e78513/src/internal_api.rs#L54

we insert first into adapter before model, because sometimes we may have network error, in this case we just return the error and don't even need to insert into model, we need to do this because the old design may cause errors if we have already inserted into model and the fail to insert into adapter.

So I think at least in my view, the whole process is safe, we also have simple API. Only a part of users need batch operations, this part of users need to choose correct adapter and correct database to guarantee its safety

[GopherJ]

In some adapters like FileAdapter, we can periodically call SavePolicy for persist

[nodece]

In casbin-golang, while calling addPolicies in model:

we call hasPolicy to avoid duplicated policies, if one policy already exist, then we cancel the whole operation.

It looks similar to casbin-rs.

TODO in casbin-golang

we insert first into adapter before model, because sometimes we may have network error, in this case we just return the error and don't even need to insert into model.

[Sefriol]

@Sefriol

Because of this, you would delete all of those permissions from the users before adding them again.

This wont happen, also maybe just forgot the idea that I wrote at the beginning of this issue, it's not true. For example, in casbin-rs's implementation, add_policies must all succeed for every policy, otherwise we cancel the operation. casbin golang won't cancel the operation but I also don't think it will cause any critical bug.

I probably mispresented what I was going for with this:

Let's assume a following use case, with following policy table

p, alice, data1, read
p, bob, data2, write

Now we notice that a common file, data1, should be readable by all users. We try to add right to every user, but our action fails since alice already has this right. To combat this, we actually remove rights for this file from all users and then add them again.

If any of these two actions fail at any point, both actions should be reverted, not that only removal or addition should be reverted. Can this be achieved by this current idea? If not, then I do not think this solution is sufficient enough.

We can also play with more conditions and logic, if we do it in more complex way. Setup failure states that happen outside of casbin which would also cancel these additions and removals. This way batch would have more functionality than just these two functions.

Or maybe I'm misunderstanding something.

[GopherJ]

@Sefriol I think in your example, you have two operations, first of all, let's say you have 1000 users and you try to add data1's read access for every one, then in our logic and our idea, this action will fail because alice has already the access. We will cancel this operation.

But util now, the whole process has no problem, the policy in memory and in adapter wont have any changes. Then it's user who decides what needs to be done in case of failing. He/she can try with a fewer list of policies, he can try to add one by one for every users. It's user who decides.

If we want this to succeed, I believe in casbin golang it will already succeed, but maybe @nodece @divypatel9881 can correct me. In casbin-rs it will fail because alice has already this policy, currently I don't want to change the fact that it will fail because I would like to have the correct number of inserted policies passed to watcher.

I can also make it succeed, like in adapter level, I can check if the fail is caused by Unique Constraint Conflict, if yes I don't revert the transaction, if it's other error, I revert still the transaction, but this part has't been done in casbin-rs and I'm also not sure if it needs to be done.

However, at least I didn't see any difficulties to make it succeed for your example. Also file is a bad example, I'd prefer using save_policy for files.

[GopherJ]

@nodece Yes I find it's similar and maybe even better because we don't need to insert and remove...I'll maybe change my method...

[nodece]

we actually remove rights for this file from all users and then add them again.

This is users logic. Nothing to do with Casbin.

When try add data, read to every users, we fist check it whether exists Casbin, it found alice already has this policy, we will Interrupt this operation. If the check passes, we will perform add policy to every users.

[Sefriol]

we actually remove rights for this file from all users and then add them again.

This is users logic. Nothing to do with Casbin.

When try add data, read to every users, we fist check it whether exists Casbin, it found alice already has this policy, we will Interrupt this operation. If the check passes, we will perform add policy to every users.

Yes, it's user's logic, which would be defined by the user. I'm not saying that these specific use cases should implemented, but give users of the library a change to create them. Giving them more customizability, so that they can define themselves what to include in a batch would be a nice feature IMO. addPolicies and removePolicies should also be created by using these functions. It would be great example for users wanting to create more complex casbin logic.

As a quick example:

function addPolicies(policies) {
    this.startBatch()
    for (policy in policies) {
          res, err = this.addPolicy(policy)
          if (!res || err) {
              this.abortBatch()
              return false
          }
    }
    if(this.commitBatch()) return true
    return false
}

Or something like that. Just me throwing ideas :)

[nodece]

@Sefriol Sounds good, but importing it will complicate Casbin.