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

Explain enforcement in log #425

Merged
merged 1 commit into from
May 5, 2020
Merged

Explain enforcement in log #425

merged 1 commit into from
May 5, 2020

Conversation

dovics
Copy link
Member

@dovics dovics commented Apr 16, 2020

For #355
Explain why an enforcement was or wasn't satisfied.Controlled by autoExplain in enforce,Print the policy related to the result to the log.

such as some(where (p.eft == allow)), the explanation will be all allowed policy.

whether it hits depends on user selected effect. User can modify it by interface EffectorEx

@coveralls
Copy link

coveralls commented Apr 16, 2020

Coverage Status

Coverage increased (+1.5%) to 73.322% when pulling 5585d2a on dovics:explian into 907df79 on casbin:master.

@hsluoyz
Copy link
Member

hsluoyz commented Apr 16, 2020

I think logs are not very friendly for users. They want to return matched rules:

In some scenarios it would be useful to have a way to explain why an enforcement was or wasn't satisfied.
The simplest idea could be to just inform of the matched rules that allowed satisfying (or not) the model.

Can we add a function called EnforceEx(), which is similar to Enforce(), but returns matched rule(s) additionally?

@dovics
Copy link
Member Author

dovics commented Apr 16, 2020

what do you think of changing the signature of enforce()

func (e *Enforcer) enforce(matcher string, rvals ...interface{}) (ok bool,explains [][]string,  err error)

or

func (e *Enforcer) enforce(matcher string, explains *[][]string, rvals ...interface{}) (ok bool, err error)

otherwise we need to split enforce(), I think it is difficult to do

@dovics
Copy link
Member Author

dovics commented Apr 17, 2020

@hsluoyz,@nodece, I try to add EnforceEx() and EnforceExWithMatch().

I think it is better to return all the policies related to the result. And the result and hitPolicies are returned directly in the return value

effect/default_effector.go Outdated Show resolved Hide resolved
@nodece
Copy link
Member

nodece commented Apr 18, 2020

Thanks @dovics , Must approve.

@nodece nodece requested a review from hsluoyz April 18, 2020 09:02
Copy link
Member

@hsluoyz hsluoyz left a comment

Choose a reason for hiding this comment

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

image

effect/default_effector.go Outdated Show resolved Hide resolved
@dovics dovics requested a review from hsluoyz April 22, 2020 12:48
@nodece
Copy link
Member

nodece commented May 5, 2020

@hsluoyz Is there any progress?

Copy link
Member

@hsluoyz hsluoyz 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 33a2f4e into casbin:master May 5, 2020
@github-actions
Copy link

github-actions bot commented May 7, 2020

🎉 This PR is included in version 2.4.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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.

None yet

4 participants