Repository: casbin/casbin
Pull request #918 (2021-11-16 03:26:04)

fix: abac not using policy
Signed-off-by: abingcbc <abingcbc626@gmail.com>

Fix: https://github.com/casbin/casbin/issues/917

## How it works

Ignore all policies when matcher doesn't contain any match related to policy.

casbin-bot:  @tangyang9464 @closetool @sagilio please review
hsluoyz: @nodece plz review
hsluoyz: @silverspace plz review
Abingcbc: > Can you provide a more reasonable test case? I try to remove the matcher check and run the test, it returns another error: ![image](https://user-images.githubusercontent.com/42855245/142418159-f810ed85-0f03-4e12-9b5e-abe664ae3d2b.png)

@sagilio sa I don't understand what you mean, can you describe in more detail？
Abingcbc: @sagilio Yep. I realized this error when  I wrote the test. But if the matcher doesn't use policy, no policy should be matched for the request. So no matter what policy definitions look like,  policy should have no effect on the request?
sagilio: > @sagilio Yep. I realized this error when I wrote the test. But if the matcher doesn't use policy, no policy should be matched for the request. So no matter what policy definitions look like, policy should have no effect on the request?

Yeah, you changes is right，we need not scan.all policies when matcher is not include rtype. But this just a enhance about performance. I do not know why the wrong result returned in #917.
Abingcbc: > > @sagilio Yep. I realized this error when I wrote the test. But if the matcher doesn't use policy, no policy should be matched for the request. So no matter what policy definitions look like, policy should have no effect on the request?
> 
> Yeah, you changes is right，we need not scan.all policies when matcher is not include rtype. But this just a enhance about performance. I do not know why the wrong result returned in #917.

I think the root cause is that `policy` is for other model like `rbac` and the request's model is `abac`.

When the matcher doesn't use any policy and the request meets the judgment of the matcher, the request will match all policies because the matcher always returns true.
Therefore, if there is `deny` policy, the result of merged effects will be `deny`.

Take https://github.com/casbin/casbin/issues/917 as an example, request:
```
{ User: 'alice', Superuser: true }, { Owner: 'bob'}, read
```

will match policy

```
p, foobar, /blah, read, deny
```

and the merged effect is `deny`, although it is allowed in respect to `abac`.



sagilio: @Abingcbc I see, you are right. We may need a more effective test case to descript the situation.
Abingcbc: > @Abingcbc I see, you are right. We may need a more effective test case to descript the situation.

@sagilio I have updated the test case. PTAL ❤️
hsluoyz: @sagilio plz review

Patch:
diff --git a/enforcer.go b/enforcer.go
index 62d6288..cf2011f 100644
--- a/enforcer.go
+++ b/enforcer.go
@@ -17,6 +17,7 @@ package casbin
 import (
 	"errors"
 	"fmt"
+	"strings"
 
 	"github.com/Knetic/govaluate"
 	"github.com/casbin/casbin/v2/effector"
@@ -550,7 +551,7 @@ func (e *Enforcer) enforce(matcher string, explains *[]string, rvals ...interfac
 	var effect effector.Effect
 	var explainIndex int
 
-	if policyLen := len(e.model["p"][pType].Policy); policyLen != 0 {
+	if policyLen := len(e.model["p"][pType].Policy); policyLen != 0 && strings.Contains(expString, pType + "_"){
 		policyEffects = make([]effector.Effect, policyLen)
 		matcherResults = make([]float64, policyLen)
 
diff --git a/examples/abac_not_using_policy_model.conf b/examples/abac_not_using_policy_model.conf
new file mode 100644
index 0000000..64c708a
--- /dev/null
+++ b/examples/abac_not_using_policy_model.conf
@@ -0,0 +1,11 @@
+[request_definition]
+r = sub, obj, act
+
+[policy_definition]
+p = sub, obj, act, eft
+
+[policy_effect]
+e = some(where (p.eft == allow)) && !some(where (p.eft == deny))
+
+[matchers]
+m = r.sub == r.obj.Owner
\ No newline at end of file
diff --git a/examples/abac_rule_effect_policy.csv b/examples/abac_rule_effect_policy.csv
new file mode 100644
index 0000000..bea9962
--- /dev/null
+++ b/examples/abac_rule_effect_policy.csv
@@ -0,0 +1,4 @@
+p, alice, /data1, read, deny
+p, alice, /data1, write, allow
+p, bob, /data2, write, deny
+p, bob, /data2, read, allow
\ No newline at end of file


Test patch:
diff --git a/model_test.go b/model_test.go
index 89ec206..c0810f7 100644
--- a/model_test.go
+++ b/model_test.go
@@ -549,6 +549,17 @@ func newTestSubject(name string, age int) testSub {
 	return s
 }
 
+func TestABACNotUsingPolicy(t *testing.T) {
+	e, _ := NewEnforcer("examples/abac_not_using_policy_model.conf", "examples/abac_rule_effect_policy.csv")
+	data1 := newTestResource("data1", "alice")
+	data2 := newTestResource("data2", "bob")
+
+	testEnforce(t, e, "alice", data1, "read", true)
+	testEnforce(t, e, "alice", data1, "write", true)
+	testEnforce(t, e, "alice", data2, "read", false)
+	testEnforce(t, e, "alice", data2, "write", false)
+}
+
 func TestABACPolicy(t *testing.T) {
 	e, _ := NewEnforcer("examples/abac_rule_model.conf", "examples/abac_rule_policy.csv")
 	m := e.GetModel()