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: field_index is incorrect in RBAC with domains mode (#345) #346

Merged

Conversation

truc0
Copy link
Contributor

@truc0 truc0 commented Jun 11, 2024

Short description

get_all_named_{things} functions (things can be actions, subjects, roles) uses hard-coded index for getting "things" from policies, which causes wrong result when using RBAC with domains mode (which a new field domain is inserted to the index 1).

The PR does:

  • use Model.get_field_index for getting correct field index when getting the actions, subjects
  • a new testcase is added

Details

The root cause

get_all_named_{things} functions (things can be actions, subjects, roles) use fixed field index while fetching the corresponding resources, which causes incorrect behavior when the field indexes change.

According to the documentation, the RBAC mode with domains have the convenient putting domain as the second param of a policy, while the subject is located at the second param (field_index=1) in other mode.

Note: Conventionally, the domain token name in policy definition is dom and is placed as the second token (sub, dom, obj, act).

Solution of this PR

To make pycasbin works correctly, the field_index of "things"(actions, subjects, roles) should be determined from the config file (instead of hard-coded). The Model.get_field_index method is used in this PR for getting correct index from config.

What's more

Changes to documentation

Now pycasbin can detect the position of dom and act, there's no need to explicitly set the field index of dom using set_field_index API.

Similar situation

Now get_all_named_roles is still using hard-coded field index (but it won't fail in RBAC with domains mode), this can be optimized using similar API.

Close #345

@casbin-bot
Copy link
Member

@Nekotoxin please review

@casbin-bot casbin-bot requested a review from Nekotoxin June 11, 2024 15:21
@CLAassistant
Copy link

CLAassistant commented Jun 11, 2024

CLA assistant check
All committers have signed the CLA.

@hsluoyz
Copy link
Member

hsluoyz commented Jun 11, 2024

@leeqvip plz review

@truc0 truc0 requested a review from leeqvip June 11, 2024 16:07
@leeqvip leeqvip merged commit 9f6a379 into casbin:master Jun 11, 2024
30 checks passed
github-actions bot pushed a commit that referenced this pull request Jun 11, 2024
## [1.36.2](v1.36.1...v1.36.2) (2024-06-11)

### Bug Fixes

* field_index is incorrect in RBAC with domains mode ([#345](#345)) ([#346](#346)) ([9f6a379](9f6a379))
Copy link

🎉 This PR is included in version 1.36.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@truc0 truc0 deleted the fix-345-get-all-objects-failed-with-rbac-domain branch June 11, 2024 16:34
@truc0 truc0 restored the fix-345-get-all-objects-failed-with-rbac-domain branch June 11, 2024 16:34
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.

RBAC with Domain model ,get_all_subjects error
5 participants