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

Allow reauthorization with also initially undiscovered unknowns in the Expr #1466

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

luxas
Copy link

@luxas luxas commented Feb 12, 2025

Description of changes

Fixes: #1465
Please read the issue description first for context.

There might be "undiscovered" unknowns in a partially evaluated Expr residual. In my example in #1465, that would be resource.newlabels that is during the first partial evaluation call omitted and thus unknown, but not yet evaluated by Cedar into a unknown("resource.newlabels"), as the LHS of the && didn't proceed to evaluate the RHS (#1445).

In the issue I described three quick potential solutions, of which this one probably is the most realistic to actually implement.

This PR solves the described issue by moving the reauthorization substitution logic from expr.substitute to partial_interpret, where unknowns can be looked up any number of times needed, also while proceeding to the RHS of an && (of course, given the LHS evaluated to true). The code isn't super neat, having to pass the mappings around everywhere, I'm definitely open to discuss neater solutions.

Note: Expr.substitute is now almost unused; the only place I saw it used is in Context.substitute, which indeed is used.

Longer-term, indeed RFC 95 is the better solution, addressing this bug and probably a whole class of others in one go. However, I wanted to open this PR to show the approach, as at the end of the day, the fix wasn't very complicated (and hopefully I didn't break anything else).

If this becomes an accepted approach to go forward in the short term, I'll make tests, etc. so this PR becomes ready to merge.

Checklist for requesting a review

The change in this PR is (choose one, and delete the other options):

  • A change/bug fix "invisible" to users (e.g., documentation, changes to "internal" crates like cedar-policy-core, cedar-validator, etc.). Also this only touches experimental code.

I confirm that this PR (choose one, and delete the other options):

  • Does not update the CHANGELOG because my change does not significantly impact released code.

I confirm that cedar-spec (choose one, and delete the other options):

  • Does not require updates because my change does not impact the Cedar formal model or DRT infrastructure. (AFAIK there is no formal model yet for partial evaluation)

I confirm that docs.cedarpolicy.com (choose one, and delete the other options):

  • Does not require updates because my change does not impact the Cedar language specification.

q: Request,
pset: &PolicySet,
entities: &Entities,
mappings: Option<&HashMap<SmolStr, Value>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

What if we made this Fn(&str) -> Value
I think that would give us more freedom later to change the representation of the mappings

Copy link
Author

Choose a reason for hiding this comment

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

Absolutely, happy to do that 👍

@luxas
Copy link
Author

luxas commented Feb 24, 2025

Any other comments than? #1466 (comment)

If this is accepted, then I'll change to use a function, and implement some unit tests.

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.

reauthorize only practically works with one unknown
2 participants