-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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: add IPatternFuncs #1029
base: master
Are you sure you want to change the base?
feat: add IPatternFuncs #1029
Conversation
@tangyang9464 @closetool @sagilio please review |
@tangyang9464 @JalinWang @imp2002 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, the new matcher uses IsPattern
to pre-check the tokens whether needed to call the Match function to avoid calling duplicate and useless match functions and storing useless match results.
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
Thank you for these changes. Looks good to me too. I have a related question, forgive me if I'm misunderstanding something here btw... If we create a role/domain manager with a max hierarchy of less than the default 10, for example 4. How is that value preserved when the enforcer has hardcoded calls to recreate the role manager with functions like this?
|
And as a follow up clarification question... What's the best flow or best time to customise/recreate the role manager in terms of setting domain matching functions and max hierarchy level? Should it be once we've created the enforcer with our model but before we've done the first initial load of the policy data? |
@kizjig In fact, you must call
|
Thanks. So if I take that approach in your code above, and then afterwards load policy data from disk/file, it will automatically build the role links for the incoming data right (assuming I'm trying to avoid loading/processing the data twice. So I see the flow working as something like...
Does that sound correct? |
@kizjig do like this
Unfortunately, autoBuildRoleLinks is set to true by default when the enforcer is created, so e.EnableAutoBuildRoleLinks(true) has no effect. Maybe we will add a NewEnforcerWithOutBuild API to set autoBuildRoleLinks when creating an enforcer. So for now, we can only do it like the above |
Yes. thanks. That's what I had in mind. I am glad I was on the same page. |
Can we get this change progressed and merged in soon please? |
@abichinger |
@hsluoyz renaming //drm is defaultrolemanager
func (e *Enforcer) SetNamedRoleMatchingFunc(ptype string, isMatch drm.MatchingFunc, isPattern drm.IsPatternFunc) bool
How about I do the following renames: (second solution)
I would prefer the second solution. |
add function SetNamedRoleMatcher add function SetNamedDomainMatcher mark AddNamedMatchingFunc and AddNamedDomainMatchingFunc as deprecated Signed-off-by: Andreas Bichinger <andreas.bichinger@gmail.com>
3ac5fef
to
4256642
Compare
why this one is not merged yet? |
fix: #1027
Changelog
@kizjig please review these changes.
roleManager.AddDomainMatchingFunc("KeyMatch", util.KeyMatch)
needs to be replaced with:Benchmarks
Signed-off-by: Andreas Bichinger andreas.bichinger@gmail.com