cedar-policy / cedar-go

Golang implementation of the Cedar Policy Language
Apache License 2.0
85 stars 9 forks source link

Allow extension call nodes to be programmatically added to the AST #54

Closed patjakdev closed 2 weeks ago

patjakdev commented 2 weeks ago

Today, users can add extension nodes (decimal, ip, datetime, and duration) to the graph, but only if they have a valid instance of the Go value type associated with those nodes. For example:

duration1 := types.DurationFromMillis(100)
duration2 := types.DurationFromMillis(1000)
policy := ast.Permit().When(ast.Value(duration1).LessThan(ast.Value(duration2)))

However, users might also want to just take a string, plunk it into the AST unparsed, and let the evaluator be the one that handles any errors in parsing just like if a Cedar policy had an invalid string specified in Cedar text.

This issue proposes adding some new public AST functions to add such unevaluated extension method call nodes to the AST. Their use would look something like this:

policy := ast.Permit().When(ast.DurationExtensionCall("100").LessThan(ast.DurationExtensionCall("1000"))

Under the hood, these helpers would just call the existing internal.ast.ExtensionCall() function.

micahhausler commented 2 weeks ago

Reading the Cedar docs on decimal extension

Constructor calls such as decimal(context.num) and decimal(if true then "100.01" else "1.01") can evaluate properly, assuming context.num is a string of the accepted format, but will not validate. To validate, the decimal() constructor must be given a string literal. If calling decimal() with the string literal would produce an overflow, the constructor call is also deemed invalid.

Would you rather see something that will evaluate like so,

func (lhs Node) DecimalExtensionCall(rhs Node) Node {
    return wrapNode(ast.ExtensionCall("decimal", rhs.Node))
}

or something that will validate?

func (lhs Node) DecimalExtensionCall(arg string) Node {
    return wrapNode(ast.ExtensionCall("decimal", ast.String(arg))
}
micahhausler commented 2 weeks ago

After thinking about it, I think the former makes sense. I was working on a CEL translator, and don't know the underlying type, so the latter wouldn't even be useful to me

patjakdev commented 2 weeks ago

After thinking about it, I think the former makes sense. I was working on a CEL translator, and don't know the underlying type, so the latter wouldn't even be useful to me

Sounds good @micahhausler. I would prefer the former approach as well, since it allows for users to construct policies that can evaluate, even if they won't validate. Given that (a) there seems to be some hunger in the community for authoring such policies and (b) cedar-go doesn't even support validation yet, I think it makes sense to allow AST constructors to pass an arbitrary node to the extension call node builders. It would hardly be the most surprising thing you're allowed to do with the AST builder :)