fix: abac not using policy

[Abingcbc]

Signed-off-by: abingcbc abingcbc626@gmail.com

Fix: #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

[nodece]

LGTM

[sagilio]

Can you provide a more reasonable test case? I try to remove the matcher check and run the test, it returns another error:

Then I try to add eft to the abac_model.conf, the new test is passed:

I think your changes have nothing to do with the error in the issue.

[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 are right，we need not to scan all policies when matcher is not included rtype. But this just is a enhancment about performance. I do not know why the wrong result be 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 #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

[sagilio]

LGTM

[github-actions]

🎉 This PR is included in version 2.39.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[harshalmittal4]

Hi @Abingcbc , @hsluoyz , @sagilio
I was seeking to understand the abac policy and read the discussion above and at #917, and thought the request must not depend on policy file for a model like below, i.e. it must be allowed everytime the condition r.sub == r.obj.Owner holds. Is this correct?

model -

[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

I used request as alice, { Owner: 'alice'}, write with the above model and the below policy in online editor -

p, alice, /data1, read, deny
p, alice, /data1, write, allow
p, bob, /data2, write, allow
p, bob, /data2, read, allow

and got the result as false.

On replacing p, alice, /data1, read, deny in policy file with p, alice, /data1, read, allow (so that all eft in policy become allow), the result changes to true.

Is this the expected behaviour for the above case that the request depends on policy file instead of just relying on matcher m = r.sub == r.obj.Owner?
Thanks

[hsluoyz]

@harshalmittal4 the online editor is pure frontend (https://github.com/casbin/casbin-editor), which uses Node-Casbin: https://github.com/casbin/node-casbin for Casbin enforcement. Usually we have latest features in Golang version Casbin and Node-Casbin maybe still doesn't have the feature of this PR: #918 yet.

Issue proposed here: apache/casbin-node-casbin#433

[harshalmittal4]

Thanks @hsluoyz , got it. I'm currently getting more familiar with Casbin and would like to try contributing to some issues once I get comfortable