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

Add variable substitution for proof configs #626

Merged
merged 2 commits into from
Sep 11, 2024

Conversation

loneil
Copy link
Contributor

@loneil loneil commented Sep 11, 2024

Allow proof configs to have variable values that will be substituted at runtime when the proof is generated.

The way this works is that when the proof is generated from the config (generate_proof_request) it calls a helper class function replace_proof_variables that recurses over the generated proof.
This finds any nested value that starts with $ and then looks for if the $<var_name> is contained in a map of preset variables and runs a function that is mapped to that variable.

That recursive helper method invokes another class in variableSubsitutions.py that contains a "static variable" map of variable names, and also overrides for contains and getitem allowing "dynamic variables" that can have parameters in the variable name.

So this allows a VCAuth operator to potentially just drop in their own variableSubsitutions.py allowing for operator-defined variable allowances and calculations.

Supported here are
$now get now time.time()
$today_str string rep of today's date ie 20240911
$tomorrow_str string rep of tomorrow date ie 20240912

and a dynamic variable $threshold_years_X that will do a string rep of today minus X years (like the 19 year check in LCRB DAV)
So adding $threshold_years_10 would substitute in 20240911

Added unit test coverage for this stuff.

Error handling
If a proof has a $<var_name> that does not resolve, throw back a 400 when trying to get the proof

image

Signed-off-by: Lucas ONeil <lucasoneil@gmail.com>
Signed-off-by: Lucas ONeil <lucasoneil@gmail.com>
@loneil
Copy link
Contributor Author

loneil commented Sep 11, 2024

For thoughts:

  • Is $<var_name> a good enough check for if to look into the substitution map? Could "real" non-variable values exist that start with $
  • I pass the whole proof in to the recursive method to look for variables, could just pass in the requested_predicates part if that's the only thing that would get variables.
  • Any other out-of-the-box variables we need?

@loneil loneil marked this pull request as ready for review September 11, 2024 19:47
Copy link
Contributor

@jamshale jamshale left a comment

Choose a reason for hiding this comment

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

I'm not very familiar with this codebase anymore but the code looks really good. I like the unit testing with the feature.

I'm not too sure about the $ placeholder. I think it's really unlikely it gets used and matches the substitution map. If you wanted to be more careful you could possibly think of a placeholder even less likely to be used that has multiple characters.

I will approve, but other contributors should have a look.

Copy link
Member

@esune esune left a comment

Choose a reason for hiding this comment

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

Looks good! I would not worry too much about the $ being a possible "real" value, I think we just need to outline what the templating language expects and its limitations and go with it. In the end, we also will be approving/rejecting (and probably helping compile) the proof-requests that will make it into VC-AuthN.

I don't see a reason to process anything other than the predicates section, at least not now. If the processing time isn't significantly higher we may leave the whole payload for processing though.

@loneil
Copy link
Contributor Author

loneil commented Sep 11, 2024

I don't see a reason to process anything other than the predicates section, at least not now. If the processing time isn't significantly higher we may leave the whole payload for processing though.

Yeah it goes through the whole payload. It's recursive, but the payload is always going to be such a relatively small bit of JSON in the grand scheme of thigs it won't make any real difference for performance if it was restricted to just the predicates.

@loneil loneil merged commit a44ce58 into main Sep 11, 2024
6 checks passed
@esune esune deleted the feature/variableSubstitution branch September 12, 2024 17:39
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.

3 participants