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
feat: support domain matching when getting permissions #992
Conversation
@tangyang9464 @closetool @sagilio please review |
@sasakiyori @tangyang9464 plz review |
@Abingcbc I have an idea, why not just use
|
@tangyang9464 Thx for your idea! I will refactor this PR. BTW what do you mean by
|
@Abingcbc I wanted to combine the two methods before, but now I think it's inappropriate, please ignore. |
@tangyang9464 PTAL |
@sasakiyori @tangyang9464 plz review |
@tangyang9464 plz review |
rbac_api.go
Outdated
if matched { | ||
// Copy rule to prevent changing policies through returned slice | ||
newRule := make([]string, len(rule)) | ||
copy(newRule, rule) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will the rule change affect the original policy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, the rule
is a slice. If users update the value in returned slice, the policy will also be changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should also check if this bug happens anywhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, You're right. Could you do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will fix this issue in a new PR
@tangyang9464 plz review |
@tangyang9464 @hsluoyz plz review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
🎉 This PR is included in version 2.50.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
@Abingcbc @tangyang9464 Hi, I don't think this PR has solved this problem totally. policy
test code e, _ = NewEnforcer("examples/rbac_with_domain_pattern_model.conf", "examples/rbac_with_domain_pattern_policy.csv")
e.AddNamedDomainMatchingFunc("g", "KeyMatch", util.KeyMatch)
res, _ := e.GetRolesForUser("bob", "*")
t.Log(res)
res, _ = e.GetRolesForUser("bob", "domain1")
t.Log(res)
res, _ = e.GetRolesForUser("bob", "domain2")
t.Log(res) expected result: /cc @hsluoyz |
} | ||
} else { | ||
for _, d := range domain { | ||
matched := rm.(*defaultrolemanager.RoleManager).Match(d, rule[domainIndex]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's more, we can't assume the RoleManager
is the default one. And this dependency should not be introduced into Enforcer
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, we may refactor from the first imported dependency
Lines 730 to 745 in 9b43426
func (e *Enforcer) AddNamedMatchingFunc(ptype, name string, fn defaultrolemanager.MatchingFunc) bool { | |
if rm, ok := e.rmMap[ptype]; ok { | |
rm.(*defaultrolemanager.RoleManager).AddMatchingFunc(name, fn) | |
return true | |
} | |
return false | |
} | |
// AddNamedDomainMatchingFunc add MatchingFunc by ptype to RoleManager | |
func (e *Enforcer) AddNamedDomainMatchingFunc(ptype, name string, fn defaultrolemanager.MatchingFunc) bool { | |
if rm, ok := e.rmMap[ptype]; ok { | |
rm.(*defaultrolemanager.RoleManager).AddDomainMatchingFunc(name, fn) | |
return true | |
} | |
return false | |
} |
An interface may be better than struct.
I think we should modify the code of // load or create a RoleManager instance of domain
func (dm *DomainManager) getRoleManager(domain string, store bool) *RoleManagerImpl {
var rm *RoleManagerImpl
var ok bool
if rm, ok = dm.load(domain); !ok {
rm = newRoleManagerWithMatchingFunc(dm.maxHierarchyLevel, dm.matchingFunc)
if store {
dm.rmMap.Store(domain, rm)
}
if dm.domainMatchingFunc != nil {
dm.rmMap.Range(func(key, value interface{}) bool {
domain2 := key.(string)
rm2 := value.(*RoleManagerImpl)
if domain != domain2 && dm.Match(domain, domain2) {
rm.copyFrom(rm2)
}
return true // change to append()
})
}
}
return rm
} |
Sure, this PR only adds domain matching when getting permissions. And |
fix: #969
How it works
GetImplicitPermissionsForUser
depends onGetFilteredPolicies
to query permissions. But whenenforcer
gets filtered policies, it doesn't support domain matching.So there are main changes in this PR:
match
of role manager public and reuse this function for domain matching when getting policies.GetFilteredPolicyWithDomainMatching
for the user requirements mentioned in[Question] Get implicit permissions with pattern-matching in RBAC don't work #969.