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

fix: *matcher result should be boolean or number* for KeyGet2 #347

Merged
merged 4 commits into from
Mar 2, 2022

Conversation

Shivansh-yadav13
Copy link
Member

@Shivansh-yadav13 Shivansh-yadav13 commented Feb 12, 2022

Fix: #332

  • added a case for result type string.
  • please suggest changes if required.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1833860796

  • -4 of 4 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 77.173%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/coreEnforcer.ts 0 4 0.0%
Totals Coverage Status
Change from base Build 1746426510: -0.2%
Covered Lines: 1323
Relevant Lines: 1644

💛 - Coveralls

@coveralls
Copy link

coveralls commented Feb 12, 2022

Pull Request Test Coverage Report for Build 1919832746

  • 4 of 5 (80.0%) changed or added relevant lines in 1 file are covered.
  • 4 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.07%) to 77.45%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/coreEnforcer.ts 4 5 80.0%
Files with Coverage Reduction New Missed Lines %
src/util/util.ts 4 78.08%
Totals Coverage Status
Change from base Build 1746426510: 0.07%
Covered Lines: 1328
Relevant Lines: 1645

💛 - Coveralls

@casbin-bot
Copy link
Member

@Gabriel-403 @Zxilly @kingiw @nodece please review

@Shivansh-yadav13 Shivansh-yadav13 changed the title Fix: *matcher result should be boolean or number* for KeyGet2 fix: *matcher result should be boolean or number* for KeyGet2 Feb 12, 2022
@Shivansh-yadav13 Shivansh-yadav13 changed the title fix: *matcher result should be boolean or number* for KeyGet2 fix: *matcher result should be boolean or number* Feb 20, 2022
@hsluoyz
Copy link
Member

hsluoyz commented Feb 20, 2022

@nodece @Zxilly plz review

@hsluoyz
Copy link
Member

hsluoyz commented Feb 20, 2022

@Shivansh-yadav13

  1. Don't fix two bugs in one PR. Separate into two PRs.
  2. AT the issue sender to review

@Shivansh-yadav13
Copy link
Member Author

@tomfriedhof can you please review and suggest changes if required.

src/coreEnforcer.ts Outdated Show resolved Hide resolved
@grzegorz-kap
Copy link

grzegorz-kap commented Feb 21, 2022

@Shivansh-yadav13 , when I was debugging it the value of 'result' was undefined.

In our implementation, we have multiple instances of Casbin enforcer.

Steps to reproduce:

  1. Create a new Enforce with an empty permission set and call enforce
  2. Create a new Enforce with some permissions and call enforce (an error is thrown: matcher result should be boolean or number - result is undefined)
  3. Repeat step 2 (no error)

The issue does not occur if step 1 has some permissions

model:

[request_definition]
r = sub, dom, obj, act, v

[policy_definition]
p = sub, dom, obj, act, exp

[role_definition]
g = _, _, _

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

[matchers]
m = g(r.sub, p.sub, r.dom) && r.dom == p.dom && r.obj == p.obj && r.act == p.act && eval(p.exp)

An example of 'exp' from the policy_defintion
(r.v.office=='london'||r.v.office=='warsaw')&&(r.v.floor=='first')

@Shivansh-yadav13
Copy link
Member Author

This is what happening

const enforcer1 = await newEnforcer('examples/test_model.conf');
await enforcer1.enforce('sub', 'dom', '/data1', 'read', { office: 'london', floor: 'first' });

const enforcer2 = await newEnforcer('examples/test_model.conf', 'examples/test_policy.csv');
await enforcer2.enforce('sub', 'dom', '/data1', 'read', { office: 'london', floor: 'first' });

Error --> matcher result should be boolean or number

const enforcer1 = await newEnforcer('examples/test_model.conf', 'examples/test_policy.csv');
await enforcer1.enforce('sub', 'dom', '/data1', 'read', { office: 'london', floor: 'first' });

const enforcer2 = await newEnforcer('examples/test_model.conf');
await enforcer2.enforce('sub', 'dom', '/data1', 'read', { office: 'london', floor: 'first' });

No Error

const enforcer1 = await newEnforcer('examples/test_model.conf');
await enforcer1.enforce('sub', 'dom', '/data1', 'read', { office: 'london', floor: 'first' });

const enforcer2 = await newEnforcer('examples/test_model.conf');
await enforcer2.enforce('sub', 'dom', '/data1', 'read', { office: 'london', floor: 'first' });

No Error

@Shivansh-yadav13
Copy link
Member Author

Shivansh-yadav13 commented Feb 27, 2022

Steps to reproduce:

  1. Create a new Enforce with an empty permission set and call enforce
  2. Create a new Enforce with some permissions and call enforce (an error is thrown: matcher result should be boolean or number - result is undefined)
  3. Repeat step 2 (no error)

@grzegorz-kap Ig the error you are facing is resolved after #348, can you please confirm.

@Shivansh-yadav13 Shivansh-yadav13 changed the title fix: *matcher result should be boolean or number* fix: *matcher result should be boolean or number* for KeyGet2 Feb 27, 2022
@nodece
Copy link
Member

nodece commented Mar 1, 2022

Could you add unit tests to cover this case?

Copy link
Contributor

@Zxilly Zxilly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@hsluoyz hsluoyz merged commit 0257078 into casbin:master Mar 2, 2022
github-actions bot pushed a commit that referenced this pull request Mar 2, 2022
## [5.13.2](v5.13.1...v5.13.2) (2022-03-02)

### Bug Fixes

* *matcher result should be boolean or number* for KeyGet2 ([#347](#347)) ([0257078](0257078))
@github-actions
Copy link

github-actions bot commented Mar 2, 2022

🎉 This PR is included in version 5.13.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Zxilly added a commit that referenced this pull request Apr 15, 2022
* fix: *matcher result should be boolean or number* for KeyGet2 (#347)

* fix: *matcher result should be boolean or number* for  KeyGet2

* fix: updated error hint

* fix: basic keyGet2 test

* fix: basic_keyget2_policy.csv

(cherry picked from commit 0257078)
Signed-off-by: Zxilly <zhouxinyu1001@gmail.com>

* feat(rbac): add `getUsersForRoleInDomain` & `getRolesForUserInDomain` (#351)

Documentation indicates that there is existance of `getUsersForRoleInDomain` and
`getRolesForUserInDomain` api, but it does not yet exists. This commit implements these functions,
by aliasing them to existing rbac function that had already cater for domain apis, to increase and
improve dev experience.

re #304

(cherry picked from commit 4896ca2)

* feat: #357 Support keyMatch5 (#359)

(cherry picked from commit e6a6d8a)

* test: use new utils

Signed-off-by: Zxilly <zhouxinyu1001@gmail.com>

Co-authored-by: Shivansh Yadav <yadavshivansh@gmail.com>
Co-authored-by: Chen Wen Kang <23054115+cwkang1998@users.noreply.github.com>
Co-authored-by: ZCDC_Ren <kuanglong0312@sina.com>
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 this pull request may close these issues.

keyGet2 always returns *matcher result should be boolean or number*
7 participants