feat: detect free variables in rule expressions #179
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Geal
approved these changes
Aug 7, 2023
|
It would be nice to merge the implementations, but there might be issues with trait implementation? Maybe if keeping the trait implementations in biscuit-auth |
|
Maybe we can keep the biscuit-auth types as newtypes if we want to avoid
orphan instances. (It depends on where the traits are defined ofc).
|
|
Are you ok with the error span being on the whole rule instead of just the head for head variables? Also for checks & policies, it will require a bit of work to collect scope errors in all rule bodies without aborting on the first rule with free variables. |
divarvel
commented
Aug 29, 2023
| @@ -328,33 +328,38 @@ impl Rule { | |||
| } | |||
|
|
|||
| pub fn validate_variables(&self) -> Result<(), String> { | |||
There was a problem hiding this comment.
this would need to return a 3-valued enum (Head, Body, Both) to signal where the free variables were found, in order to only return the relevant span in the parsing error.
divarvel
added a commit
that referenced
this pull request
May 24, 2024
This only happens during evaluation. Earlier checks depend on changes from #179
Free variables are already forbidden in rule heads, but not in rule expressions.
divarvel
force-pushed
the
forbid-free-variables
branch
from
a2ddd67
to
b64cbab
Compare
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Free variables are already forbidden in rule heads, but not in rule expressions.
Open questions
The reported error span was the rule head when we were looking for free variables in the rule head. Now free variables can be found in the whole expression, so for now the whole expression is reported as the error span, which is not perfect.
The ideal solution would be to have multiple errors, one per free variable instance, with the span being only the variable itself. That may be hard to achieve, so maybe just being able to keep the expression span (or the rule head span) would be good, but that's still quite a bit of work.
I noticed a fair amount of duplication between the
buildermodule inbiscuit-parserand the one inbiscuit-auth. For instance,validate_variablesfrom biscuit-auth is not used anywhere, only the one frombiscuit-parser. Could thebiscuit-authbuildermodule be significantly reduced to use types and functions frombiscuit-parser?ToDo