8000 fix: HasLink() couldn't get a correct result when RoleManager has MatchingFunc by uran0sH · Pull Request #702 · casbin/casbin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: HasLink() couldn't get a correct result when RoleManager has MatchingFunc #702

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 15, 2021
Merged

fix: HasLink() couldn't get a correct result when RoleManager has MatchingFunc #702

merged 1 commit into from
Feb 15, 2021

Conversation

uran0sH
Copy link
Contributor
@uran0sH uran0sH commented Feb 4, 2021

fix: #701

@uran0sH uran0sH mentioned this pull request Feb 4, 2021
Closed
@hsluoyz
Copy link
Member
hsluoyz commented Feb 4, 2021

@RobotHuang plz fix CI:

image

@hsluoyz
Copy link
Member
hsluoyz commented Feb 4, 2021

@abichinger please review.

@abichinger
Copy link
Member
abichinger commented Feb 4, 2021 •
edited
Loading

LGTM, although the linter is not happy

@hsluoyz Should the RoleManager with MatchingFunc support one-to-many links?

For example, what is the expected result of:

rm.AddLink("u1", "g\\d+")
rm.HasLink("u1", "g1") // true?
rm.HasLink("u1", "g2") // true?

The current result is false both times

8000
@uran0sH uran0sH force-pushed the master branch 2 times, most recently from b414616 to 9f377f1 Compare February 4, 2021 21:40
hsluoyz
hsluoyz requested changes Feb 5, 2021
View reviewed changes
@uran0sH
Copy link
Contributor Author
uran0sH commented Feb 5, 2021

@abichinger now we don't support this, and we must add role g1 to check whether u1 and g1 has a link. We could discuss if we should add this feat in the future.

@hsluoyz hsluoyz mentioned this pull request Feb 6, 2021
Merged
hsluoyz
hsluoyz requested changes Feb 9, 2021
View reviewed changes
@uran0sH uran0sH force-pushed the master branch 2 times, most recently from 15b334c to 1193861 Compare February 10, 2021 03:05
@uran0sH
Copy link
Contributor Author
uran0sH commented Feb 10, 2021

@abichinger @hsluoyz now support one-to-many links. plz review.

@hsluoyz
Copy link
Member
hsluoyz commented Feb 10, 2021

@abichinger

abichinger
abichinger suggested changes Feb 10, 2021
View reviewed changes
Copy link
Member
@abichinger abichinger left a comment

Choose a reason for hiding this comment

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

The current implementation creates unwanted links.

Example:

_ = rm.Clear()

_ = rm.AddLink("u1", "g\\d+")
testRole(t, rm, "g1", "g2", false)

Output:

role_manager_test.go:301: g1, g2: true
role_manager_test.go:301: g1 < g2: true, supposed to be false
FAIL

@RobotHuang Please have a look at the pycasbin implementation of the RoleManager.
I think it would be good to keep the implementations close to each other.

@uran0sH
Copy link
Contributor Author
uran0sH commented Feb 11, 2021

@abichinger fine, I will see how pycasbin implements, and it takes some time to change the implementation in casbin.

@uran0sH
Copy link
Contributor Author
uran0sH commented Feb 11, 2021

@abichinger Does pycasbin support one-to-many links? I change the code in casbin and it doesn't support one-to-many links.

@abichinger
Copy link
Member

@RobotHuang yes it does
https://github.com/casbin/pycasbin/blob/382546bd9ff5c82da2a9003e2cb6c9917c2495d6/tests/rbac/test_role_manager.py#L171

@uran0sH uran0sH force-pushed the master branch 4 times, most recently from 6075326 to 54e79a8 Compare February 12, 2021 15:25
@uran0sH
Copy link
Contributor Author
uran0sH commented Feb 12, 2021

@abichinger @hsluoyz plz review.

abichinger
abichinger suggested changes Feb 12, 2021
View reviewed changes
@uran0sH uran0sH requested a review from hsluoyz February 13, 2021 02:28
@uran0sH
fix: HasLink() couldn't get a correct result when RoleManager has Mat…
74e9839 
…chingFunc

Signed-off-by: RobotHuang <1183598761@qq.com>

feat: HasLink() with MatchingFunc supports one-to-many links

Signed-off-by: RobotHuang <1183598761@qq.com>
@hsluoyz
Copy link
Member
hsluoyz commented Feb 14, 2021

@abichinger

abichinger
abichinger approved these changes Feb 14, 2021
View reviewed changes
Copy link
Member
@abichinger abichinger left a comment

Choose a reason for hiding this comment

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

Looks good

@hsluoyz hsluoyz merged commit 66e5978 into casbin:master Feb 15, 2021
@github-actions
Copy link

🎉 This PR is included in version 2.23.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@sagilio sagilio mentioned this pull request Aug 6, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hsluoyz hsluoyz hsluoyz approved these changes

@abichinger abichinger abichinger approved these changes

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RoleManager with MatchingFunc depends on order of link creation.
3 participants
@uran0sH @hsluoyz @abichinger
0