Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] Confusing result of evaluation of a policy containing an OR not wrapped in parentheses #939

Closed
maxtwardowski opened this issue Dec 16, 2021 · 4 comments · Fixed by #941
Assignees
Labels

Comments

@maxtwardowski
Copy link

maxtwardowski commented Dec 16, 2021

Describe the bug
The enforcer seems to ignore the remaining components of the matcher when one of its evaluated expressions contains an x || y condition. However, it works as intended when the condition is wrapped in parentheses (x || y). The issue is not reproducible in the live editor but I've created a Go repl.it for a simple repro.

To Reproduce

  1. Create a model.conf file with content as follows:
[request_definition]
r = sub, obj, act

[policy_definition]
p = sub_rule, obj_rule, act

[policy_effect]
e = some(where (p.eft == allow))

[matchers]
m = eval(p.sub_rule) && eval(p.obj_rule) && (p.act == '*' || r.act == p.act)
  1. Create a policy.conf file with an expression containing an OR (||) - for simplicity let's just have a dummy || false:
p, r.sub.Role == 'admin' || false, r.obj.ResourceType == 'users', write
  1. Execute the code below - check whether admin can write resource of type definitely NOT users:
package main

import (
	"fmt"
        "embed"

        "github.com/casbin/casbin/v2"
        casbin_fs_adapter "github.com/naucon/casbin-fs-adapter"
        "github.com/pkg/errors"
)

//go:embed model.conf policy.conf
var f embed.FS

type subject struct {
	Role string
}

type object struct {
	ResourceType string

	Role string
}

func main() {
        model, err := casbin_fs_adapter.NewModel(f, "model.conf")
	if err != nil {
		panic(err)
	}
	policies := casbin_fs_adapter.NewAdapter(f, "policy.conf")

	e, err := casbin.NewEnforcer(model, policies)
	if err != nil {
		panic(errors.Wrap(err, "failed to initialize casbin enforcer"))
	}

        s := subject{Role: "admin"}
        o := object{ResourceType: "definitely NOT users"}
        a := "write"

        res, err := e.Enforce(s, o, a)
	if err != nil {
		panic(errors.Wrap(err, "failed to authorize resource access"))
	}

        fmt.Println("enforce res:", res)
}
  1. The result of the Enforce call is true even though it clearly should be false since 'users' != 'definitely NOT users'
  2. Now change the policy so that the condition is wrapped in parentheses i.e.:
    p, (r.sub.Role == 'admin' || false), r.obj.ResourceType == 'users', write
  3. The result of the Enforce is false as expected.

Expected behavior
Parentheses shouldn't matter in this case IMHO - the current behavior is quite confusing especially that it doesn't affect AND conditions.

@casbin-bot
Copy link
Member

@tangyang9464 @closetool @sagilio

@hsluoyz
Copy link
Member

hsluoyz commented Dec 17, 2021

@Abingcbc
Copy link
Member

@maxtwardowski There are some bugs in your reproduction progress.

invalid request size: expected 2, got 3

I will use your example https://replit.com/@MaxTwardowski1/Casbin-OR-bug-repro?v=1 as a test case :)

@maxtwardowski
Copy link
Author

@maxtwardowski There are some bugs in your reproduction progress.

invalid request size: expected 2, got 3

I will use your example https://replit.com/@MaxTwardowski1/Casbin-OR-bug-repro?v=1 as a test case :)

@Abingcbc Oops, I must've pasted some wrong model file... Fixed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants