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

Better display for errors::Logic #191

Merged
merged 1 commit into from
Nov 26, 2024
Merged

Conversation

divarvel
Copy link
Collaborator

@divarvel divarvel commented Oct 26, 2023

authorization failed: an allow policy matched (policy index: 0), and the following checks failed: [Authorizer(FailedAuthorizerCheck { check_id: 0, rule: "check if right(\"efgh\")" }), Block(FailedBlockCheck { block_id: 1, check_id: 0, rule: "check if operation(\"read\")" })]

@@ -168,9 +171,9 @@ pub enum Signature {
#[derive(Error, Clone, Debug, PartialEq, Eq)]
#[cfg_attr(feature = "serde-error", derive(serde::Serialize, serde::Deserialize))]
pub enum Logic {
#[error("a rule provided by a block is generating facts with the authority or ambient tag, or has head variables not used in its body")]
#[error("a rule provided by a block is producing a fact with unbound variables")]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i wanted to completely remove it, but it's still used. the strange thing is that it explicitly mentions block rules. Where is the error for authorizer rules with unbound variables in facts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

after a bit of digging:

  • unbound variables are checked by the parser when constructing rules, so all rules constructed by the parser will be checked, regardless of their origin, and the error will be a parser error, not a Logic error
  • this error can indeed only happen when a block is added to the authorizer, so adding a broken rule to the authorizer will not be caught here (rules constructed by the parser will be ok, it can only happen for manually constructed rules)
  • rules with unbound variables in head will not generate any fact (but not crash)

I think the ergonomics of this could be improved:

  • by being clearer on invariants that Rule and friends should guarantee, no matter how they were constructed (this would be a bit of work because i think that we should do it in other places as well)
  • for now, by calling validate_variables when adding rules to the authorizer, not only rules blocks

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense, we should avoid those kinds of footguns

@divarvel divarvel changed the title Better display for logic error Better display for logic errors Oct 27, 2023
@divarvel divarvel changed the title Better display for logic errors Better display for errors::Logic Oct 27, 2023
@divarvel divarvel force-pushed the better-display-for-logic-error branch from 04675aa to 61e5f86 Compare November 25, 2024 08:47
@divarvel divarvel changed the base branch from main to v5 November 25, 2024 08:47
Copy link

codspeed-hq bot commented Nov 25, 2024

CodSpeed Performance Report

Merging #191 will not alter performance

Comparing better-display-for-logic-error (eb2a7c4) with v5 (6ad6f12)

Summary

✅ 12 untouched benchmarks

Copy link

codecov bot commented Nov 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.61%. Comparing base (6ad6f12) to head (eb2a7c4).
Report is 2 commits behind head on v5.

Additional details and impacted files
@@            Coverage Diff             @@
##               v5     #191      +/-   ##
==========================================
+ Coverage   65.57%   65.61%   +0.03%     
==========================================
  Files          27       27              
  Lines        6987     6994       +7     
==========================================
+ Hits         4582     4589       +7     
  Misses       2405     2405              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@divarvel divarvel marked this pull request as ready for review November 25, 2024 09:13
@divarvel divarvel force-pushed the better-display-for-logic-error branch 6 times, most recently from b28199c to c1eef6d Compare November 25, 2024 14:28
@divarvel divarvel force-pushed the better-display-for-logic-error branch from c1eef6d to eb2a7c4 Compare November 25, 2024 14:36
@divarvel divarvel merged commit 255abf2 into v5 Nov 26, 2024
7 checks passed
@divarvel divarvel deleted the better-display-for-logic-error branch November 26, 2024 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants