Bug in priority effectors

[aditya-nambiar]

The function def enforce_ex(self, *rvals) in core_enforcer.py does not consider the priority of the matched policies.

It in fact returns the first effect that is matched. This would work ONLY if we iterate through the policies in sorted order but I dont see that being done. Or we should collect all the effects and the priorities to make a call, however the function final_effect in PriorityEffector does not use priorities at all.

Hence I believe we should fix the order in which we process the policies in priority order.

I also tested that the results change if I add policies in different order ( which I believe should not be the case ? )

I hope the issue makes sense

[casbin-bot]

@techoner @Nekotoxin

[hsluoyz]

@aditya-nambiar do Golang Casbin and Casbin-Editor (Node-Casbin) have the same issue?

• https://github.com/casbin/casbin
• https://casbin.org/editor

[aditya-nambiar]

@aditya-nambiar do Golang Casbin and Casbin-Editor (Node-Casbin) have the same issue?

• https://github.com/casbin/casbin
• https://casbin.org/editor

We dont use Go, but I tried the online editor and it works correctly there - https://editor.casbin.org/#FQF92UR9P
So probably only a pycasbin issue

[hsluoyz]

@BustDot

[aditya-nambiar]

@BustDot Could you confirm if this is a bug?

[hsluoyz]

@aditya-nambiar can you share your:

1. Model
2. Policy
3. Request
4. Result (you got)
5. Result (you expect)

And:

6. python script repo to test it
7. Shared link of https://casbin.org/editor/ to test it

[aditya-nambiar]

I have added all the context in this gist - https://gist.github.com/aditya-nambiar/d85f3e722dbda4aa0ba31a1e3a695208
cc
@hsluoyz

[BustDot]

@BustDot Could you confirm if this is a bug?

Note that the smaller priority value will have a higher priority, which is illustrated in casbin doc. So priority 10 is higher than 20. And when call load_policy()，it will call sort_policies_by_priority() in model/model.py.

[aditya-nambiar]

@BustDot yes make sense that smaller value has a higher priority. The bug I am pointing is the fact that the priority is not respected but policies are run according to the order of insertion.

I noticed that sort_policies_by_priority is only called on load, but if I am not loading but adding to the policy using other api's the sorting order is not maintained.

The bug is resolved if I do a sort before iterating over the policies inside enforce_ex, but that might lead to perf issues ?

[BustDot]

@BustDot yes make sense that smaller value has a higher priority. The bug I am pointing is the fact that the priority is not respected but policies are run according to the order of insertion.

I noticed that sort_policies_by_priority is only called on load, but if I am not loading but adding to the policy using other api's the sorting order is not maintained.

The bug is resolved if I do a sort before iterating over the policies inside enforce_ex, but that might lead to perf issues ?

I also thought about the same question and I found that in model/policy.py add_policy() will find out whether it is a priority policy and if it is, it will insert it in the correct index. So it will not lead to perf issues. 😀

[aditya-nambiar]

@BustDot yes that makes sense. Or we use some sorted set like datastructure.
Thanks for looking into this!

[aditya-nambiar]

@BustDot sorry I misunderstood your last statement, I thought this is something you were planning to add. But looks like it is already present.

I digged deeper into the add_policy code and feel like there is a bug there. Adding it to the right index is not happening correctly.
I changed the code a bit - https://gist.github.com/aditya-nambiar/b90eb9259d9d6ba2235e5af35eeb17a9

Where I do an actual swap if index added is lower and then it works. Let me know if you want me to file a PR with this change ?

[BustDot]

@aditya-nambiar Sorry for not getting back to you sooner. I think this change will definitely work. Please file a PR.

[aditya-nambiar]

@BustDot PTAL - #313