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(#210): allowed value log in response policy evaluation #212

Merged
merged 8 commits into from
Jun 30, 2023

Conversation

fredmaggiowski
Copy link
Member

@fredmaggiowski fredmaggiowski commented Jun 28, 2023

Allowed documentation states:

Allowed is a helper method that'll return true if all of these conditions hold: - the result set only has one element - there is only one expression in the result set's only element - that expression has the value true - there are no bindings.

If bindings are present, this will yield false: it would be a pitfall to return true for a query like data.authz.allow = x, which always has result set element with value true, but could also have a binding x: false.

Since the policy is actually returning the new body it is expected to return a single expression with value different from true

To verify:

  • policy failure logs false
  • differentiate somehow between Evaluation for response and evaluation for request; maybe replicate the Allowed function but with a type assertion on boolean expression

Note:

The verifyAllowed function is supposed to be an internal utility only for logging purposes; I would not use it to decide what to return since it is too lax, I'd still rely on the results.Allowed func for that until a better solution is found.

@fredmaggiowski fredmaggiowski changed the title fix(#210): allowed value simplification fix(210): allowed value simplification Jun 28, 2023
@fredmaggiowski fredmaggiowski changed the title fix(210): allowed value simplification fix(#210): allowed value simplification Jun 28, 2023
@coveralls
Copy link

Pull Request Test Coverage Report for Build 5403545644

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 84.363%

Totals Coverage Status
Change from base Build 5401248238: 0.0%
Covered Lines: 1775
Relevant Lines: 2104

💛 - Coveralls

@fredmaggiowski fredmaggiowski added the bug Something isn't working label Jun 28, 2023
Copy link
Member

@davidebianchi davidebianchi left a comment

Choose a reason for hiding this comment

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

LGTM

@fredmaggiowski
Copy link
Member Author

LGTM

Please review it again, it wasn't complete and actually bugged

@fredmaggiowski fredmaggiowski marked this pull request as ready for review June 28, 2023 18:34
core/sdk_test.go Outdated Show resolved Hide resolved
core/opaevaluator.go Outdated Show resolved Hide resolved
Co-authored-by: Davide Bianchi <10374360+davidebianchi@users.noreply.github.com>
@fredmaggiowski fredmaggiowski changed the title fix(#210): allowed value simplification fix(#210): allowed value log in response policy evaluation Jun 29, 2023
@fredmaggiowski fredmaggiowski merged commit d6af895 into main Jun 30, 2023
6 checks passed
@fredmaggiowski fredmaggiowski deleted the fix/allowed-log-in-response-policy branch June 30, 2023 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The policy evaluation completed log is incorrect for response evaluation
3 participants