casbin4d / Casbin4D

An authorization library that supports access control models like ACL, RBAC, ABAC in Delphi
https://casbin.org
Apache License 2.0
44 stars 16 forks source link

Why use Random*100 as a replacement? #17

Closed ErikUniformAgri closed 2 years ago

ErikUniformAgri commented 2 years ago

Some times the result of an enforcement is wrong. When I enforce a request on a simple RBAC model and there is no matching policy for my request. I still get result = true

You can see in the image botch "DELETE2" and "ADD" are replaced by 93. Then the string will match wrongly Logging

Why is there a "Random*100" in TMatcher.addIdentifier? (And why does it have to be replaced anyway?) Now there is a 1% change the result is wrong and if I have 1000+ policys the result is almost guaranteed wrong.

If you run the tests several times you will see some tests will fail random

hsluoyz commented 2 years ago

@jkour

jkour commented 2 years ago

Some times the result of an enforcement is wrong. When I enforce a request on a simple RBAC model and there is no matching policy for my request. I still get result = true

I noticed that issue with the test too.

You can see in the image botch "DELETE2" and "ADD" are replaced by 93. Then the string will match wrongly Logging

Now I can see why this happens. Your screenshot helps a lot. Thanks

Why is there a "Random*100" in TMatcher.addIdentifier? (And why does it have to be replaced anyway?) The expression evaluator only works with numbers. It is a math expression parser. The comparison operators (e.g., >, <, etc.) work with strings too but all the rest do not. For example, you can not do not (user = root).

Therefore, I replace the tags with numbers.

The issue I think appears because the random function generates the same number. We need a different way to achieve the same result.

The obvious workaround is to check whether a number has already been used. I do not know if it will be efficient enough though. I am thinking that if you have thousands of policies, it may become a data mining problem.

Now there is a 1% change the result is wrong and if I have 1000+ policys the result is almost guaranteed wrong. No, this is unacceptable. We need to fix it

I'll have a look soon.

ErikUniformAgri commented 2 years ago

Maybe a simple solution is to generate a number between 0 and 1.000.000 or an other big number. To exclude a number is already taken you should make a list of user numbers and check if the generated number is already in the list.

I noticed chaning 100 to 1000000 did work for this case, but made some other tests fail

jkour commented 2 years ago

I rewrote some parts in Matcher.

Can you fork Random_Numbers and try it? In my run, tests do not fail anymore

ErikUniformAgri commented 2 years ago

It looks like it works fine. In my previous test I thought some tests failed when the tag number exceeds 100 (of 10000 or a high number). But even when I put fPass to 100 instead of 0, all tests seem to work.

But the solution could be more siple. Now the numbers are not random, but consecutive. That is a good thing, but you can achieve the same by creating a variable like fNextId instead of the fPass and fIDValues. Everytime you add a tag you just increment the value by 1

jkour commented 2 years ago

@ErikUniformAgri Yes, you are right. I implemented it this way. Thanks.